From ba41954854b7ded6cc6f983f23018b3850c3807e Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Fri, 5 Apr 2024 12:05:46 +0200 Subject: [PATCH] Email: trigger email verification flow (#85587) * Add email and email_verified to id token if identity is a user * Add endpoint to trigger email verification for user * Add function to clear stored id tokens and use it when email verification is completed --- pkg/api/api.go | 1 + pkg/api/user.go | 33 +++++++++++++++++-- pkg/api/user_test.go | 18 +++++----- pkg/services/auth/id.go | 5 +++ pkg/services/auth/identity/requester.go | 2 ++ pkg/services/auth/idimpl/service.go | 11 +++++-- pkg/services/auth/idtest/mock.go | 22 +++++++++++++ .../authn/authnimpl/sync/user_sync.go | 1 + pkg/services/authn/identity.go | 6 ++++ pkg/services/user/identity.go | 5 +++ pkg/services/user/model.go | 1 + pkg/services/user/userimpl/store.go | 1 + pkg/services/user/userimpl/verifier.go | 14 ++++++-- pkg/services/user/userimpl/verifier_test.go | 21 ++++++++++-- 14 files changed, 122 insertions(+), 19 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 1dde93806db..55d218a1d06 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -191,6 +191,7 @@ func (hs *HTTPServer) registerRoutes() { // update user email if hs.Cfg.Smtp.Enabled && hs.Cfg.VerifyEmailEnabled { r.Get("/user/email/update", reqSignedInNoAnonymous, routing.Wrap(hs.UpdateUserEmail)) + r.Post("/api/user/email/start-verify", reqSignedInNoAnonymous, routing.Wrap(hs.StartEmailVerificaton)) } // invited diff --git a/pkg/api/user.go b/pkg/api/user.go index 0ba24a3516a..e529497546f 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -264,12 +264,12 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC if err != nil { return response.Error(http.StatusBadRequest, "Invalid email address", err) } - return hs.verifyEmailUpdate(ctx, normalized, user.EmailUpdateAction, usr) + return hs.startEmailVerification(ctx, normalized, user.EmailUpdateAction, usr) } if len(cmd.Login) != 0 && usr.Login != cmd.Login { normalized, err := ValidateAndNormalizeEmail(cmd.Login) if err == nil && usr.Email != normalized { - return hs.verifyEmailUpdate(ctx, cmd.Login, user.LoginUpdateAction, usr) + return hs.startEmailVerification(ctx, cmd.Login, user.LoginUpdateAction, usr) } } } @@ -284,7 +284,34 @@ 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 { +func (hs *HTTPServer) StartEmailVerificaton(c *contextmodel.ReqContext) response.Response { + namespace, id := c.SignedInUser.GetNamespacedID() + if !identity.IsNamespace(namespace, identity.NamespaceUser) { + return response.Error(http.StatusBadRequest, "Only users can verify their email", nil) + } + + if c.SignedInUser.IsEmailVerified() { + // email is already verified so we don't need to trigger the flow. + return response.Respond(http.StatusNotModified, nil) + } + + userID, err := strconv.ParseInt(id, 10, 64) + if err != nil { + return response.Error(http.StatusInternalServerError, "Got invalid user id", err) + } + + usr, err := hs.userService.GetByID(c.Req.Context(), &user.GetUserByIDQuery{ + ID: userID, + }) + + if err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to fetch user", err) + } + + return hs.startEmailVerification(c.Req.Context(), usr.Email, user.EmailUpdateAction, usr) +} + +func (hs *HTTPServer) startEmailVerification(ctx context.Context, email string, field user.UpdateEmailActionType, usr *user.User) response.Response { if err := hs.userVerifier.Start(ctx, user.StartVerifyEmailCommand{ User: *usr, Email: email, diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 1f445a128ca..e66cbd5209e 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -10,13 +10,6 @@ import ( "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" @@ -30,23 +23,30 @@ import ( "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/login/social/socialtest" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/auth/idtest" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/login/authinfoimpl" "github.com/grafana/grafana/pkg/services/login/authinfotest" + "github.com/grafana/grafana/pkg/services/notifications" "github.com/grafana/grafana/pkg/services/org/orgimpl" "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/searchusers/filters" "github.com/grafana/grafana/pkg/services/secrets/database" + "github.com/grafana/grafana/pkg/services/secrets/fakes" secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" + tempuser "github.com/grafana/grafana/pkg/services/temp_user" + "github.com/grafana/grafana/pkg/services/temp_user/tempuserimpl" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/services/user/userimpl" "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/web/webtest" ) const newEmail = "newemail@localhost" @@ -397,7 +397,7 @@ func setupUpdateEmailTests(t *testing.T, cfg *setting.Cfg) (*user.User, *HTTPSer require.NoError(t, err) nsMock := notifications.MockNotificationService() - verifier := userimpl.ProvideVerifier(cfg, userSvc, tempUserService, nsMock) + verifier := userimpl.ProvideVerifier(cfg, userSvc, tempUserService, nsMock, &idtest.MockService{}) hs := &HTTPServer{ Cfg: cfg, @@ -620,7 +620,7 @@ func TestUser_UpdateEmail(t *testing.T) { hs.tempUserService = tempUserSvc hs.NotificationService = nsMock hs.SecretsService = fakes.NewFakeSecretsService() - hs.userVerifier = userimpl.ProvideVerifier(settings, userSvc, tempUserSvc, nsMock) + hs.userVerifier = userimpl.ProvideVerifier(settings, userSvc, tempUserSvc, nsMock, &idtest.MockService{}) // User is internal hs.authInfoService = &authinfotest.FakeService{ExpectedError: user.ErrUserNotFound} }) diff --git a/pkg/services/auth/id.go b/pkg/services/auth/id.go index 06d302d0f15..8c1b3021d73 100644 --- a/pkg/services/auth/id.go +++ b/pkg/services/auth/id.go @@ -11,6 +11,9 @@ import ( type IDService interface { // SignIdentity signs a id token for provided identity that can be forwarded to plugins and external services SignIdentity(ctx context.Context, identity identity.Requester) (string, error) + + // RemoveIDToken removes any locally stored id tokens for key + RemoveIDToken(ctx context.Context, identity identity.Requester) error } type IDSigner interface { @@ -19,5 +22,7 @@ type IDSigner interface { type IDClaims struct { jwt.Claims + Email string `json:"email"` + EmailVerified bool `json:"email_verified"` AuthenticatedBy string `json:"authenticatedBy,omitempty"` } diff --git a/pkg/services/auth/identity/requester.go b/pkg/services/auth/identity/requester.go index 90e73c10ede..9bdd2a1cfd2 100644 --- a/pkg/services/auth/identity/requester.go +++ b/pkg/services/auth/identity/requester.go @@ -32,6 +32,8 @@ type Requester interface { // GetEmail returns the email of the active entity. // Can be empty. GetEmail() string + // IsEmailVerified returns if email is verified for entity. + IsEmailVerified() bool // GetIsGrafanaAdmin returns true if the user is a server admin GetIsGrafanaAdmin() bool // GetLogin returns the login of the active entity diff --git a/pkg/services/auth/idimpl/service.go b/pkg/services/auth/idimpl/service.go index 48eac95a955..13f897260fe 100644 --- a/pkg/services/auth/idimpl/service.go +++ b/pkg/services/auth/idimpl/service.go @@ -90,7 +90,7 @@ func (s *Service) SignIdentity(ctx context.Context, id identity.Requester) (stri } if identity.IsNamespace(namespace, identity.NamespaceUser) { - if err := s.setUserClaims(ctx, identifier, claims); err != nil { + if err := s.setUserClaims(ctx, id, identifier, claims); err != nil { return "", err } } @@ -130,7 +130,11 @@ func (s *Service) SignIdentity(ctx context.Context, id identity.Requester) (stri return result.(string), nil } -func (s *Service) setUserClaims(ctx context.Context, identifier string, claims *auth.IDClaims) error { +func (s *Service) RemoveIDToken(ctx context.Context, id identity.Requester) error { + return s.cache.Delete(ctx, prefixCacheKey(id.GetCacheKey())) +} + +func (s *Service) setUserClaims(ctx context.Context, ident identity.Requester, identifier string, claims *auth.IDClaims) error { id, err := strconv.ParseInt(identifier, 10, 64) if err != nil { return err @@ -140,6 +144,9 @@ func (s *Service) setUserClaims(ctx context.Context, identifier string, claims * return nil } + claims.Email = ident.GetEmail() + claims.EmailVerified = ident.IsEmailVerified() + info, err := s.authInfoService.GetAuthInfo(ctx, &login.GetAuthInfoQuery{UserId: id}) if err != nil { // we ignore errors when a user don't have external user auth diff --git a/pkg/services/auth/idtest/mock.go b/pkg/services/auth/idtest/mock.go index eafc244ec5b..c6a2aca8c03 100644 --- a/pkg/services/auth/idtest/mock.go +++ b/pkg/services/auth/idtest/mock.go @@ -4,8 +4,30 @@ import ( "context" "github.com/grafana/grafana/pkg/services/auth" + "github.com/grafana/grafana/pkg/services/auth/identity" ) +var _ auth.IDService = (*MockService)(nil) + +type MockService struct { + SignIdentityFn func(ctx context.Context, identity identity.Requester) (string, error) + RemoveIDTokenFn func(ctx context.Context, identity identity.Requester) error +} + +func (m *MockService) SignIdentity(ctx context.Context, identity identity.Requester) (string, error) { + if m.SignIdentityFn != nil { + return m.SignIdentityFn(ctx, identity) + } + return "", nil +} + +func (m *MockService) RemoveIDToken(ctx context.Context, identity identity.Requester) error { + if m.RemoveIDTokenFn != nil { + return m.RemoveIDTokenFn(ctx, identity) + } + return nil +} + type MockSigner struct { SignIDTokenFn func(ctx context.Context, claims *auth.IDClaims) (string, error) } diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 6ba757b5615..05a27e37b5c 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -407,4 +407,5 @@ func syncSignedInUserToIdentity(usr *user.SignedInUser, identity *authn.Identity identity.LastSeenAt = usr.LastSeenAt identity.IsDisabled = usr.IsDisabled identity.IsGrafanaAdmin = &usr.IsGrafanaAdmin + identity.EmailVerified = usr.EmailVerified } diff --git a/pkg/services/authn/identity.go b/pkg/services/authn/identity.go index ba27a85bba2..54da56849e6 100644 --- a/pkg/services/authn/identity.go +++ b/pkg/services/authn/identity.go @@ -55,6 +55,8 @@ type Identity struct { Name string // Email is the email address of the entity. Should be unique. Email string + // EmailVerified is true if entity has verified their email with grafana. + EmailVerified bool // IsGrafanaAdmin is true if the entity is a Grafana admin. IsGrafanaAdmin *bool // AuthenticatedBy is the name of the authentication client that was used to authenticate the current Identity. @@ -123,6 +125,10 @@ func (i *Identity) GetEmail() string { return i.Email } +func (i *Identity) IsEmailVerified() bool { + return i.EmailVerified +} + func (i *Identity) GetIDToken() string { return i.IDToken } diff --git a/pkg/services/user/identity.go b/pkg/services/user/identity.go index a7ffbf677ba..3a25d7f9f18 100644 --- a/pkg/services/user/identity.go +++ b/pkg/services/user/identity.go @@ -22,6 +22,7 @@ type SignedInUser struct { Login string Name string Email string + EmailVerified bool AuthenticatedBy string ApiKeyID int64 `xorm:"api_key_id"` IsServiceAccount bool `xorm:"is_service_account"` @@ -241,6 +242,10 @@ func (u *SignedInUser) GetEmail() string { return u.Email } +func (u *SignedInUser) IsEmailVerified() bool { + return u.EmailVerified +} + // GetDisplayName returns the display name of the active entity // The display name is the name if it is set, otherwise the login or email func (u *SignedInUser) GetDisplayName() string { diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index 5a51644c9fb..0c852186578 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -228,6 +228,7 @@ type StartVerifyEmailCommand struct { } type CompleteEmailVerifyCommand struct { + User identity.Requester Code string } diff --git a/pkg/services/user/userimpl/store.go b/pkg/services/user/userimpl/store.go index 9a8b337542f..b5912134afe 100644 --- a/pkg/services/user/userimpl/store.go +++ b/pkg/services/user/userimpl/store.go @@ -382,6 +382,7 @@ func (ss *sqlStore) GetSignedInUser(ctx context.Context, query *user.GetSignedIn u.uid as user_uid, u.is_admin as is_grafana_admin, u.email as email, + u.email_verified as email_verified, u.login as login, u.name as name, u.is_disabled as is_disabled, diff --git a/pkg/services/user/userimpl/verifier.go b/pkg/services/user/userimpl/verifier.go index d6ab7284a2f..3017a9b6189 100644 --- a/pkg/services/user/userimpl/verifier.go +++ b/pkg/services/user/userimpl/verifier.go @@ -7,6 +7,8 @@ import ( "net/mail" "time" + "github.com/grafana/grafana/pkg/services/auth" + "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/notifications" tempuser "github.com/grafana/grafana/pkg/services/temp_user" "github.com/grafana/grafana/pkg/services/user" @@ -22,8 +24,8 @@ var ( var _ user.Verifier = (*Verifier)(nil) -func ProvideVerifier(cfg *setting.Cfg, us user.Service, ts tempuser.Service, ns notifications.Service) *Verifier { - return &Verifier{cfg, us, ts, ns} +func ProvideVerifier(cfg *setting.Cfg, us user.Service, ts tempuser.Service, ns notifications.Service, is auth.IDService) *Verifier { + return &Verifier{cfg, us, ts, ns, is} } type Verifier struct { @@ -31,6 +33,7 @@ type Verifier struct { us user.Service ts tempuser.Service ns notifications.Service + is auth.IDService } func (s *Verifier) Start(ctx context.Context, cmd user.StartVerifyEmailCommand) error { @@ -145,5 +148,10 @@ func (s *Verifier) Complete(ctx context.Context, cmd user.CompleteEmailVerifyCom return err } - return nil + // We store email and email verified in id tokens. So whenever we perform and update / confirmation we need to + // remove the current token, so a new one can be generated with correct values. + return s.is.RemoveIDToken( + ctx, + &user.SignedInUser{UserID: usr.ID, OrgID: usr.OrgID, NamespacedID: authn.NamespacedID(authn.NamespaceUser, usr.ID)}, + ) } diff --git a/pkg/services/user/userimpl/verifier_test.go b/pkg/services/user/userimpl/verifier_test.go index 0e68088c56b..0c3e0f960e1 100644 --- a/pkg/services/user/userimpl/verifier_test.go +++ b/pkg/services/user/userimpl/verifier_test.go @@ -7,6 +7,8 @@ import ( "github.com/stretchr/testify/assert" + "github.com/grafana/grafana/pkg/services/auth/identity" + "github.com/grafana/grafana/pkg/services/auth/idtest" "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" @@ -19,6 +21,7 @@ func TestVerifier_Start(t *testing.T) { ts := &tempusertest.FakeTempUserService{} us := &usertest.FakeUserService{} ns := notifications.MockNotificationService() + is := &idtest.MockService{} type calls struct { expireCalled bool @@ -26,7 +29,7 @@ func TestVerifier_Start(t *testing.T) { updateCalled bool } - verifier := ProvideVerifier(setting.NewCfg(), us, ts, ns) + verifier := ProvideVerifier(setting.NewCfg(), us, ts, ns, is) t.Run("should error if email already exist for other user", func(t *testing.T) { us.ExpectedUser = &user.User{ID: 1} err := verifier.Start(context.Background(), user.StartVerifyEmailCommand{ @@ -113,15 +116,17 @@ func TestVerifier_Complete(t *testing.T) { ts := &tempusertest.FakeTempUserService{} us := &usertest.FakeUserService{} ns := notifications.MockNotificationService() + is := &idtest.MockService{} type calls struct { updateCalled bool updateStatusCalled bool + removeTokenCalled bool } cfg := setting.NewCfg() cfg.VerificationEmailMaxLifetime = 1 * time.Hour - verifier := ProvideVerifier(cfg, us, ts, ns) + verifier := ProvideVerifier(cfg, us, ts, ns, is) t.Run("should return error for invalid code", func(t *testing.T) { ts.GetTempUserByCodeFN = func(ctx context.Context, query *tempuser.GetTempUserByCodeQuery) (*tempuser.TempUserDTO, error) { return nil, tempuser.ErrTempUserNotFound @@ -195,6 +200,11 @@ func TestVerifier_Complete(t *testing.T) { return nil } + is.RemoveIDTokenFn = func(ctx context.Context, identity identity.Requester) error { + c.removeTokenCalled = true + return nil + } + us.ExpectedUser = &user.User{Email: "initial@email.com"} us.ExpectedError = nil us.UpdateFn = func(ctx context.Context, cmd *user.UpdateUserCommand) error { @@ -210,6 +220,7 @@ func TestVerifier_Complete(t *testing.T) { assert.NoError(t, err) assert.True(t, c.updateCalled) assert.True(t, c.updateStatusCalled) + assert.True(t, c.removeTokenCalled) }) t.Run("should update user email and login if login is an email on valid code", func(t *testing.T) { @@ -230,6 +241,11 @@ func TestVerifier_Complete(t *testing.T) { return nil } + is.RemoveIDTokenFn = func(ctx context.Context, identity identity.Requester) error { + c.removeTokenCalled = true + return nil + } + us.ExpectedUser = &user.User{Email: "initial@email.com", Login: "other@email.com"} us.ExpectedError = nil us.UpdateFn = func(ctx context.Context, cmd *user.UpdateUserCommand) error { @@ -245,5 +261,6 @@ func TestVerifier_Complete(t *testing.T) { assert.NoError(t, err) assert.True(t, c.updateCalled) assert.True(t, c.updateStatusCalled) + assert.True(t, c.removeTokenCalled) }) }