sec(frontend): refuse to save a group with zero permissions

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 = `
+    <div id="group-modal" class="hidden">
+      <span id="group-modal-title"></span>
+      <form id="group-form">
+        <input id="group-id" />
+        <input id="group-name" />
+        <textarea id="group-description"></textarea>
+        <div id="permissions-list"></div>
+      </form>
+    </div>
+  `;
+}
+
+async function submitForm(): Promise<void> {
+  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<HTMLButtonElement>('.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<HTMLSelectElement>('.perm-action')!;
+    const resourceSelect = document.querySelector<HTMLSelectElement>('.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();
+  });
+});

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' }],
       });
     });
 

frontend/src/__tests__/groups.test.ts

@@ -104,7 +104,7 @@
     document.body.innerHTML = `
       <div id="groups-list"></div>
     `;
     userState.setAllUsers(mockUsers as any);
     groupState.setCurrentEditingGroup(null);
   });
 
@@ -165,7 +165,7 @@
       // Set up users so group-1 has exactly 1 member
       userState.setAllUsers([
         { id: 'user-1', email: 'admin@test.com', groups: ['00000000-0000-5000-8000-000000000001', 'group-1'], mfa_enabled: true },
       ] as any);
 
       const group = mockGroups[0];
       if (!group) throw new Error('Test data missing');
@@ -289,7 +289,7 @@
     it('should attach click handlers to delete buttons', async () => {
       (api.deleteGroup as jest.Mock).mockResolvedValue(undefined);
       mockConfirmDialog.mockResolvedValue(true);
       userState.setAvailableGroups(mockGroups as any);
 
       groupList.renderGroups(mockGroups);
 
@@ -718,7 +718,7 @@
       }));
     });
 
-    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 = `
         <div id="group-modal">
           <form id="group-form">
@@ -729,11 +729,15 @@
       `;
 
       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();
     });
   });
 
@@ -849,7 +853,7 @@
     document.body.innerHTML = `
       <div id="groups-list"></div>
     `;
     userState.setAvailableGroups(mockGroups as any);
     jest.clearAllMocks();
     mockConfirmDialog.mockResolvedValue(true);
     (api.deleteGroup as jest.Mock).mockResolvedValue(undefined);
@@ -922,22 +926,22 @@
       </form>
     `;
     // Clear window properties
     delete (window as any).openCreateGroupModal;
     delete (window as any).closeGroupModal;
     delete (window as any).addPermission;
     jest.clearAllMocks();
   });
 
   describe('setupGroupHandlers', () => {
     it('should expose openCreateGroupModal on window', () => {
       groupHandlers.setupGroupHandlers();
       expect((window as any).openCreateGroupModal).toBeDefined();
       expect(typeof (window as any).openCreateGroupModal).toBe('function');
     });
 
     it('should expose closeGroupModal on window', () => {
       groupHandlers.setupGroupHandlers();
       expect((window as any).closeGroupModal).toBeDefined();
       expect(typeof (window as any).closeGroupModal).toBe('function');
     });
 

frontend/src/groups/groupModals.ts

@@ -106,6 +106,22 @@ export async function saveGroup(e: Event): Promise<void> {
   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

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),

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

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

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)