Skip to content

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Sep 24, 2025

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 identifiers Scanned... but I haven't bothered doing this yet in case someone has better ideas.

  • Public API changes documented in changelogs (optional)

Copy link

codecov bot commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 90.68736% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.44%. Comparing base (ef440ee) to head (1383ff8).
⚠️ Report is 62 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...atrix-sdk/src/authentication/oauth/qrcode/login.rs 91.19% 20 Missing and 17 partials ⚠️
crates/matrix-sdk/src/client/mod.rs 66.66% 1 Missing and 3 partials ⚠️
.../src/authentication/oauth/qrcode/secure_channel.rs 90.90% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Sep 24, 2025

CodSpeed Performance Report

Merging #5711 will not alter performance

Comparing Johennes:johannes/qr-login-1 (1383ff8) with main (a11daf2)

Summary

✅ 50 untouched

@Johennes Johennes force-pushed the johannes/qr-login-1 branch 6 times, most recently from 1bf637a to 4ee68ef Compare September 25, 2025 12:29
@Johennes Johennes marked this pull request as ready for review September 25, 2025 12:49
@Johennes Johennes requested a review from a team as a code owner September 25, 2025 12:49
@Johennes Johennes requested review from poljar and removed request for a team September 25, 2025 12:49
@poljar
Copy link
Contributor

poljar commented Oct 3, 2025

Could we split this PR up a bit?

  • Commit 1 is an easy code move.
  • Commit 2 is a breaking change lacking a changelog entry
  • Commit 3 is a breaking change lacking a changelog entry and a motivation in the commit message.
  • Commit 4 is the meat of the PR.

Ideally we would merge 1-3 as separate PRs and then discuss 4 in a more detailed manner.

@Johennes
Copy link
Contributor Author

Johennes commented Oct 3, 2025

Ok, sure. I'll put this one back into draft until the others have landed.

Signed-off-by: Johannes Marbach <[email protected]>
@Johennes Johennes force-pushed the johannes/qr-login-1 branch from 4ee68ef to 6cb59d0 Compare October 8, 2025 12:37
@Johennes Johennes marked this pull request as ready for review October 8, 2025 18:41
@Johennes
Copy link
Contributor Author

Johennes commented Oct 8, 2025

This is now reduced to the formerly last commit with the previous ones having landed on main.

Copy link
Contributor

@poljar poljar left a 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]>
@Johennes
Copy link
Contributor Author

Sorry, took me a bit longer to address everything and unbreak the CI. Should be ready for re-review now.

@Johennes Johennes requested a review from poljar October 13, 2025 17:20
Copy link
Contributor

@poljar poljar left a 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]>
Comment on lines 545 to 546
&& let Ok(homeserver) = Url::parse(&well_known.homeserver.base_url)
{
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@Johennes Johennes requested a review from poljar October 15, 2025 12:50
Don't swallow URL parsing errors

Signed-off-by: Johannes Marbach <[email protected]>
Copy link
Contributor

@poljar poljar left a 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.

@Johennes
Copy link
Contributor Author

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.

Oh, not annoying at all and a lot better than the initial PR. Thanks for the review. 👍

Let me know if you'd like to clean up the history or if I should just squash merge.

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.

@poljar poljar merged commit e8fb133 into matrix-org:main Oct 15, 2025
52 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants