From 8d9521fb6d657fd9429edadb611842f3da112145 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Thu, 14 Mar 2024 13:25:28 +0100 Subject: [PATCH] Refactor: Email verification (#84393) * Update template names * Add verifier that we can use to start verify process * Use userVerifier when verifying email on update * Add tests --------- Co-authored-by: Ieva --- ...fy_email_update.mjml => verify_email.mjml} | 2 +- ...rify_email_update.txt => verify_email.txt} | 2 +- pkg/api/http_server.go | 3 + pkg/api/user.go | 69 ++--------- pkg/api/user_test.go | 3 + pkg/server/wire.go | 3 + pkg/services/notifications/notifications.go | 11 +- pkg/services/temp_user/tempusertest/fake.go | 37 ++++++ pkg/services/user/error.go | 1 + pkg/services/user/model.go | 6 + pkg/services/user/user.go | 4 + pkg/services/user/userimpl/verifier.go | 82 +++++++++++++ pkg/services/user/userimpl/verifier_test.go | 108 ++++++++++++++++++ 13 files changed, 267 insertions(+), 64 deletions(-) rename emails/templates/{verify_email_update.mjml => verify_email.mjml} (94%) rename emails/templates/{verify_email_update.txt => verify_email.txt} (75%) create mode 100644 pkg/services/temp_user/tempusertest/fake.go create mode 100644 pkg/services/user/userimpl/verifier.go create mode 100644 pkg/services/user/userimpl/verifier_test.go diff --git a/emails/templates/verify_email_update.mjml b/emails/templates/verify_email.mjml similarity index 94% rename from emails/templates/verify_email_update.mjml rename to emails/templates/verify_email.mjml index 75d4add8164..b830496868b 100644 --- a/emails/templates/verify_email_update.mjml +++ b/emails/templates/verify_email.mjml @@ -6,7 +6,7 @@ - {{ Subject .Subject .TemplateData "Verify your new email - {{.Name}}" }} + {{ Subject .Subject .TemplateData "Verify your email - {{.Name}}" }} diff --git a/emails/templates/verify_email_update.txt b/emails/templates/verify_email.txt similarity index 75% rename from emails/templates/verify_email_update.txt rename to emails/templates/verify_email.txt index ae0d29e314e..8af1d2c1935 100644 --- a/emails/templates/verify_email_update.txt +++ b/emails/templates/verify_email.txt @@ -1,4 +1,4 @@ -[[HiddenSubject .Subject "Verify your new email - [[.Name]]"]] +[[HiddenSubject .Subject "Verify your email - [[.Name]]"]] Hi [[.Name]], diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 05c422fb197..93f00b9c483 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -217,6 +217,7 @@ type HTTPServer struct { clientConfigProvider grafanaapiserver.DirectRestConfigProvider namespacer request.NamespaceMapper anonService anonymous.Service + userVerifier user.Verifier } type ServerOptions struct { @@ -259,6 +260,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi annotationRepo annotations.Repository, tagService tag.Service, searchv2HTTPService searchV2.SearchHTTPService, oauthTokenService oauthtoken.OAuthTokenService, statsService stats.Service, authnService authn.Service, pluginsCDNService *pluginscdn.Service, promGatherer prometheus.Gatherer, starApi *starApi.API, promRegister prometheus.Registerer, clientConfigProvider grafanaapiserver.DirectRestConfigProvider, anonService anonymous.Service, + userVerifier user.Verifier, ) (*HTTPServer, error) { web.Env = cfg.Env m := web.New() @@ -361,6 +363,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi clientConfigProvider: clientConfigProvider, namespacer: request.GetNamespaceMapper(cfg), anonService: anonService, + userVerifier: userVerifier, } if hs.Listener != nil { hs.log.Debug("Using provided listener") diff --git a/pkg/api/user.go b/pkg/api/user.go index 26b6c1007f2..49a9572a79a 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -15,7 +15,6 @@ import ( "github.com/grafana/grafana/pkg/services/auth/identity" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/login" - "github.com/grafana/grafana/pkg/services/notifications" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/team" tempuser "github.com/grafana/grafana/pkg/services/temp_user" @@ -245,25 +244,22 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC usr, err := hs.userService.GetByID(ctx, &query) if err != nil { if errors.Is(err, user.ErrUserNotFound) { - return response.Error(http.StatusNotFound, user.ErrUserNotFound.Error(), nil) + return response.Error(http.StatusNotFound, user.ErrUserNotFound.Error(), err) } return response.Error(http.StatusInternalServerError, "Failed to get user", err) } if len(cmd.Email) != 0 && usr.Email != cmd.Email { - // Email is being updated - newEmail, err := ValidateAndNormalizeEmail(cmd.Email) + normalized, err := ValidateAndNormalizeEmail(cmd.Email) if err != nil { return response.Error(http.StatusBadRequest, "Invalid email address", err) } - - return hs.verifyEmailUpdate(ctx, newEmail, user.EmailUpdateAction, usr) + return hs.verifyEmailUpdate(ctx, normalized, user.EmailUpdateAction, usr) } if len(cmd.Login) != 0 && usr.Login != cmd.Login { - // Username is being updated. If it's an email, go through the email verification flow - newEmailLogin, err := ValidateAndNormalizeEmail(cmd.Login) - if err == nil && newEmailLogin != usr.Email { - return hs.verifyEmailUpdate(ctx, newEmailLogin, user.LoginUpdateAction, usr) + normalized, err := ValidateAndNormalizeEmail(cmd.Login) + if err == nil && usr.Email != normalized { + return hs.verifyEmailUpdate(ctx, cmd.Login, user.LoginUpdateAction, usr) } } } @@ -279,55 +275,12 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC } func (hs *HTTPServer) verifyEmailUpdate(ctx context.Context, email string, field user.UpdateEmailActionType, usr *user.User) response.Response { - // Verify that email is not already being used - query := user.GetUserByLoginQuery{LoginOrEmail: email} - existingUsr, err := hs.userService.GetByLogin(ctx, &query) - if err != nil && !errors.Is(err, user.ErrUserNotFound) { - return response.Error(http.StatusInternalServerError, "Failed to validate if email is already in use", err) - } - if existingUsr != nil { - return response.Error(http.StatusConflict, "Email is already being used", nil) - } - - // Invalidate any pending verifications for this user - expireCmd := tempuser.ExpirePreviousVerificationsCommand{InvitedByUserID: usr.ID} - err = hs.tempUserService.ExpirePreviousVerifications(ctx, &expireCmd) - if err != nil { - return response.Error(http.StatusInternalServerError, "Could not invalidate pending email verifications", err) - } - - code, err := util.GetRandomString(20) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to generate random string", err) - } - - tempCmd := tempuser.CreateTempUserCommand{ - OrgID: -1, + if err := hs.userVerifier.VerifyEmail(ctx, user.VerifyEmailCommand{ + User: *usr, Email: email, - Code: code, - Status: tempuser.TmpUserEmailUpdateStarted, - // used to fetch the User in the second step of the verification flow - InvitedByUserID: usr.ID, - // used to determine if the user was updating their email or username in the second step of the verification flow - Name: string(field), - } - - tempUser, err := hs.tempUserService.CreateTempUser(ctx, &tempCmd) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to create email change", err) - } - - emailCmd := notifications.SendVerifyEmailCommand{Email: tempUser.Email, Code: tempUser.Code, User: usr} - err = hs.NotificationService.SendVerificationEmail(ctx, &emailCmd) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to send verification email", err) - } - - // Record email as sent - emailSentCmd := tempuser.UpdateTempUserWithEmailSentCommand{Code: tempUser.Code} - err = hs.tempUserService.UpdateTempUserWithEmailSent(ctx, &emailSentCmd) - if err != nil { - return response.Error(http.StatusInternalServerError, "Failed to record verification email", err) + Action: field, + }); err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to generate email verification", err) } return response.Success("Email sent for verification") diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index ee1ffe50417..960d592136c 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -397,6 +397,7 @@ func setupUpdateEmailTests(t *testing.T, cfg *setting.Cfg) (*user.User, *HTTPSer require.NoError(t, err) nsMock := notifications.MockNotificationService() + verifier := userimpl.ProvideVerifier(userSvc, tempUserService, nsMock) hs := &HTTPServer{ Cfg: cfg, @@ -404,6 +405,7 @@ func setupUpdateEmailTests(t *testing.T, cfg *setting.Cfg) (*user.User, *HTTPSer userService: userSvc, tempUserService: tempUserService, NotificationService: nsMock, + userVerifier: verifier, } return usr, hs, nsMock } @@ -618,6 +620,7 @@ func TestUser_UpdateEmail(t *testing.T) { hs.tempUserService = tempUserSvc hs.NotificationService = nsMock hs.SecretsService = fakes.NewFakeSecretsService() + hs.userVerifier = userimpl.ProvideVerifier(userSvc, tempUserSvc, nsMock) // User is internal hs.authInfoService = &authinfotest.FakeService{ExpectedError: user.ErrUserNotFound} }) diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 1d97ab375ec..883eb58bfb1 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -151,6 +151,7 @@ import ( tempuser "github.com/grafana/grafana/pkg/services/temp_user" "github.com/grafana/grafana/pkg/services/temp_user/tempuserimpl" "github.com/grafana/grafana/pkg/services/updatechecker" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/services/user/userimpl" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tsdb/azuremonitor" @@ -383,6 +384,8 @@ var wireBasicSet = wire.NewSet( idimpl.ProvideService, wire.Bind(new(auth.IDService), new(*idimpl.Service)), cloudmigrationimpl.ProvideService, + userimpl.ProvideVerifier, + wire.Bind(new(user.Verifier), new(*userimpl.Verifier)), // Kubernetes API server grafanaapiserver.WireSet, apiregistry.WireSet, diff --git a/pkg/services/notifications/notifications.go b/pkg/services/notifications/notifications.go index c387acc6283..b69b43f01c8 100644 --- a/pkg/services/notifications/notifications.go +++ b/pkg/services/notifications/notifications.go @@ -43,10 +43,13 @@ type Service interface { } var mailTemplates *template.Template -var tmplResetPassword = "reset_password" -var tmplSignUpStarted = "signup_started" -var tmplWelcomeOnSignUp = "welcome_on_signup" -var tmplVerifyEmail = "verify_email_update" + +const ( + tmplResetPassword = "reset_password" + tmplSignUpStarted = "signup_started" + tmplWelcomeOnSignUp = "welcome_on_signup" + tmplVerifyEmail = "verify_email" +) func ProvideService(bus bus.Bus, cfg *setting.Cfg, mailer Mailer, store TempUserStore) (*NotificationService, error) { ns := &NotificationService{ diff --git a/pkg/services/temp_user/tempusertest/fake.go b/pkg/services/temp_user/tempusertest/fake.go new file mode 100644 index 00000000000..17d8c2c1330 --- /dev/null +++ b/pkg/services/temp_user/tempusertest/fake.go @@ -0,0 +1,37 @@ +package tempusertest + +import ( + "context" + + tempuser "github.com/grafana/grafana/pkg/services/temp_user" +) + +var _ tempuser.Service = (*FakeTempUserService)(nil) + +type FakeTempUserService struct { + tempuser.Service + CreateTempUserFN func(ctx context.Context, cmd *tempuser.CreateTempUserCommand) (*tempuser.TempUser, error) + ExpirePreviousVerificationsFN func(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error + UpdateTempUserWithEmailSentFN func(ctx context.Context, cmd *tempuser.UpdateTempUserWithEmailSentCommand) error +} + +func (f *FakeTempUserService) CreateTempUser(ctx context.Context, cmd *tempuser.CreateTempUserCommand) (*tempuser.TempUser, error) { + if f.CreateTempUserFN != nil { + return f.CreateTempUserFN(ctx, cmd) + } + return nil, nil +} + +func (f *FakeTempUserService) ExpirePreviousVerifications(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error { + if f.ExpirePreviousVerificationsFN != nil { + return f.ExpirePreviousVerificationsFN(ctx, cmd) + } + return nil +} + +func (f *FakeTempUserService) UpdateTempUserWithEmailSent(ctx context.Context, cmd *tempuser.UpdateTempUserWithEmailSentCommand) error { + if f.UpdateTempUserWithEmailSentFN != nil { + return f.UpdateTempUserWithEmailSentFN(ctx, cmd) + } + return nil +} diff --git a/pkg/services/user/error.go b/pkg/services/user/error.go index ba70fd24baf..8fd192704fb 100644 --- a/pkg/services/user/error.go +++ b/pkg/services/user/error.go @@ -18,6 +18,7 @@ var ( ) var ( + ErrEmailConflict = errutil.Conflict("user.email-conflict", errutil.WithPublicMessage("Email is already being used")) ErrEmptyUsernameAndEmail = errutil.BadRequest( "user.empty-username-and-email", errutil.WithPublicMessage("Need to specify either username or email"), ) diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index 0fd966e828b..1d7e8db976e 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -220,6 +220,12 @@ type GetUserByIDQuery struct { ID int64 } +type VerifyEmailCommand struct { + User User + Email string + Action UpdateEmailActionType +} + type ErrCaseInsensitiveLoginConflict struct { Users []User } diff --git a/pkg/services/user/user.go b/pkg/services/user/user.go index ab8555ec508..1466fed7c66 100644 --- a/pkg/services/user/user.go +++ b/pkg/services/user/user.go @@ -29,3 +29,7 @@ type Service interface { SetUserHelpFlag(context.Context, *SetUserHelpFlagCommand) error GetProfile(context.Context, *GetUserProfileQuery) (*UserProfileDTO, error) } + +type Verifier interface { + VerifyEmail(ctx context.Context, cmd VerifyEmailCommand) error +} diff --git a/pkg/services/user/userimpl/verifier.go b/pkg/services/user/userimpl/verifier.go new file mode 100644 index 00000000000..719a530937a --- /dev/null +++ b/pkg/services/user/userimpl/verifier.go @@ -0,0 +1,82 @@ +package userimpl + +import ( + "context" + "errors" + "fmt" + + "github.com/grafana/grafana/pkg/services/notifications" + tempuser "github.com/grafana/grafana/pkg/services/temp_user" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/util" +) + +var _ user.Verifier = (*Verifier)(nil) + +func ProvideVerifier(us user.Service, ts tempuser.Service, ns notifications.Service) *Verifier { + return &Verifier{us, ts, ns} +} + +type Verifier struct { + us user.Service + ts tempuser.Service + ns notifications.Service +} + +func (s *Verifier) VerifyEmail(ctx context.Context, cmd user.VerifyEmailCommand) error { + usr, err := s.us.GetByLogin(ctx, &user.GetUserByLoginQuery{ + LoginOrEmail: cmd.Email, + }) + + if err != nil && !errors.Is(err, user.ErrUserNotFound) { + return err + } + + // if email is already used by another user we stop here + if usr != nil && usr.ID != cmd.User.ID { + return user.ErrEmailConflict.Errorf("email already used") + } + + code, err := util.GetRandomString(20) + if err != nil { + return fmt.Errorf("failed to generate verification code: %w", err) + } + + // invalidate any pending verifications for user + if err = s.ts.ExpirePreviousVerifications( + ctx, &tempuser.ExpirePreviousVerificationsCommand{InvitedByUserID: cmd.User.ID}, + ); err != nil { + return fmt.Errorf("failed to expire previous verifications: %w", err) + } + + tmpUsr, err := s.ts.CreateTempUser(ctx, &tempuser.CreateTempUserCommand{ + OrgID: -1, + // used to determine if the user was updating their email or username in the second step of the verification flow + Name: string(cmd.Action), + // used to fetch the User in the second step of the verification flow + InvitedByUserID: cmd.User.ID, + Email: cmd.Email, + Code: code, + Status: tempuser.TmpUserEmailUpdateStarted, + }) + + if err != nil { + return fmt.Errorf("failed to generate temp user for email verification: %w", err) + } + + if err := s.ns.SendVerificationEmail(ctx, ¬ifications.SendVerifyEmailCommand{ + User: &cmd.User, + Code: tmpUsr.Code, + Email: cmd.Email, + }); err != nil { + return fmt.Errorf("failed to send verification email: %w", err) + } + + if err := s.ts.UpdateTempUserWithEmailSent(ctx, &tempuser.UpdateTempUserWithEmailSentCommand{ + Code: tmpUsr.Code, + }); err != nil { + return fmt.Errorf("failed to mark email as sent: %w", err) + } + + return nil +} diff --git a/pkg/services/user/userimpl/verifier_test.go b/pkg/services/user/userimpl/verifier_test.go new file mode 100644 index 00000000000..c57dc2fabf8 --- /dev/null +++ b/pkg/services/user/userimpl/verifier_test.go @@ -0,0 +1,108 @@ +package userimpl + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/grafana/grafana/pkg/services/notifications" + tempuser "github.com/grafana/grafana/pkg/services/temp_user" + "github.com/grafana/grafana/pkg/services/temp_user/tempusertest" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" +) + +func TestVerifier_VerifyEmail(t *testing.T) { + ts := &tempusertest.FakeTempUserService{} + us := &usertest.FakeUserService{} + ns := notifications.MockNotificationService() + + type calls struct { + expireCalled bool + createCalled bool + updateCalled bool + } + + verifier := ProvideVerifier(us, ts, ns) + t.Run("should error if email already exist for other user", func(t *testing.T) { + us.ExpectedUser = &user.User{ID: 1} + err := verifier.VerifyEmail(context.Background(), user.VerifyEmailCommand{ + User: user.User{ID: 2}, + Email: "some@email.com", + Action: user.EmailUpdateAction, + }) + + assert.ErrorIs(t, err, user.ErrEmailConflict) + }) + + t.Run("should succeed when no user has the email", func(t *testing.T) { + us.ExpectedUser = nil + var c calls + ts.ExpirePreviousVerificationsFN = func(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error { + c.expireCalled = true + return nil + } + + ts.CreateTempUserFN = func(ctx context.Context, cmd *tempuser.CreateTempUserCommand) (*tempuser.TempUser, error) { + c.createCalled = true + return &tempuser.TempUser{ + OrgID: cmd.OrgID, + Email: cmd.Email, + Name: cmd.Name, + InvitedByUserID: cmd.InvitedByUserID, + Code: cmd.Code, + }, nil + } + + ts.UpdateTempUserWithEmailSentFN = func(ctx context.Context, cmd *tempuser.UpdateTempUserWithEmailSentCommand) error { + c.updateCalled = true + return nil + } + err := verifier.VerifyEmail(context.Background(), user.VerifyEmailCommand{ + User: user.User{ID: 2}, + Email: "some@email.com", + Action: user.EmailUpdateAction, + }) + + assert.ErrorIs(t, err, nil) + assert.True(t, c.expireCalled) + assert.True(t, c.createCalled) + assert.True(t, c.updateCalled) + }) + + t.Run("should succeed when the user holding the email is the same user that want to verify it", func(t *testing.T) { + us.ExpectedUser = &user.User{ID: 2} + var c calls + ts.ExpirePreviousVerificationsFN = func(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error { + c.expireCalled = true + return nil + } + + ts.CreateTempUserFN = func(ctx context.Context, cmd *tempuser.CreateTempUserCommand) (*tempuser.TempUser, error) { + c.createCalled = true + return &tempuser.TempUser{ + OrgID: cmd.OrgID, + Email: cmd.Email, + Name: cmd.Name, + InvitedByUserID: cmd.InvitedByUserID, + Code: cmd.Code, + }, nil + } + + ts.UpdateTempUserWithEmailSentFN = func(ctx context.Context, cmd *tempuser.UpdateTempUserWithEmailSentCommand) error { + c.updateCalled = true + return nil + } + err := verifier.VerifyEmail(context.Background(), user.VerifyEmailCommand{ + User: user.User{ID: 2}, + Email: "some@email.com", + Action: user.EmailUpdateAction, + }) + + assert.ErrorIs(t, err, nil) + assert.True(t, c.expireCalled) + assert.True(t, c.createCalled) + assert.True(t, c.updateCalled) + }) +}