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 <livio.a@gmail.com>

internal/api/ui/login/external_provider_handler.go

@@ -558,42 +558,69 @@ 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) {
+	err = l.updateExternalUserEmail(ctx, user, externalUser)
-		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),
+	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: authReq.UserID},
+			ObjectRoot:      models.ObjectRoot{AggregateID: user.ID},
 			EmailAddress:    externalUser.Email,
 			IsEmailVerified: externalUser.IsEmailVerified,
 		},
 		emailCodeGenerator)
-			logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update email")
+	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
-	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 {
-		if err == nil {
+		return err
-			_, err = l.command.ChangeHumanPhone(setContext(ctx, authReq.UserOrgID),
+	}
+	_, err = l.command.ChangeHumanPhone(setContext(ctx, user.ResourceOwner),
 		&domain.Phone{
-					ObjectRoot:      models.ObjectRoot{AggregateID: authReq.UserID},
+			ObjectRoot:      models.ObjectRoot{AggregateID: user.ID},
 			PhoneNumber:     externalUser.Phone,
 			IsPhoneVerified: externalUser.IsPhoneVerified,
 		},
-				authReq.UserOrgID,
+		user.ResourceOwner,
 		phoneCodeGenerator)
-			logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update phone")
+	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{
-	if externalUser.FirstName != user.Human.FirstName ||
+		ObjectRoot:        models.ObjectRoot{AggregateID: user.ID},
-		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,
@@ -601,9 +628,34 @@ func (l *Login) updateExternalUser(ctx context.Context, authReq *domain.AuthRequ
 		PreferredLanguage: externalUser.PreferredLanguage,
 		Gender:            user.Human.Gender,
 	})
-		logging.WithFields("authReq", authReq.ID, "user", authReq.UserID).OnError(err).Error("unable to update profile")
+	return err
+}
+
+func hasEmailChanged(user *query.User, externalUser *domain.ExternalUser) bool {
+	externalUser.Email = externalUser.Email.Normalize()
+	if externalUser.Email == "" {
+		return false
 	}
-	return nil
+	// 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) {
@@ -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
 }

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)
+			}
+		})
+	}
+}

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 {

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{

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 {

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{