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

feat(auth): get rid of the cargo feature flag for OIDC 🔥 #4635

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Feb 6, 2025

OIDC authentication has been used in production in multiple embeddings of the Matrix Rust SDK, some of them for months already, and they're considered stable for everyday use. As such, the feature is not considered experimental anymore, especially since the future of authentication will rely on OIDC and related mechanisms.


The cross-process lock used for OIDC relies on the encryption crate, which implements a cross-process locking mechanism. So lots of code have to be guarded against the e2e-encryption feature flag, as we need to have access to the crypto features to enable the cross-process lock, unfortunately. This dependency should go away as soon as we generalize the concept of the cross-process lock, so it doesn't make use of the crypto store for that purpose.

OIDC authentication has been used in production in multiple embeddings
of the Matrix Rust SDK, some of them for months already, and they're
considered stable for everyday use. As such, the feature is not
considered experimental anymore, especially since the future of
authentication will rely on OIDC and related mechanisms.
@bnjbvr bnjbvr requested a review from a team as a code owner February 6, 2025 14:18
@bnjbvr bnjbvr requested review from Hywan and removed request for a team February 6, 2025 14:18
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.73%. Comparing base (ce44c6e) to head (787985d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ates/matrix-sdk/src/authentication/qrcode/login.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4635   +/-   ##
=======================================
  Coverage   85.73%   85.73%           
=======================================
  Files         292      292           
  Lines       33492    33492           
=======================================
+ Hits        28715    28716    +1     
+ Misses       4777     4776    -1     

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

@bnjbvr bnjbvr changed the title feat(auth): get rid of the cargo feature flag for OIDC feat(auth): get rid of the cargo feature flag for OIDC 🔥 Feb 6, 2025
@bnjbvr
Copy link
Member Author

bnjbvr commented Feb 6, 2025

Now trying to pinpoint which previously-optional dependency requires to compile udp on wasm…

@bnjbvr
Copy link
Member Author

bnjbvr commented Feb 6, 2025

It's mas-oidc-client which is the culprit here. Unfortunately, quick experiments to make it compatible by removing features is unfruitful, because it's using a different version of tracing that's not our special patched version :/ Will investigate further next week.

@zecakeh
Copy link
Collaborator

zecakeh commented Feb 7, 2025

There is #4593, fwiw.

@zecakeh
Copy link
Collaborator

zecakeh commented Feb 9, 2025

It might also be a little premature to remove the feature, given that it doesn't implement the latest versions of the MSCs (#4550 and downgrading to OAuth 2.0 instead of OIDC).

@bnjbvr
Copy link
Member Author

bnjbvr commented Feb 11, 2025

It might also be a little premature to remove the feature, given that it doesn't implement the latest versions of the MSCs (#4550 and downgrading to OAuth 2.0 instead of OIDC).

We discuss it internally, and we still think it's fine to get rid of the feature. Our implementation is quite stable and "production-ready", in that it's used in the ElementX apps. While it's also been constantly evolving for years, this is the fate of such a feature, and changes are expected to happen slowly and in a non-disruptive manner, which is a sufficient sign of maturity.


On the other hand, getting rid of the mas-oidc-client crate might be a good justification to hold off, since 1. it prevents compilation on wasm targets at the moment, and it's unclear it's worth investing into making it compile there, 2. there's been discussions about not using it, because it might not be well maintained and support all the features we need/use in the medium term.

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