From f653589609dda88a0c8d22f6b438de8a1fa346d9 Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Mon, 7 Oct 2024 07:12:44 +0200 Subject: [PATCH] fix: twilio code generation and verification (#8728) # Which Problems Are Solved The recently added possibility to generate and verify codes through Twilio verification service did failed on checking OTP SMS code through the session API. Additionally, password codes generated by the V2 API and sent through phone would always use the internal generator and verification mechanism rather than the configured. # How the Problems Are Solved - Correctly set the verifier for OTP SMS for the session API - Always use the internal verifier for OTP Email (for now) - Select the generator / verifier based on the configuration for password codes with notification type SMS for V2 APIs # Additional Changes None # Additional Context - relates to #8678 - reported by customer --------- Co-authored-by: Stefan Benz <46600784+stebenz@users.noreply.github.com> --- internal/command/session.go | 1 + internal/command/session_otp.go | 2 +- internal/command/session_otp_test.go | 38 ++++++ internal/command/user_v2_human_test.go | 5 + internal/command/user_v2_model_test.go | 2 + internal/command/user_v2_password.go | 12 +- internal/command/user_v2_password_test.go | 140 +++++++++++++++++++-- internal/repository/user/human_password.go | 2 + 8 files changed, 188 insertions(+), 14 deletions(-) diff --git a/internal/command/session.go b/internal/command/session.go index dafb10b818..d00e541e62 100644 --- a/internal/command/session.go +++ b/internal/command/session.go @@ -55,6 +55,7 @@ func (c *Commands) NewSessionCommands(cmds []SessionCommand, session *SessionWri createCode: c.newEncryptedCodeWithDefault, createPhoneCode: c.newPhoneCode, createToken: c.sessionTokenCreator, + getCodeVerifier: c.phoneCodeVerifierFromConfig, now: time.Now, } } diff --git a/internal/command/session_otp.go b/internal/command/session_otp.go index 6a4517c982..475eed6f36 100644 --- a/internal/command/session_otp.go +++ b/internal/command/session_otp.go @@ -180,7 +180,7 @@ func CheckOTPEmail(code string) SessionCommand { writeModel, cmd.eventstore.FilterToQueryReducer, cmd.otpAlg, - cmd.getCodeVerifier, + nil, // email currently always uses local code checks succeededEvent, failedEvent, ) diff --git a/internal/command/session_otp_test.go b/internal/command/session_otp_test.go index 3e061749b0..30559b9cca 100644 --- a/internal/command/session_otp_test.go +++ b/internal/command/session_otp_test.go @@ -12,6 +12,7 @@ import ( "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" "github.com/zitadel/zitadel/internal/notification/senders" + "github.com/zitadel/zitadel/internal/notification/senders/mock" "github.com/zitadel/zitadel/internal/repository/org" "github.com/zitadel/zitadel/internal/repository/session" "github.com/zitadel/zitadel/internal/repository/user" @@ -763,6 +764,7 @@ func TestCheckOTPSMS(t *testing.T) { userID string otpCodeChallenge *OTPCode otpAlg crypto.EncryptionAlgorithm + getCodeVerifier func(ctx context.Context, id string) (senders.CodeGenerator, error) } type args struct { code string @@ -948,6 +950,41 @@ func TestCheckOTPSMS(t *testing.T) { }, }, }, + { + name: "check ok (external)", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher(user.NewHumanOTPSMSAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate)), + ), + expectFilter(), // recheck + ), + userID: "userID", + otpCodeChallenge: &OTPCode{ + Code: nil, + Expiry: 0, + GeneratorID: "generatorID", + VerificationID: "verificationID", + CreationDate: testNow, + }, + getCodeVerifier: func(ctx context.Context, id string) (senders.CodeGenerator, error) { + sender := mock.NewMockCodeGenerator(gomock.NewController(t)) + sender.EXPECT().VerifyCode("verificationID", "code").Return(nil) + return sender, nil + }, + }, + args: args{ + code: "code", + }, + res: res{ + commands: []eventstore.Command{ + user.NewHumanOTPSMSCheckSucceededEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, nil), + session.NewOTPSMSCheckedEvent(context.Background(), &session.NewAggregate("sessionID", "instanceID").Aggregate, + testNow, + ), + }, + }, + }, { name: "check ok, locked in the meantime", fields: fields{ @@ -996,6 +1033,7 @@ func TestCheckOTPSMS(t *testing.T) { sessionWriteModel: sessionModel, eventstore: tt.fields.eventstore(t), otpAlg: tt.fields.otpAlg, + getCodeVerifier: tt.fields.getCodeVerifier, now: func() time.Time { return testNow }, diff --git a/internal/command/user_v2_human_test.go b/internal/command/user_v2_human_test.go index 24899a9301..a50061c010 100644 --- a/internal/command/user_v2_human_test.go +++ b/internal/command/user_v2_human_test.go @@ -2967,6 +2967,7 @@ func TestCommandSide_ChangeUserHuman(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), @@ -3040,6 +3041,7 @@ func TestCommandSide_ChangeUserHuman(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), @@ -3091,6 +3093,7 @@ func TestCommandSide_ChangeUserHuman(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), @@ -3152,6 +3155,7 @@ func TestCommandSide_ChangeUserHuman(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), @@ -3213,6 +3217,7 @@ func TestCommandSide_ChangeUserHuman(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), diff --git a/internal/command/user_v2_model_test.go b/internal/command/user_v2_model_test.go index ba5180cae3..2476d27d58 100644 --- a/internal/command/user_v2_model_test.go +++ b/internal/command/user_v2_model_test.go @@ -1494,6 +1494,7 @@ func TestCommandSide_userHumanWriteModel_password(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), @@ -1556,6 +1557,7 @@ func TestCommandSide_userHumanWriteModel_password(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), eventFromEventPusher( diff --git a/internal/command/user_v2_password.go b/internal/command/user_v2_password.go index 3cddf956a1..67bee2c28f 100644 --- a/internal/command/user_v2_password.go +++ b/internal/command/user_v2_password.go @@ -55,14 +55,20 @@ func (c *Commands) requestPasswordReset(ctx context.Context, userID string, retu return nil, nil, err } } - code, err := c.newEncryptedCode(ctx, c.eventstore.Filter, domain.SecretGeneratorTypePasswordResetCode, c.userEncryption) //nolint:staticcheck + var passwordCode *EncryptedCode + var generatorID string + if notificationType == domain.NotificationTypeSms { + passwordCode, generatorID, err = c.newPhoneCode(ctx, c.eventstore.Filter, domain.SecretGeneratorTypePasswordResetCode, c.userEncryption, c.defaultSecretGenerators.PasswordVerificationCode) //nolint:staticcheck + } else { + passwordCode, err = c.newEncryptedCode(ctx, c.eventstore.Filter, domain.SecretGeneratorTypePasswordResetCode, c.userEncryption) //nolint:staticcheck + } if err != nil { return nil, nil, err } - cmd := user.NewHumanPasswordCodeAddedEventV2(ctx, UserAggregateFromWriteModel(&model.WriteModel), code.Crypted, code.Expiry, notificationType, urlTmpl, returnCode) + cmd := user.NewHumanPasswordCodeAddedEventV2(ctx, UserAggregateFromWriteModelCtx(ctx, &model.WriteModel), passwordCode.CryptedCode(), passwordCode.CodeExpiry(), notificationType, urlTmpl, returnCode, generatorID) if returnCode { - plainCode = &code.Plain + plainCode = &passwordCode.Plain } if err = c.pushAppendAndReduce(ctx, model, cmd); err != nil { return nil, nil, err diff --git a/internal/command/user_v2_password_test.go b/internal/command/user_v2_password_test.go index 2575e9442c..b51be864fe 100644 --- a/internal/command/user_v2_password_test.go +++ b/internal/command/user_v2_password_test.go @@ -14,6 +14,7 @@ import ( "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/repository/instance" "github.com/zitadel/zitadel/internal/repository/user" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -327,11 +328,23 @@ func TestCommands_RequestPasswordResetURLTemplate(t *testing.T) { } func TestCommands_requestPasswordReset(t *testing.T) { + defaultGenerators := &SecretGenerators{ + OTPSMS: &crypto.GeneratorConfig{ + Length: 8, + Expiry: time.Hour, + IncludeLowerLetters: true, + IncludeUpperLetters: true, + IncludeDigits: true, + IncludeSymbols: true, + }, + } type fields struct { - checkPermission domain.PermissionCheck - eventstore func(t *testing.T) *eventstore.Eventstore - userEncryption crypto.EncryptionAlgorithm - newCode encrypedCodeFunc + checkPermission domain.PermissionCheck + eventstore func(t *testing.T) *eventstore.Eventstore + userEncryption crypto.EncryptionAlgorithm + newCode encrypedCodeFunc + newEncryptedCodeWithDefault encryptedCodeWithDefaultFunc + defaultSecretGenerators *SecretGenerators } type args struct { ctx context.Context @@ -449,6 +462,7 @@ func TestCommands_requestPasswordReset(t *testing.T) { domain.NotificationTypeEmail, "", false, + "", ), ), ), @@ -489,6 +503,7 @@ func TestCommands_requestPasswordReset(t *testing.T) { domain.NotificationTypeEmail, "https://example.com/password/changey?userID={{.UserID}}&code={{.Code}}&orgID={{.OrgID}}", false, + "", ), ), ), @@ -518,6 +533,36 @@ func TestCommands_requestPasswordReset(t *testing.T) { language.English, domain.GenderUnspecified, "email", false), ), ), + expectFilter( + eventFromEventPusher( + instance.NewSMSConfigActivatedEvent( + context.Background(), + &instance.NewAggregate("instanceID").Aggregate, + "id", + ), + ), + ), + expectFilter( + eventFromEventPusher( + instance.NewSMSConfigTwilioAddedEvent( + context.Background(), + &instance.NewAggregate("instanceID").Aggregate, + "id", + "", + "sid", + "senderNumber", + &crypto.CryptoValue{CryptoType: crypto.TypeEncryption, Algorithm: "enc", KeyID: "id", Crypted: []byte("crypted")}, + "", + ), + ), + eventFromEventPusher( + instance.NewSMSConfigActivatedEvent( + context.Background(), + &instance.NewAggregate("instanceID").Aggregate, + "id", + ), + ), + ), expectPush( user.NewHumanPasswordCodeAddedEventV2(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, &crypto.CryptoValue{ @@ -530,11 +575,82 @@ func TestCommands_requestPasswordReset(t *testing.T) { domain.NotificationTypeSms, "https://example.com/password/changey?userID={{.UserID}}&code={{.Code}}&orgID={{.OrgID}}", false, + "", ), ), ), - checkPermission: newMockPermissionCheckAllowed(), - newCode: mockEncryptedCode("code", 10*time.Minute), + defaultSecretGenerators: defaultGenerators, + checkPermission: newMockPermissionCheckAllowed(), + newEncryptedCodeWithDefault: mockEncryptedCodeWithDefault("code", 10*time.Minute), + }, + args: args{ + ctx: context.Background(), + userID: "userID", + urlTmpl: "https://example.com/password/changey?userID={{.UserID}}&code={{.Code}}&orgID={{.OrgID}}", + notificationType: domain.NotificationTypeSms, + }, + res: res{ + details: &domain.ObjectDetails{ + ResourceOwner: "org1", + }, + code: nil, + }, + }, + { + name: "code generated template sms external", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "username", "firstname", "lastname", "nickname", "displayname", + language.English, domain.GenderUnspecified, "email", false), + ), + ), + expectFilter( + eventFromEventPusher( + instance.NewSMSConfigActivatedEvent( + context.Background(), + &instance.NewAggregate("instanceID").Aggregate, + "id", + ), + ), + ), + expectFilter( + eventFromEventPusher( + instance.NewSMSConfigTwilioAddedEvent( + context.Background(), + &instance.NewAggregate("instanceID").Aggregate, + "id", + "", + "sid", + "senderNumber", + &crypto.CryptoValue{CryptoType: crypto.TypeEncryption, Algorithm: "enc", KeyID: "id", Crypted: []byte("crypted")}, + "verifyServiceSid", + ), + ), + eventFromEventPusher( + instance.NewSMSConfigActivatedEvent( + context.Background(), + &instance.NewAggregate("instanceID").Aggregate, + "id", + ), + ), + ), + expectPush( + user.NewHumanPasswordCodeAddedEventV2(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + nil, + 0, + domain.NotificationTypeSms, + "https://example.com/password/changey?userID={{.UserID}}&code={{.Code}}&orgID={{.OrgID}}", + false, + "id", + ), + ), + ), + checkPermission: newMockPermissionCheckAllowed(), + newCode: mockEncryptedCode("code", 10*time.Minute), + defaultSecretGenerators: defaultGenerators, }, args: args{ ctx: context.Background(), @@ -572,6 +688,7 @@ func TestCommands_requestPasswordReset(t *testing.T) { domain.NotificationTypeEmail, "", true, + "", ), ), ), @@ -594,11 +711,14 @@ func TestCommands_requestPasswordReset(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c := &Commands{ - checkPermission: tt.fields.checkPermission, - eventstore: tt.fields.eventstore(t), - userEncryption: tt.fields.userEncryption, - newEncryptedCode: tt.fields.newCode, + checkPermission: tt.fields.checkPermission, + eventstore: tt.fields.eventstore(t), + userEncryption: tt.fields.userEncryption, + newEncryptedCode: tt.fields.newCode, + newEncryptedCodeWithDefault: tt.fields.newEncryptedCodeWithDefault, + defaultSecretGenerators: tt.fields.defaultSecretGenerators, } + got, gotPlainCode, err := c.requestPasswordReset(tt.args.ctx, tt.args.userID, tt.args.returnCode, tt.args.urlTmpl, tt.args.notificationType) require.ErrorIs(t, err, tt.res.err) assertObjectDetails(t, tt.res.details, got) diff --git a/internal/repository/user/human_password.go b/internal/repository/user/human_password.go index 4251a7987f..49de702319 100644 --- a/internal/repository/user/human_password.go +++ b/internal/repository/user/human_password.go @@ -137,6 +137,7 @@ func NewHumanPasswordCodeAddedEventV2( notificationType domain.NotificationType, urlTemplate string, codeReturned bool, + generatorID string, ) *HumanPasswordCodeAddedEvent { return &HumanPasswordCodeAddedEvent{ BaseEvent: *eventstore.NewBaseEventForPush( @@ -150,6 +151,7 @@ func NewHumanPasswordCodeAddedEventV2( URLTemplate: urlTemplate, CodeReturned: codeReturned, TriggeredAtOrigin: http.DomainContext(ctx).Origin(), + GeneratorID: generatorID, } }