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 <ieva.vasiljeva@grafana.com>

emails/templates/verify_email.mjml

@@ -6,7 +6,7 @@
   <mj-head>
     <!-- ⬇ Don't forget to specify an email subject below! ⬇ -->
     <mj-title>
-      {{ Subject .Subject .TemplateData "Verify your new email - {{.Name}}" }}
+      {{ Subject .Subject .TemplateData "Verify your email - {{.Name}}" }}
     </mj-title>
     <mj-include path="./partials/layout/head.mjml" />
   </mj-head>

emails/templates/verify_email.txt

@@ -1,4 +1,4 @@
-[[HiddenSubject .Subject "Verify your new email - [[.Name]]"]]
+[[HiddenSubject .Subject "Verify your email - [[.Name]]"]]
 
 Hi [[.Name]],
 

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")

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")

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

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,

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{

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

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"),
 	)

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
 }

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

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, &notifications.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
+}

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