interledger / web-monetization-extension

An open-source browser extension that enables Web Monetization.
Apache License 2.0
49 stars 3 forks source link

[BUG] Extension unable to disconnect after public key has already been dissociated from the wallet #332

Closed RabebOthmani closed 2 months ago

RabebOthmani commented 3 months ago

Steps to reproduce

  1. Go to rafiki.money --> Settings --> Developer Keys
  2. Revoke key from wallet
  3. On the extension UI, click on the disconnect button
  4. Extension unable to disconnect

Expected result

Extension should gracefully disconnect from Wallet

Actual result

Extension shows payment screen immediately after clicking on disconnect. Going back to the extension settings screen shows wallet still connected

Screenshots or videos

No response

Additional context

No response

Operating system

Windows

Operating system version

No response

Browsers

Chrome

Browser version

No response

Extension version

Alpha

raducristianpopa commented 3 months ago

For the disconnecting part, we can check if the returned error message is invalid_client when making the API call. If it is returning invalid_client can we make the assumption that the user does not have the key anymore and we can safely clear the storage`?

if this is happening during the monetization flow, should we have an error state for the extension? We can open the popup pragmatically when this happens. Maybe the error page in the popup can prompt the public key for the user again to be reuploaded, alongside a disconnect button that just clears the storage?

CC @sidvishnoi @dianafulga @tselit

sidvishnoi commented 3 months ago

Sounds good.

We should also update the error message on connect screen when key isn't synced with wallet (instead of bad request one).

sidvishnoi commented 3 months ago

For the action-required error states, I think we can add routes under /errors/*. This we we can keep the current default screens same, instead of adding alerts in them (which end up adding more scrollbars). I've that MissingHostPermission route, can move that under errors (not to be mixed with react-router errors). Ideally, we can add a ErrorLayout and "slot" content inside it for each error page (See MissingHostPermission for example).

tselit commented 3 months ago

To do: @tselit to update the Figma design file. Maybe create a story for public key input. Points noted by @RabebOthmani:

sidvishnoi commented 3 months ago

For the disconnecting part, we can check if the returned error message is invalid_client when making the API call. If it is returning invalid_client can we make the assumption that the user does not have the key anymore and we can safely clear the storage`?

Just realized the client doesn't return any response on a bad request, so we can't really know if it's a invalid_client error or something else šŸ˜¬. I think this is something that should be handled at client level, or in Rafiki - so we get better error message in return.

Update: Filed https://github.com/interledger/open-payments/issues/482

sidvishnoi commented 3 months ago

Alternatively,

raducristianpopa commented 3 months ago

Alternatively,

we can assume 400 bad request in given state means key removed,

or, use our own OpenPayments client, so we've better control (added benefit of less webpack/tooling fiddling, but more maintenance)

This should be done at the SDK level. Let's not implement our own version for the OP SDK.

sidvishnoi commented 3 months ago

For now, I'll assume with 400 bad request means that only, and add a TODO when SDK has better error code.

sidvishnoi commented 3 months ago

Further investigation:

during payment
  - create quote fails with 401 + Signature validation error: could not find key in list of client keys
  - create outgoing payment with same
  - create incoming payment grant fails with 400 + invalid_client

As there can be multiple reasons for each 400/401, I think it's better to wait on SDK improvements for this so we know which error is it. https://github.com/interledger/open-payments/issues/482

Sending draft PR to allow disconnect on-demand at least.

sidvishnoi commented 3 months ago

@raducristianpopa @RabebOthmani @tselit

If a key is revoked in between a payment, user can add the key again to same wallet and continue like before (no need to connect again; tested with rafiki.money at least). They can also disconnect and connect again from scratch.

Should we support both options in the error screen? Is the first option complicated for user to follow - as in which one they should go for? Also, after user has copied the key, they need to signal to extension to "try payments again" (or we can keep checking for key validity over and over in background after user copies the public key until we've the successful integration).

Thoughts?

sidvishnoi commented 3 months ago

Had a call with @raducristianpopa

We think we should show both copy-key and disconnect options.

With copy-key (to add again), we'll show them public key and wallet address again, along with a button to copy it. When user has copied the key, they'd need to press a "I already copied the key" button in popup. On that, we'll verify if we have the right key by trying to rotate the grant token.

Secondly, we'll show the Disconnect button, which will clear the storage (existing grant is cleared from storage, but it'll stay stale in user's wallet). The user should copy key if they don't want the stale grant in their wallet. On disconnect, we'll show the connect screen from scratch that we show on first install.

To make the decision/UX simpler, we can use a wizard to guide user and show other details only when asked for. We'd need UX feedback on this. cc @RabebOthmani @tselit

sidvishnoi commented 3 months ago

UI as of https://github.com/interledger/web-monetization-extension/pull/366/commits/1e33ae7d59394728af9118b54a5a68375abeb989

Screenshot ![image](https://github.com/interledger/web-monetization-extension/assets/8426945/7adc47f7-9cb3-4b97-9a4c-ed1b558d6b55)
sidvishnoi commented 3 months ago

I realized we're not using the font we import, so above screenshot is with my system defaults.

Looks like this with that font ![image](https://github.com/interledger/web-monetization-extension/assets/8426945/881278fb-b48b-405c-9375-4fd7489dc197)
RabebOthmani commented 3 months ago

Basically, what we're saying is that there are 2 scenarios here: something went wrong accidently and the key was dissociated from the wallet or the user made the decision to revoke the key. The scenario that is more likely to be true should guide the first option/solution offered to the user. My hypothesis (at least based on the Rafiki wallet) is that it's very hard and unlikely that a user dissociated a key by accident, therefore the first option would be to dissociate the wallet then the user is redirected to the configuration screen. The second option in case, the dissociation was an accident would be to ask them to copy the key again and reconnect. @tselit @sidvishnoi @raducristianpopa thoughts?

sidvishnoi commented 2 months ago

Some relevant notes from Slack:

I just tired to link the key to another wallet and got ā€œinternal server errorā€ understandably as I didnā€™t remove it from the previous wallet - @RabebOthmani

Adding existing key to any address/account leads to same error. So, user either needs to first remove the existing key (e.g. they want to change their wallet address), or we need to add an option to "regenerate key".

tselit commented 2 months ago

Updated the UI & flow for the key dissociated scenario in Figma: https://www.figma.com/design/0S8Oj8ZB5MvkGtIrVRswDH/Web-monetization?node-id=517-504&t=rMLJcdULlNgocWTE-4

This ^^ is ready for peer review, and then we can send it off to Madalina (Design) for final review/approval.