[MM-34725] app/user: check if username or email is in use before patching a user (#17392)

* app/user: check if username or email is in use before patching a user

* reflect review comments

* fix tests

app/bot.go

@@ -178,11 +178,17 @@ func (a *App) PatchBot(botUserId string, botPatch *model.BotPatch) (*model.Bot,
 	if nErr != nil {
 		var appErr *model.AppError
 		var invErr *store.ErrInvalidInput
+		var conErr *store.ErrConflict
 		switch {
 		case errors.As(nErr, &appErr):
 			return nil, appErr
 		case errors.As(nErr, &invErr):
-			return nil, model.NewAppError("PatchBot", "app.user.update.find.app_error", nil, invErr.Error(), http.StatusBadRequest)
+			return nil, model.NewAppError("PatchBot", "app.user.update.find.app_error", nil, nErr.Error(), http.StatusBadRequest)
+		case errors.As(nErr, &conErr):
+			if cErr, ok := nErr.(*store.ErrConflict); ok && cErr.Resource == "Username" {
+				return nil, model.NewAppError("PatchBot", "app.user.save.username_exists.app_error", nil, nErr.Error(), http.StatusBadRequest)
+			}
+			return nil, model.NewAppError("PatchBot", "app.user.save.email_exists.app_error", nil, nErr.Error(), http.StatusBadRequest)
 		default:
 			return nil, model.NewAppError("PatchBot", "app.user.update.finding.app_error", nil, nErr.Error(), http.StatusInternalServerError)
 		}

app/bot_test.go

@@ -204,7 +204,7 @@ func TestPatchBot(t *testing.T) {
 
 		_, err = th.App.PatchBot(bot.UserId, botPatch)
 		require.NotNil(t, err)
-		require.Equal(t, "app.user.update.find.app_error", err.Id)
+		require.Equal(t, "app.user.save.username_exists.app_error", err.Id)
 	})
 }
 

app/user.go

@@ -1281,15 +1281,13 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User,
 			}
 		}
 
-		if _, appErr := a.GetUserByEmail(user.Email); appErr == nil {
-			return nil, model.NewAppError("UpdateUser", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
-		}
-
-		// Don't set new eMail on user account if email verification is required, this will be done as a post-verification action
-		// to avoid users being able to set non-controlled eMails as their account email
-
 		if *a.Config().EmailSettings.RequireEmailVerification {
 			newEmail = user.Email
+			// Don't set new eMail on user account if email verification is required, this will be done as a post-verification action
+			// to avoid users being able to set non-controlled eMails as their account email
+			if _, appErr := a.GetUserByEmail(newEmail); appErr == nil {
+				return nil, model.NewAppError("UpdateUser", "app.user.save.email_exists.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
+			}
 
 			//  When a bot is created, prev.Email will be an autogenerated faked email,
 			//  which will not match a CLI email input during bot to user conversions.
@@ -1305,11 +1303,17 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User,
 	if err != nil {
 		var appErr *model.AppError
 		var invErr *store.ErrInvalidInput
+		var conErr *store.ErrConflict
 		switch {
 		case errors.As(err, &appErr):
 			return nil, appErr
 		case errors.As(err, &invErr):
 			return nil, model.NewAppError("UpdateUser", "app.user.update.find.app_error", nil, invErr.Error(), http.StatusBadRequest)
+		case errors.As(err, &conErr):
+			if cErr, ok := err.(*store.ErrConflict); ok && cErr.Resource == "Username" {
+				return nil, model.NewAppError("UpdateUser", "app.user.save.username_exists.app_error", nil, "", http.StatusBadRequest)
+			}
+			return nil, model.NewAppError("UpdateUser", "app.user.save.email_exists.app_error", nil, "", http.StatusBadRequest)
 		default:
 			return nil, model.NewAppError("UpdateUser", "app.user.update.finding.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}

app/user_test.go

@@ -471,7 +471,7 @@ func TestUpdateUserEmail(t *testing.T) {
 		user.Email = newEmail
 		user3, err := th.App.UpdateUser(user, false)
 		require.NotNil(t, err)
-		assert.Equal(t, err.Id, "store.sql_user.update.email_taken.app_error")
+		assert.Equal(t, err.Id, "app.user.save.email_exists.app_error")
 		assert.Nil(t, user3)
 	})
 
@@ -514,7 +514,7 @@ func TestUpdateUserEmail(t *testing.T) {
 		user.Email = newEmail
 		user3, err := th.App.UpdateUser(user, false)
 		require.NotNil(t, err)
-		assert.Equal(t, err.Id, "store.sql_user.update.email_taken.app_error")
+		assert.Equal(t, err.Id, "app.user.save.email_exists.app_error")
 		assert.Nil(t, user3)
 	})
 }
@@ -1457,3 +1457,37 @@ func TestDeactivateMfa(t *testing.T) {
 		require.Nil(t, err)
 	})
 }
+
+func TestPatchUser(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	testUser := th.CreateUser()
+	defer th.App.PermanentDeleteUser(testUser)
+
+	t.Run("Patch with a username already exists", func(t *testing.T) {
+		_, err := th.App.PatchUser(testUser.Id, &model.UserPatch{
+			Username: model.NewString(th.BasicUser.Username),
+		}, true)
+
+		require.NotNil(t, err)
+		require.Equal(t, "app.user.save.username_exists.app_error", err.Id)
+	})
+
+	t.Run("Patch with a email already exists", func(t *testing.T) {
+		_, err := th.App.PatchUser(testUser.Id, &model.UserPatch{
+			Email: model.NewString(th.BasicUser.Email),
+		}, true)
+
+		require.NotNil(t, err)
+		require.Equal(t, "app.user.save.email_exists.app_error", err.Id)
+	})
+
+	t.Run("Patch username with a new username", func(t *testing.T) {
+		_, err := th.App.PatchUser(testUser.Id, &model.UserPatch{
+			Username: model.NewString(model.NewId()),
+		}, true)
+
+		require.Nil(t, err)
+	})
+}