feat(bindings): OIDC in AuthenticationService. #2412

pixlwave · 2023-08-16T09:52:28Z

This PR builds on top of #1019 exposing OIDC to the bindings.

The authentication service can be configured for OIDC (performing dynamic registration) and supports login via OIDC.
The FFI client now converts Session to/from the OIDC Sessions when necessary.
UnknownToken errors are now broadcast from the refresh logic depending on the errors encountered.
SessionChanges are also broadcast to indicate that the app should update the session in whatever secure storage is being used.

Left todo in this PR:

Check if the account URL needs to be stored or is already part of the Session (for use later).
Detect cancellation from the WebView properly.
Return the URL for RP initiated sign out when calling logout().

bindings/matrix-sdk-ffi/src/authentication_service.rs

bindings/matrix-sdk-ffi/src/client.rs

pixlwave · 2023-08-16T11:11:17Z

bindings/matrix-sdk-ffi/src/client.rs

    /// Log out the current user
    pub fn logout(&self) -> Result<(), ClientError> {
-        RUNTIME.block_on(self.inner.matrix_auth().logout())?;
+        let Some(auth_api) = self.inner.auth_api() else {


This feels super weird to me that we don't call client.logout() and have it direct the logout request appropriately, but as client.logout() was removed since I first started this branch, I've left the logic here.

crates/matrix-sdk/src/client/mod.rs

pixlwave · 2023-08-16T11:21:47Z

crates/matrix-sdk/src/client/futures.rs

+                        }
+                        _ => {
+                            trace!("Token refresh: Token refresh failed.");
+                            return Err(refresh_error.into());


I presume there's also an error type for homeserver refresh that matches InvalidGrant to signal that the refresh was specifically denied. If so this should be handled here.

According to the spec:

If the token refresh fails and the error response included a soft_logout: true property, then the client can treat it as a soft logout and attempt to obtain a new access token by re-logging in. If the error response does not include a soft_logout: true property, the client should consider the user as being logged out.

So the error doesn't matter, it's always considered a logout. I'm wondering if we should do the same for OIDC, because if the refresh token fails endlessly no request will be able to be made.

I'm going to defer this one to @hughns who suggested we should only use InvalidGrant as the signal to logout the user if OIDC token refresh fails.

It's quite a while since I last discussed soft logout with @sandhose so my memory is hazy on it...

But, I suspect it is the case that Matrix's soft logout concept would no longer be possible.

Instead -

If a client founds itself with an invalid refresh token - most likely meaning that the session was signed out from another client or My Account UI - then it could ask the user whether they want to attempt re-authenticate or whether to Logout completely.

If the user asks to re-authenticate then you can reuse the existing device ID in the urn:matrix:client:device:XXX scope in the authorization request.

If the user says Logout completely then you would destroy all the local data.

Ok, so for now, using InvalidGrant is correct for OIDC. Given that native refresh is going to sign the user out on any error in the current implementation, I'm just going to update this PR to do the same with a note that it isn't correct.

- Use OIDC for logout when appropriate. - Handle token refresh through OIDC too. - Support for server discovery over http. - Allow server's that support OIDC but not passwords to work. - Only sign out users if token refresh is explicitly refused. - Rebase on latest zecakeh/oidc-mas # Conflicts: # crates/matrix-sdk/src/client/mod.rs # crates/matrix-sdk/src/oidc/mod.rs

pixlwave · 2023-08-16T17:52:50Z

Looks like I'm missing some feature checks, job for tomorrow.

codecov · 2023-08-16T18:17:38Z

Codecov Report

Patch coverage: 28.08% and project coverage change: -0.17% ⚠️

Comparison is base (0dac508) 77.06% compared to head (839e795) 76.89%.
Report is 44 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2412      +/-   ##
==========================================
- Coverage   77.06%   76.89%   -0.17%     
==========================================
  Files         181      181              
  Lines       19135    19186      +51     
==========================================
+ Hits        14746    14754       +8     
- Misses       4389     4432      +43

Files Changed	Coverage Δ
crates/matrix-sdk/src/lib.rs	`100.00% <ø> (ø)`
crates/matrix-sdk/src/oidc/data_serde.rs	`0.00% <ø> (ø)`
crates/matrix-sdk/src/oidc/mod.rs	`0.00% <0.00%> (ø)`
crates/matrix-sdk/src/client/mod.rs	`75.51% <18.36%> (-4.99%)`	⬇️
crates/matrix-sdk/src/client/futures.rs	`50.00% <38.88%> (-31.82%)`	⬇️
crates/matrix-sdk/src/matrix_auth/mod.rs	`88.28% <100.00%> (+0.21%)`	⬆️

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

pixlwave · 2023-08-17T12:26:29Z

Feature checks added, although I'm not sure I understand the CI failures, around error: unused import: error::ErrorKind. That was already present before this PR 🤔

zecakeh

I tried to do a thorough review, though mostly focusing on interacting with the OIDC API

bindings/matrix-sdk-ffi/src/authentication_service.rs

bindings/matrix-sdk-ffi/src/client.rs

crates/matrix-sdk/src/client/mod.rs

zecakeh

One error I didn't see before, and a few nitpicks

bindings/matrix-sdk-ffi/src/authentication_service.rs

bindings/matrix-sdk-ffi/src/client.rs

bnjbvr

Excited to have OIDC soon in ElementX; I have a few questions below.

Also, the FFI layer seems to do a lot, especially in authentication_service.rs. The role of the FFI layer is to fallibly convert data types, then transmit the data to the SDK itself for processing, then convert it back to looser types that the FFI can handle. If a SDK user wanted to handle OIDC, would they have to duplicate most of the logic of the FFI layer? If so, could we instead put some of that code in the non-FFI layer, please?

bindings/matrix-sdk-ffi/src/client_builder.rs

crates/matrix-sdk/src/client/mod.rs

bnjbvr · 2023-08-18T14:30:47Z

bindings/matrix-sdk-ffi/src/client.rs

    // FFI-only fields (for now)
+    /// The URL for the homeserver used for this session.
    pub homeserver_url: String,
+    /// Additional data for this session session if OpenID Connect was used


nit: session session

Also, this is a string that contains a serialized JSON blob, breaking the FFI layer boundary by allowing stringly typed data.

A few questions (very uninformed, as I don't know much about OIDC at all):

does it need to be stored, or could it be transient and passed as a function argument somewhere?

if it does need to be stored, could it be stored as the final deserialized format instead of JSON?

On the stringly typed data and your questions:

Yes it needs to be stored, it contains additional OIDC related data that is required for the app to restore a session correctly, so storing it alongside everything else makes the most sense to me.

The app has absolutely no interest in the contents of this struct. If we make it a struct, we would need to a) convert all these properties to types that Uniffi is happy to bridge and b) Then implement Codable conformances on the Swift side (and presumably similar on the Kotlin side) which seems like a lot of unnecessary work given we aren't interested in the data.

Remove check for Consent prompt. MAS supports it but isn't currently exposing that in it's well-known.

jplatte

This PR contains non-trivial amounts of matrix-sdk changes alongside the FFI changes. Could you please move those out into a separate PR?

bindings/matrix-sdk-ffi/src/client.rs

bindings/matrix-sdk-ffi/src/authentication_service.rs

pixlwave commented Aug 16, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/authentication_service.rs Outdated Show resolved Hide resolved

pixlwave commented Aug 16, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/client.rs Show resolved Hide resolved

pixlwave commented Aug 16, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/client.rs Show resolved Hide resolved

pixlwave force-pushed the doug/oidc-ffi branch from 6ae456c to 8034ee4 Compare August 16, 2023 11:09

pixlwave commented Aug 16, 2023

View reviewed changes

crates/matrix-sdk/src/client/mod.rs Show resolved Hide resolved

pixlwave commented Aug 16, 2023

View reviewed changes

pixlwave force-pushed the doug/oidc-ffi branch from 8034ee4 to de21126 Compare August 16, 2023 11:22

fix(bindings): Correct detection of OIDC cancellation.

be9dbe1

pixlwave force-pushed the doug/oidc-ffi branch from e3f0c3f to be9dbe1 Compare August 16, 2023 16:35

chore(bindings): Expose the OIDC account URL.

e2a4d46

pixlwave marked this pull request as ready for review August 16, 2023 17:40

pixlwave requested a review from a team as a code owner August 16, 2023 17:40

pixlwave requested review from poljar and removed request for a team August 16, 2023 17:40

feat(bindings): Support for RP initiated logout.

2a11150

pixlwave force-pushed the doug/oidc-ffi branch from 7df2b28 to 2a11150 Compare August 16, 2023 17:54

pixlwave force-pushed the doug/oidc-ffi branch from 1f2f702 to a765b3c Compare August 17, 2023 11:31

fix(oidc): Add missing feature checks.

05f626e

pixlwave force-pushed the doug/oidc-ffi branch from a765b3c to 05f626e Compare August 17, 2023 12:03

pixlwave requested review from a team and bnjbvr and removed request for a team and poljar August 17, 2023 12:23

zecakeh reviewed Aug 18, 2023

View reviewed changes

crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved

zecakeh reviewed Aug 18, 2023

View reviewed changes

crates/matrix-sdk/src/client/mod.rs Show resolved Hide resolved

fix(bindings): PR comments.

9026dbc

pixlwave force-pushed the doug/oidc-ffi branch from aa83a9f to 9026dbc Compare August 18, 2023 11:20

zecakeh reviewed Aug 18, 2023

View reviewed changes

fix(bindings): PR comments.

2b85735

bnjbvr requested changes Aug 18, 2023

View reviewed changes

Address more PR comments.

690da73

Remove check for Consent prompt. MAS supports it but isn't currently exposing that in it's well-known.

pixlwave force-pushed the doug/oidc-ffi branch from f01162d to 690da73 Compare August 21, 2023 09:09

Add unknown token broadcast back for native token refresh.

efd4439

pixlwave requested a review from bnjbvr August 21, 2023 11:51

jplatte reviewed Aug 21, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/client.rs Outdated Show resolved Hide resolved

bindings/matrix-sdk-ffi/src/authentication_service.rs Outdated Show resolved Hide resolved

bindings/matrix-sdk-ffi/src/authentication_service.rs Outdated Show resolved Hide resolved

Handle more errors.

839e795

jplatte enabled auto-merge (squash) August 22, 2023 09:01

jplatte approved these changes Aug 22, 2023

View reviewed changes

bnjbvr approved these changes Aug 22, 2023

View reviewed changes

jplatte merged commit 60392c2 into main Aug 22, 2023

jplatte deleted the doug/oidc-ffi branch August 22, 2023 09:06

pixlwave mentioned this pull request Aug 24, 2023

Add support for login with OIDC. #859

Closed

turt2live mentioned this pull request Jan 25, 2025

MSC2966: Usage of OAuth 2.0 Dynamic Client Registration in Matrix matrix-org/matrix-spec-proposals#2966

Merged

feat(bindings): OIDC in AuthenticationService. #2412

feat(bindings): OIDC in AuthenticationService. #2412

Uh oh!

Conversation

pixlwave commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pixlwave Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pixlwave Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

zecakeh Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pixlwave Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughns Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

pixlwave Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

pixlwave commented Aug 16, 2023

Uh oh!

codecov bot commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pixlwave commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zecakeh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zecakeh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnjbvr Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

pixlwave Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

jplatte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pixlwave commented Aug 16, 2023 •

edited

Loading

zecakeh Aug 18, 2023 •

edited

Loading

pixlwave Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading

pixlwave commented Aug 17, 2023 •

edited

Loading

zecakeh left a comment •

edited

Loading