fix(login): improve auth handlers #7969
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 ↗︎
|
Thanks for your contribution! 🎉Please make sure you tick the following checkboxes before marking this Pull Request (PR) as ready for review:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7969 +/- ##
==========================================
- Coverage 62.75% 62.45% -0.30%
==========================================
Files 1341 1342 +1
Lines 111029 110812 -217
==========================================
- Hits 69672 69213 -459
- Misses 37431 37721 +290
+ Partials 3926 3878 -48
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
livio-a
changed the title
adlerhurst
requested changes
May 22, 2024
internal/auth/repository/eventsourcing/handler/refresh_token.go
Outdated
Show resolved
Hide resolved
adlerhurst
reviewed
May 22, 2024
adlerhurst
previously approved these changes
May 22, 2024
adlerhurst
approved these changes
May 22, 2024
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 A customer noted that after upgrade to 2.53.0, users were no longer able to reset their passwords through the login UI. This was due to a accidental change in #7969 # How the Problems Are Solved The `preferred_login_name` is now correctly read from the database. # Additional Changes None. # Additional Context relates to #7969
livio-a
added a commit
that referenced
this pull request
May 29, 2024
# Which Problems Are Solved A customer noted that after upgrade to 2.53.0, users were no longer able to reset their passwords through the login UI. This was due to a accidental change in #7969 # How the Problems Are Solved The `preferred_login_name` is now correctly read from the database. # Additional Changes None. # Additional Context relates to #7969 (cherry picked from commit eca8ffd)
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.
Which Problems Are Solved
During the implementation of #7486 it was noticed, that projections in the
authdatabase schema could be blocked.Investigations suggested, that this is due to the use of GORM and it's inability to use an existing (sql) transaction.
With the improved / simplified handling (see below) there should also be a minimal improvement in performance, resp. reduced database update statements.
How the Problems Are Solved
The handlers in
authare exchanged to proper (sql) statements and gorm usage is removed for any writing part.To further improve / simplify the handling of the users, a new
auth.users3table is created, where only attributes are handled, which are not yet available from theprojections.users,projections.login_nameandprojections.user_auth_methodsdo not provide. This reduces the events handled in that specific handler by a lot.Additional Changes
None
Additional Context
relates to #7486