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

@@ -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()

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

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

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)

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

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

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

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