feat(oauth): add flow for logging in using a QR code generated on the logged out device#5711
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 <n0-0ne+github@mailbox.org>
4ee68ef to
6cb59d0
Compare
|
This is now reduced to the formerly last commit with the previous ones having landed on |
poljar
left a comment
There was a problem hiding this comment.
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 <n0-0ne+github@mailbox.org>
Remove assert Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Remove misleading comment Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Clarify that the check code has to be supplied in digits representation Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Use builder to avoid introducing another method and add example in doc comment Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Annotate steps of secure channel setup as per the MSC Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Handle homeserver swap and annotate OAuth login steps Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Guard builder with feature flag to fix Wasm build errors Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Update changelog Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Ammend comments on the futures to match the new builder pattern Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Fix import to make example code compile Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
|
Sorry, took me a bit longer to address everything and unbreak the CI. Should be ready for re-review now. |
poljar
left a comment
There was a problem hiding this comment.
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 <n0-0ne+github@mailbox.org>
Re-resolve well-known after homeserver change Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Unbreak the CI Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
| && let Ok(homeserver) = Url::parse(&well_known.homeserver.base_url) | ||
| { |
There was a problem hiding this comment.
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.
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.
Stopped swallowing the error in this method with 1383ff8.
There was a problem hiding this comment.
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 <n0-0ne+github@mailbox.org>
poljar
left a comment
There was a problem hiding this comment.
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.