perf(oidc): optimize token creation #7822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7822 +/- ##
=======================================
Coverage ? 62.75%
=======================================
Files ? 1341
Lines ? 111030
Branches ? 0
=======================================
Hits ? 69679
Misses ? 37426
Partials ? 3925
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Some bugs found during manual tests:
|
livio-a
approved these changes
May 16, 2024
muhlemmer
added a commit
that referenced
this pull request
May 23, 2024
# Which Problems Are Solved After #7822 was merged we discovered that v2 tokens that where obtained through an IDP using the v1 login, can't be used for zitadel API calls. - Because we used to store the AMR claim on the auth request, but internally use the domain.UserAuthMethod type. AMR has no notion of an IDP login, so that "factor" was lost during conversion. Rendering those v2 tokens invalid on the zitadel API. - A wrong check on machine user tokens falsly allowed some tokens to be valid - The client ID was set to tokens from client credentials and JWT profile, which made client queries fail in the validation middleware. The middleware expects client ID unset for machine users. # How the Problems Are Solved Store the domain.AuthMethods directly in the auth requests and session, instead of using AMR claims with lossy conversion. - IDPs have seperate auth method, which is not an AMR claim - Machine users are treated specialy, eg auth methods are not required. - Do not set the client ID for client credentials and JWT profile # Additional Changes Cleaned up mostly unused `oidc.getInfoFromRequest()`. # Additional Context - Bugs were introduced in #7822 and not yet part of a release. - Reported internally.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Optimize token creation by managing creation directly through the command package.
This minimizes the amount of calls that need to made to the database. The old implementation made repeated calls through OIDC's Storage interface. In this PR we attempt to resolve all business logic in the command package, with a single query of related write models.
This optimization migrates old repository tokens to the v2 token implementation already used by the OIDC session API. The "old" token repository is kept for now, so that after a zitadel upgrade old tokens can still be refreshed. During a refresh of a "v1" token a "v2" token will now be returned. In theory we should be able to remove the token repository related code for the release after this one.
Users that query the event API to aggregate user activity, eg Daily Active Users, should be aware that there is a new event type
user.token.v2.added. The legacy eventuser.token.addedstill exists, but new events of this type will no longer be created after ZITADEL is upgraded.Note that due to some small changes in device authorization events, it is possible that device authorization flows that where started and still in progress during an upgrade, might fail.
Definition of Ready