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

Fix problems with verification dialog #6811

Merged
merged 2 commits into from
Oct 5, 2021
Merged

Conversation

duxovni
Copy link
Contributor

@duxovni duxovni commented Sep 15, 2021

Fixes element-hq/element-web#18536:

  • We no longer show loading spinners when we're waiting for the current user to perform an action on a separate device
  • Updated some language to make it clearer that we're waiting for user action
  • When checking if there are other devices we can verify against, SetupEncryptionStore now only looks for devices that we've cross-signed. Previously it looked for all non-dehydrated devices, including the current device, so it always returned true.
  • Added an option to reset cross-signing keys if other recovery methods have been lost
  • Change which options are shown, depending on whether other cross-signed devices exist and whether a security key exists.

Remaining problems to address:

  • Need design feedback on the new layout and phrasing
  • In earlier conversations with designers we discussed making the "skip" button less prominent since it's rarely the correct option, like moving it to the upper-right corner of the dialog as a grey link: https://www.figma.com/file/X4XTH9iS2KGJ2wFKDqkyed/Compound?node-id=4260%3A45554
  • Currently I'm reusing ConfirmDestroyCrossSigningDialog for the reset option, but the scary "you almost certainly don't want to do this" text may not be appropriate if we've just told the user that their only option is to reset. AccessSecretStorageDialog has its own custom message for its reset option that's a bit gentler and may be a better fit.
  • If the user is getting this verification dialog immediately after logging in, it's a little awkward to immediately ask them to reauthenticate in order to reset their keys. Also need to make sure this works with non-password-based authentication methods.
  • I need to look more carefully at what errors bootstrapCrossSigning could potentially throw, and think about how best to handle them in this context.
  • If you attempt to reset your cross-signing keys, then give up on the interactive auth, and verify using another login, at the end of the verification session Element will still sign your device keys with the new cross-signing keys it generated during the cancelled reset. The homeserver then rejects the device keys because they're signed by the wrong SSK, even though you completed the cross-signing UI flow properly.

This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://615c4a9ee366cd111b1697f0--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

You shouldn't be manually changing the language files as these changes are done through Weblate. You should only run the yarn i18n script that will auto-generate the English file that is used as the base

@duxovni duxovni force-pushed the fayed/fix-verification-dialog branch 2 times, most recently from 1107737 to 23aedf5 Compare September 15, 2021 14:30
@duxovni
Copy link
Contributor Author

duxovni commented Sep 15, 2021

Also fixes element-hq/element-web#17765

@duxovni
Copy link
Contributor Author

duxovni commented Sep 16, 2021

On reauthentication during resets: It looks like during the cross-signing key reset, we do start by trying to authenticate with the token we're already holding, and then fall back to interactive auth if the homeserver wants us to. In the case of Synapse, it'll let us use our existing token to upload cross-signing keys when we first create the account, but for any later attempt at a reset, it wants us to reauthenticate. So I think we're already doing the best we can do here, and anything further is up to homeserver implementors.

@duxovni
Copy link
Contributor Author

duxovni commented Sep 16, 2021

Resolved the issue with cancelling a key reset; the callback passed to bootstrapCrossSigning needs to throw an error if authentication fails, in order to interrupt the control flow before the newly-generated keys get added to our own cross-signing info.

@duxovni
Copy link
Contributor Author

duxovni commented Sep 16, 2021

Having poked around at that, I feel like I have a better understanding of why this code is wrapped in a try-catch block. An error generated by cancelling authentication just takes us back to the verification dialog, which is the desired behavior.

@duxovni duxovni assigned duxovni and unassigned duxovni Sep 17, 2021
@duxovni duxovni marked this pull request as ready for review September 24, 2021 20:20
@duxovni duxovni requested a review from a team as a code owner September 24, 2021 20:20
@duxovni
Copy link
Contributor Author

duxovni commented Sep 24, 2021

Going to continue discussing this with @amshakal and figure out any other UI changes that would be good to have, but essentially this is in the review phase now.

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

That looks pretty good to me 👏 💯

@germain-gg
Copy link
Contributor

It looks like some CI tests are failing, but they seem unrelated.
Merging develop into your current branch should fix all issues

@novocaine novocaine requested a review from a team September 27, 2021 11:43
@novocaine novocaine requested review from amshakal and removed request for a team September 27, 2021 11:44
@novocaine
Copy link
Contributor

UI changes require design to check and sign-off, so I've set @amshakal as reviewer

@nadonomy
Copy link
Contributor

Hey @duxovni, just reviewing with @amshakal there's some usability issues with the top right Skip interaction etc. Do you have more context on these discussions? (Might be easier for us to chat in a Matrix room?)

@amshakal
Copy link

amshakal commented Sep 30, 2021

I've cleaned up the flow and taken @duxovni through it on a call.

In the proposed changes we have :

  1. Updated the skip button to 'close', althought it will perform the same function.
  2. Kept the size of the modal the same throughout the flow to make it less jarring for the user.
  3. Reduced places where the primary CTA is red in colour, to make that user doesn't feel like they are about to do something alarming.
  4. And overall, we have created the designs based on the new design system.

- Don't show loading spinners while waiting for user action
- When checking if there are other devices we can verify against, only
  look for devices that are actually cross-signed.
- Adjust displayed options depending on whether other devices and/or
  recovery keys exist, and add an option to reset cross-signing keys
  if necessary.
- Various minor clarifying adjustments to UI styling/text

Signed-off-by: Faye Duxovni <fayed@element.io>
@duxovni
Copy link
Contributor Author

duxovni commented Oct 5, 2021

I've fixed all the new design issues Amsha and I discussed, and also addressed an issue with the reset flow when secret storage is set up.

While examining this whole verification flow, Amsha noticed several design problems in preexisting code that are beyond the scope of this PR, so I'm spinning those off into element-hq/element-web#19279 to deal with separately and avoid delaying the fixes for element-hq/element-web#18536 and element-hq/element-web#17765.

If the code still looks good (and maybe if we can find a known-good commit to rebase onto so I can make sure none of these failing tests are my fault :p), I think this is ready to merge.

@germain-gg
Copy link
Contributor

Code still looks good to me 🚀

Having had a quick look at the failing, it looks like the regression that was fixed as part of element-hq/element-web#19247. Merging develop into your branch should help fix the issue

@amshakal
Copy link

amshakal commented Oct 5, 2021

That sounds good to me. Thanks for documenting all the proposed UI changes in another issue, @duxovni. 👯

@duxovni duxovni merged commit 4180434 into develop Oct 5, 2021
@duxovni duxovni deleted the fayed/fix-verification-dialog branch October 5, 2021 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verification dialog is misleading if you managed to never set up SSSS
6 participants