MM-63619: improve Groups API error semantics (#30961)

Instead of 5xx errors, return `http.StatusInvalidRequest` when
adding or deleting invalid user ids from groups.

Fixes: https://mattermost.atlassian.net/browse/MM-63619

server/channels/api4/group.go

@@ -274,13 +274,13 @@ func patchGroup(c *Context, w http.ResponseWriter, r *http.Request) {
 				c.Err = model.NewAppError("Api4.patchGroup", "api.ldap_groups.existing_reserved_name_error", nil, "", http.StatusBadRequest)
 				return
 			}
-			//check if a user already has this group name
+			// check if a user already has this group name
 			user, _ := c.App.GetUserByUsername(*groupPatch.Name)
 			if user != nil {
 				c.Err = model.NewAppError("Api4.patchGroup", "api.ldap_groups.existing_user_name_error", nil, "", http.StatusBadRequest)
 				return
 			}
-			//check if a mentionable group already has this name
+			// check if a mentionable group already has this name
 			searchOpts := model.GroupSearchOpts{
 				FilterAllowReference: true,
 			}
@@ -914,7 +914,6 @@ func getGroupsByTeamCommon(c *Context, r *http.Request) ([]byte, *model.AppError
 		Groups: groups,
 		Count:  totalCount,
 	})
