Skip to content

Support for MSC3906 v2 #3193

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

Support for MSC3906 v2 #3193

wants to merge 12 commits into from

Conversation

hughns
Copy link
Member

@hughns hughns commented Mar 6, 2023 •
edited by github-actions bot
Loading

This PR adds support for v2 of MSC3906 which is used for Sign in with QR.

Both v1 (default) and v2 are supported so compatibility is maintained.

Signed-off-by: Hugh Nimmo-Smith [email protected]

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Notes: Updates to protocol used for Sign in with QR code

Here's what your changelog entry will look like:

✨ Features

  • Updates to protocol used for Sign in with QR code (#3193). Contributed by @hughns.

@hughns hughns mentioned this pull request Mar 6, 2023
Closed
6 tasks
@hughns hughns force-pushed the hughns/msc3906-v2 branch from 3639083 to b1d7788 Compare March 9, 2023 20:22
@hughns hughns force-pushed the hughns/msc3906-v2 branch from a8cc3a1 to 59ee0a2 Compare March 9, 2023 21:57
@hughns hughns force-pushed the hughns/msc3906-v2 branch from 59ee0a2 to 7bb10a1 Compare March 9, 2023 22:09
@hughns
Copy link
Member Author

hughns commented Mar 9, 2023

n.b. the code smells reported by SonarCloud are for deliberate use of deprecated MSC3906 v1 types

@hughns hughns marked this pull request as ready for review March 9, 2023 22:16
@hughns hughns requested a review from a team as a code owner March 9, 2023 22:16
@hughns hughns requested review from richvdh and justjanne March 9, 2023 22:16
@hughns hughns marked this pull request as draft March 9, 2023 22:19
@hughns hughns marked this pull request as ready for review March 9, 2023 22:32
@hughns hughns force-pushed the hughns/msc3906-v2 branch from 24a045d to b6699ba Compare March 9, 2023 22:34
@richvdh
Copy link
Member

richvdh commented Mar 13, 2023

for links: previous impl was #2747

richvdh
richvdh reviewed Mar 13, 2023
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Afraid I have to step away for a meeting, but I've left a few initial thoughts/stupid questions.

As a general theme: I would like to see more doc-comments explaining what things do/what they are for, even if it seems obvious. Writing out in words how an API is used/what it does can be really useful in terms of helping people understand, and maintaining a clean API.

src/rendezvous/MSC3906Rendezvous.ts Outdated
private _code?: string;

/**
* @param channel - The secure channel used for communication
* @param client - The Matrix client in used on the device already logged in
* @param onFailure - Callback for when the rendezvous fails
* @param flow - The flow to use. Defaults to MSC3906 v1 for backwards compatibility.
Copy link
Member

Choose a reason for hiding this comment

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

please could we have some more words explaining what a "flow" means and how I, as a user of this API, should pick one?

src/rendezvous/channels/MSC3903ECDHv2RendezvousChannel.ts
Comment on lines +102 to 103
flow,
intent,
Copy link
Member

Choose a reason for hiding this comment

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

While trying to get a handle on how these pieces fit together, I find it a bit odd that this is implementing a "MSC3903 rendezvous channel", but there is no mention in MSC3903 of this bit. If this is specific to MSC3906, shouldn't it be in MSC3906Rendezvous rather than here?

src/rendezvous/MSC3906Rendezvous.ts Outdated
Comment on lines 136 to 140
/**
*
* @returns the checksum of the secure channel if the rendezvous set up was successful, otherwise undefined
*/
public async startAfterShowingCode(): Promise<string | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

Particularly given this is a public function, please could it have a better doc-comment? What does it actually do, beyond returning a success flag?

@hughns hughns force-pushed the hughns/msc3906-v2 branch from e4559eb to c4ea55b Compare April 5, 2023 17:20
@justjanne
Copy link
Contributor

Please make sure to rebase this PR so it can be compared against the current codebase cleanly

@richvdh
Copy link
Member

richvdh commented Dec 5, 2023

@hughns what's the state of play with this? Can it be closed for now?

@hughns hughns marked this pull request as draft December 11, 2023 10:23
@hughns
Copy link
Member Author

hughns commented Dec 11, 2023

Closing this in preference to moving directly to supporting Sign in with QR with OIDC.

@hughns hughns closed this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh left review comments

@justjanne justjanne Awaiting requested review from justjanne justjanne is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hughns @richvdh @justjanne