-
Notifications
You must be signed in to change notification settings - Fork 334
feat(oauth): add flow for logging in using a QR code generated on the logged out device #5711
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.
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5711 +/- ##
==========================================
+ Coverage 88.43% 88.44% +0.01%
==========================================
Files 360 360
Lines 99770 100214 +444
Branches 99770 100214 +444
==========================================
+ Hits 88232 88637 +405
- Misses 7402 7419 +17
- Partials 4136 4158 +22 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5711 will not alter performanceComparing Summary
|
1bf637a
to
4ee68ef
Compare
Could we split this PR up a bit?
Ideally we would merge 1-3 as separate PRs and then discuss 4 in a more detailed manner. |
Ok, sure. I'll put this one back into draft until the others have landed. |
Signed-off-by: Johannes Marbach <[email protected]>
4ee68ef
to
6cb59d0
Compare
This is now reduced to the formerly last commit with the previous ones having landed on |
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.
Ok, this looks mostly good though needs a bit more work.
I'd like to change the public API a bit so we don't increase the number of methods.
The biggest problem is that the login is broken if the client chooses to use a default homeserver for the rendezvous.
Reuse SecureChannel::login in SecureChannel::new Signed-off-by: Johannes Marbach <[email protected]>
Remove assert Signed-off-by: Johannes Marbach <[email protected]>
Remove misleading comment Signed-off-by: Johannes Marbach <[email protected]>
Clarify that the check code has to be supplied in digits representation Signed-off-by: Johannes Marbach <[email protected]>
Use builder to avoid introducing another method and add example in doc comment Signed-off-by: Johannes Marbach <[email protected]>
Annotate steps of secure channel setup as per the MSC Signed-off-by: Johannes Marbach <[email protected]>
Handle homeserver swap and annotate OAuth login steps Signed-off-by: Johannes Marbach <[email protected]>
Guard builder with feature flag to fix Wasm build errors Signed-off-by: Johannes Marbach <[email protected]>
Update changelog Signed-off-by: Johannes Marbach <[email protected]>
Ammend comments on the futures to match the new builder pattern Signed-off-by: Johannes Marbach <[email protected]>
Fix import to make example code compile Signed-off-by: Johannes Marbach <[email protected]>
Sorry, took me a bit longer to address everything and unbreak the CI. Should be ready for re-review 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.
There's still a step missing when we replace the homeserver.
But I think that this is good to go after we add the last step.
Link to the respective reverse method in doc comments Signed-off-by: Johannes Marbach <[email protected]>
Re-resolve well-known after homeserver change Signed-off-by: Johannes Marbach <[email protected]>
Unbreak the CI Signed-off-by: Johannes Marbach <[email protected]>
crates/matrix-sdk/src/client/mod.rs
Outdated
&& let Ok(homeserver) = Url::parse(&well_known.homeserver.base_url) | ||
{ |
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.
Don't we need to throw an error here, otherwise this turns into a no-op if we can't parse the URL.
Am I missing something?
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.
Hm, I think you're right. I guess that should throw in maybe_update_login_well_known
, too, then?
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.
Stopped swallowing the error in this method with 1383ff8.
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_update_login_well_known
sounds like a best effort method, so it might be fine there.
Though looking at the place where it's used it might make more sense to error out there as well.
Don't swallow URL parsing errors Signed-off-by: Johannes Marbach <[email protected]>
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.
Alright, this looks good now. Thanks for patiently working on this.
I know it's a bit slow and annoying but looking at the resulting code/docs/comments I think it was very much worth it.
Let me know if you'd like to clean up the history or if I should just squash merge.
Oh, not annoying at all and a lot better than the initial PR. Thanks for the review. 👍
If squash-merging is an option, I'll take it. Otherwise, I would squash it into a single commit locally .. which sort of leads to the same result. |
This pull request is a step towards adding the complementary QR login variant where the new device is the one generating the QR code and the existing device is the one scanning the QR code. Only the establishment of the secure channel and the completion of the login by the new device are covered in this change.
Naming feels really hard here. The new identifiers are all prefixed
Generated...
. We could prefix the existing identifiersScanned...
but I haven't bothered doing this yet in case someone has better ideas.