-
 	if err != nil {
 		return nil, model.NewAppError("Api4.getGroupsByTeam", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
@@ -1346,6 +1345,13 @@ func addGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	for _, userID := range newMembers.UserIds {
+		if !model.IsValidId(userID) {
+			c.SetInvalidParamWithDetails("user_id", fmt.Sprintf("UserID %s is invalid", userID))
+			return
+		}
+	}
+
 	auditRec := c.MakeAuditRecord("addGroupMembers", audit.Fail)
 	defer c.LogAuditRec(auditRec)
 	audit.AddEventParameter(auditRec, "addGroupMembers_userids", newMembers.UserIds)
@@ -1414,6 +1420,13 @@ func deleteGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	for _, userID := range deleteBody.UserIds {
+		if !model.IsValidId(userID) {
+			c.SetInvalidParamWithDetails("user_id", fmt.Sprintf("UserID %s is invalid", userID))
+			return
+		}
+	}
+
 	auditRec := c.MakeAuditRecord("deleteGroupMembers", audit.Fail)
 	defer c.LogAuditRec(auditRec)
 	audit.AddEventParameter(auditRec, "deleteGroupMembers_userids", deleteBody.UserIds)

server/channels/api4/group_test.go

@@ -67,6 +67,7 @@ func TestGetGroup(t *testing.T) {
 	require.Error(t, err)
 	CheckUnauthorizedStatus(t, response)
 }
+
 func TestCreateGroup(t *testing.T) {
 	th := Setup(t)
 	defer th.TearDown()
@@ -2528,86 +2529,190 @@ func TestAddMembersToGroup(t *testing.T) {
 	th := Setup(t)
 	defer th.TearDown()
 
-	// 1. Test with custom source
+	// Set license for all tests
-	id := model.NewId()
-	group, err := th.App.CreateGroup(&model.Group{
-		DisplayName: "dn_" + id,
-		Name:        model.NewPointer("name" + id),
-		Source:      model.GroupSourceCustom,
-		Description: "description_" + id,
-	})
-	assert.Nil(t, err)
-
-	user1, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user1", Password: "test-password-1", Username: "test-user-1", Roles: model.SystemUserRoleId})
-	assert.Nil(t, appErr)
-
-	user2, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user2", Password: "test-password-2", Username: "test-user-2", Roles: model.SystemUserRoleId})
-	assert.Nil(t, appErr)
-
-	members := &model.GroupModifyMembers{
-		UserIds: []string{user1.Id, user2.Id},
-	}
-
 	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))
 
-	//Empty group members returns bad request
+	// setup creates a fresh group and users for each test
-	_, resp, nullErr := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, nil)
+	setup := func(t *testing.T) (*model.Group, []*model.User) {
-	require.Error(t, nullErr)
+		// Create custom group
-	CheckBadRequestStatus(t, resp)
+		id := model.NewId()
+		group, err := th.App.CreateGroup(&model.Group{
+			DisplayName: "dn_" + id,
+			Name:        model.NewPointer("name" + id),
+			Source:      model.GroupSourceCustom,
+			Description: "description_" + id,
+		})
+		require.Nil(t, err)
 
-	groupMembers, response, upsertErr := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, members)
+		// Create test users with random usernames to prevent collisions
-	require.NoError(t, upsertErr)
+		users := make([]*model.User, 3)
-	CheckOKStatus(t, response)
+		for i := 0; i < 3; i++ {
+			randomId := model.NewId()
+			user, appErr := th.App.CreateUser(th.Context, &model.User{
+				Email:    th.GenerateTestEmail(),
+				Nickname: fmt.Sprintf("test user%d-%s", i+1, randomId),
+				Password: fmt.Sprintf("test-password-%d", i+1),
+				Username: fmt.Sprintf("test-user-%d-%s", i+1, randomId),
+				Roles:    model.SystemUserRoleId,
+			})
+			require.Nil(t, appErr)
+			users[i] = user
+		}
 
-	assert.Len(t, groupMembers, 2)
+		return group, users
-
-	count, countErr := th.App.GetGroupMemberCount(group.Id, nil)
-	assert.Nil(t, countErr)
-
-	assert.Equal(t, count, int64(2))
-
-	// 2. Test invalid group ID
-	_, response, upsertErr = th.Client.UpsertGroupMembers(context.Background(), "abc123", members)
-	require.Error(t, upsertErr)
-	CheckBadRequestStatus(t, response)
-
-	// 3. Test invalid user ID
-	invalidMembers := &model.GroupModifyMembers{
-		UserIds: []string{"abc123"},
 	}
 
-	_, response, upsertErr = th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, invalidMembers)
+	t.Run("empty group members returns bad request", func(t *testing.T) {
-	require.Error(t, upsertErr)
+		group, _ := setup(t)
-	CheckInternalErrorStatus(t, response)
 
-	// 4. Test with ldap source
+		_, resp, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, nil)
-	ldapId := model.NewId()
+		require.Error(t, err)
-	ldapGroup, err := th.App.CreateGroup(&model.Group{
+		CheckBadRequestStatus(t, resp)
-		DisplayName: "dn_" + ldapId,
-		Name:        model.NewPointer("name" + ldapId),
-		Source:      model.GroupSourceLdap,
-		Description: "description_" + ldapId,
-		RemoteId:    model.NewPointer(model.NewId()),
 	})
-	assert.Nil(t, err)
 
-	_, response, upsertErr = th.SystemAdminClient.UpsertGroupMembers(context.Background(), ldapGroup.Id, members)
+	t.Run("successfully add members to custom group", func(t *testing.T) {
+		group, users := setup(t)
 
-	require.Error(t, upsertErr)
+		members := &model.GroupModifyMembers{
-	CheckBadRequestStatus(t, response)
+			UserIds: []string{users[0].Id, users[1].Id},
+		}
+
+		groupMembers, response, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, members)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		require.Len(t, groupMembers, 2)
+
+		count, countErr := th.App.GetGroupMemberCount(group.Id, nil)
+		require.Nil(t, countErr)
+		require.Equal(t, int64(2), count)
+	})
+
+	t.Run("adding existing members", func(t *testing.T) {
+		group, users := setup(t)
+
+		// First, add two users to the group
+		initialMembers := &model.GroupModifyMembers{
+			UserIds: []string{users[0].Id, users[1].Id},
+		}
+
+		returnedMembers, response, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, initialMembers)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+		require.Len(t, returnedMembers, 2)
+
+		// Try to add a user that's already in the group
+		existingMembers := &model.GroupModifyMembers{
+			UserIds: []string{users[0].Id},
+		}
+
+		groupMembers, response, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, existingMembers)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		// Should return empty array since no new members were added
+		require.Len(t, groupMembers, 1)
+
+		// Verify the group still has the original member count
+		count, countErr := th.App.GetGroupMemberCount(group.Id, nil)
+		require.Nil(t, countErr)
+		require.Equal(t, int64(2), count)
+
+		// Try with multiple users - one already in group, one not
+		mixedMembers := &model.GroupModifyMembers{
+			UserIds: []string{users[0].Id, users[2].Id},
+		}
+
+		groupMembers, response, err = th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, mixedMembers)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		// Should only return the new member
+		require.Len(t, groupMembers, 2)
+
+		// Verify the group now has 3 members
+		count, countErr = th.App.GetGroupMemberCount(group.Id, nil)
+		require.Nil(t, countErr)
+		require.Equal(t, int64(3), count)
+	})
+
+	t.Run("invalid group ID", func(t *testing.T) {
+		_, users := setup(t)
+
+		members := &model.GroupModifyMembers{
+			UserIds: []string{users[0].Id, users[1].Id},
+		}
+
+		_, response, err := th.Client.UpsertGroupMembers(context.Background(), "abc123", members)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+	})
+
+	t.Run("invalid user ID format", func(t *testing.T) {
+		group, _ := setup(t)
+
+		invalidMembers := &model.GroupModifyMembers{
+			UserIds: []string{"abc123"},
+		}
+
+		_, response, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, invalidMembers)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+	})
+
+	t.Run("non-existent user ID", func(t *testing.T) {
+		group, _ := setup(t)
+
+		nonExistentID := model.NewId()
+		nonExistentMembers := &model.GroupModifyMembers{
+			UserIds: []string{nonExistentID},
+		}
+
+		_, response, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, nonExistentMembers)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+		require.Contains(t, err.Error(), fmt.Sprintf(`User with username "%s" could not be found.`, nonExistentID))
+	})
+
+	t.Run("ldap group rejects adding members", func(t *testing.T) {
+		_, users := setup(t)
+
+		// Create LDAP group
+		ldapId := model.NewId()
+		ldapGroup, err := th.App.CreateGroup(&model.Group{
+			DisplayName: "dn_" + ldapId,
+			Name:        model.NewPointer("name" + ldapId),
+			Source:      model.GroupSourceLdap,
+			Description: "description_" + ldapId,
+			RemoteId:    model.NewPointer(model.NewId()),
+		})
+		require.Nil(t, err)
+
+		members := &model.GroupModifyMembers{
+			UserIds: []string{users[0].Id, users[1].Id},
+		}
+
+		_, response, upsertErr := th.SystemAdminClient.UpsertGroupMembers(context.Background(), ldapGroup.Id, members)
+		require.Error(t, upsertErr)
+		CheckBadRequestStatus(t, response)
+	})
 }
 
 func TestDeleteMembersFromGroup(t *testing.T) {
 	th := Setup(t)
 	defer th.TearDown()
 
-	// 1. Test with custom source
+	// Create test users
 	user1, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user1", Password: "test-password-1", Username: "test-user-1", Roles: model.SystemUserRoleId})
-	assert.Nil(t, appErr)
+	require.Nil(t, appErr)
 
 	user2, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user2", Password: "test-password-2", Username: "test-user-2", Roles: model.SystemUserRoleId})
-	assert.Nil(t, appErr)
+	require.Nil(t, appErr)
 
+	user3, appErr := th.App.CreateUser(th.Context, &model.User{Email: th.GenerateTestEmail(), Nickname: "test user3", Password: "test-password-3", Username: "test-user-3", Roles: model.SystemUserRoleId})
+	require.Nil(t, appErr)
+
+	// Create custom group with two members
 	id := model.NewId()
 	g := &model.Group{
 		DisplayName: "dn_" + id,
@@ -2619,46 +2724,9 @@ func TestDeleteMembersFromGroup(t *testing.T) {
 		Group:   *g,
 		UserIds: []string{user1.Id, user2.Id},
 	})
-	assert.Nil(t, err)
+	require.Nil(t, err)
 
-	members := &model.GroupModifyMembers{
+	// Create LDAP group with the same members
-		UserIds: []string{user1.Id},
-	}
-
-	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))
-
-	_, resp, nullErr := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, nil)
-	require.Error(t, nullErr)
-	CheckBadRequestStatus(t, resp)
-
-	groupMembers, response, deleteErr := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, members)
-	require.NoError(t, deleteErr)
-	CheckOKStatus(t, response)
-
-	assert.Len(t, groupMembers, 1)
-	assert.Equal(t, groupMembers[0].UserId, user1.Id)
-
-	users, usersErr := th.App.GetGroupMemberUsers(group.Id)
-	assert.Nil(t, usersErr)
-
-	assert.Len(t, users, 1)
-	assert.Equal(t, users[0].Id, user2.Id)
-
-	// 2. Test invalid group ID
-	_, response, deleteErr = th.Client.DeleteGroupMembers(context.Background(), "abc123", members)
-	require.Error(t, deleteErr)
-	CheckBadRequestStatus(t, response)
-
-	// 3. Test invalid user ID
-	invalidMembers := &model.GroupModifyMembers{
-		UserIds: []string{"abc123"},
-	}
-
-	_, response, deleteErr = th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, invalidMembers)
-	require.Error(t, deleteErr)
-	CheckInternalErrorStatus(t, response)
-
-	// 4. Test with ldap source
 	ldapId := model.NewId()
 	g1 := &model.Group{
 		DisplayName: "dn_" + ldapId,
@@ -2671,10 +2739,86 @@ func TestDeleteMembersFromGroup(t *testing.T) {
 		Group:   *g1,
 		UserIds: []string{user1.Id, user2.Id},
 	})
