diff --git a/frontend/src/__tests__/group-edit-empty-permissions.test.ts b/frontend/src/__tests__/group-edit-empty-permissions.test.ts new file mode 100644 index 000000000..32ab24953 --- /dev/null +++ b/frontend/src/__tests__/group-edit-empty-permissions.test.ts @@ -0,0 +1,145 @@ +/** + * Regression tests for issue #237: removing every permission row from the + * group edit form and saving reported "Group updated successfully" while + * silently leaving the group's stored permissions untouched. + * + * collectPermissions() (groupModals.ts) skips any row whose action or + * resource is still the empty "Select Action"/"Select Resource" placeholder, + * so clicking Remove on every real row -- or leaving only a placeholder row + * -- collects as permissions: []. The update API's documented contract + * (applyUpdateGroupRequest, internal/auth/service_api.go) treats an empty + * permissions list as "not sent" and leaves the group's permissions + * unchanged -- there is no way for this endpoint to express "clear all + * permissions". So the form would show success and reload a group that + * still carried its old privileges (possibly admin:*). + * + * The fix refuses to submit when collectPermissions() returns [], with an + * explicit error telling the operator to delete the group instead. + */ + +import './setup'; + +jest.mock('../api', () => ({ + getGroup: jest.fn(), + updateGroup: jest.fn(), + createGroup: jest.fn(), +})); + +jest.mock('../users/userActions', () => ({ + loadUsers: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn().mockResolvedValue(true), +})); + +jest.mock('../users/utils', () => ({ + ...jest.requireActual('../users/utils'), + showError: jest.fn(), + showSuccess: jest.fn(), +})); + +import * as api from '../api'; +import * as groupState from '../groups/state'; +import * as groupModals from '../groups/groupModals'; +import { showError, showSuccess } from '../users/utils'; + +function setUpModalDom(): void { + document.body.innerHTML = ` + + `; +} + +async function submitForm(): Promise { + const event = { preventDefault: jest.fn() } as unknown as Event; + await groupModals.saveGroup(event); +} + +const ADMIN_GROUP: api.APIGroup = { + id: 'admin-group-id', + name: 'Admins', + description: 'Old description', + permissions: [{ action: 'admin', resource: '*' }], + created_at: '2024-01-01T00:00:00Z', +}; + +describe('regression: saving a group with every permission row removed (#237)', () => { + beforeEach(() => { + setUpModalDom(); + groupState.setCurrentEditingGroup(null); + jest.clearAllMocks(); + }); + + test('removing every permission row and saving does not call updateGroup and shows an error, not success', async () => { + (api.getGroup as jest.Mock).mockResolvedValue(ADMIN_GROUP); + await groupModals.openEditGroupModal(ADMIN_GROUP.id); + + // Remove the one loaded permission row via its own Remove button -- + // the exact operator action the issue describes. + const removeBtn = document.querySelector('.remove-permission-btn'); + expect(removeBtn).not.toBeNull(); + removeBtn!.click(); + expect(document.querySelectorAll('.permission-item')).toHaveLength(0); + + await submitForm(); + + // The group's real admin:* permissions must never be silently kept by + // submitting an empty list the backend interprets as "unchanged" -- + // submission must not happen at all. + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(showSuccess).not.toHaveBeenCalled(); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + }); + + test('a group whose only remaining row is left on the placeholder also refuses to save', async () => { + (api.getGroup as jest.Mock).mockResolvedValue(ADMIN_GROUP); + await groupModals.openEditGroupModal(ADMIN_GROUP.id); + + // Reset the loaded row back to the "Select Action" / "Select Resource" + // placeholder instead of removing it outright -- the issue's second + // reproduction shape. + const actionSelect = document.querySelector('.perm-action')!; + const resourceSelect = document.querySelector('.perm-resource')!; + actionSelect.value = ''; + resourceSelect.value = ''; + + await submitForm(); + + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(showSuccess).not.toHaveBeenCalled(); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + }); + + test('creating a new group with no permission rows also refuses to save', async () => { + groupModals.openCreateGroupModal(); + (document.getElementById('group-name') as HTMLInputElement).value = 'New Group'; + + await submitForm(); + + expect(api.createGroup).not.toHaveBeenCalled(); + expect(showSuccess).not.toHaveBeenCalled(); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + }); + + test('a group that still has at least one real permission row saves normally', async () => { + (api.getGroup as jest.Mock).mockResolvedValue(ADMIN_GROUP); + await groupModals.openEditGroupModal(ADMIN_GROUP.id); + + await submitForm(); + + expect(api.updateGroup).toHaveBeenCalledWith('admin-group-id', { + name: 'Admins', + description: 'Old description', + permissions: [{ action: 'admin', resource: '*' }], + }); + expect(showError).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts b/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts index 57d6bd996..4f260daf8 100644 --- a/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts +++ b/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts @@ -579,7 +579,12 @@ describe('regression: group edit preserves constraints.accounts (#1629)', () => test('removing the offending row unblocks the save', async () => { // The verdict lives on the row, so deleting the row clears it. Removing // the permission outright is an explicit edit, not a silent drop. - await openEdit(groupWithAccounts([''])); + // A second, unrelated clean permission is included so removing the + // offending row exercises this refusal in isolation from the separate + // "a group must keep at least one permission" refusal (issue #237). + const group = groupWithAccounts(['']); + group.permissions.push({ action: 'view', resource: 'plans' }); + await openEdit(group); const removeBtn = document.querySelector('.permission-item .remove-permission-btn') as HTMLButtonElement; removeBtn.click(); await clickSave(); @@ -588,7 +593,7 @@ describe('regression: group edit preserves constraints.accounts (#1629)', () => expect(api.updateGroup).toHaveBeenCalledWith('operators-group-id', { name: 'Prod Operators', description: 'Old description', - permissions: [], + permissions: [{ action: 'view', resource: 'plans' }], }); }); diff --git a/frontend/src/__tests__/groups.test.ts b/frontend/src/__tests__/groups.test.ts index 81f729b31..0ebe7f9b2 100644 --- a/frontend/src/__tests__/groups.test.ts +++ b/frontend/src/__tests__/groups.test.ts @@ -718,7 +718,7 @@ describe('groups/groupModals', () => { })); }); - it('should handle missing permissions list', async () => { + it('should handle a missing permissions list without throwing, and refuse to save (issue #237)', async () => { document.body.innerHTML = `
@@ -729,11 +729,15 @@ describe('groups/groupModals', () => { `; const event = { preventDefault: jest.fn() } as unknown as Event; - await groupModals.saveGroup(event); - - expect(api.createGroup).toHaveBeenCalledWith(expect.objectContaining({ - permissions: [], - })); + // Must not throw even though #permissions-list is entirely absent from + // the DOM (collectPermissions() returns [] rather than erroring). + await expect(groupModals.saveGroup(event)).resolves.toBeUndefined(); + + // A group must grant at least one permission -- an empty list is + // refused rather than submitted as a group with zero permissions + // (issue #237: the update path's [] means "leave unchanged", so + // submitting it here would be indistinguishable from that). + expect(api.createGroup).not.toHaveBeenCalled(); }); }); diff --git a/frontend/src/groups/groupModals.ts b/frontend/src/groups/groupModals.ts index b9e8bfae7..192744ab3 100644 --- a/frontend/src/groups/groupModals.ts +++ b/frontend/src/groups/groupModals.ts @@ -106,6 +106,22 @@ export async function saveGroup(e: Event): Promise { const description = (document.getElementById('group-description') as HTMLTextAreaElement).value; const permissions = collectPermissions(); + // Refuse an empty permission list rather than submit it. collectPermissions + // silently drops any row left on the "Select Action"/"Select Resource" + // placeholder, so removing every real row (or leaving only a placeholder + // row) collects as []. The update API treats an empty list as "not sent" + // and leaves the group's stored permissions untouched (issue #237), so + // submitting would show "Group updated successfully" while silently + // keeping the group's old privileges -- there is no way for this form to + // express "clear all permissions". + if (permissions.length === 0) { + showError( + 'A group must grant at least one permission. ' + + 'To remove all privileges, delete the group instead.' + ); + return; + } + try { if (currentEditingGroup) { // Update existing group diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 52ef624f1..13d2c2ca7 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -65,7 +65,7 @@ func (h *Handler) createGroup(ctx context.Context, req *events.LambdaFunctionURL // are disjoint. Unrecognized errors pass through for handleRequestError. func mapGroupAuthError(err error) error { switch { - case errors.Is(err, auth.ErrInvalidPermission): + case errors.Is(err, auth.ErrInvalidPermission), errors.Is(err, auth.ErrEmptyPermissions): // Malformed input, not an authorization failure. return NewClientError(400, err.Error()) case errors.Is(err, auth.ErrPermissionCeiling), diff --git a/internal/auth/errors.go b/internal/auth/errors.go index c9732cd3f..ad66392b0 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -63,6 +63,16 @@ var ( // tenant-wide. Mapped to 403 (issue #1629). ErrSystemManagedGroup = errors.New("system-managed group cannot be modified") + // ErrEmptyPermissions is returned when a group update explicitly sends + // permissions as an empty list. APIUpdateGroupRequest.Permissions is a + // pointer so a nil value (field omitted) can be told apart from a + // pointer to an empty slice (field sent as `[]`): the former means + // "leave unchanged" (this endpoint has no other way to express that), + // the latter must be refused rather than silently treated as a no-op + // that reports success while the group's real permissions -- possibly + // admin:* -- are left untouched (issue #237). Mapped to 400. + ErrEmptyPermissions = errors.New("a group must grant at least one permission; delete the group to remove all privileges") + // ErrCurrentPasswordIncorrect is returned by UpdateUserProfile when the // caller-supplied current password does not match the stored hash. Mapped // to 401 at the API layer (the acting user is verifying their own diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 33816deb4..0fb894ea1 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -102,6 +102,16 @@ type APICreateGroupRequest struct { // APIUpdateGroupRequest is the request type for updating groups via API. // AllowedAccounts has no omitempty: clients must be able to send an explicit // empty slice to clear account restrictions. Nil means "not sent". +// +// Permissions has the same "nil means not sent" contract, but encoding/json +// already gives UpdateGroupAPI what it needs without a pointer: decoding an +// omitted field leaves the slice nil, while decoding an explicit `[]` leaves +// it non-nil with length 0 (json.Unmarshal allocates the slice before +// leaving it empty). UpdateGroupAPI checks req.Permissions != nil && +// len(req.Permissions) == 0 to refuse that explicit-empty case -- this +// endpoint has no way to express "clear all permissions" (a group must +// always grant at least one) -- rather than silently keeping the group's +// old permissions while reporting success (issue #237). type APIUpdateGroupRequest struct { Name string `json:"name,omitempty"` Description string `json:"description,omitempty"` @@ -356,6 +366,16 @@ func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID strin return nil, fmt.Errorf("%w: %q is seeded and maintained by migrations", ErrSystemManagedGroup, group.Name) } + // req.Permissions == nil means the field was omitted: leave the group's + // permissions unchanged (this endpoint's pre-existing "not sent" + // contract). A non-nil-but-empty slice means the client explicitly sent + // `"permissions": []` (encoding/json allocates the slice before leaving + // it empty, so this is distinguishable from omission after decode), + // which this endpoint has no way to honor as "clear all permissions" -- + // refuse rather than silently no-op it (issue #237). + if req.Permissions != nil && len(req.Permissions) == 0 { + return nil, ErrEmptyPermissions + } perms := apiPermissionsToPermissions(req.Permissions) if err := s.checkGrantCeiling(ctx, actorUserID, perms, group.Permissions); err != nil { return nil, err diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index b351547cd..cdb5e42cc 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -406,6 +406,76 @@ func TestService_CreateGroupAPI(t *testing.T) { func TestService_UpdateGroupAPI(t *testing.T) { ctx := context.Background() + // Issue #237: an explicit `"permissions": []` must be refused (400), not + // silently treated as "leave unchanged". Pre-fix, APIUpdateGroupRequest. + // Permissions was a plain []APIPermission, which cannot distinguish + // "field omitted" from "field sent as []" once decoded -- both left the + // slice empty, and apiPermissionsToPermissions/applyUpdateGroupRequest's + // `len(perms) > 0` check silently kept the group's OLD permissions + // (possibly admin:*) while UpdateGroup still returned success. + t.Run("explicit empty permissions list is refused, not silently kept", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + existingGroup := &Group{ + ID: "group-123", + Name: "Admins", + Description: "Old description", + Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + } + mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once() + + req := APIUpdateGroupRequest{ + Name: "Admins", + Description: "New description", + Permissions: []APIPermission{}, // explicit []: every row removed and saved + } + + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) + + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, ErrEmptyPermissions) + // The group's real admin:* permissions must never be silently kept by + // a write that reports success -- UpdateGroup must not even be called. + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + + // Negative control: an omitted Permissions field (nil pointer) is the + // pre-existing "leave unchanged" contract and must still work exactly as + // before -- this refusal must not overreach into that case. + t.Run("omitted permissions field leaves the group's permissions unchanged", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + existingGroup := &Group{ + ID: "group-123", + Name: "Admins", + Description: "Old description", + Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + } + mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once() + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + req := APIUpdateGroupRequest{ + Name: "Admins", + Description: "New description", + // Permissions left as its zero value: nil pointer, field omitted. + } + + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) + + require.NoError(t, err) + apiGroup, ok := result.(*APIGroup) + require.True(t, ok) + assert.Equal(t, "New description", apiGroup.Description) + assert.Equal(t, []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, existingGroup.Permissions, + "omitting the field must leave permissions untouched") + mockStore.AssertExpectations(t) + }) + t.Run("successful group update", func(t *testing.T) { mockStore := new(MockStore) mockEmail := new(MockEmailSender)