-
-
Notifications
You must be signed in to change notification settings - Fork 828
Fix problems with verification dialog #6811
Conversation
There was a problem hiding this 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
1107737
to
23aedf5
Compare
Also fixes element-hq/element-web#17765 |
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. |
Resolved the issue with cancelling a key reset; the callback passed to |
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. |
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. |
There was a problem hiding this 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 👏 💯
It looks like some CI tests are failing, but they seem unrelated. |
UI changes require design to check and sign-off, so I've set @amshakal as reviewer |
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?) |
I've cleaned up the flow and taken @duxovni through it on a call. In the proposed changes we have :
|
- 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>
dd0d5e1
to
9a16b46
Compare
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. |
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 |
That sounds good to me. Thanks for documenting all the proposed UI changes in another issue, @duxovni. 👯 |
Fixes element-hq/element-web#18536:
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.Remaining problems to address:
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.bootstrapCrossSigning
could potentially throw, and think about how best to handle them in this context.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 addType: [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.