-	assert.Nil(t, err)
+	require.Nil(t, err)
 
-	_, response, deleteErr = th.SystemAdminClient.DeleteGroupMembers(context.Background(), ldapGroup.Id, members)
+	// Set license
+	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))
 
-	require.Error(t, deleteErr)
+	t.Run("Fail with nil member list", func(t *testing.T) {
-	CheckBadRequestStatus(t, response)
+		_, resp, err := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, nil)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
+
+	t.Run("Success with valid member removal", func(t *testing.T) {
+		members := &model.GroupModifyMembers{
+			UserIds: []string{user1.Id},
+		}
+
+		groupMembers, response, err := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, members)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		require.Len(t, groupMembers, 1)
+		require.Equal(t, groupMembers[0].UserId, user1.Id)
+
+		// Verify only one user remains in the group
+		users, usersErr := th.App.GetGroupMemberUsers(group.Id)
+		require.Nil(t, usersErr)
+		require.Len(t, users, 1)
+		require.Equal(t, users[0].Id, user2.Id)
+	})
+
+	t.Run("Fail with invalid group ID", func(t *testing.T) {
+		members := &model.GroupModifyMembers{
+			UserIds: []string{user1.Id},
+		}
+
+		_, response, err := th.Client.DeleteGroupMembers(context.Background(), "abc123", members)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+	})
+
+	t.Run("Fail with invalid user ID format", func(t *testing.T) {
+		invalidMembers := &model.GroupModifyMembers{
+			UserIds: []string{"abc123"},
+		}
+
+		_, response, err := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, invalidMembers)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+	})
+
+	t.Run("Fail with non-existent user ID", func(t *testing.T) {
+		nonExistentID := model.NewId()
+		nonExistentMembers := &model.GroupModifyMembers{
+			UserIds: []string{nonExistentID},
+		}
+
+		_, response, err := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, nonExistentMembers)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+		require.Contains(t, err.Error(), fmt.Sprintf(`User with username "%s" could not be found.`, nonExistentID))
+	})
+
+	t.Run("Fail with user not in group", func(t *testing.T) {
+		validNonMemberMembers := &model.GroupModifyMembers{
+			UserIds: []string{user3.Id},
+		}
+
+		_, response, err := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, validNonMemberMembers)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+		require.Contains(t, err.Error(), fmt.Sprintf(`User with username "%s" could not be found.`, user3.Id))
+	})
+
+	t.Run("Fail with LDAP source group", func(t *testing.T) {
+		members := &model.GroupModifyMembers{
+			UserIds: []string{user1.Id},
+		}
+
+		_, response, err := th.SystemAdminClient.DeleteGroupMembers(context.Background(), ldapGroup.Id, members)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, response)
+	})
 }

