Skip to content

MSC3131: Verifying with QR codes v2#3131

Open
uhoreg wants to merge 4 commits intomatrix-org:old_masterfrom
uhoreg:qr_codes_2
Open

MSC3131: Verifying with QR codes v2#3131
uhoreg wants to merge 4 commits intomatrix-org:old_masterfrom
uhoreg:qr_codes_2

Conversation

@uhoreg
Copy link
Member

@uhoreg uhoreg commented Apr 16, 2021

@uhoreg uhoreg changed the title MSCxxxx: Verifying with QR codes v2 MSC3131: Verifying with QR codes v2 Apr 16, 2021
@uhoreg uhoreg added e2e kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal labels Apr 16, 2021
@uhoreg
Copy link
Member Author

uhoreg commented Apr 16, 2021

(not sure if this should be kind:feature or kind:maintenance)

If Bob selects verification by scanning a QR code, then his client will send an
`m.key.verification.start` event with `method` set to `m.qr_code.scan.v2`.
When Alice's client receives this event, then it will show the QR code if it
has not already done so.
Copy link
Contributor

Choose a reason for hiding this comment

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

So from my perspective this looks great. It should fit much better into the existing flow and should in general be a bit easier to implement even. One thing that was raised to eventually push MSC1544 through, was that the user may notice he can't reliably scan the QR code only while trying. In the old proposal this was no issue, since you only sent the start after a successful scan, but in this case the user has to send m.key.verification.start before even trying to scan, since the other client may not have shown the QR code (and I don't think requiring to immediately show the QR code is a good idea, even if it would fix that). So if a user fails the scan, they basically have to restart the verification. While I do think that is okay, in the discussion around MSC1544 it was said, that this would lead to poor UX.

Do you plan to address that? One way I could see to fix that would be to have an additional restart step, that can be used to reset the verification to m.key.verification.start a limited number of times and can be used to select an alternative method (an independent proposal from this, not adding that here, only maybe referencing it). I think that could be used to improve the UX again, while still retaining the advantages of this proposal and only adding moderate complexity.

In general I really like this proposal though and I especially like that it encourages to only start the camera on the users request instead of preemptively! Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that was raised to eventually push MSC1544 through, was that the user may notice he can't reliably scan the QR code only while trying. In the old proposal this was no issue, since you only sent the start after a successful scan, but in this case the user has to send m.key.verification.start before even trying to scan, since the other client may not have shown the QR code (and I don't think requiring to immediately show the QR code is a good idea, even if it would fix that). So if a user fails the scan, they basically have to restart the verification. While I do think that is okay, in the discussion around MSC1544 it was said, that this would lead to poor UX.

I don't remember this being brought up in that discussion, at least not by me or by anyone on the spec core team. I did say that with the way 1544 works, no method has been "selected" until the QR code has been scanned, but that was on a protocol level and not on a UI level. I think even in Element, once you've chosen to scan a QR code, if you can't scan, then you need to re-start the verification. So it isn't something Element was pushing for.

We don't have any current plans for changing the verification method after a verification has already started, but if we feel the need to do so, we can add it later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this can be added at a later point. I just remembered this as something raised by @turt2live, but if you think it is not a concern at the moment, I am fine with it!

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants