Users: Improve conflict error handling (#26958)

* API: Improve error handling (#26934)

* New ErrUserAlreadyExists error has been introduced

* Create user endpoint returns 412 Precondition Failed on ErrUserAlreadyExists errors

* Make ErrUserAlreadyExists error message clearer

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>

* Use errors.Is instead of equality comparator on AdminCreateUser handler

* Improve sqlstore/user test definition

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

* Improve sqlstore/user tests for ErrUserAlreadyExists cases

* Remove no needed string fmt and err declaration on sqlstore/user tests

* Code improvements for sqlstore/user tests

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

* Use err.Error() instead of sentinel error value on AdminCreateUser

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

* Add ErrUserAlreadyExists handling for signup & org invite use cases

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

pkg/api/admin_users.go

 package api
 package api
 
 
 import (
 import (
+	"errors"
+	"fmt"
+
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/infra/metrics"
 	"github.com/grafana/grafana/pkg/infra/metrics"
@@ -29,8 +32,12 @@ func AdminCreateUser(c *models.ReqContext, form dtos.AdminCreateUserForm) Respon
 	}
 	}
 
 
 	if err := bus.Dispatch(&cmd); err != nil {
 	if err := bus.Dispatch(&cmd); err != nil {
-		if err == models.ErrOrgNotFound {
-			return Error(400, models.ErrOrgNotFound.Error(), nil)
+		if errors.Is(err, models.ErrOrgNotFound) {
+			return Error(400, err.Error(), nil)
+		}
+
+		if errors.Is(err, models.ErrUserAlreadyExists) {
+			return Error(412, fmt.Sprintf("User with email '%s' or username '%s' already exists", form.Email, form.Login), err)
 		}
 		}
 
 
 		return Error(500, "failed to create user", err)
 		return Error(500, "failed to create user", err)

pkg/api/admin_users_test.go

@@ -259,6 +259,26 @@ func TestAdminApiEndpoint(t *testing.T) {
 			})
 			})
 		})
 		})
 	})
 	})
+
+	Convey("When a server admin attempts to create a user with an already existing email/login", t, func() {
+		bus.AddHandler("test", func(cmd *models.CreateUserCommand) error {
+			return models.ErrUserAlreadyExists
+		})
+
+		createCmd := dtos.AdminCreateUserForm{
+			Login:    TestLogin,
+			Password: TestPassword,
+		}
+
+		adminCreateUserScenario("Should return an error", "/api/admin/users", "/api/admin/users", createCmd, func(sc *scenarioContext) {
+			sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec()
+			So(sc.resp.Code, ShouldEqual, 412)
+
+			respJSON, err := simplejson.NewJson(sc.resp.Body.Bytes())
+			So(err, ShouldBeNil)
+			So(respJSON.Get("error").MustString(), ShouldEqual, "User already exists")
+		})
+	})
 }
 }
 
 
 func putAdminScenario(desc string, url string, routePattern string, role models.RoleType, cmd dtos.AdminUpdateUserPermissionsForm, fn scenarioFunc) {
 func putAdminScenario(desc string, url string, routePattern string, role models.RoleType, cmd dtos.AdminUpdateUserPermissionsForm, fn scenarioFunc) {

pkg/api/org_invite.go

 package api
 package api
 
 
 import (
 import (
+	"errors"
 	"fmt"
 	"fmt"
 
 
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/dtos"
@@ -81,6 +82,7 @@ func AddOrgInvite(c *models.ReqContext, inviteDto dtos.AddInviteForm) Response {
 			if err == models.ErrSmtpNotEnabled {
 			if err == models.ErrSmtpNotEnabled {
 				return Error(412, err.Error(), err)
 				return Error(412, err.Error(), err)
 			}
 			}
+
 			return Error(500, "Failed to send email invite", err)
 			return Error(500, "Failed to send email invite", err)
 		}
 		}
 
 
@@ -181,6 +183,10 @@ func (hs *HTTPServer) CompleteInvite(c *models.ReqContext, completeInvite dtos.C
 	}
 	}
 
 
 	if err := bus.Dispatch(&cmd); err != nil {
 	if err := bus.Dispatch(&cmd); err != nil {
+		if errors.Is(err, models.ErrUserAlreadyExists) {
+			return Error(412, fmt.Sprintf("User with email '%s' or username '%s' already exists", completeInvite.Email, completeInvite.Username), err)
+		}
+
 		return Error(500, "failed to create user", err)
 		return Error(500, "failed to create user", err)
 	}
 	}
 
 

pkg/api/signup.go

 package api
 package api
 
 
 import (
 import (
+	"errors"
+
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/events"
 	"github.com/grafana/grafana/pkg/events"
@@ -78,14 +80,12 @@ func (hs *HTTPServer) SignUpStep2(c *models.ReqContext, form dtos.SignUpStep2For
 		createUserCmd.EmailVerified = true
 		createUserCmd.EmailVerified = true
 	}
 	}
 
 
-	// check if user exists
-	existing := models.GetUserByLoginQuery{LoginOrEmail: form.Email}
-	if err := bus.Dispatch(&existing); err == nil {
-		return Error(401, "User with same email address already exists", nil)
-	}
-
 	// dispatch create command
 	// dispatch create command
 	if err := bus.Dispatch(&createUserCmd); err != nil {
 	if err := bus.Dispatch(&createUserCmd); err != nil {
+		if errors.Is(err, models.ErrUserAlreadyExists) {
+			return Error(401, "User with same email address already exists", nil)
+		}
+
 		return Error(500, "Failed to create user", err)
 		return Error(500, "Failed to create user", err)
 	}
 	}
 
 

pkg/models/user.go

@@ -7,8 +7,9 @@ import (
 
 
 // Typed errors
 // Typed errors
 var (
 var (
-	ErrUserNotFound     = errors.New("User not found")
-	ErrLastGrafanaAdmin = errors.New("Cannot remove last grafana admin")
+	ErrUserNotFound      = errors.New("User not found")
+	ErrUserAlreadyExists = errors.New("User already exists")
+	ErrLastGrafanaAdmin  = errors.New("Cannot remove last grafana admin")
 )
 )
 
 
 type Password string
 type Password string

pkg/services/sqlstore/user.go

@@ -68,6 +68,11 @@ func CreateUser(ctx context.Context, cmd *models.CreateUserCommand) error {
 			cmd.Email = cmd.Login
 			cmd.Email = cmd.Login
 		}
 		}
 
 
+		exists, _ := sess.Where("email=? OR login=?", cmd.Email, cmd.Login).Get(&models.User{})
+		if exists {
+			return models.ErrUserAlreadyExists
+		}
+
 		// create user
 		// create user
 		user := models.User{
 		user := models.User{
 			Email:         cmd.Email,
 			Email:         cmd.Email,

pkg/services/sqlstore/user_test.go

@@ -568,6 +568,40 @@ func TestUserDataAccess(t *testing.T) {
 				So(query.Result.IsAdmin, ShouldEqual, true)
 				So(query.Result.IsAdmin, ShouldEqual, true)
 			})
 			})
 		})
 		})
+
+		Convey("Given one user", func() {
+			const email = "user@test.com"
+			const username = "user"
+			createUserCmd := &models.CreateUserCommand{
+				Email: email,
+				Name:  "user",
+				Login: username,
+			}
+			err := CreateUser(context.Background(), createUserCmd)
+			So(err, ShouldBeNil)
+
+			Convey("When trying to create a new user with the same email, an error is returned", func() {
+				createUserCmd := &models.CreateUserCommand{
+					Email:        email,
+					Name:         "user2",
+					Login:        "user2",
+					SkipOrgSetup: true,
+				}
+				err := CreateUser(context.Background(), createUserCmd)
+				So(err, ShouldEqual, models.ErrUserAlreadyExists)
+			})
+
+			Convey("When trying to create a new user with the same login, an error is returned", func() {
+				createUserCmd := &models.CreateUserCommand{
+					Email:        "user2@test.com",
+					Name:         "user2",
+					Login:        username,
+					SkipOrgSetup: true,
+				}
+				err := CreateUser(context.Background(), createUserCmd)
+				So(err, ShouldEqual, models.ErrUserAlreadyExists)
+			})
+		})
 	})
 	})
 }
 }