server/channels/app/group.go

@@ -323,11 +323,14 @@ func (a *App) UpsertGroupMember(groupID string, userID string) (*model.GroupMemb
 	if err != nil {
 		var invErr *store.ErrInvalidInput
 		var appErr *model.AppError
+		var nfErr *store.ErrNotFound
 		switch {
 		case errors.As(err, &appErr):
 			return nil, appErr
 		case errors.As(err, &invErr):
 			return nil, model.NewAppError("UpsertGroupMember", "app.group.uniqueness_error", nil, "", http.StatusBadRequest).Wrap(err)
+		case errors.As(err, &nfErr):
+			return nil, model.NewAppError("UpsertGroupMember", "app.group.user_not_found", map[string]any{"Username": nfErr.ID}, "", http.StatusBadRequest).Wrap(err)
 		default:
 			return nil, model.NewAppError("UpsertGroupMember", "app.update_error", nil, "", http.StatusInternalServerError).Wrap(err)
 		}
@@ -811,11 +814,14 @@ func (a *App) UpsertGroupMembers(groupID string, userIDs []string) ([]*model.Gro
 	if err != nil {
 		var invErr *store.ErrInvalidInput
 		var appErr *model.AppError
+		var nfErr *store.ErrNotFound
 		switch {
 		case errors.As(err, &appErr):
 			return nil, appErr
 		case errors.As(err, &invErr):
 			return nil, model.NewAppError("UpsertGroupMembers", "app.group.uniqueness_error", nil, "", http.StatusBadRequest).Wrap(err)
+		case errors.As(err, &nfErr):
+			return nil, model.NewAppError("UpsertGroupMembers", "app.group.user_not_found", map[string]any{"Username": nfErr.ID}, "", http.StatusBadRequest).Wrap(err)
 		default:
 			return nil, model.NewAppError("UpsertGroupMembers", "app.update_error", nil, "", http.StatusInternalServerError).Wrap(err)
 		}
@@ -835,11 +841,15 @@ func (a *App) DeleteGroupMembers(groupID string, userIDs []string) ([]*model.Gro
 	if err != nil {
 		var invErr *store.ErrInvalidInput
 		var appErr *model.AppError
+		var nfErr *store.ErrNotFound
 		switch {
 		case errors.As(err, &appErr):
 			return nil, appErr
 		case errors.As(err, &invErr):
 			return nil, model.NewAppError("DeleteGroupMember", "app.group.uniqueness_error", nil, "", http.StatusBadRequest).Wrap(err)
+		case errors.As(err, &nfErr):
+			return nil, model.NewAppError("DeleteGroupMember", "app.group.user_not_found", map[string]any{"Username": nfErr.ID}, "", http.StatusBadRequest).Wrap(err)
+
 		default:
 			return nil, model.NewAppError("DeleteGroupMember", "app.update_error", nil, "", http.StatusInternalServerError).Wrap(err)
 		}

server/i18n/en.json

@@ -5370,6 +5370,10 @@
     "id": "app.group.uniqueness_error",
     "translation": "group member already exists"
   },
+  {
+    "id": "app.group.user_not_found",
+    "translation": "Error updating group. User with username \"{{.Username}}\" could not be found."
+  },
   {
     "id": "app.group.username_conflict",
     "translation": "user with username \"{{.Username}}\" already exists."