Skip to content
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

Support to log in using a QR code #3466

Merged
merged 7 commits into from
May 28, 2024
Merged

Support to log in using a QR code #3466

merged 7 commits into from
May 28, 2024

Conversation

poljar
Copy link
Contributor

@poljar poljar commented May 24, 2024

This implements part of MSC4108. We're only supporting the case where new device scans the QR code and gets logged in by the existing device.

Opening as a draft as I want to reword the commit messages a bit, add some more documentation (mainly to insert some jokes about rendezvous), and possibly one or two more tests.

Best reviewed commit by commit, although there is one giant commit. Reviewer please have mercy on me.

  • Public API changes documented in changelogs (optional)

@poljar poljar force-pushed the poljar/qr-login-pr branch 2 times, most recently from 79b1b95 to 813a9b4 Compare May 27, 2024 06:37
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Looking really good! Congrats for the quality of the code and the commits, thank you.

I've left a couple of feedback, but nothing too difficult to fix.

crates/matrix-sdk/src/matrix_auth/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/oidc/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/mod.rs Show resolved Hide resolved
crates/matrix-sdk/Cargo.toml Outdated Show resolved Hide resolved
crates/matrix-sdk/src/authentication/qrcode/mod.rs Outdated Show resolved Hide resolved
@poljar poljar marked this pull request as ready for review May 27, 2024 10:59
@poljar poljar requested a review from a team as a code owner May 27, 2024 10:59
@poljar poljar requested review from bnjbvr and removed request for a team May 27, 2024 10:59
@bnjbvr bnjbvr requested review from Hywan and removed request for bnjbvr May 27, 2024 11:01
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback!

@poljar
Copy link
Contributor Author

poljar commented May 27, 2024

Thanks for addressing my feedback!

No no, thanks for the swift review.

Copy link

codecov bot commented May 27, 2024

Codecov Report

Attention: Patch coverage is 93.53846% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 83.65%. Comparing base (664a64b) to head (4d8bfbd).

Files Patch % Lines
...ates/matrix-sdk/src/authentication/qrcode/login.rs 89.74% 12 Missing ⚠️
...dk/src/authentication/qrcode/rendezvous_channel.rs 95.06% 4 Missing ⚠️
...ix-sdk/src/authentication/qrcode/secure_channel.rs 92.10% 3 Missing ⚠️
...atrix-sdk/src/authentication/qrcode/oidc_client.rs 96.87% 1 Missing ⚠️
crates/matrix-sdk/src/oidc/mod.rs 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3466      +/-   ##
==========================================
+ Coverage   83.28%   83.65%   +0.36%     
==========================================
  Files         248      254       +6     
  Lines       25272    25592     +320     
==========================================
+ Hits        21049    21408     +359     
+ Misses       4223     4184      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@poljar poljar enabled auto-merge (rebase) May 28, 2024 10:32
@poljar poljar merged commit 7dd08c3 into main May 28, 2024
38 checks passed
@poljar poljar deleted the poljar/qr-login-pr branch May 28, 2024 10:43
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