From f20539ef8f4784ff59beea922a7b95ba1d6e0926 Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Fri, 13 Dec 2024 12:37:20 +0100 Subject: [PATCH] fix(login): make sure first email verification is done before MFA check (#9039) # Which Problems Are Solved During authentication in the login UI, there is a check if the user's MFA is already checked or needs to be setup. In cases where the user was just set up or especially, if the user was just federated without a verified email address, this can lead to the problem, where OTP Email cannot be setup as there's no verified email address. # How the Problems Are Solved - Added a check if there's no verified email address on the user and require a mail verification check before checking for MFA. Note: that if the user had a verified email address, but changed it and has not verified it, they will still be prompted with an MFA check before the email verification. This is make sure, we don't break the existing behavior and the user's authentication is properly checked. # Additional Changes None # Additional Context - closes https://github.com/zitadel/zitadel/issues/9035 --- .../eventsourcing/eventstore/auth_request.go | 9 +++ .../eventstore/auth_request_test.go | 56 +++++++++++++++++-- internal/user/model/user_view.go | 1 + internal/user/repository/view/model/user.go | 2 + internal/user/repository/view/user_by_id.sql | 4 ++ 5 files changed, 66 insertions(+), 6 deletions(-) diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request.go b/internal/auth/repository/eventsourcing/eventstore/auth_request.go index e098350c07..e35e7b5143 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request.go @@ -1081,6 +1081,15 @@ func (repo *AuthRequestRepo) nextSteps(ctx context.Context, request *domain.Auth } } + // If the user never had a verified email, we need to verify it. + // This prevents situations, where OTP email is the only MFA method and no verified email is set. + // If the user had a verified email, but change it and has not yet verified the new one, we'll verify it after we checked the MFA methods. + if user.VerifiedEmail == "" && !user.IsEmailVerified { + return append(steps, &domain.VerifyEMailStep{ + InitPassword: !user.PasswordSet && len(idps.Links) == 0, + }), nil + } + step, ok, err := repo.mfaChecked(userSession, request, user, isInternalLogin && len(request.LinkingUsers) == 0) if err != nil { return nil, err diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go index ccd53e06a1..dda8c54872 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go @@ -156,6 +156,7 @@ type mockViewUser struct { PasswordChanged time.Time PasswordChangeRequired bool IsEmailVerified bool + VerifiedEmail string OTPState int32 MFAMaxSetUp int32 MFAInitSkipped time.Time @@ -222,6 +223,7 @@ func (m *mockViewUser) UserByID(context.Context, string, string) (*user_view_mod PasswordSet: m.PasswordSet, PasswordChangeRequired: m.PasswordChangeRequired, IsEmailVerified: m.IsEmailVerified, + VerifiedEmail: m.VerifiedEmail, OTPState: m.OTPState, MFAMaxSetUp: m.MFAMaxSetUp, MFAInitSkipped: m.MFAInitSkipped, @@ -1403,6 +1405,7 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { PasswordVerification: testNow.Add(-5 * time.Minute), }, userViewProvider: &mockViewUser{ + VerifiedEmail: "verified", PasswordSet: true, PasswordlessTokens: user_view_model.WebAuthNTokens{&user_view_model.WebAuthNView{ID: "id", State: int32(user_model.MFAStateReady)}}, OTPState: int32(user_model.MFAStateReady), @@ -1439,9 +1442,10 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { PasswordVerification: testNow.Add(-5 * time.Minute), }, userViewProvider: &mockViewUser{ - PasswordSet: true, - OTPState: int32(user_model.MFAStateReady), - MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + VerifiedEmail: "verified", + PasswordSet: true, + OTPState: int32(user_model.MFAStateReady), + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), }, userEventProvider: &mockEventUser{}, orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, @@ -1469,6 +1473,45 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { }, { "external user, mfa not verified, mfa check step", + fields{ + userSessionViewProvider: &mockViewUserSession{ + PasswordVerification: testNow.Add(-5 * time.Minute), + ExternalLoginVerification: testNow.Add(-5 * time.Minute), + }, + userViewProvider: &mockViewUser{ + VerifiedEmail: "verified", + PasswordSet: true, + OTPState: int32(user_model.MFAStateReady), + MFAMaxSetUp: int32(domain.MFALevelSecondFactor), + }, + userEventProvider: &mockEventUser{}, + orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, + lockoutPolicyProvider: &mockLockoutPolicy{ + policy: &query.LockoutPolicy{ + ShowFailures: true, + }, + }, + idpUserLinksProvider: &mockIDPUserLinks{}, + }, + args{ + &domain.AuthRequest{ + UserID: "UserID", + SelectedIDPConfigID: "IDPConfigID", + LoginPolicy: &domain.LoginPolicy{ + AllowUsernamePassword: true, + SecondFactors: []domain.SecondFactorType{domain.SecondFactorTypeTOTP}, + PasswordCheckLifetime: 10 * 24 * time.Hour, + ExternalLoginCheckLifetime: 10 * 24 * time.Hour, + SecondFactorCheckLifetime: 18 * time.Hour, + }, + }, false}, + []domain.NextStep{&domain.MFAVerificationStep{ + MFAProviders: []domain.MFAType{domain.MFATypeTOTP}, + }}, + nil, + }, + { + "external user, mfa not verified, email never verified, email verification step", fields{ userSessionViewProvider: &mockViewUserSession{ PasswordVerification: testNow.Add(-5 * time.Minute), @@ -1500,8 +1543,8 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { SecondFactorCheckLifetime: 18 * time.Hour, }, }, false}, - []domain.NextStep{&domain.MFAVerificationStep{ - MFAProviders: []domain.MFAType{domain.MFATypeTOTP}, + []domain.NextStep{&domain.VerifyEMailStep{ + InitPassword: false, }}, nil, }, @@ -1573,13 +1616,14 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { nil, }, { - "email not verified and password change required, mail verification step", + "email not verified (but had before) and password change required, mail verification step", fields{ userSessionViewProvider: &mockViewUserSession{ PasswordVerification: testNow.Add(-5 * time.Minute), SecondFactorVerification: testNow.Add(-5 * time.Minute), }, userViewProvider: &mockViewUser{ + VerifiedEmail: "verified", PasswordSet: true, PasswordChangeRequired: true, MFAMaxSetUp: int32(domain.MFALevelSecondFactor), diff --git a/internal/user/model/user_view.go b/internal/user/model/user_view.go index 6806d78ebd..c5f6c0da2c 100644 --- a/internal/user/model/user_view.go +++ b/internal/user/model/user_view.go @@ -40,6 +40,7 @@ type HumanView struct { Gender Gender Email string IsEmailVerified bool + VerifiedEmail string Phone string IsPhoneVerified bool Country string diff --git a/internal/user/repository/view/model/user.go b/internal/user/repository/view/model/user.go index 62c64edc0c..c7c0248924 100644 --- a/internal/user/repository/view/model/user.go +++ b/internal/user/repository/view/model/user.go @@ -79,6 +79,7 @@ type HumanView struct { AvatarKey string `json:"storeKey" gorm:"column:avatar_key"` Email string `json:"email" gorm:"column:email"` IsEmailVerified bool `json:"-" gorm:"column:is_email_verified"` + VerifiedEmail string `json:"-" gorm:"column:verified_email"` Phone string `json:"phone" gorm:"column:phone"` IsPhoneVerified bool `json:"-" gorm:"column:is_phone_verified"` Country string `json:"country" gorm:"column:country"` @@ -170,6 +171,7 @@ func UserToModel(user *UserView) *model.UserView { Gender: model.Gender(user.Gender), Email: user.Email, IsEmailVerified: user.IsEmailVerified, + VerifiedEmail: user.VerifiedEmail, Phone: user.Phone, IsPhoneVerified: user.IsPhoneVerified, Country: user.Country, diff --git a/internal/user/repository/view/user_by_id.sql b/internal/user/repository/view/user_by_id.sql index 1720ad7998..bd34f77d80 100644 --- a/internal/user/repository/view/user_by_id.sql +++ b/internal/user/repository/view/user_by_id.sql @@ -42,6 +42,7 @@ SELECT , h.gender , h.email , h.is_email_verified + , n.verified_email , h.phone , h.is_phone_verified , (SELECT COALESCE((SELECT state FROM auth_methods WHERE method_type = 1), 0)) AS otp_state @@ -77,6 +78,9 @@ FROM projections.users13 u LEFT JOIN projections.users13_humans h ON u.instance_id = h.instance_id AND u.id = h.user_id + LEFT JOIN projections.users13_notifications n + ON u.instance_id = n.instance_id + AND u.id = n.user_id LEFT JOIN projections.login_names3 l ON u.instance_id = l.instance_id AND u.id = l.user_id