From 46c26bbd0bdd83fa19625f8a54fd80f8b52293ae Mon Sep 17 00:00:00 2001 From: Xavi Lacasa <114113189+volcanonoodle@users.noreply.github.com> Date: Fri, 16 Feb 2024 18:54:59 +0100 Subject: [PATCH] Auth: Fix email verification bypass when using basic authentication (#82914) --- emails/templates/invited_to_org.mjml | 2 +- emails/templates/new_user_invite.mjml | 2 +- emails/templates/ng_alert_notification.mjml | 2 +- emails/templates/reset_password.mjml | 2 +- emails/templates/signup_started.mjml | 2 +- emails/templates/verify_email_update.mjml | 40 + emails/templates/verify_email_update.txt | 6 + emails/templates/welcome_on_signup.mjml | 2 +- pkg/api/api.go | 5 + pkg/api/http_server.go | 4 +- pkg/api/user.go | 189 +++++ pkg/api/user_test.go | 688 +++++++++++++++++- pkg/services/cleanup/cleanup.go | 16 + pkg/services/notifications/mock.go | 28 +- pkg/services/notifications/models.go | 6 + pkg/services/notifications/notifications.go | 24 + pkg/services/temp_user/model.go | 19 +- pkg/services/temp_user/temp_user.go | 2 + pkg/services/temp_user/tempuserimpl/store.go | 26 + .../temp_user/tempuserimpl/store_test.go | 57 +- .../temp_user/tempuserimpl/temp_user.go | 16 + pkg/services/user/model.go | 7 + pkg/setting/setting.go | 14 +- public/api-merged.json | 21 + public/emails/verify_email_update.html | 215 ++++++ public/emails/verify_email_update.txt | 9 + public/openapi3.json | 21 + 27 files changed, 1403 insertions(+), 22 deletions(-) create mode 100644 emails/templates/verify_email_update.mjml create mode 100644 emails/templates/verify_email_update.txt create mode 100644 public/emails/verify_email_update.html create mode 100644 public/emails/verify_email_update.txt diff --git a/emails/templates/invited_to_org.mjml b/emails/templates/invited_to_org.mjml index 8444cae1887..848a2c9a597 100644 --- a/emails/templates/invited_to_org.mjml +++ b/emails/templates/invited_to_org.mjml @@ -4,7 +4,7 @@ - + {{ Subject .Subject .TemplateData "{{ .InvitedBy }} has added you to the {{ .OrgName }} organization" }} diff --git a/emails/templates/new_user_invite.mjml b/emails/templates/new_user_invite.mjml index 4f94d0f879a..eecc501b041 100644 --- a/emails/templates/new_user_invite.mjml +++ b/emails/templates/new_user_invite.mjml @@ -4,7 +4,7 @@ - + {{ Subject .Subject .TemplateData "{{ .InvitedBy }} has invited you to join Grafana" }} diff --git a/emails/templates/ng_alert_notification.mjml b/emails/templates/ng_alert_notification.mjml index 2a75a010535..adf7cb1fda5 100644 --- a/emails/templates/ng_alert_notification.mjml +++ b/emails/templates/ng_alert_notification.mjml @@ -4,7 +4,7 @@ - + {{ Subject .Subject .TemplateData "{{ .Title }}" }} diff --git a/emails/templates/reset_password.mjml b/emails/templates/reset_password.mjml index 8a0161f4533..81614a70290 100644 --- a/emails/templates/reset_password.mjml +++ b/emails/templates/reset_password.mjml @@ -4,7 +4,7 @@ - + {{ Subject .Subject .TemplateData "Reset your Grafana password - {{.Name}}" }} diff --git a/emails/templates/signup_started.mjml b/emails/templates/signup_started.mjml index ff211fbd95c..19beca31247 100644 --- a/emails/templates/signup_started.mjml +++ b/emails/templates/signup_started.mjml @@ -4,7 +4,7 @@ - + {{ Subject .Subject .TemplateData "Welcome to Grafana, please complete your sign up!" }} diff --git a/emails/templates/verify_email_update.mjml b/emails/templates/verify_email_update.mjml new file mode 100644 index 00000000000..75d4add8164 --- /dev/null +++ b/emails/templates/verify_email_update.mjml @@ -0,0 +1,40 @@ + + + + + + + + + {{ Subject .Subject .TemplateData "Verify your new email - {{.Name}}" }} + + + + + + + + + + +

Hi {{ .Name }},

+ + + Please click the following link to verify your email within {{ .VerificationEmailLifetimeHours }} hour(s). + + + Verify Email + + + You can also copy and paste this link into your browser directly: + + + {{ .AppUrl }}user/email/update?code={{ .Code }} + + + + + + + + diff --git a/emails/templates/verify_email_update.txt b/emails/templates/verify_email_update.txt new file mode 100644 index 00000000000..ae0d29e314e --- /dev/null +++ b/emails/templates/verify_email_update.txt @@ -0,0 +1,6 @@ +[[HiddenSubject .Subject "Verify your new email - [[.Name]]"]] + +Hi [[.Name]], + +Copy and paste the following link directly in your browser to verify your email within [[.VerificationEmailLifetimeHours]] hour(s). +[[.AppUrl]]user/email/update?code=[[.Code]] diff --git a/emails/templates/welcome_on_signup.mjml b/emails/templates/welcome_on_signup.mjml index 686842eb8ce..918a4ebf7d7 100644 --- a/emails/templates/welcome_on_signup.mjml +++ b/emails/templates/welcome_on_signup.mjml @@ -4,7 +4,7 @@ - + {{ Subject .Subject .TemplateData "Welcome to Grafana" }} diff --git a/pkg/api/api.go b/pkg/api/api.go index ec95b4a7b2f..7f183d79f4b 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -189,6 +189,11 @@ func (hs *HTTPServer) registerRoutes() { r.Post("/api/user/signup", quota(user.QuotaTargetSrv), quota(org.QuotaTargetSrv), routing.Wrap(hs.SignUp)) r.Post("/api/user/signup/step2", routing.Wrap(hs.SignUpStep2)) + // update user email + if hs.Cfg.Smtp.Enabled && hs.Cfg.VerifyEmailEnabled { + r.Get("/user/email/update", reqSignedInNoAnonymous, routing.Wrap(hs.UpdateUserEmail)) + } + // invited r.Get("/api/user/invite/:code", routing.Wrap(hs.GetInviteInfoByCode)) r.Post("/api/user/invite/complete", routing.Wrap(hs.CompleteInvite)) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index c7cce87d03f..a52555fb366 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -179,7 +179,7 @@ type HTTPServer struct { queryDataService query.Service serviceAccountsService serviceaccounts.Service authInfoService login.AuthInfoService - NotificationService *notifications.NotificationService + NotificationService notifications.Service DashboardService dashboards.DashboardService dashboardProvisioningService dashboards.DashboardProvisioningService folderService folder.Service @@ -242,7 +242,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi dataSourcesService datasources.DataSourceService, queryDataService query.Service, pluginFileStore plugins.FileStore, serviceaccountsService serviceaccounts.Service, authInfoService login.AuthInfoService, storageService store.StorageService, - notificationService *notifications.NotificationService, dashboardService dashboards.DashboardService, + notificationService notifications.Service, dashboardService dashboards.DashboardService, dashboardProvisioningService dashboards.DashboardProvisioningService, folderService folder.Service, dsGuardian guardian.DatasourceGuardianProvider, alertNotificationService *alerting.AlertNotificationService, dashboardsnapshotsService dashboardsnapshots.Service, pluginSettings pluginSettings.Service, diff --git a/pkg/api/user.go b/pkg/api/user.go index dd71c70a14d..73e30ac0a86 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -4,16 +4,21 @@ import ( "context" "errors" "net/http" + "net/mail" + "net/url" "strconv" "strings" + "time" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "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" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" @@ -125,6 +130,7 @@ func (hs *HTTPServer) GetUserByLoginOrEmail(c *contextmodel.ReqContext) response // 200: okResponse // 401: unauthorisedError // 403: forbiddenError +// 409: conflictError // 500: internalServerError func (hs *HTTPServer) UpdateSignedInUser(c *contextmodel.ReqContext) response.Response { cmd := user.UpdateUserCommand{} @@ -165,6 +171,7 @@ func (hs *HTTPServer) UpdateSignedInUser(c *contextmodel.ReqContext) response.Re // 401: unauthorisedError // 403: forbiddenError // 404: notFoundError +// 409: conflictError // 500: internalServerError func (hs *HTTPServer) UpdateUser(c *contextmodel.ReqContext) response.Response { cmd := user.UpdateUserCommand{} @@ -228,6 +235,39 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC return response.Err(user.ErrEmptyUsernameAndEmail.Errorf("user cannot be created with empty username and email")) } + // If email is being updated, we need to verify it. Likewise, if username is being updated and the new username + // is an email, we also need to verify it. + // To avoid breaking changes, email verification is implemented in a way that if the email field is being updated, + // all the other fields being updated in the same request are disregarded. We do this because email might need to + // be verified and if so, it goes through a different code flow. + if hs.Cfg.Smtp.Enabled && hs.Cfg.VerifyEmailEnabled { + query := user.GetUserByIDQuery{ID: cmd.UserID} + 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.StatusInternalServerError, "Failed to get user", err) + } + + if len(cmd.Email) != 0 && usr.Email != cmd.Email { + // Email is being updated + newEmail, err := ValidateAndNormalizeEmail(cmd.Email) + if err != nil { + return response.Error(http.StatusBadRequest, "Invalid email address", err) + } + + return hs.verifyEmailUpdate(ctx, newEmail, 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) + } + } + } + if err := hs.userService.Update(ctx, &cmd); err != nil { if errors.Is(err, user.ErrCaseInsensitive) { return response.Error(http.StatusConflict, "Update would result in user login conflict", err) @@ -238,6 +278,104 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC return response.Success("User updated") } +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, + 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) + } + + return response.Success("Email sent for verification") +} + +// swagger:route GET /user/email/update user updateUserEmail +// +// Update user email. +// +// Update the email of user given a verification code. +// +// Responses: +// 302: okResponse +func (hs *HTTPServer) UpdateUserEmail(c *contextmodel.ReqContext) response.Response { + var err error + + q := c.Req.URL.Query() + code, err := url.QueryUnescape(q.Get("code")) + if err != nil || code == "" { + return hs.RedirectResponseWithError(c, errors.New("bad request data")) + } + + tempUser, err := hs.validateEmailCode(c.Req.Context(), code) + if err != nil { + return hs.RedirectResponseWithError(c, err) + } + + cmd, err := hs.updateCmdFromEmailVerification(c.Req.Context(), tempUser) + if err != nil { + return hs.RedirectResponseWithError(c, err) + } + + if err := hs.userService.Update(c.Req.Context(), cmd); err != nil { + if errors.Is(err, user.ErrCaseInsensitive) { + return hs.RedirectResponseWithError(c, errors.New("update would result in user login conflict")) + } + return hs.RedirectResponseWithError(c, errors.New("failed to update user")) + } + + // Mark temp user as completed + updateTmpUserCmd := tempuser.UpdateTempUserStatusCommand{Code: code, Status: tempuser.TmpUserEmailUpdateCompleted} + if err := hs.tempUserService.UpdateTempUserStatus(c.Req.Context(), &updateTmpUserCmd); err != nil { + return hs.RedirectResponseWithError(c, errors.New("failed to update verification status")) + } + + return response.Redirect(hs.Cfg.AppSubURL + "/profile") +} + func (hs *HTTPServer) isExternalUser(ctx context.Context, userID int64) (bool, error) { getAuthQuery := login.GetAuthInfoQuery{UserId: userID} var err error @@ -603,6 +741,57 @@ func getUserID(c *contextmodel.ReqContext) (int64, *response.NormalResponse) { return userID, nil } +func (hs *HTTPServer) updateCmdFromEmailVerification(ctx context.Context, tempUser *tempuser.TempUserDTO) (*user.UpdateUserCommand, error) { + userQuery := user.GetUserByLoginQuery{LoginOrEmail: tempUser.InvitedByLogin} + usr, err := hs.userService.GetByLogin(ctx, &userQuery) + if err != nil { + if errors.Is(err, user.ErrUserNotFound) { + return nil, user.ErrUserNotFound + } + return nil, errors.New("failed to get user") + } + + cmd := &user.UpdateUserCommand{UserID: usr.ID, Email: tempUser.Email} + + switch tempUser.Name { + case string(user.EmailUpdateAction): + // User updated the email field + if _, err := mail.ParseAddress(usr.Login); err == nil { + // If username was also an email, we update it to keep it in sync with the email field + cmd.Login = tempUser.Email + } + case string(user.LoginUpdateAction): + // User updated the username field with a new email + cmd.Login = tempUser.Email + default: + return nil, errors.New("trying to update email on unknown field") + } + return cmd, nil +} + +func (hs *HTTPServer) validateEmailCode(ctx context.Context, code string) (*tempuser.TempUserDTO, error) { + tempUserQuery := tempuser.GetTempUserByCodeQuery{Code: code} + tempUser, err := hs.tempUserService.GetTempUserByCode(ctx, &tempUserQuery) + if err != nil { + if errors.Is(err, tempuser.ErrTempUserNotFound) { + return nil, errors.New("invalid email verification code") + } + return nil, errors.New("failed to read temp user") + } + + if tempUser.Status != tempuser.TmpUserEmailUpdateStarted { + return nil, errors.New("invalid email verification code") + } + if !tempUser.EmailSent { + return nil, errors.New("verification email was not recorded as sent") + } + if tempUser.EmailSentOn.Add(hs.Cfg.VerificationEmailMaxLifetime).Before(time.Now()) { + return nil, errors.New("invalid email verification code") + } + + return tempUser, nil +} + // swagger:parameters searchUsers type SearchUsersParams struct { // Limit the maximum number of users to return per page diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index e8b96c5b2fa..ee1ffe50417 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -5,9 +5,18 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" + "strings" "testing" "time" + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/notifications" + "github.com/grafana/grafana/pkg/services/secrets/fakes" + tempuser "github.com/grafana/grafana/pkg/services/temp_user" + "github.com/grafana/grafana/pkg/services/temp_user/tempuserimpl" + "github.com/grafana/grafana/pkg/web/webtest" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -40,6 +49,8 @@ import ( "github.com/grafana/grafana/pkg/setting" ) +const newEmail = "newEmail@localhost" + func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { settings := setting.NewCfg() sqlStore := db.InitTestDB(t) @@ -69,7 +80,6 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { hs.authInfoService = srv orgSvc, err := orgimpl.ProvideService(sqlStore, sqlStore.Cfg, quotatest.New(false, nil)) require.NoError(t, err) - require.NoError(t, err) userSvc, err := userimpl.ProvideService(sqlStore, orgSvc, sc.cfg, nil, nil, quotatest.New(false, nil), supportbundlestest.NewFakeBundleService()) require.NoError(t, err) hs.userService = userSvc @@ -363,6 +373,682 @@ func TestHTTPServer_UpdateUser(t *testing.T) { }, hs) } +func setupUpdateEmailTests(t *testing.T, cfg *setting.Cfg) (*user.User, *HTTPServer, *notifications.NotificationServiceMock) { + t.Helper() + + sqlStore := db.InitTestDB(t) + sqlStore.Cfg = cfg + + tempUserService := tempuserimpl.ProvideService(sqlStore, cfg) + orgSvc, err := orgimpl.ProvideService(sqlStore, cfg, quotatest.New(false, nil)) + require.NoError(t, err) + userSvc, err := userimpl.ProvideService(sqlStore, orgSvc, cfg, nil, nil, quotatest.New(false, nil), supportbundlestest.NewFakeBundleService()) + require.NoError(t, err) + + // Create test user + createUserCmd := user.CreateUserCommand{ + Email: "testuser@localhost", + Name: "testuser", + Login: "loginuser", + Company: "testCompany", + IsAdmin: true, + } + usr, err := userSvc.Create(context.Background(), &createUserCmd) + require.NoError(t, err) + + nsMock := notifications.MockNotificationService() + + hs := &HTTPServer{ + Cfg: cfg, + SQLStore: sqlStore, + userService: userSvc, + tempUserService: tempUserService, + NotificationService: nsMock, + } + return usr, hs, nsMock +} + +func TestUser_UpdateEmail(t *testing.T) { + cases := []struct { + Name string + Field user.UpdateEmailActionType + }{ + { + Name: "Updating Email field", + Field: user.EmailUpdateAction, + }, + { + Name: "Updating Login (username) field", + Field: user.LoginUpdateAction, + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + t.Run("With verification disabled should update without verifying", func(t *testing.T) { + tests := []struct { + name string + smtpConfigured bool + verifyEmailEnabled bool + }{ + { + name: "SMTP not configured", + smtpConfigured: false, + verifyEmailEnabled: true, + }, + { + name: "config verify_email_enabled = false", + smtpConfigured: true, + verifyEmailEnabled: false, + }, + { + name: "config verify_email_enabled = false and SMTP not configured", + smtpConfigured: false, + verifyEmailEnabled: false, + }, + } + for _, ttt := range tests { + settings := setting.NewCfg() + settings.Smtp.Enabled = ttt.smtpConfigured + settings.VerifyEmailEnabled = ttt.verifyEmailEnabled + + usr, hs, nsMock := setupUpdateEmailTests(t, settings) + + updateUserCommand := user.UpdateUserCommand{ + Email: usr.Email, + Name: "newName", + Login: usr.Login, + UserID: usr.ID, + } + + switch tt.Field { + case user.LoginUpdateAction: + updateUserCommand.Login = newEmail + case user.EmailUpdateAction: + updateUserCommand.Email = newEmail + } + + fn := func(sc *scenarioContext) { + // User is internal + sc.authInfoService.ExpectedError = user.ErrUserNotFound + + sc.fakeReqWithParams("PUT", sc.url, nil).exec() + assert.Equal(t, http.StatusOK, sc.resp.Code) + + // Verify that no email has been sent after update + require.False(t, nsMock.EmailVerified) + + userQuery := user.GetUserByIDQuery{ID: usr.ID} + updatedUsr, err := hs.userService.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + + // Verify fields have been updated + require.NotEqual(t, usr.Name, updatedUsr.Name) + require.Equal(t, updateUserCommand.Name, updatedUsr.Name) + + switch tt.Field { + case user.LoginUpdateAction: + require.Equal(t, usr.Email, updatedUsr.Email) + require.NotEqual(t, usr.Login, updatedUsr.Login) + require.Equal(t, updateUserCommand.Login, updatedUsr.Login) + case user.EmailUpdateAction: + require.Equal(t, usr.Login, updatedUsr.Login) + require.NotEqual(t, usr.Email, updatedUsr.Email) + require.Equal(t, updateUserCommand.Email, updatedUsr.Email) + } + + // Verify other fields have been kept + require.Equal(t, usr.Company, updatedUsr.Company) + } + + updateUserScenario(t, updateUserContext{ + desc: ttt.name, + url: fmt.Sprintf("/api/users/%d", usr.ID), + routePattern: "/api/users/:id", + cmd: updateUserCommand, + fn: fn, + }, hs) + + updateSignedInUserScenario(t, updateUserContext{ + desc: ttt.name, + url: "/api/user", + routePattern: "/api/user", + cmd: updateUserCommand, + fn: fn, + }, hs) + } + }) + }) + } + + doReq := func(req *http.Request, usr *user.User) (*http.Response, error) { + r := webtest.RequestWithSignedInUser( + req, + authedUserWithPermissions( + usr.ID, + usr.OrgID, + []accesscontrol.Permission{ + { + Action: accesscontrol.ActionUsersWrite, + Scope: accesscontrol.ScopeGlobalUsersAll, + }, + }, + ), + ) + client := &http.Client{ + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }} + return client.Do(r) + } + + sendUpdateReq := func(server *webtest.Server, usr *user.User, body string) { + req := server.NewRequest( + http.MethodPut, + "/api/user", + strings.NewReader(body), + ) + req.Header.Add("Content-Type", "application/json") + res, err := doReq(req, usr) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, res.StatusCode) + require.NoError(t, res.Body.Close()) + } + + sendVerificationReq := func(server *webtest.Server, usr *user.User, code string) { + url := fmt.Sprintf("/user/email/update?code=%s", url.QueryEscape(code)) + req := server.NewGetRequest(url) + res, err := doReq(req, usr) + require.NoError(t, err) + assert.Equal(t, http.StatusFound, res.StatusCode) + require.NoError(t, res.Body.Close()) + } + + getVerificationTempUser := func(tempUserSvc tempuser.Service, code string) *tempuser.TempUserDTO { + tmpUserQuery := tempuser.GetTempUserByCodeQuery{Code: code} + tmpUser, err := tempUserSvc.GetTempUserByCode(context.Background(), &tmpUserQuery) + require.NoError(t, err) + return tmpUser + } + + verifyEmailData := func(tempUserSvc tempuser.Service, nsMock *notifications.NotificationServiceMock, originalUsr *user.User, newEmail string) { + verification := nsMock.EmailVerification + tmpUsr := getVerificationTempUser(tempUserSvc, verification.Code) + + require.True(t, nsMock.EmailVerified) + require.Equal(t, newEmail, verification.Email) + require.Equal(t, originalUsr.ID, verification.User.ID) + require.Equal(t, tmpUsr.Code, verification.Code) + } + + verifyUserNotUpdated := func(userSvc user.Service, usr *user.User) { + userQuery := user.GetUserByIDQuery{ID: usr.ID} + checkUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.Equal(t, usr.Email, checkUsr.Email) + require.Equal(t, usr.Login, checkUsr.Login) + require.Equal(t, usr.Name, checkUsr.Name) + } + + setupScenario := func(cfg *setting.Cfg) (*webtest.Server, user.Service, tempuser.Service, *notifications.NotificationServiceMock) { + settings := setting.NewCfg() + settings.Smtp.Enabled = true + settings.VerificationEmailMaxLifetime = 1 * time.Hour + settings.VerifyEmailEnabled = true + + if cfg != nil { + settings = cfg + } + + nsMock := notifications.MockNotificationService() + sqlStore := db.InitTestDB(t) + sqlStore.Cfg = settings + + tempUserSvc := tempuserimpl.ProvideService(sqlStore, settings) + orgSvc, err := orgimpl.ProvideService(sqlStore, settings, quotatest.New(false, nil)) + require.NoError(t, err) + userSvc, err := userimpl.ProvideService(sqlStore, orgSvc, settings, nil, nil, quotatest.New(false, nil), supportbundlestest.NewFakeBundleService()) + require.NoError(t, err) + + server := SetupAPITestServer(t, func(hs *HTTPServer) { + hs.Cfg = settings + + hs.SQLStore = sqlStore + hs.userService = userSvc + hs.tempUserService = tempUserSvc + hs.NotificationService = nsMock + hs.SecretsService = fakes.NewFakeSecretsService() + // User is internal + hs.authInfoService = &authinfotest.FakeService{ExpectedError: user.ErrUserNotFound} + }) + + return server, userSvc, tempUserSvc, nsMock + } + + createUser := func(userSvc user.Service, name string, email string, login string) *user.User { + createUserCmd := user.CreateUserCommand{ + Email: email, + Name: name, + Login: login, + Company: "testCompany", + IsAdmin: true, + } + usr, err := userSvc.Create(context.Background(), &createUserCmd) + require.NoError(t, err) + return usr + } + + t.Run("Update Email and disregard other fields", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + newName := "newName" + body := fmt.Sprintf(`{"email": "%s", "name": "%s"}`, newEmail, newName) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow, when user clicks email button + code := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, code) + + // Verify Email has been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Email, updatedUsr.Email) + require.Equal(t, newEmail, updatedUsr.Email) + // Fields unchanged + require.Equal(t, originalUsr.Login, updatedUsr.Login) + require.Equal(t, originalUsr.Name, updatedUsr.Name) + require.NotEqual(t, newName, updatedUsr.Name) + }) + + t.Run("Update Email when Login was also an email should update both", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "email@localhost") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + body := fmt.Sprintf(`{"email": "%s"}`, newEmail) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow, when user clicks email button + code := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, code) + + // Verify Email and Login have been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Email, updatedUsr.Email) + require.Equal(t, newEmail, updatedUsr.Email) + require.Equal(t, newEmail, updatedUsr.Login) + // Fields unchanged + require.Equal(t, originalUsr.Name, updatedUsr.Name) + }) + + t.Run("Update Login with an email should update Email too", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + body := fmt.Sprintf(`{"login": "%s"}`, newEmail) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow, when user clicks email button + code := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, code) + + // Verify Email and Login have been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Email, updatedUsr.Email) + require.NotEqual(t, originalUsr.Login, updatedUsr.Login) + require.Equal(t, newEmail, updatedUsr.Email) + require.Equal(t, newEmail, updatedUsr.Login) + // Fields unchanged + require.Equal(t, originalUsr.Name, updatedUsr.Name) + }) + + t.Run("Update Login should not need verification if it is not an email", func(t *testing.T) { + server, userSvc, _, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + newLogin := "newLogin" + newName := "newName" + body := fmt.Sprintf(`{"login": "%s", "name": "%s"}`, newLogin, newName) + sendUpdateReq(server, originalUsr, body) + + // Verify that email has not been sent + require.False(t, nsMock.EmailVerified) + + // Verify Login has been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Login, updatedUsr.Login) + require.NotEqual(t, originalUsr.Name, updatedUsr.Name) + require.Equal(t, newLogin, updatedUsr.Login) + require.Equal(t, newName, updatedUsr.Name) + // Fields unchanged + require.Equal(t, originalUsr.Email, updatedUsr.Email) + }) + + t.Run("Update Login should not need verification if it is being updated to the already configured email", func(t *testing.T) { + server, userSvc, _, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + body := fmt.Sprintf(`{"login": "%s"}`, originalUsr.Email) + sendUpdateReq(server, originalUsr, body) + + // Verify that email has not been sent + require.False(t, nsMock.EmailVerified) + + // Verify Login has been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Login, updatedUsr.Login) + require.Equal(t, originalUsr.Email, updatedUsr.Login) + require.Equal(t, originalUsr.Email, updatedUsr.Email) + }) + + t.Run("Update Login and Email with different email values at once should disregard the Login update", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + newLogin := "newEmail2@localhost" + body := fmt.Sprintf(`{"email": "%s", "login": "%s"}`, newEmail, newLogin) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow, when user clicks email button + code := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, code) + + // Verify only Email has been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Email, updatedUsr.Email) + require.Equal(t, newEmail, updatedUsr.Email) + // Fields unchanged + require.NotEqual(t, newLogin, updatedUsr.Login) + require.Equal(t, originalUsr.Login, updatedUsr.Login) + require.Equal(t, originalUsr.Name, updatedUsr.Name) + }) + + t.Run("Update Login and Email with different email values at once when Login was already an email should update both with Email", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "email@localhost") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + newLogin := "newEmail2@localhost" + body := fmt.Sprintf(`{"email": "%s", "login": "%s"}`, newEmail, newLogin) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow, when user clicks email button + code := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, code) + + // Verify only Email has been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Email, updatedUsr.Email) + require.NotEqual(t, originalUsr.Login, updatedUsr.Login) + require.NotEqual(t, newLogin, updatedUsr.Login) + require.Equal(t, newEmail, updatedUsr.Email) + require.Equal(t, newEmail, updatedUsr.Login) + // Fields unchanged + require.Equal(t, originalUsr.Name, updatedUsr.Name) + }) + + t.Run("Email verification should expire", func(t *testing.T) { + cfg := setting.NewCfg() + cfg.Smtp.Enabled = true + cfg.VerificationEmailMaxLifetime = 0 // Expire instantly + cfg.VerifyEmailEnabled = true + + server, userSvc, tempUserSvc, nsMock := setupScenario(cfg) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + body := fmt.Sprintf(`{"email": "%s"}`, newEmail) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow, when user clicks email button + code := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, code) + + // Verify user has not been updated + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, newEmail, updatedUsr.Email) + require.Equal(t, originalUsr.Email, updatedUsr.Email) + require.Equal(t, originalUsr.Login, updatedUsr.Login) + }) + + t.Run("A new verification should revoke other pending verifications", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // First email verification + firstNewEmail := "newEmail1@localhost" + body := fmt.Sprintf(`{"email": "%s"}`, firstNewEmail) + sendUpdateReq(server, originalUsr, body) + verifyEmailData(tempUserSvc, nsMock, originalUsr, firstNewEmail) + firstCode := nsMock.EmailVerification.Code + + // Second email verification + secondNewEmail := "newEmail2@localhost" + body = fmt.Sprintf(`{"email": "%s"}`, secondNewEmail) + sendUpdateReq(server, originalUsr, body) + verifyEmailData(tempUserSvc, nsMock, originalUsr, secondNewEmail) + secondCode := nsMock.EmailVerification.Code + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Try to follow through with the first verification unsuccessfully + sendVerificationReq(server, originalUsr, firstCode) + verifyUserNotUpdated(userSvc, originalUsr) + + // Follow through with second verification successfully + sendVerificationReq(server, originalUsr, secondCode) + + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.NotEqual(t, originalUsr.Email, updatedUsr.Email) + require.Equal(t, secondNewEmail, updatedUsr.Email) + // Fields unchanged + require.Equal(t, originalUsr.Login, updatedUsr.Login) + }) + + t.Run("Email verification should fail if code is not valid", func(t *testing.T) { + server, userSvc, tempUserSvc, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Start email update + body := fmt.Sprintf(`{"email": "%s"}`, newEmail) + sendUpdateReq(server, originalUsr, body) + + // Verify email data + verifyEmailData(tempUserSvc, nsMock, originalUsr, newEmail) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Second part of the verification flow should fail if using the wrong code + sendVerificationReq(server, originalUsr, "notTheRightCode") + verifyUserNotUpdated(userSvc, originalUsr) + }) + + t.Run("Email verification code can only be used once", func(t *testing.T) { + server, userSvc, _, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name", "email@localhost", "login") + + // Start email update + require.NotEqual(t, originalUsr.Email, newEmail) + + body := fmt.Sprintf(`{"email": "%s"}`, newEmail) + sendUpdateReq(server, originalUsr, body) + + // Verify user has not been updated yet + verifyUserNotUpdated(userSvc, originalUsr) + + // Use code to verify successfully + codeToReuse := nsMock.EmailVerification.Code + sendVerificationReq(server, originalUsr, codeToReuse) + + // User should have an updated Email + userQuery := user.GetUserByIDQuery{ID: originalUsr.ID} + updatedUsr, err := userSvc.GetByID(context.Background(), &userQuery) + require.NoError(t, err) + require.Equal(t, newEmail, updatedUsr.Email) + + // Change email back to what it was + body = fmt.Sprintf(`{"email": "%s"}`, originalUsr.Email) + sendUpdateReq(server, originalUsr, body) + sendVerificationReq(server, originalUsr, nsMock.EmailVerification.Code) + verifyUserNotUpdated(userSvc, originalUsr) + + // Re-use code to verify new email again, unsuccessfully + sendVerificationReq(server, originalUsr, codeToReuse) + verifyUserNotUpdated(userSvc, originalUsr) + }) + + t.Run("Update Email with an email that is already being used should fail", func(t *testing.T) { + testCases := []struct { + description string + clashLogin bool + }{ + { + description: "when Email clashes", + clashLogin: false, + }, + { + description: "when Login clashes", + clashLogin: true, + }, + } + for _, tt := range testCases { + t.Run(tt.description, func(t *testing.T) { + server, userSvc, _, nsMock := setupScenario(nil) + + originalUsr := createUser(userSvc, "name1", "email1@localhost", "login1@localhost") + badUsr := createUser(userSvc, "name2", "email2@localhost", "login2") + + // Verify that no email has been sent yet + require.False(t, nsMock.EmailVerified) + + // Update `badUsr` to use the same email as `originalUsr` + body := fmt.Sprintf(`{"email": "%s"}`, originalUsr.Email) + if tt.clashLogin { + body = fmt.Sprintf(`{"login": "%s"}`, originalUsr.Login) + } + req := server.NewRequest( + http.MethodPut, + "/api/user", + strings.NewReader(body), + ) + req.Header.Add("Content-Type", "application/json") + res, err := doReq(req, badUsr) + require.NoError(t, err) + assert.Equal(t, http.StatusConflict, res.StatusCode) + require.NoError(t, res.Body.Close()) + + // Verify that no email has been sent + require.False(t, nsMock.EmailVerified) + + // Verify user has not been updated + verifyUserNotUpdated(userSvc, badUsr) + }) + } + }) +} + type updateUserContext struct { desc string url string diff --git a/pkg/services/cleanup/cleanup.go b/pkg/services/cleanup/cleanup.go index d7a25bec37e..3b1a2c5e201 100644 --- a/pkg/services/cleanup/cleanup.go +++ b/pkg/services/cleanup/cleanup.go @@ -102,6 +102,7 @@ func (srv *CleanUpService) clean(ctx context.Context) { {"expire old user invites", srv.expireOldUserInvites}, {"delete stale short URLs", srv.deleteStaleShortURLs}, {"delete stale query history", srv.deleteStaleQueryHistory}, + {"expire old email verifications", srv.expireOldVerifications}, } logger := srv.log.FromContext(ctx) @@ -238,6 +239,21 @@ func (srv *CleanUpService) expireOldUserInvites(ctx context.Context) { } } +func (srv *CleanUpService) expireOldVerifications(ctx context.Context) { + logger := srv.log.FromContext(ctx) + maxVerificationLifetime := srv.Cfg.VerificationEmailMaxLifetime + + cmd := tempuser.ExpireTempUsersCommand{ + OlderThan: time.Now().Add(-maxVerificationLifetime), + } + + if err := srv.tempUserService.ExpireOldVerifications(ctx, &cmd); err != nil { + logger.Error("Problem expiring email verifications", "error", err.Error()) + } else { + logger.Debug("Expired email verifications", "rows affected", cmd.NumExpired) + } +} + func (srv *CleanUpService) deleteStaleShortURLs(ctx context.Context) { logger := srv.log.FromContext(ctx) cmd := shorturls.DeleteShortUrlCommand{ diff --git a/pkg/services/notifications/mock.go b/pkg/services/notifications/mock.go index 2e874ad6570..89240b26bcb 100644 --- a/pkg/services/notifications/mock.go +++ b/pkg/services/notifications/mock.go @@ -2,13 +2,17 @@ package notifications import ( "context" + + "github.com/grafana/grafana/pkg/services/user" ) type NotificationServiceMock struct { - Webhook SendWebhookSync - EmailSync SendEmailCommandSync - Email SendEmailCommand - ShouldError error + Webhook SendWebhookSync + EmailSync SendEmailCommandSync + Email SendEmailCommand + EmailVerified bool + EmailVerification SendVerifyEmailCommand + ShouldError error WebhookHandler func(context.Context, *SendWebhookSync) error EmailHandlerSync func(context.Context, *SendEmailCommandSync) error @@ -39,4 +43,20 @@ func (ns *NotificationServiceMock) SendEmailCommandHandler(ctx context.Context, return ns.ShouldError } +func (ns *NotificationServiceMock) SendResetPasswordEmail(ctx context.Context, cmd *SendResetPasswordEmailCommand) error { + // TODO: Implement if needed + return ns.ShouldError +} + +func (ns *NotificationServiceMock) ValidateResetPasswordCode(ctx context.Context, query *ValidateResetPasswordCodeQuery, userByLogin GetUserByLoginFunc) (*user.User, error) { + // TODO: Implement if needed + return nil, ns.ShouldError +} + +func (ns *NotificationServiceMock) SendVerificationEmail(ctx context.Context, cmd *SendVerifyEmailCommand) error { + ns.EmailVerified = true + ns.EmailVerification = *cmd + return ns.ShouldError +} + func MockNotificationService() *NotificationServiceMock { return &NotificationServiceMock{} } diff --git a/pkg/services/notifications/models.go b/pkg/services/notifications/models.go index 1b266a6ed6b..289b2566c27 100644 --- a/pkg/services/notifications/models.go +++ b/pkg/services/notifications/models.go @@ -51,3 +51,9 @@ type SendResetPasswordEmailCommand struct { type ValidateResetPasswordCodeQuery struct { Code string } + +type SendVerifyEmailCommand struct { + User *user.User + Code string + Email string +} diff --git a/pkg/services/notifications/notifications.go b/pkg/services/notifications/notifications.go index 7773883b610..c387acc6283 100644 --- a/pkg/services/notifications/notifications.go +++ b/pkg/services/notifications/notifications.go @@ -28,15 +28,25 @@ type EmailSender interface { SendEmailCommandHandlerSync(ctx context.Context, cmd *SendEmailCommandSync) error SendEmailCommandHandler(ctx context.Context, cmd *SendEmailCommand) error } +type PasswordResetMailer interface { + SendResetPasswordEmail(ctx context.Context, cmd *SendResetPasswordEmailCommand) error + ValidateResetPasswordCode(ctx context.Context, query *ValidateResetPasswordCodeQuery, userByLogin GetUserByLoginFunc) (*user.User, error) +} +type EmailVerificationMailer interface { + SendVerificationEmail(ctx context.Context, cmd *SendVerifyEmailCommand) error +} type Service interface { WebhookSender EmailSender + PasswordResetMailer + EmailVerificationMailer } var mailTemplates *template.Template var tmplResetPassword = "reset_password" var tmplSignUpStarted = "signup_started" var tmplWelcomeOnSignUp = "welcome_on_signup" +var tmplVerifyEmail = "verify_email_update" func ProvideService(bus bus.Bus, cfg *setting.Cfg, mailer Mailer, store TempUserStore) (*NotificationService, error) { ns := &NotificationService{ @@ -257,6 +267,20 @@ func (ns *NotificationService) ValidateResetPasswordCode(ctx context.Context, qu return user, nil } +func (ns *NotificationService) SendVerificationEmail(ctx context.Context, cmd *SendVerifyEmailCommand) error { + return ns.SendEmailCommandHandlerSync(ctx, &SendEmailCommandSync{ + SendEmailCommand: SendEmailCommand{ + To: []string{cmd.Email}, + Template: tmplVerifyEmail, + Data: map[string]any{ + "Code": url.QueryEscape(cmd.Code), + "Name": cmd.User.Name, + "VerificationEmailLifetimeHours": int(ns.Cfg.VerificationEmailMaxLifetime.Hours()), + }, + }, + }) +} + func (ns *NotificationService) signUpStartedHandler(ctx context.Context, evt *events.SignUpStarted) error { if !ns.Cfg.VerifyEmailEnabled { return nil diff --git a/pkg/services/temp_user/model.go b/pkg/services/temp_user/model.go index 1e05d15d832..b0949a79381 100644 --- a/pkg/services/temp_user/model.go +++ b/pkg/services/temp_user/model.go @@ -15,11 +15,14 @@ var ( type TempUserStatus string const ( - TmpUserSignUpStarted TempUserStatus = "SignUpStarted" - TmpUserInvitePending TempUserStatus = "InvitePending" - TmpUserCompleted TempUserStatus = "Completed" - TmpUserRevoked TempUserStatus = "Revoked" - TmpUserExpired TempUserStatus = "Expired" + TmpUserSignUpStarted TempUserStatus = "SignUpStarted" + TmpUserInvitePending TempUserStatus = "InvitePending" + TmpUserCompleted TempUserStatus = "Completed" + TmpUserRevoked TempUserStatus = "Revoked" + TmpUserExpired TempUserStatus = "Expired" + TmpUserEmailUpdateStarted TempUserStatus = "EmailUpdateStarted" + TmpUserEmailUpdateCompleted TempUserStatus = "EmailUpdateCompleted" + TmpUserEmailUpdateExpired TempUserStatus = "EmailUpdateExpired" ) // TempUser holds data for org invites and unconfirmed sign ups @@ -67,6 +70,12 @@ type ExpireTempUsersCommand struct { NumExpired int64 } +type ExpirePreviousVerificationsCommand struct { + InvitedByUserID int64 + + NumExpired int64 +} + type UpdateTempUserWithEmailSentCommand struct { Code string } diff --git a/pkg/services/temp_user/temp_user.go b/pkg/services/temp_user/temp_user.go index 91b7e49801d..162e35459cd 100644 --- a/pkg/services/temp_user/temp_user.go +++ b/pkg/services/temp_user/temp_user.go @@ -11,4 +11,6 @@ type Service interface { GetTempUsersQuery(ctx context.Context, query *GetTempUsersQuery) ([]*TempUserDTO, error) GetTempUserByCode(ctx context.Context, query *GetTempUserByCodeQuery) (*TempUserDTO, error) ExpireOldUserInvites(ctx context.Context, cmd *ExpireTempUsersCommand) error + ExpireOldVerifications(ctx context.Context, cmd *ExpireTempUsersCommand) error + ExpirePreviousVerifications(ctx context.Context, cmd *ExpirePreviousVerificationsCommand) error } diff --git a/pkg/services/temp_user/tempuserimpl/store.go b/pkg/services/temp_user/tempuserimpl/store.go index a0ac0f66702..9affd894083 100644 --- a/pkg/services/temp_user/tempuserimpl/store.go +++ b/pkg/services/temp_user/tempuserimpl/store.go @@ -16,6 +16,8 @@ type store interface { GetTempUsersQuery(ctx context.Context, query *tempuser.GetTempUsersQuery) ([]*tempuser.TempUserDTO, error) GetTempUserByCode(ctx context.Context, query *tempuser.GetTempUserByCodeQuery) (*tempuser.TempUserDTO, error) ExpireOldUserInvites(ctx context.Context, cmd *tempuser.ExpireTempUsersCommand) error + ExpireOldVerifications(ctx context.Context, cmd *tempuser.ExpireTempUsersCommand) error + ExpirePreviousVerifications(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error } type xormStore struct { @@ -175,3 +177,27 @@ func (ss *xormStore) ExpireOldUserInvites(ctx context.Context, cmd *tempuser.Exp return nil }) } + +func (ss *xormStore) ExpireOldVerifications(ctx context.Context, cmd *tempuser.ExpireTempUsersCommand) error { + return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + var rawSQL = "UPDATE temp_user SET status = ?, updated = ? WHERE created <= ? AND status = ?" + if result, err := sess.Exec(rawSQL, string(tempuser.TmpUserEmailUpdateExpired), time.Now().Unix(), cmd.OlderThan.Unix(), string(tempuser.TmpUserEmailUpdateStarted)); err != nil { + return err + } else if cmd.NumExpired, err = result.RowsAffected(); err != nil { + return err + } + return nil + }) +} + +func (ss *xormStore) ExpirePreviousVerifications(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error { + return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + var rawSQL = "UPDATE temp_user SET status = ?, updated = ? WHERE invited_by_user_id = ? AND status = ?" + if result, err := sess.Exec(rawSQL, string(tempuser.TmpUserEmailUpdateExpired), time.Now().Unix(), cmd.InvitedByUserID, string(tempuser.TmpUserEmailUpdateStarted)); err != nil { + return err + } else if cmd.NumExpired, err = result.RowsAffected(); err != nil { + return err + } + return nil + }) +} diff --git a/pkg/services/temp_user/tempuserimpl/store_test.go b/pkg/services/temp_user/tempuserimpl/store_test.go index a5db34a8167..6bcd65acd0e 100644 --- a/pkg/services/temp_user/tempuserimpl/store_test.go +++ b/pkg/services/temp_user/tempuserimpl/store_test.go @@ -117,7 +117,32 @@ func TestIntegrationTempUserCommandsAndQueries(t *testing.T) { require.False(t, queryResult[0].EmailSentOn.UTC().Before(queryResult[0].Created.UTC())) }) - t.Run("Should be able expire temp user", func(t *testing.T) { + t.Run("Should be able expire all pending verifications from a user", func(t *testing.T) { + userID := int64(99) + verifications := 5 + cmd := tempuser.CreateTempUserCommand{ + OrgID: -1, + Name: "email-update", + Code: "asd", + Email: "e@as.co", + Status: tempuser.TmpUserEmailUpdateStarted, + InvitedByUserID: userID, + } + db := db.InitTestDB(t) + store = &xormStore{db: db, cfg: db.Cfg} + + for i := 0; i < verifications; i++ { + tempUser, err = store.CreateTempUser(context.Background(), &cmd) + require.Nil(t, err) + } + + cmd2 := tempuser.ExpirePreviousVerificationsCommand{InvitedByUserID: userID} + err := store.ExpirePreviousVerifications(context.Background(), &cmd2) + require.Nil(t, err) + require.Equal(t, int64(verifications), cmd2.NumExpired) + }) + + t.Run("Should be able expire temp user related to org invite", func(t *testing.T) { setup(t) createdAt := time.Unix(tempUser.Created, 0) cmd2 := tempuser.ExpireTempUsersCommand{OlderThan: createdAt.Add(1 * time.Second)} @@ -133,4 +158,34 @@ func TestIntegrationTempUserCommandsAndQueries(t *testing.T) { require.Equal(t, int64(0), cmd2.NumExpired) }) }) + + t.Run("Should be able expire temp user related to email verification", func(t *testing.T) { + cmd := tempuser.CreateTempUserCommand{ + OrgID: 2256, + Name: "email-update", + Code: "asd", + Email: "e@as.co", + Status: tempuser.TmpUserEmailUpdateStarted, + InvitedByUserID: 99, + } + db := db.InitTestDB(t) + store = &xormStore{db: db, cfg: db.Cfg} + + tempUser, err = store.CreateTempUser(context.Background(), &cmd) + require.Nil(t, err) + + createdAt := time.Unix(tempUser.Created, 0) + cmd2 := tempuser.ExpireTempUsersCommand{OlderThan: createdAt.Add(1 * time.Second)} + err := store.ExpireOldVerifications(context.Background(), &cmd2) + require.Nil(t, err) + require.Equal(t, int64(1), cmd2.NumExpired) + + t.Run("Should do nothing when no temp users to expire", func(t *testing.T) { + createdAt := time.Unix(tempUser.Created, 0) + cmd2 := tempuser.ExpireTempUsersCommand{OlderThan: createdAt.Add(1 * time.Second)} + err := store.ExpireOldVerifications(context.Background(), &cmd2) + require.Nil(t, err) + require.Equal(t, int64(0), cmd2.NumExpired) + }) + }) } diff --git a/pkg/services/temp_user/tempuserimpl/temp_user.go b/pkg/services/temp_user/tempuserimpl/temp_user.go index e6dd24fe452..89795f13640 100644 --- a/pkg/services/temp_user/tempuserimpl/temp_user.go +++ b/pkg/services/temp_user/tempuserimpl/temp_user.go @@ -68,3 +68,19 @@ func (s *Service) ExpireOldUserInvites(ctx context.Context, cmd *tempuser.Expire } return nil } + +func (s *Service) ExpireOldVerifications(ctx context.Context, cmd *tempuser.ExpireTempUsersCommand) error { + err := s.store.ExpireOldVerifications(ctx, cmd) + if err != nil { + return err + } + return nil +} + +func (s *Service) ExpirePreviousVerifications(ctx context.Context, cmd *tempuser.ExpirePreviousVerificationsCommand) error { + err := s.store.ExpirePreviousVerifications(ctx, cmd) + if err != nil { + return err + } + return nil +} diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index c771aa0e993..0fd966e828b 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -19,6 +19,13 @@ const ( HelpFlagDashboardHelp1 ) +type UpdateEmailActionType string + +const ( + EmailUpdateAction UpdateEmailActionType = "email-update" + LoginUpdateAction UpdateEmailActionType = "login-update" +) + type User struct { ID int64 `xorm:"pk autoincr 'id'"` UID string `json:"uid" xorm:"uid"` diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 05e89eed226..95a28baafa5 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -314,9 +314,10 @@ type Cfg struct { DateFormats DateFormats // User - UserInviteMaxLifetime time.Duration - HiddenUsers map[string]struct{} - CaseInsensitiveLogin bool // Login and Email will be considered case insensitive + UserInviteMaxLifetime time.Duration + HiddenUsers map[string]struct{} + CaseInsensitiveLogin bool // Login and Email will be considered case insensitive + VerificationEmailMaxLifetime time.Duration // Service Accounts SATokenExpirationDayLimit int @@ -1700,6 +1701,13 @@ func readUserSettings(iniFile *ini.File, cfg *Cfg) error { } } + verificationEmailMaxLifetimeVal := valueAsString(users, "verification_email_max_lifetime_duration", "1h") + verificationEmailMaxLifetimeDuration, err := gtime.ParseDuration(verificationEmailMaxLifetimeVal) + if err != nil { + return err + } + cfg.VerificationEmailMaxLifetime = verificationEmailMaxLifetimeDuration + return nil } diff --git a/public/api-merged.json b/public/api-merged.json index 4d724641091..725bd8a0d9a 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -9602,6 +9602,9 @@ "403": { "$ref": "#/responses/forbiddenError" }, + "409": { + "$ref": "#/responses/conflictError" + }, "500": { "$ref": "#/responses/internalServerError" } @@ -9632,6 +9635,21 @@ } } }, + "/user/email/update": { + "get": { + "description": "Update the email of user given a verification code.", + "tags": [ + "user" + ], + "summary": "Update user email.", + "operationId": "updateUserEmail", + "responses": { + "302": { + "$ref": "#/responses/okResponse" + } + } + } + }, "/user/helpflags/clear": { "get": { "tags": [ @@ -10277,6 +10295,9 @@ "404": { "$ref": "#/responses/notFoundError" }, + "409": { + "$ref": "#/responses/conflictError" + }, "500": { "$ref": "#/responses/internalServerError" } diff --git a/public/emails/verify_email_update.html b/public/emails/verify_email_update.html new file mode 100644 index 00000000000..be8542832ca --- /dev/null +++ b/public/emails/verify_email_update.html @@ -0,0 +1,215 @@ + + + + + {{ Subject .Subject .TemplateData "Verify your new email - {{.Name}}" }} + {{ __dangerouslyInjectHTML `` }} + + {{ __dangerouslyInjectHTML `` }} + + + + {{ __dangerouslyInjectHTML `` }} + {{ __dangerouslyInjectHTML `` }} + {{ __dangerouslyInjectHTML `` }} + + + {{ __dangerouslyInjectHTML `` }} + + + + + + + +
+ {{ __dangerouslyInjectHTML `` }} +
+ + + + + + +
+ {{ __dangerouslyInjectHTML `` }} +
+ + + + + + +
+ + + + + + +
+ +
+
+
+ {{ __dangerouslyInjectHTML `` }} +
+
+ {{ __dangerouslyInjectHTML `` }} +
+ + + + + + +
+ {{ __dangerouslyInjectHTML `` }} +
+ + + + + + + + + + + + + + + + + + +
+
+

Hi {{ .Name }},

+
+
+
Please click the following link to verify your email within {{ .VerificationEmailLifetimeHours }} hour(s).
+
+ + + + + + +
+ Verify Email +
+
+
You can also copy and paste this link into your browser directly:
+
+ +
+
+ {{ __dangerouslyInjectHTML `` }} +
+
+ {{ __dangerouslyInjectHTML `` }} +
+ + + + + + +
+ {{ __dangerouslyInjectHTML `` }} +
+ + + + + + +
+
© {{ now | date "2006" }} Grafana Labs. Sent by Grafana v{{ .BuildVersion }}.
+
+
+ {{ __dangerouslyInjectHTML `` }} +
+
+ {{ __dangerouslyInjectHTML `` }} +
+ + + diff --git a/public/emails/verify_email_update.txt b/public/emails/verify_email_update.txt new file mode 100644 index 00000000000..cfdd2665bec --- /dev/null +++ b/public/emails/verify_email_update.txt @@ -0,0 +1,9 @@ +{{HiddenSubject .Subject "Verify your new email - {{.Name}}"}} + +Hi {{.Name}}, + +Copy and paste the following link directly in your browser to verify your email within {{.VerificationEmailLifetimeHours}} hour(s). +{{.AppUrl}}user/email/update?code={{.Code}} + + +Sent by Grafana v{{.BuildVersion}} (c) {{now | date "2006"}} Grafana Labs diff --git a/public/openapi3.json b/public/openapi3.json index 14ae4182c7c..bfa8084a60b 100644 --- a/public/openapi3.json +++ b/public/openapi3.json @@ -23207,6 +23207,9 @@ "403": { "$ref": "#/components/responses/forbiddenError" }, + "409": { + "$ref": "#/components/responses/conflictError" + }, "500": { "$ref": "#/components/responses/internalServerError" } @@ -23241,6 +23244,21 @@ ] } }, + "/user/email/update": { + "get": { + "description": "Update the email of user given a verification code.", + "operationId": "updateUserEmail", + "responses": { + "302": { + "$ref": "#/components/responses/okResponse" + } + }, + "summary": "Update user email.", + "tags": [ + "user" + ] + } + }, "/user/helpflags/clear": { "get": { "operationId": "clearHelpFlags", @@ -23911,6 +23929,9 @@ "404": { "$ref": "#/components/responses/notFoundError" }, + "409": { + "$ref": "#/components/responses/conflictError" + }, "500": { "$ref": "#/components/responses/internalServerError" }