Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Short-Authentication-String Verification #2461

Merged
merged 13 commits into from
Jan 28, 2019
Merged

Short-Authentication-String Verification #2461

merged 13 commits into from
Jan 28, 2019

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Jan 18, 2019

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).

sassy

Requires matrix-org/matrix-js-sdk#818

@bwindels
Copy link
Contributor

Could you add some screenshots or even better GIFs to better understand the changes?

@uhoreg
Copy link
Member

uhoreg commented Jan 23, 2019

The js-sdk usage looks reasonable. The GIF looks exciting! 😀

@dbkr dbkr requested a review from a team January 24, 2019 17:09
@jryans jryans self-assigned this Jan 24, 2019
Copy link
Collaborator

@jryans jryans left a 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)")}
Copy link
Collaborator

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.

Copy link
Member Author

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.") }
Copy link
Collaborator

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?

Copy link
Member Author

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.

src/components/views/dialogs/IncomingSasDialog.js Outdated Show resolved Hide resolved
src/components/views/verification/VerificationShowSas.js Outdated Show resolved Hide resolved
src/components/views/verification/VerificationShowSas.js Outdated Show resolved Hide resolved
@jryans jryans assigned dbkr and unassigned jryans Jan 24, 2019
jryans and others added 4 commits January 25, 2019 17:30
Co-Authored-By: dbkr <dbkr@users.noreply.github.com>
Co-Authored-By: dbkr <dbkr@users.noreply.github.com>
Co-Authored-By: dbkr <dbkr@users.noreply.github.com>
@ara4n
Copy link
Member

ara4n commented Jan 27, 2019

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).

Why would we force a DM?

I'd expect us to go query their profile to provide better-than-mxid labels.

@dbkr
Copy link
Member Author

dbkr commented Jan 28, 2019

Why would we force a DM?

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants