[cristim]

Summary

The group edit form skips permission rows with an empty action or resource, so removing every row and saving submitted an empty permission list. The backend's update contract treats an empty list as "not sent" and leaves stored permissions untouched, so the UI showed "Group updated successfully" and reloaded a group that still carried its old privileges, possibly admin:*. An admin who believed they had stripped a group's privileges had not.

Update: an independent review of this PR found the frontend-only guard insufficient -- any non-browser client (CLI, scripts, curl) could still send PUT /api/groups/{id} with "permissions": [] directly and hit the same silent no-op. This PR now fixes both layers.

Root cause

frontend/src/groups/groupModals.ts's collectPermissions() (line ~405) skips any row whose action or resource is still the empty "Select Action"/"Select Resource" placeholder. saveGroup() submitted whatever collectPermissions() returned without checking for the empty case.

internal/auth/service_api.go's UpdateGroupAPI / applyUpdateGroupRequest (if len(perms) > 0) treats an empty permissions list as "field not sent" -- a deliberate, documented contract for the omitted-field case, but it also silently absorbed an explicit "permissions": [], which is indistinguishable at that point in the code from omission.

Fix

Frontend: saveGroup() now refuses to submit (for both create and update) when collectPermissions() returns an empty list, showing: "A group must grant at least one permission. To remove all privileges, delete the group instead."

Backend: encoding/json already distinguishes an omitted permissions field (decodes to nil) from an explicit "permissions": [] (decodes to a non-nil, zero-length slice) -- no API type change needed. UpdateGroupAPI now checks req.Permissions != nil && len(req.Permissions) == 0 and returns the new ErrEmptyPermissions sentinel (mapped to HTTP 400 by mapGroupAuthError) for that explicit-empty case. An omitted field still leaves permissions unchanged, exactly as before.

Regression tests

Frontend -- new frontend/src/__tests__/group-edit-empty-permissions.test.ts:

• removing every permission row and saving does not call updateGroup and shows an error, not success
• a group whose only remaining row is left on the placeholder also refuses to save
• creating a new group with no permission rows also refuses to save
• a group that still has at least one real permission row saves normally (negative control)

Also fixed two pre-existing tests whose assertions encoded the buggy behavior as correct:

• groups.test.ts "should handle missing permissions list" expected createGroup to be called with permissions: []; updated to assert the save is now refused instead.
• group-edit-unrepresentable-permissions.test.ts "removing the offending row unblocks the save" removed the group's one and only permission row and expected updateGroup to be called with permissions: []; added a second, unrelated clean permission so the test exercises the unrepresentable-constraint refusal in isolation from this new empty-permissions refusal.

Backend -- new tests in internal/auth/service_api_test.go under TestService_UpdateGroupAPI:

• explicit empty permissions list is refused, not silently kept -- sends Permissions: []APIPermission{}, asserts ErrEmptyPermissions, and asserts UpdateGroup (the store write) is never called.
• omitted permissions field leaves the group's permissions unchanged -- negative control: omitted field still succeeds and leaves the group's stored permissions untouched.

Proved pre-fix failure for both:

• Frontend: git stash push -- frontend/src/groups/groupModals.ts then ran group-edit-empty-permissions.test.ts -- 3 of 4 new tests failed (updateGroup/createGroup called once when it should not have been). Restored via git stash pop and reran: all 4 pass.
• Backend: with the new presence check temporarily disabled (leaving only the sentinel error definition in place so the test still compiles), TestService_UpdateGroupAPI/explicit_empty_permissions_list_is_refused failed with a mock panic ("I don't know what to return because the method call was unexpected" on UpdateGroup) -- i.e. the store write fired exactly as the issue describes. Restored the check and reran: all 6 TestService_UpdateGroupAPI subtests pass.

Verification

• npm test -- group -- 133 passed (3 suites)
• npm test -- users -- 2969 passed / 1 skipped (92 suites)
• npx eslint src/groups/groupModals.ts src/__tests__/group-edit-empty-permissions.test.ts src/__tests__/groups.test.ts src/__tests__/group-edit-unrepresentable-permissions.test.ts -- 0 errors (16 pre-existing no-explicit-any warnings in groups.test.ts, confirmed present on main before this change, unrelated to this diff)
• npm run build -- webpack production build succeeds (pre-existing entrypoint-size warning only)
• GOTOOLCHAIN=go1.26.6 go build ./... -- clean
• GOTOOLCHAIN=go1.26.6 go test -race -short ./internal/auth/... -- 801 passed
• GOTOOLCHAIN=go1.26.6 go test -race -short ./internal/api/... -- 2216 passed
• GOTOOLCHAIN=go1.26.6 go vet ./internal/auth/... ./internal/api/... -- clean
• GOTOOLCHAIN=go1.26.6 go mod tidy -diff -- clean
• golangci-lint v2.10.1 (exact CI pin, installed via go install .../golangci-lint/v2/cmd/golangci-lint@v2.10.1) on ./internal/auth/... ./internal/api/... -- 0 issues
• gocyclo -over 10 on the two changed backend files -- clean

Closes #237

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: 76760d57-e2f8-421c-942d-7dc0f4909507

📥 Commits

Reviewing files that changed from the base of the PR and between 2de05b2 and 2af296d.

📒 Files selected for processing (4)

• internal/api/handler_groups.go
• internal/auth/errors.go
• internal/auth/service_api.go
• internal/auth/service_api_test.go

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.

---

📝 Walkthrough

Walkthrough

The group form rejects submissions when it collects no permissions. The update API rejects an explicitly empty permissions list and treats omitted permissions as unchanged. Tests cover empty submissions and valid permission updates.

Changes

Group permission validation

Layer / File(s)	Summary
Reject empty form submissions frontend/src/groups/groupModals.ts, frontend/src/__tests__/group-edit-empty-permissions.test.ts, frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts, frontend/src/__tests__/groups.test.ts	saveGroup displays an error and returns when it collects no permissions. Tests cover empty create and edit submissions, a valid edit, and retaining a valid permission after removing an unrepresentable one.
Reject explicit empty API updates internal/auth/errors.go, internal/auth/service_api.go, internal/api/handler_groups.go, internal/auth/service_api_test.go	UpdateGroupAPI returns ErrEmptyPermissions for an explicitly empty permissions list. Omitted permissions remain unchanged. The API handler maps the error to HTTP 400. Tests cover both update cases.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

Merge Risk: ⚪ Minimal · up to 2af29

Empty permission lists are refused rather than reported as successful updates, while updates that omit permissions remain supported. No actionable merge-blocking risk is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #237 requires the group form to reject an empty collected permission list and explain that the group must grant at least one permission and must be deleted to remove all privileges. saveGroup …
Out of Scope Changes check	✅ Passed	The changes remain connected to issue #237. The frontend changes prevent false-success submissions. The API validation and tests preserve the omitted-versus-empty permission contract for other API cli…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: the frontend refuses to save groups with zero permissions.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent adversarial review (two rounds) + local verification: MERGE at 2af296d. The UI refuses to save a group with zero permissions, and the backend now returns 400 for an explicit "permissions": [] while absent or null still means unchanged (proven through the real decode path with a throwaway test); both directions are pinned by mutation-checked tests. Follow-up (in #394's spirit): add a handler-level test pinning the 400 mapping, and fix the stale 'pointer' wording in errors.go/test comments.