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 ↗︎
|
|
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.
livio-a
added a commit
that referenced
this pull request
May 28, 2024
# Which Problems Are Solved As already mentioned and (partially) fixed in #7992 we discovered, issues with v2 tokens that where obtained through an IDP, with passwordless authentication or with password authentication (wihtout any 2FA set up) using the v1 login for zitadel API calls - (Previous) authentication through an IdP is now correctly treated as auth method in case of a reauth even when the user is not redirected to the IdP - There were some cases where passwordless authentication was successfully checked but not correctly set as auth method, which denied access to ZITADEL API - Users with password and passwordless, but no 2FA set up which authenticate just wich password can access the ZITADEL API again Additionally while testing we found out that because of #7969 the login UI could completely break / block with the following error: `sql: Scan error on column index 3, name "state": converting NULL to int32 is unsupported (Internal)` # How the Problems Are Solved - IdP checks are treated the same way as other factors and it's ensured that a succeeded check within the configured timeframe will always provide the idp auth method - `MFATypesAllowed` checks for possible passwordless authentication - As with the v1 login, the token check now only requires MFA if the policy is set or the user has 2FA set up - UserAuthMethodsRequirements now always uses the correctly policy to check for MFA enforcement - `State` column is handled as nullable and additional events set the state to active (as before #7969) # Additional Changes - Console now also checks for 403 (mfa required) errors (e.g. after setting up the first 2FA in console) and redirects the user to the login UI (with the current id_token as id_token_hint) - Possible duplicates in auth methods / AMRs are removed now as well. # Additional Context - Bugs were introduced in #7822 and # and 7969 and only part of a pre-release. - partially already fixed with #7992 - Reported internally.
|
🎉 This PR is included in version 2.53.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
livio-a
added a commit
that referenced
this pull request
May 29, 2024
# Which Problems Are Solved Among others #7822 changed the event type of the `user.token.added` to `user.token.v2.added`. To make customers aware of this in case they use it for calculating DAU / MAU, resp. for an audit trail, we want to raise awareness. # How the Problems Are Solved Technical advisory to state the change. # Additional Changes None. # Additional Context Relates to #7822 Co-authored-by: Fabi <[email protected]>
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