From 1528d6f5c404c24e552913b903e32f3b337e11eb Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 11 Oct 2023 14:27:43 +0200 Subject: [PATCH] Authn: Prevent empty username and email during sync (#76330) * Move errors to error file * Move check for both empty username and email to user service * Move check for empty email and username to user service Update * Wrap inner error * Set username in test --- pkg/api/admin_users.go | 9 +------ pkg/api/user.go | 9 +------ .../authn/authnimpl/sync/user_sync.go | 2 +- pkg/services/quota/quotaimpl/quota_test.go | 1 + pkg/services/user/error.go | 24 +++++++++++++++++++ pkg/services/user/model.go | 13 ---------- pkg/services/user/userimpl/user.go | 19 +++++++++++++++ pkg/services/user/userimpl/user_test.go | 20 ++++++++++++++++ 8 files changed, 67 insertions(+), 30 deletions(-) create mode 100644 pkg/services/user/error.go diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 9030ba71876..6539fb3de0b 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -57,13 +57,6 @@ func (hs *HTTPServer) AdminCreateUser(c *contextmodel.ReqContext) response.Respo OrgID: form.OrgId, } - if len(cmd.Login) == 0 { - cmd.Login = cmd.Email - if len(cmd.Login) == 0 { - return response.Error(400, "Validation error, need specify either username or email", nil) - } - } - if len(cmd.Password) < 4 { return response.Error(400, "Password is missing or too short", nil) } @@ -78,7 +71,7 @@ func (hs *HTTPServer) AdminCreateUser(c *contextmodel.ReqContext) response.Respo return response.Error(http.StatusPreconditionFailed, fmt.Sprintf("User with email '%s' or username '%s' already exists", form.Email, form.Login), err) } - return response.Error(http.StatusInternalServerError, "failed to create user", err) + return response.ErrOrFallback(http.StatusInternalServerError, "failed to create user", err) } metrics.MApiAdminUserCreate.Inc() diff --git a/pkg/api/user.go b/pkg/api/user.go index 0437baa5fbf..32b7db52597 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -221,18 +221,11 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC return response.Error(http.StatusForbidden, "User info cannot be updated for external Users", nil) } - if len(cmd.Login) == 0 { - cmd.Login = cmd.Email - if len(cmd.Login) == 0 { - return response.Error(http.StatusBadRequest, "Validation error, need to specify either username or email", nil) - } - } - 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) } - return response.Error(http.StatusInternalServerError, "Failed to update user", err) + return response.ErrOrFallback(http.StatusInternalServerError, "Failed to update user", err) } return response.Success("User updated") diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 97bc58482a8..0b0a5b09b91 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -91,7 +91,7 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth usr, errCreate = s.createUser(ctx, id) if errCreate != nil { s.log.FromContext(ctx).Error("Failed to create user", "error", errCreate, "auth_module", id.AuthenticatedBy, "auth_id", id.AuthID) - return errSyncUserInternal.Errorf("unable to create user") + return errSyncUserInternal.Errorf("unable to create user: %w", errCreate) } } else { // update user diff --git a/pkg/services/quota/quotaimpl/quota_test.go b/pkg/services/quota/quotaimpl/quota_test.go index 9a3a1cf7d03..42960015213 100644 --- a/pkg/services/quota/quotaimpl/quota_test.go +++ b/pkg/services/quota/quotaimpl/quota_test.go @@ -98,6 +98,7 @@ func TestIntegrationQuotaCommandsAndQueries(t *testing.T) { u, err := userService.Create(context.Background(), &user.CreateUserCommand{ Name: "TestUser", + Login: "TestUser", SkipOrgSetup: true, }) require.NoError(t, err) diff --git a/pkg/services/user/error.go b/pkg/services/user/error.go new file mode 100644 index 00000000000..ba70fd24baf --- /dev/null +++ b/pkg/services/user/error.go @@ -0,0 +1,24 @@ +package user + +import ( + "errors" + + "github.com/grafana/grafana/pkg/util/errutil" +) + +var ( + ErrCaseInsensitive = errors.New("case insensitive conflict") + ErrUserNotFound = errors.New("user not found") + ErrUserAlreadyExists = errors.New("user already exists") + ErrLastGrafanaAdmin = errors.New("cannot remove last grafana admin") + ErrProtectedUser = errors.New("cannot adopt protected user") + ErrNoUniqueID = errors.New("identifying id not found") + ErrLastSeenUpToDate = errors.New("last seen is already up to date") + ErrUpdateInvalidID = errors.New("unable to update invalid id") +) + +var ( + ErrEmptyUsernameAndEmail = errutil.BadRequest( + "user.empty-username-and-email", errutil.WithPublicMessage("Need to specify either username or email"), + ) +) diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index 9f8f816772a..81b184286ab 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -1,7 +1,6 @@ package user import ( - "errors" "fmt" "strings" "time" @@ -20,18 +19,6 @@ const ( HelpFlagDashboardHelp1 ) -// Typed errors -var ( - ErrCaseInsensitive = errors.New("case insensitive conflict") - ErrUserNotFound = errors.New("user not found") - ErrUserAlreadyExists = errors.New("user already exists") - ErrLastGrafanaAdmin = errors.New("cannot remove last grafana admin") - ErrProtectedUser = errors.New("cannot adopt protected user") - ErrNoUniqueID = errors.New("identifying id not found") - ErrLastSeenUpToDate = errors.New("last seen is already up to date") - ErrUpdateInvalidID = errors.New("unable to update invalid id") -) - type User struct { ID int64 `xorm:"pk autoincr 'id'"` Version int diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index 621546db957..fced32139d9 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -98,6 +98,15 @@ func (s *Service) Usage(ctx context.Context, _ *quota.ScopeParameters) (*quota.M } func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*user.User, error) { + if len(cmd.Login) == 0 { + cmd.Login = cmd.Email + } + + // if login is still empty both email and login field is missing + if len(cmd.Login) == 0 { + return nil, user.ErrEmptyUsernameAndEmail.Errorf("user cannot be created with empty username and email") + } + cmdOrg := org.GetOrgIDForNewUserCommand{ Email: cmd.Email, Login: cmd.Login, @@ -215,10 +224,20 @@ func (s *Service) GetByEmail(ctx context.Context, query *user.GetUserByEmailQuer } func (s *Service) Update(ctx context.Context, cmd *user.UpdateUserCommand) error { + if len(cmd.Login) == 0 { + cmd.Login = cmd.Email + } + + // if login is still empty both email and login field is missing + if len(cmd.Login) == 0 { + return user.ErrEmptyUsernameAndEmail.Errorf("user cannot be created with empty username and email") + } + if s.cfg.CaseInsensitiveLogin { cmd.Login = strings.ToLower(cmd.Login) cmd.Email = strings.ToLower(cmd.Email) } + return s.store.Update(ctx, cmd) } diff --git a/pkg/services/user/userimpl/user_test.go b/pkg/services/user/userimpl/user_test.go index 43c9c5d3b5d..dc6bcbbc6e1 100644 --- a/pkg/services/user/userimpl/user_test.go +++ b/pkg/services/user/userimpl/user_test.go @@ -38,6 +38,16 @@ func TestUserService(t *testing.T) { require.NoError(t, err) }) + t.Run("create user should fail when username and email are empty", func(t *testing.T) { + _, err := userService.Create(context.Background(), &user.CreateUserCommand{ + Email: "", + Login: "", + Name: "name", + }) + + require.ErrorIs(t, err, user.ErrEmptyUsernameAndEmail) + }) + t.Run("get user by ID", func(t *testing.T) { userService.cfg = setting.NewCfg() userService.cfg.CaseInsensitiveLogin = false @@ -88,6 +98,16 @@ func TestUserService(t *testing.T) { require.NoError(t, err) }) + t.Run("update user should fail with empty username and password", func(t *testing.T) { + err := userService.Update(context.Background(), &user.UpdateUserCommand{ + Email: "", + Login: "", + Name: "name", + }) + + require.ErrorIs(t, err, user.ErrEmptyUsernameAndEmail) + }) + t.Run("GetByID - email conflict", func(t *testing.T) { userService.cfg.CaseInsensitiveLogin = true userStore.ExpectedError = errors.New("email conflict")