From 57e189e590f2ea4c614dfa71f6b495d0544e5592 Mon Sep 17 00:00:00 2001 From: Stefan Benz <46600784+stebenz@users.noreply.github.com> Date: Fri, 26 May 2023 09:24:52 +0200 Subject: [PATCH] fix: token for post authentication action and change phone and email (#5933) * fix: token for post authentication action and change phone and email * fix checks and add tests * improve change checks and add tests * add more tests * remove unintended test --------- Co-authored-by: Livio Spring --- .../api/ui/login/external_provider_handler.go | 144 ++++++--- .../login/external_provider_handler_test.go | 286 ++++++++++++++++++ internal/command/user_human_email.go | 10 +- internal/command/user_human_email_test.go | 54 ++++ internal/command/user_human_phone.go | 10 +- internal/command/user_human_phone_test.go | 60 ++++ 6 files changed, 514 insertions(+), 50 deletions(-) create mode 100644 internal/api/ui/login/external_provider_handler_test.go diff --git a/internal/api/ui/login/external_provider_handler.go b/internal/api/ui/login/external_provider_handler.go index 0012d1f3ea..a5a365ea55 100644 --- a/internal/api/ui/login/external_provider_handler.go +++ b/internal/api/ui/login/external_provider_handler.go @@ -558,54 +558,106 @@ func (l *Login) updateExternalUser(ctx context.Context, authReq *domain.AuthRequ if user.Human == nil { return errors.ThrowPreconditionFailed(nil, "LOGIN-WLTce", "Errors.User.NotHuman") } - if externalUser.Email != "" && (externalUser.Email != user.Human.Email || externalUser.IsEmailVerified != user.Human.IsEmailVerified) { - emailCodeGenerator, err := l.query.InitEncryptionGenerator(ctx, domain.SecretGeneratorTypeVerifyEmailCode, l.userCodeAlg) - logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update email") - if err == nil { - _, err = l.command.ChangeHumanEmail(setContext(ctx, authReq.UserOrgID), - &domain.Email{ - ObjectRoot: models.ObjectRoot{AggregateID: authReq.UserID}, - EmailAddress: externalUser.Email, - IsEmailVerified: externalUser.IsEmailVerified, - }, - emailCodeGenerator) - logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update email") - } - } - if externalUser.Phone != "" && (externalUser.Phone != user.Human.Phone || externalUser.IsPhoneVerified != user.Human.IsPhoneVerified) { - phoneCodeGenerator, err := l.query.InitEncryptionGenerator(ctx, domain.SecretGeneratorTypeVerifyPhoneCode, l.userCodeAlg) - logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update phone") - if err == nil { - _, err = l.command.ChangeHumanPhone(setContext(ctx, authReq.UserOrgID), - &domain.Phone{ - ObjectRoot: models.ObjectRoot{AggregateID: authReq.UserID}, - PhoneNumber: externalUser.Phone, - IsPhoneVerified: externalUser.IsPhoneVerified, - }, - authReq.UserOrgID, - phoneCodeGenerator) - logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update phone") - } - } - if externalUser.FirstName != user.Human.FirstName || - externalUser.LastName != user.Human.LastName || - externalUser.NickName != user.Human.NickName || - externalUser.DisplayName != user.Human.DisplayName || - externalUser.PreferredLanguage != user.Human.PreferredLanguage { - _, err = l.command.ChangeHumanProfile(setContext(ctx, authReq.UserOrgID), &domain.Profile{ - ObjectRoot: models.ObjectRoot{AggregateID: authReq.UserID}, - FirstName: externalUser.FirstName, - LastName: externalUser.LastName, - NickName: externalUser.NickName, - DisplayName: externalUser.DisplayName, - PreferredLanguage: externalUser.PreferredLanguage, - Gender: user.Human.Gender, - }) - logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update profile") - } + err = l.updateExternalUserEmail(ctx, user, externalUser) + logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update email") + + err = l.updateExternalUserPhone(ctx, user, externalUser) + logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update phone") + + err = l.updateExternalUserProfile(ctx, user, externalUser) + logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update profile") + return nil } +func (l *Login) updateExternalUserEmail(ctx context.Context, user *query.User, externalUser *domain.ExternalUser) error { + changed := hasEmailChanged(user, externalUser) + if !changed { + return nil + } + // if the email has changed and / or was not verified, we change it + emailCodeGenerator, err := l.query.InitEncryptionGenerator(ctx, domain.SecretGeneratorTypeVerifyEmailCode, l.userCodeAlg) + if err != nil { + return err + } + _, err = l.command.ChangeHumanEmail(setContext(ctx, user.ResourceOwner), + &domain.Email{ + ObjectRoot: models.ObjectRoot{AggregateID: user.ID}, + EmailAddress: externalUser.Email, + IsEmailVerified: externalUser.IsEmailVerified, + }, + emailCodeGenerator) + return err +} + +func (l *Login) updateExternalUserPhone(ctx context.Context, user *query.User, externalUser *domain.ExternalUser) error { + changed, err := hasPhoneChanged(user, externalUser) + if !changed || err != nil { + return err + } + // if the phone has changed and / or was not verified, we change it + phoneCodeGenerator, err := l.query.InitEncryptionGenerator(ctx, domain.SecretGeneratorTypeVerifyPhoneCode, l.userCodeAlg) + if err != nil { + return err + } + _, err = l.command.ChangeHumanPhone(setContext(ctx, user.ResourceOwner), + &domain.Phone{ + ObjectRoot: models.ObjectRoot{AggregateID: user.ID}, + PhoneNumber: externalUser.Phone, + IsPhoneVerified: externalUser.IsPhoneVerified, + }, + user.ResourceOwner, + phoneCodeGenerator) + return err +} + +func (l *Login) updateExternalUserProfile(ctx context.Context, user *query.User, externalUser *domain.ExternalUser) error { + if externalUser.FirstName == user.Human.FirstName && + externalUser.LastName == user.Human.LastName && + externalUser.NickName == user.Human.NickName && + externalUser.DisplayName == user.Human.DisplayName && + externalUser.PreferredLanguage == user.Human.PreferredLanguage { + return nil + } + _, err := l.command.ChangeHumanProfile(setContext(ctx, user.ResourceOwner), &domain.Profile{ + ObjectRoot: models.ObjectRoot{AggregateID: user.ID}, + FirstName: externalUser.FirstName, + LastName: externalUser.LastName, + NickName: externalUser.NickName, + DisplayName: externalUser.DisplayName, + PreferredLanguage: externalUser.PreferredLanguage, + Gender: user.Human.Gender, + }) + return err +} + +func hasEmailChanged(user *query.User, externalUser *domain.ExternalUser) bool { + externalUser.Email = externalUser.Email.Normalize() + if externalUser.Email == "" { + return false + } + // ignore if the same email is not set to verified anymore + if externalUser.Email == user.Human.Email && user.Human.IsEmailVerified { + return false + } + return externalUser.Email != user.Human.Email || externalUser.IsEmailVerified != user.Human.IsEmailVerified +} + +func hasPhoneChanged(user *query.User, externalUser *domain.ExternalUser) (_ bool, err error) { + if externalUser.Phone == "" { + return false, nil + } + externalUser.Phone, err = externalUser.Phone.Normalize() + if err != nil { + return false, err + } + // ignore if the same phone is not set to verified anymore + if externalUser.Phone == user.Human.Phone && user.Human.IsPhoneVerified { + return false, nil + } + return externalUser.Phone != user.Human.Phone || externalUser.IsPhoneVerified != user.Human.IsPhoneVerified, nil +} + func (l *Login) ldapProvider(ctx context.Context, identityProvider *query.IDPTemplate) (*ldap.Provider, error) { password, err := crypto.DecryptString(identityProvider.LDAPIDPTemplate.BindPassword, l.idpConfigAlg) if err != nil { @@ -851,6 +903,8 @@ func tokens(session idp.Session) *oidc.Tokens[*oidc.IDTokenClaims] { return s.Tokens case *jwt.Session: return s.Tokens + case *oauth.Session: + return s.Tokens } return nil } diff --git a/internal/api/ui/login/external_provider_handler_test.go b/internal/api/ui/login/external_provider_handler_test.go new file mode 100644 index 0000000000..b1724fc35c --- /dev/null +++ b/internal/api/ui/login/external_provider_handler_test.go @@ -0,0 +1,286 @@ +package login + +import ( + "testing" + + "github.com/zitadel/zitadel/internal/domain" + "github.com/zitadel/zitadel/internal/query" +) + +func Test_hasEmailChanged(t *testing.T) { + type args struct { + user *query.User + externalUser *domain.ExternalUser + } + tests := []struct { + name string + args args + want bool + }{ + { + "no external mail", + args{ + user: &query.User{}, + externalUser: &domain.ExternalUser{}, + }, + false, + }, + { + "same email unverified", + args{ + user: &query.User{ + Human: &query.Human{ + Email: domain.EmailAddress("email@test.com"), + }, + }, + externalUser: &domain.ExternalUser{ + Email: domain.EmailAddress("email@test.com"), + }, + }, + false, + }, + { + "same email verified", + args{ + user: &query.User{ + Human: &query.Human{ + Email: domain.EmailAddress("email@test.com"), + IsEmailVerified: true, + }, + }, + externalUser: &domain.ExternalUser{ + Email: domain.EmailAddress("email@test.com"), + IsEmailVerified: true, + }, + }, + false, + }, + { + "email already verified", + args{ + user: &query.User{ + Human: &query.Human{ + Email: domain.EmailAddress("email@test.com"), + IsEmailVerified: true, + }, + }, + externalUser: &domain.ExternalUser{ + Email: domain.EmailAddress("email@test.com"), + }, + }, + false, + }, + { + "email changed to verified", + args{ + user: &query.User{ + Human: &query.Human{ + Email: domain.EmailAddress("email@test.com"), + }, + }, + externalUser: &domain.ExternalUser{ + Email: domain.EmailAddress("email@test.com"), + IsEmailVerified: true, + }, + }, + true, + }, + { + "email changed", + args{ + user: &query.User{ + Human: &query.Human{ + Email: domain.EmailAddress("email@test.com"), + }, + }, + externalUser: &domain.ExternalUser{ + Email: domain.EmailAddress("new-email@test.com"), + }, + }, + true, + }, + { + "email changed and verified", + args{ + user: &query.User{ + Human: &query.Human{ + Email: domain.EmailAddress("email@test.com"), + IsEmailVerified: false, + }, + }, + externalUser: &domain.ExternalUser{ + Email: domain.EmailAddress("new-email@test.com"), + IsEmailVerified: true, + }, + }, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := hasEmailChanged(tt.args.user, tt.args.externalUser); got != tt.want { + t.Errorf("hasEmailChanged() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_hasPhoneChanged(t *testing.T) { + type args struct { + user *query.User + externalUser *domain.ExternalUser + } + tests := []struct { + name string + args args + want bool + wantErr bool + }{ + { + "no external phone", + args{ + user: &query.User{}, + externalUser: &domain.ExternalUser{}, + }, + false, + false, + }, + { + "invalid phone", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("invalid"), + }, + }, + false, + true, + }, + { + "same phone unverified", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + false, + false, + }, + { + "same phone verified", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + IsPhoneVerified: true, + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("+41791234567"), + IsPhoneVerified: true, + }, + }, + false, + false, + }, + { + "phone already verified", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + IsPhoneVerified: true, + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + false, + false, + }, + { + "phone changed to verified", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("+41791234567"), + IsPhoneVerified: true, + }, + }, + true, + false, + }, + { + "phone changed", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("+4179654321"), + }, + }, + true, + false, + }, + { + "phone changed", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("+4179654321"), + IsPhoneVerified: true, + }, + }, + true, + false, + }, + { + "normalized phone unchanged", + args{ + user: &query.User{ + Human: &query.Human{ + Phone: domain.PhoneNumber("+41791234567"), + }, + }, + externalUser: &domain.ExternalUser{ + Phone: domain.PhoneNumber("0791234567"), + }, + }, + false, + false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := hasPhoneChanged(tt.args.user, tt.args.externalUser) + if (err != nil) != tt.wantErr { + t.Errorf("hasPhoneChanged() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("hasPhoneChanged() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/command/user_human_email.go b/internal/command/user_human_email.go index 66a4982863..cdc737a00e 100644 --- a/internal/command/user_human_email.go +++ b/internal/command/user_human_email.go @@ -33,12 +33,16 @@ func (c *Commands) ChangeHumanEmail(ctx context.Context, email *domain.Email, em } userAgg := UserAggregateFromWriteModel(&existingEmail.WriteModel) changedEvent, hasChanged := existingEmail.NewChangedEvent(ctx, userAgg, email.EmailAddress) - if !hasChanged { + + // only continue if there were changes or there were no changes and the email should be set to verified + if !hasChanged && !(email.IsEmailVerified && existingEmail.IsEmailVerified != email.IsEmailVerified) { return nil, caos_errs.ThrowPreconditionFailed(nil, "COMMAND-2b7fM", "Errors.User.Email.NotChanged") } - events := []eventstore.Command{changedEvent} - + events := make([]eventstore.Command, 0) + if hasChanged { + events = append(events, changedEvent) + } if email.IsEmailVerified { events = append(events, user.NewHumanEmailVerifiedEvent(ctx, userAgg)) } else { diff --git a/internal/command/user_human_email_test.go b/internal/command/user_human_email_test.go index 45b63a3cd9..3f41b2b5a3 100644 --- a/internal/command/user_human_email_test.go +++ b/internal/command/user_human_email_test.go @@ -219,6 +219,60 @@ func TestCommandSide_ChangeHumanEmail(t *testing.T) { }, }, }, + { + name: "email verified, ok", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "nickname", + "displayname", + language.German, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + ), + expectPush( + []*repository.Event{ + eventFromEventPusher( + user.NewHumanEmailVerifiedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + ), + ), + }, + ), + ), + }, + args: args{ + ctx: context.Background(), + email: &domain.Email{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "user1", + }, + EmailAddress: "email@test.ch", + IsEmailVerified: true, + }, + resourceOwner: "org1", + }, + res: res{ + want: &domain.Email{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "user1", + ResourceOwner: "org1", + }, + EmailAddress: "email@test.ch", + IsEmailVerified: true, + }, + }, + }, { name: "email changed with code, ok", fields: fields{ diff --git a/internal/command/user_human_phone.go b/internal/command/user_human_phone.go index e548807693..525333957c 100644 --- a/internal/command/user_human_phone.go +++ b/internal/command/user_human_phone.go @@ -6,6 +6,7 @@ import ( "github.com/zitadel/zitadel/internal/eventstore" "github.com/zitadel/logging" + "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" caos_errs "github.com/zitadel/zitadel/internal/errors" @@ -27,11 +28,16 @@ func (c *Commands) ChangeHumanPhone(ctx context.Context, phone *domain.Phone, re userAgg := UserAggregateFromWriteModel(&existingPhone.WriteModel) changedEvent, hasChanged := existingPhone.NewChangedEvent(ctx, userAgg, phone.PhoneNumber) - if !hasChanged { + + // only continue if there were changes or there were no changes and the phone should be set to verified + if !hasChanged && !(phone.IsPhoneVerified && existingPhone.IsPhoneVerified != phone.IsPhoneVerified) { return nil, caos_errs.ThrowPreconditionFailed(nil, "COMMAND-wF94r", "Errors.User.Phone.NotChanged") } - events := []eventstore.Command{changedEvent} + events := make([]eventstore.Command, 0) + if hasChanged { + events = append(events, changedEvent) + } if phone.IsPhoneVerified { events = append(events, user.NewHumanPhoneVerifiedEvent(ctx, userAgg)) } else { diff --git a/internal/command/user_human_phone_test.go b/internal/command/user_human_phone_test.go index 4d4480803a..c7f7535a68 100644 --- a/internal/command/user_human_phone_test.go +++ b/internal/command/user_human_phone_test.go @@ -188,6 +188,66 @@ func TestCommandSide_ChangeHumanPhone(t *testing.T) { }, }, }, + { + name: "phone changed to verified, ok", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "nickname", + "displayname", + language.German, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + eventFromEventPusher( + user.NewHumanPhoneChangedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "+41711234567", + ), + ), + ), + expectPush( + []*repository.Event{ + eventFromEventPusher( + user.NewHumanPhoneVerifiedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + ), + ), + }, + ), + ), + }, + args: args{ + ctx: context.Background(), + email: &domain.Phone{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "user1", + }, + PhoneNumber: "+41711234567", + IsPhoneVerified: true, + }, + resourceOwner: "org1", + }, + res: res{ + want: &domain.Phone{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "user1", + ResourceOwner: "org1", + }, + PhoneNumber: "+41711234567", + IsPhoneVerified: true, + }, + }, + }, { name: "phone changed with code, ok", fields: fields{