From d80d575f6c395153e3c417a2c2d295ff8ad2100c Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 14 May 2025 10:17:30 -0300 Subject: [PATCH] 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 | 19 +- server/channels/api4/group_test.go | 350 ++++++++++++++++++++--------- server/channels/app/group.go | 10 + server/i18n/en.json | 4 + 4 files changed, 277 insertions(+), 106 deletions(-) diff --git a/server/channels/api4/group.go b/server/channels/api4/group.go index ba34e0ebe0..661c80ecb1 100644 --- a/server/channels/api4/group.go +++ b/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) diff --git a/server/channels/api4/group_test.go b/server/channels/api4/group_test.go index 016546d738..0db8365b3d 100644 --- a/server/channels/api4/group_test.go +++ b/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 - 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}, - } - + // Set license for all tests th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) - //Empty group members returns bad request - _, resp, nullErr := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, nil) - require.Error(t, nullErr) - CheckBadRequestStatus(t, resp) + // setup creates a fresh group and users for each test + setup := func(t *testing.T) (*model.Group, []*model.User) { + // Create custom group + 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) - require.NoError(t, upsertErr) - CheckOKStatus(t, response) + // Create test users with random usernames to prevent collisions + users := make([]*model.User, 3) + 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) - - 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"}, + return group, users } - _, response, upsertErr = th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, invalidMembers) - require.Error(t, upsertErr) - CheckInternalErrorStatus(t, response) + t.Run("empty group members returns bad request", func(t *testing.T) { + group, _ := setup(t) - // 4. Test with ldap source - 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()), + _, resp, err := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, nil) + require.Error(t, err) + CheckBadRequestStatus(t, resp) }) - 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) - CheckBadRequestStatus(t, response) + members := &model.GroupModifyMembers{ + 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{ - 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 + // Create LDAP group with the same members 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) - CheckBadRequestStatus(t, response) + t.Run("Fail with nil member list", func(t *testing.T) { + _, 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) + }) } diff --git a/server/channels/app/group.go b/server/channels/app/group.go index f410640e53..075851f747 100644 --- a/server/channels/app/group.go +++ b/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) } diff --git a/server/i18n/en.json b/server/i18n/en.json index f1bbfc667a..6ff75d4d57 100644 --- a/server/i18n/en.json +++ b/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."