BREAKING: implement Native OIDC as per MSC 3861 #2024

TheOneWithTheBraid · 2025-02-07T18:21:24Z

BREAKING: changes to the database API to better reflect the OIDC state

This implementation is based on the draft of native OIDC I implemented in < polycule >. It can be found here. Unlike in my initial draft, this implementation of course does not rely on any Flutter package but ships a minimal OIDC state machine relying on the matrix client to provide the native callback result via a Completer.

This implementation does not aim to comply to the full OIDC standard but only the subset required as per the MSCs stated below.

This PR does not aim to implement OIDC device flow as per MSC 4108 nor the changes for the application services as per MSC 4190.

Roadmap:

initial viable implementation
migrate the < polycule > implementation to this OIDC stack
excessive testing
figure out a test concept

Implements:

TheOneWithTheBraid · 2025-02-07T19:04:16Z

A comment on the changes to the database API:

I initially intended to just keep all OIDC-related state (device ID, OAuth2.0 client ID, auth metadata) in memory until the OIDC flow completes. I sadly figured out the Dart process might get killed by the platform for battery optimization. In order to complete the flow after the main process was killed, I decided to store the OAuth2 client ID and the homeserver's auth metadata in the database as well as to split the device ID from the remaining client store since now the device ID is present a long time before the rest of the account is available.

Even though I so far already implemented the database API changes with this in mind, there is no way to resume an OIDC flow from a new thread yet.

TheOneWithTheBraid · 2025-02-07T22:29:09Z

Outdated review hint

Review notice: So far, the default behavior of the updated Client.getWellknown is to only check the brand new /auth_metadata endpoint. This might require Synapse nightly builds to work. Client developers however are free to use the previous revision of the MSC suggesting to use the /auth_issuer endpoint or the .well-known entry containing the org.matrix.msc2965.authentication field. All three possible implementations are supported in this PR. Since the default is definitely going to be the new /auth_metadata, the implementation already suggests this at this point.

EDIT: I adjusted the behavior of Client.getWellknown to cope with all three permitted approaches of OIDC discovery. The implementation will therefore work with both stable Synapse releases as well as cutting edge Synapse builds already using the newest approach.

You can now easily use e.g. synapse-oidc.element.dev for testing.

TheOneWithTheBraid · 2025-02-09T13:30:52Z

The < polycule > side implementation is now merged and works like a charm. Maybe that's helpful for testing this PR in real world: https://gitlab.com/polycule_client/polycule/-/merge_requests/245

TheOneWithTheBraid · 2025-02-10T10:11:14Z

Since I unaskedly opened this PR, I'd kindly offer to take care of the OIDC code's maintenance in future too. You folks all have my MXID and can always poke me if there are future issues about the OIDC code.

TheOneWithTheBraid · 2025-02-11T15:33:41Z

Clients might require the following workaround for #2027 : #2027 (comment)

lib/encryption/utils/bootstrap.dart

TheOneWithTheBraid

Is there any update on review of this PR ? It's now been months and months of waiting and I still did not even get a response on whether you are actually willing to ever accept this contribution. Instead only vaguely related comments.

lib/encryption/utils/bootstrap.dart

td-famedly · 2025-05-19T16:01:39Z

helo :3 sorry for the delay. I will now start reading the msc(s) and try to get back to this asap!

(still expecting a marklin set on my bday!)

edit:

It's now been months and months of waiting and I still did not even get a response on whether you are actually willing to ever accept this contribution. Instead only vaguely related comments.

Sorry for the delay, everyone internally has been super busy with "work". I have not heard anything about not accepting this contribution so I will review it with the end goal of getting it merged asap! Again thank you very much for the contribution and sorry for the delay!

td-famedly

very draft review, will try to go through the whole flow and look at the client implementation after this

lib/src/database/matrix_sdk_database.dart

lib/src/client.dart

td-famedly · 2025-05-20T11:38:51Z

test/database_api_test.dart

@@ -163,7 +163,6 @@ void main() {
          now,


would it be possible to add tests for this?

td-famedly · 2025-05-20T12:17:36Z

lib/src/client.dart

+        refreshTokenLifetimeMs: customRefreshTokenLifetime?.inMilliseconds,
+      );
+
+      this.accessToken = accessToken = tokenResponse.accessToken;


nitpick repeated bit, would it be possible to deduplicate it?

We can't deduplicate this without adding a ! null check we can avoid. Better variables duplicated than unsound null safety.

td-famedly · 2025-05-20T12:21:39Z

...c_extensions/msc_3861_native_oidc/msc4191_account_management/msc4191_account_management.dart

@@ -0,0 +1,44 @@
+import 'package:matrix/matrix.dart';
+
+extension Msc4191AccountManagementExtension on Client {


only a little bit concerned that this is a draft msc

Yeah, it's a draft but a) it doesn't break anything and b) it's so much of a simple MSC, I don't see much regression in it - even if we should happen to adjust the behavior later in time.

td-famedly · 2025-05-20T12:26:26Z

..._oidc/msc2966_oidc_dynamic_client_registration/msc2966_oidc_dynamic_client_registration.dart

+  }) {
+    return OidcDynamicRegistrationData(
+      clientName: {
+        null: defaultLocale.clientName,


nitpick but having a map key as null doesn't really look right. Would {clientName: locale?} be better here? A little wrapper class would probably be a bit cleaner

Hmmmm, a null key in the map is sadly exactly what the Dart Uir class expects for its queryParameters property. I do not see need for introducing a new helper class just in order to later map it to a Uri.queryParameters instance again just because the syntax of this parameter is a bit weird. What would be the benefit for end users ? They won't touch this part of the code anyway since they only provide the parameters the SDK will later write into the Map.

td-famedly · 2025-05-20T12:32:19Z

...msc_extensions/msc_3861_native_oidc/msc2964_oidc_oauth_grants/msc2964_oidc_oauth_grants.dart

+      authEndpoint = Uri.parse(authData['authorization_endpoint'] as String);
+      tokenEndpoint = Uri.parse(authData['token_endpoint'] as String);
+      // ensure we only hand over permitted prompts
+      if (prompt != null) {


If the prompt doesn't match, do we want to continue?

Yes. A client might request a prompt the server does not know. Using this handler here, the authentication server will fall back on the default selection showing all possible ways to proceed. The only behavior we need to avoid is passing an invalid prompt since this would cause the authentication server to show an error screen.

(E.g. a client could hard-code the two prompts login and register. In this case the SDK takes care of not prompting an invalid option to the MAS if e.g. registration is disabled.)

In fact, it's a pure convenience feature making client implementation easier and more reliable.

td-famedly · 2025-05-20T12:40:07Z

...msc_extensions/msc_3861_native_oidc/msc2964_oidc_oauth_grants/msc2964_oidc_oauth_grants.dart

+        'state': state,
+        if (prompt != null) 'prompt': prompt,
+        'code_challenge':
+            // remove the "=" padding


Can you make sure to remove ALL the padding?

Dart's base64 codec only adds the trailing = as padding, doesn't it ? Any suggestions how to better handle the padding ? I'm a bit clueless on how to do this without over engineering the implementation. Help appreciated.

Implements: - MSC 3861 - Next-generation auth for Matrix, based on OAuth 2.0/OIDC - MSC 1597 - Better spec for matrix identifiers - MSC 2964 - Usage of OAuth 2.0 authorization code grant and refresh token grant - MSC 2965 - OAuth 2.0 Authorization Server Metadata discovery - MSC 2966 - Usage of OAuth 2.0 Dynamic Client Registration in Matrix - MSC 2967 - API scopes - MSC 3824 - OIDC aware clients - MSC 4191 - Account management deep-linking Signed-off-by: The one with the braid <info@braid.business>

Signed-off-by: The one with the braid <info@braid.business>

…i to client Signed-off-by: The one with the braid <info@braid.business>

Signed-off-by: The one with the braid <info@braid.business>

krille-chan · 2025-07-14T12:09:47Z

lib/src/database/database_api.dart

+
+  Future<void> storeOidcDynamicClientId(String? oidcClientId);
+
+  Future<String?> getOidcDynamicClientId();


I don't think we need this. Just use getClient() and pick what you need from it. To have it type safe we could later migrate it to returning a Dart Record.
Yeah that would fetch more data than necessary but I would prefer this over having the database API even more complex.

krille-chan · 2025-07-14T12:11:03Z

.../msc_3861_native_oidc/msc1597_matrix_identifier_syntax/msc1597_matrix_identifier_syntax.dart

+  /// MSC 2964 & MSC 2967
+  Future<String> oidcEnsureDeviceId([bool enforceNewDevice = false]) async {
+    if (!enforceNewDevice) {
+      final storedDeviceId = await database?.getDeviceId();


Why do we need the stored device ID and can't just use Client.deviceID? It should be the same

krille-chan · 2025-07-14T12:11:38Z

.../msc_3861_native_oidc/msc1597_matrix_identifier_syntax/msc1597_matrix_identifier_syntax.dart

+
+extension GenerateDeviceIdExtension on Client {
+  /// MSC 2964 & MSC 2967
+  Future<String> oidcEnsureDeviceId([bool enforceNewDevice = false]) async {


Can you add more comments what this does? The method name is super unintuitive. Links to the MSCs would also be more helpful than MSC numbers.

krille-chan · 2025-07-14T12:12:48Z

...msc_extensions/msc_3861_native_oidc/msc2964_oidc_oauth_grants/msc2964_oidc_oauth_grants.dart

+import 'package:matrix/src/utils/crypto/crypto.dart';
+
+extension OidcOauthGrantFlowExtension on Client {
+  Future<void> oidcAuthorizationGrantFlow({


The method name says nothing about what it does? No idea what this is flow. Please switch this to a proper name with dart docs

krille-chan · 2025-07-14T12:21:37Z

lib/src/client.dart

@@ -2134,6 +2194,13 @@ class Client extends MatrixApi {
      _discoveryDataLoading = database.getWellKnown().then((data) {
        _wellKnown = data;
      });
+      _oidcAuthMetadataLoading = database.getOidcAuthMetadata().then((data) {


I doubt that this really needs to be waited for. Can't we just make it part of the Database.getClient call?

krille-chan

Looks quite good so far, though I'm not that into the MSCs of Matrix. Will test out if it works with oidc login on FluffyChat soon...

krille-chan

Can you add an example how to actually use this?

TheOneWithTheBraid marked this pull request as ready for review February 9, 2025 11:56

TheOneWithTheBraid requested a review from a team as a code owner February 9, 2025 11:56

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch 3 times, most recently from ff732a4 to 83f4ec1 Compare February 11, 2025 15:33

TheOneWithTheBraid mentioned this pull request Feb 13, 2025

feat: add refresh token handling http client #2033

Open

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch from 261932d to 0ae4e30 Compare February 15, 2025 12:31

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch 2 times, most recently from 55bd288 to 2732c27 Compare March 13, 2025 07:03

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch from 2732c27 to c483116 Compare April 2, 2025 08:17

krille-chan reviewed May 9, 2025

View reviewed changes

lib/encryption/utils/bootstrap.dart Show resolved Hide resolved

TheOneWithTheBraid commented May 15, 2025

View reviewed changes

lib/encryption/utils/bootstrap.dart Show resolved Hide resolved

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch from c483116 to 895bb82 Compare May 15, 2025 11:34

td-famedly self-requested a review May 19, 2025 16:00

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch from 9ad6df5 to 0c90f2b Compare May 20, 2025 08:54

td-famedly reviewed May 20, 2025

View reviewed changes

TheOneWithTheBraid added 8 commits June 12, 2025 09:37

fix: wrong response_type parameter

ff058ca

Signed-off-by: The one with the braid <info@braid.business>

feat: implement a callback to hand over the OAuth2.0 authorization ur…

031de19

…i to client Signed-off-by: The one with the braid <info@braid.business>

feat: add parameter to enforce new dynamic client registration

e795ed5

Signed-off-by: The one with the braid <info@braid.business>

fix: still fetch auth metadata when well known is not served

2250d13

Signed-off-by: The one with the braid <info@braid.business>

feat: improve documentation

9baaab3

Signed-off-by: The one with the braid <info@braid.business>

fix: include scope into oauth2 redirect

b08a7c7

Signed-off-by: The one with the braid <info@braid.business>

fix: revert unwanted change in Client

81327c1

Signed-off-by: The one with the braid <info@braid.business>

TheOneWithTheBraid added 11 commits June 12, 2025 09:37

fix: properly calculate PKCE for OAuth2 token exchange

2b78575

Signed-off-by: The one with the braid <info@braid.business>

fix: correctly assign request form body for OAuth2 token requests

9a1238f

Signed-off-by: The one with the braid <info@braid.business>

fix: migration of device ID to individual database entry

ed68e6e

Signed-off-by: The one with the braid <info@braid.business>

chore: better parse redirect uri query

d506fa4

Signed-off-by: The one with the braid <info@braid.business>

feat: support all possible methods for OIDC discovery

11a1975

Signed-off-by: The one with the braid <info@braid.business>

fix: return .well-known after checking OIDC capabilities

2d16b7e

Signed-off-by: The one with the braid <info@braid.business>

feat: separate OIDC discovery from .well-known

0d064bb

Signed-off-by: The one with the braid <info@braid.business>

fix: avoid rate limits in Bootstrap.askSetupCrossSigning

c25b499

Signed-off-by: The one with the braid <info@braid.business>

fix: ensure to store device IDs for legacy login

dee240c

Signed-off-by: The one with the braid <info@braid.business>

fix: non-nullable database in Client.init

d78c4f3

Signed-off-by: The one with the braid <info@braid.business>

fix: clean up bad version response

48bb3b3

Signed-off-by: The one with the braid <info@braid.business>

TheOneWithTheBraid force-pushed the braid/msc3861-native-oidc branch from f287859 to 48bb3b3 Compare June 12, 2025 07:37

krille-chan reviewed Jul 14, 2025

View reviewed changes

krille-chan requested changes Jul 14, 2025

View reviewed changes

		@@ -0,0 +1,44 @@
		import 'package:matrix/matrix.dart';

		extension Msc4191AccountManagementExtension on Client {


		Future<void> storeOidcDynamicClientId(String? oidcClientId);

		Future<String?> getOidcDynamicClientId();

BREAKING: implement Native OIDC as per MSC 3861 #2024

Are you sure you want to change the base?

BREAKING: implement Native OIDC as per MSC 3861 #2024

Conversation

TheOneWithTheBraid commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheOneWithTheBraid commented Feb 7, 2025

Uh oh!

TheOneWithTheBraid commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheOneWithTheBraid commented Feb 9, 2025

Uh oh!

TheOneWithTheBraid commented Feb 10, 2025

Uh oh!

TheOneWithTheBraid commented Feb 11, 2025

Uh oh!

Uh oh!

TheOneWithTheBraid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

td-famedly commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

td-famedly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheOneWithTheBraid May 21, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krille-chan Jul 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

krille-chan Jul 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krille-chan left a comment

Choose a reason for hiding this comment

Uh oh!

krille-chan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheOneWithTheBraid commented Feb 7, 2025 •

edited

Loading

TheOneWithTheBraid commented Feb 7, 2025 •

edited

Loading

td-famedly commented May 19, 2025 •

edited

Loading

TheOneWithTheBraid May 21, 2025 •

edited

Loading

krille-chan Jul 14, 2025 •

edited

Loading

krille-chan Jul 14, 2025 •

edited

Loading