Update subscribed accounts when connected site authorizations are modified #1412
Conversation
|
I you want a UI for testing that uses subscribed accounts you can use https://portal.polymesh.live/ (Polymesh Portal - Testnet) |
|
Thanks for the PR on the queue to review tomorrow! |
|
Side note (this might be a bug?) I did not see any restriction in the signing flow for authorized accounts for a specific URL. Authorized accounts are only checked/filtered out when querying or subscribing to accounts. As a result as long as a key is in the keyring any authorized site can trigger the extension to prompt for signing with it. It just might not have an easy way to know it's in the keyring. I noticed this when I removed an authorization for a key with the current extension release 0.49.1 but was still able to sign with it. I didn't include it in this PR as I'd be concerned any changes to the signing flow could be breaking for downstream consumers of these packages. |
| @@ -271,7 +284,7 @@ export default class State { | |||
| }; | |||
| }; | |||
|
|
|||
| public udateCurrentTabsUrl (urls: string[]) { | |||
| public updateCurrentTabsUrl (urls: string[]) { | |||
There was a problem hiding this comment.
To be more specific to @bee344 mention above: When we release this - this specific name change in the api should be mentioned as a BREAKING CHANGE - it's quite minor but nonetheless will be nice to have in the CHANGELOG and release message. I do like this though.
There was a problem hiding this comment.
should preserve the original method and just mark it as deprecated and add the new one with the fixed name
There was a problem hiding this comment.
I've reintroduced udateCurrentTabsUrl and marked as deprecated.
@bee344 @ryanleecode @IkerAlus Can you guys help look into this - I think it would important to get some second eyes and clarity on this. :) |
The idea about the authorization is to make sure 1) a user actually agrees to share their accounts and 2) they can choose which accounts they share (released recently). What you describe certainly sounds like a check that has been forgotten, as I don't see why we should accept signing request for non-shared account. Regarding the gravity, I wonder how it could be exploited though, as in, a website should trigger many signing requests, with many different accounts, to try to guess what account is present in the keyring. TL;DR: it should be fixed, but it's not a huge issue unless I missed something |
I'll follow up on this by filing an issue today - and then we can tackle this eventually - and make it a bit more tolerant. |
|
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. |
Addresses #1411