-
Notifications
You must be signed in to change notification settings - Fork 50
BREAKING: implement Native OIDC as per MSC 3861 #2024
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
base: main
Are you sure you want to change the base?
BREAKING: implement Native OIDC as per MSC 3861 #2024
Conversation
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. |
Outdated review hintReview notice: So far, the default behavior of the updated EDIT: I adjusted the behavior of You can now easily use e.g. |
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 |
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. |
ff732a4
to
83f4ec1
Compare
Clients might require the following workaround for #2027 : #2027 (comment) |
261932d
to
0ae4e30
Compare
55bd288
to
2732c27
Compare
2732c27
to
c483116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c483116
to
895bb82
Compare
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:
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! |
9ad6df5
to
0c90f2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very draft review, will try to go through the whole flow and look at the client implementation after this
test/database_api_test.dart
Outdated
@@ -163,7 +163,6 @@ void main() { | |||
now, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to add tests for this?
@@ -0,0 +1,44 @@ | |||
import 'package:matrix/matrix.dart'; | |||
|
|||
extension Msc4191AccountManagementExtension on Client { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a little bit concerned that this is a draft msc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
..._oidc/msc2966_oidc_dynamic_client_registration/msc2966_oidc_dynamic_client_registration.dart
Show resolved
Hide resolved
...msc_extensions/msc_3861_native_oidc/msc2964_oidc_oauth_grants/msc2964_oidc_oauth_grants.dart
Show resolved
Hide resolved
...msc_extensions/msc_3861_native_oidc/msc2964_oidc_oauth_grants/msc2964_oidc_oauth_grants.dart
Show resolved
Hide resolved
f287859
to
48bb3b3
Compare
.../msc_3861_native_oidc/msc1597_matrix_identifier_syntax/msc1597_matrix_identifier_syntax.dart
Outdated
Show resolved
Hide resolved
.../msc_3861_native_oidc/msc1597_matrix_identifier_syntax/msc1597_matrix_identifier_syntax.dart
Show resolved
Hide resolved
import 'package:matrix/src/utils/crypto/crypto.dart'; | ||
|
||
extension OidcOauthGrantFlowExtension on Client { | ||
Future<void> oidcAuthorizationGrantFlow({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not happy with this method name. A flow is a thing, not an action but a method is an action so it would better to be more intuitive to use a verb here like: loginWithOidcAuthGrantFlow()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not called oidcLoginAuthorizationGrantFlow
. All methods related to OIDC use this prefix and I'd avoid having a gap word such as with
in a method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the major confusion comes from the work login : Using OIDC you do not "log in" using the matrix dart SDK but you login to your browser and use a token to authenticate. This process is simply not a login. That's why I prefer the technically accurate term oidcAuthorizationGrantFlow
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an example how to actually use this?
I added a README in the MSC folder for client developers to gather an example. |
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>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
Signed-off-by: The one with the braid <info@braid.business>
cff4fd8
to
8ab97e0
Compare
/// - [prompt]: the OAuth2.0 authorization prompt. This must be a valid prompt from the [getOidcDiscoveryInformation] as by https://openid.net/specs/openid-connect-prompt-create-1_0.html | ||
Future<void> oidcLoginAuthorizationGrantFlow({ | ||
required Completer<OidcCallbackResponse> nativeCompleter, | ||
required String oidcClientId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If oidc
is already part of the method name and there is no other clientId we can IMO just name this clientId
. afaik other APIs using OIDC treat it the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, renamed !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, renamed !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now contains 22 commits. Can you clean up the commit history please?
lib/src/database/database_api.dart
Outdated
|
||
Future<Map<String, Object?>?> getOidcAuthMetadata(); | ||
|
||
Future<void> storeOidcDynamicClientId(String? oidcClientId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the right approach to create methods for every single thing we store in the Client Database. This is now already messed up as we use getClient and updateClient to update single fields but now for other single fields we have methods. What about just adding one generic method getClientData(ClientData key)
and putClientData(ClientData key, value)
where ClientData
is an enum of all possible fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I migrated the String based key-value access on the client box contents to your proposed enum-based approach. I'd suggest to further extend this to a type safe approach including the values of the box but this is way beyond the scope of this OIDC-related PR.
Signed-off-by: The one with the braid <info@braid.business>
You can squash the commits as soon as you want to merge this PR. It does not make sense to rewrite the commit history before review is done. Squashing the commits is a task for the person merging this PR at the end. |
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:
Implements: