Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for canceling authorization requests without authorizing sites #1491

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

F-OBrien
Copy link
Contributor

PR #1453 added the ability to reject an authorization request, which is valuable as it throws an error that can be handled by the request originator. However, the current behavior still adds the site to the list of authorized sites, which may not always be desired.

This PR adds a new message handler pri(authorize.cancel) that:

  • Deletes the authorization request (similar to previous pri(authorize.delete.request))
  • Rejects the authorization request with an error
    = Does NOT add the URL to the list of authorized sites

The key difference is that users can now completely cancel an authorization attempt without having the site appear in their authorized sites list. The site will also prompt for authorization again for future attempts to connect the extension.

@TarikGul TarikGul requested review from Tbaut and TarikGul December 18, 2024 15:58
Copy link
Contributor

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I miss something, this adds the functionality, but it's never used? 🤔
Shouldn't you add a "Cancel" button to the packages/extension-ui/src/Popup/Authorize/Request.tsx?

@F-OBrien
Copy link
Contributor Author

I use extension-base as a dependency so was missing this functionality. I wasn't sure if it was the intended behavior that the polkadot-js extension should always add the url to the authorized list or not. Happy to adding a button if you want it.

@Tbaut
Copy link
Contributor

Tbaut commented Dec 18, 2024

I see. I think we've been back and forth with this functionality. As in the older version (the one live forever that hadn't been updated in 2 years) did add the website to the list. I changed this when adding account selection because I thought it made sense, but feedback showed the behavior wasn't welcome, pinging users over and over and I believe Talisman was behaving differently.. so I reverted.

I'd say let's keep it as is then, bc I haven't heard of anyone complaining about this for a while, and an additional button may be confusing

Copy link
Contributor

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not changing anything to the currently live version.
The only thing I'm worried about is that when changes are made to the code of this lib, we're not taking implementations other than pjs-extension into account, since I don't think it's aimed at being a lib used by the outside world 🤷

@F-OBrien
Copy link
Contributor Author

I changed this when adding account selection because I thought it made sense, but feedback showed the behavior wasn't welcome, pinging users over and over and I believe Talisman was behaving differently.. so I reverted

I think the main issue with site pinging people over and over again is caused by many dApp implementation using web3Enable which always pings every extension.

For that reason we don't use the extension-dapp package but extracted logic from it for our own signing manager plugin for our SDK. For our implementations we take the approach that a user must pass the single extension they want to connect to to when instantiating the signing manager and then only try to connect/authorize that one. It means users are only connecting to a single wallet at a time this prevents several wallets from prompting for authorization. We also implemented methods to get the installed extensions we can present a list of available extension or link to ones that are not installed.

@Tbaut
Copy link
Contributor

Tbaut commented Dec 18, 2024

it's still possible to go get all the extensions injected, and only enable the ones the user want. Dot-connect does this well with papi, but it was possible before (it needs a custom logic and extension-dapp shouldn't be used)

@F-OBrien
Copy link
Contributor Author

I just took a look at both Talisman and Subwallet to see what their approach to rejecting and canceling is.

  • Talisman only offers a method to cancel which doesn't add anything to the list of authorized urls similar to the effect of this PR.
  • SubWallet has two option reject or cancel. Reject adds the site to the list of authorized urls but also adds a isAllowed: false property to block the site. This prevents repeated prompts for authorization but still prevents any attempt from the site to trigger signing.

In general I prefer SubWallets approach. They also check the URL and key for authorization when a sign request is received which is missing from the polkadot extension. My concern with simply adding a URL to the authorized list with 0 authorized keys is that in theory a site that is somehow aware of keys in the wallet can still initiate a signing request for a unconnected key. I prefer to not give a potentially malicious site any chance for causing confusion by not marking it as authorized so feel the cancel option in this PR is more defensive.

@Tbaut
Copy link
Contributor

Tbaut commented Dec 19, 2024

Verifying that the key is authorized before signing is for sure welcome, but that's a different topic. As for the rest and more buttons I don't have a strong opinion, if you feel like adding a Cancel button back, I'd personally be ok with it despite the slightly more confusing design

@F-OBrien
Copy link
Contributor Author

@Tbaut I changed the implementation slightly but it does the same thing.

I also added a button to the UI along with a check box. After battling with CSS (which I still hate) the checkbox and note should always remain visible or at least show a scroll bar now unlike the previous reject text that was always off the screen for me on windows with chrome
image
old appearance for reference
image

I also added pagination for multiple connection requests rather than rendering them all on top of each other with only an option to action the first one.
image

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going through the thread above, I agree with all sentiments. The PR is overall sane to me. IMO the addition to the extension-base is fine, and the rest with the extension-ui looks fine.

Thanks for the PR - 👍

@TarikGul TarikGul merged commit 52acbc8 into polkadot-js:master Dec 20, 2024
4 checks passed
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Dec 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants