-
-
Notifications
You must be signed in to change notification settings - Fork 833
Short-Authentication-String Verification #2461
Conversation
Could you add some screenshots or even better GIFs to better understand the changes? |
The js-sdk usage looks reasonable. The GIF looks exciting! 😀 |
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.
Overall, it looks great! 😁 Thanks for working on this.
<AccessibleButton | ||
element="span" className="mx_linkButton" onClick={this._onSwitchToLegacyClick} | ||
> | ||
{_t("Use Legacy Verification (for older clients)")} |
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.
This feels like an unfortunate UX trap... How should people know whether or not to switch back to legacy? Can we ever remove this? There's no easy fix I guess, but some way to detect what verification methods a device supports would make this much better.
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.
Yeah, it is a bit. Unfortunately we didn't think to build in negotiation from the get-go, so we're a bit stuck with older clients. In reality we are probably ok though since there are not all that many clients that implement e2e right now, so hopefully this can go away fairly soon.
"and you probably want to press the blacklist button instead.") } | ||
</p> | ||
<p> | ||
{ _t("In future this verification process will be more sophisticated.") } |
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.
Maybe remove this paragraph, since a more sophisticated thing now exists?
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.
Yeah, wasn't sure about this since you'll still just get this screen if you don't have the labs flag turned on. Maybe still better to remove it though.
Co-Authored-By: dbkr <dbkr@users.noreply.github.com>
Why would we force a DM? I'd expect us to go query their profile to provide better-than-mxid labels. |
We were thinking about it so the incoming request would always have a relevant room to be displayed in (since it's triggered from the in-timeline crypto events). Otherwise it's not obvious how the UX would work. It also avoids making it a separate spam vector. |
Reference implementations: * https://gitlab.matrix.org/matrix-org/olm/commit/94f664e7256215f33639dbbad6aaf87ada082a9f * matrix-org/matrix-react-sdk#2461 * matrix-org/matrix-js-sdk#818 * matrix-org/matrix-react-sdk#2596 * matrix-org/matrix-js-sdk#837 Proposals: * [MSC1717](#1717) * [MSC1267](#1267) No alterations to either proposal have been made intentionally here.
Adds SAS verification behind a feature flag. Currently in dialogs rather than the right panel. Also only refers to the other party by user ID currently (we need to decide whether we're forcing the user to have a DM open with the other party before verifying: if so, we can use their member event from the 1:1 room, otherwise, we'll have to fetch their profile).
Requires matrix-org/matrix-js-sdk#818