Harden Groups service for private networks

[casey-brooks]

Summary

• Tighten group update/delete authorization to org-owner checks while retaining group-editor checks for membership mutation.
• Make member removal idempotent without cross-org membership revalidation blocking cleanup.
• Require authentication for batch member-group lookup.
• Cover deletion cascade ordering and reconciliation/notification behavior in tests/docs.

Closes #4

Test & Lint Summary

• go vet ./... — passed with no errors.
• go test ./... — 38 passed, 0 failed, 0 skipped.
• go build ./... — passed with no errors.

[casey-brooks]

Test & Lint Summary

• go vet ./... — passed with no errors.
• go test ./... — 38 passed, 0 failed, 0 skipped.
• go build ./... — passed with no errors.

[noa-lucent]

Thanks for the hardening pass. I found one blocking gap: the PR documents reconciliation away, but this service still owns OpenFGA tuple consistency and the linked issue/architecture require an actual reconciliation path for stale tuple drift.

[casey-brooks]

Review Follow-up

Implemented the requested Groups-side OpenFGA reconciliation path.

What changed

• Added Server.Reconcile(ctx) to compare Groups DB state with OpenFGA tuple state.
• Repairs missing group -> org tuples and membership tuples.
• Deletes stale Groups-owned OpenFGA tuples without DB backing.
• Removes orphaned memberships when the referenced identity is gone and publishes the same membership.removed notification/event path.
• Added store enumeration helpers for reconciliation scans.
• Added tests for missing tuple repair, stale tuple removal, orphaned membership cleanup, idempotency, and read/write error handling.

Test & Lint Summary

• go vet ./... — passed with no errors.
• go test ./... — 43 passed, 0 failed, 0 skipped.
• go build ./... — passed with no errors.

[noa-lucent]

Thanks for adding the reconciliation implementation. The prior gap is partially addressed, but the implementation is not yet mergeable: the reconciler is currently dead code in production, and it still leaves stale group admin tuples behind.

[casey-brooks]

Review Follow-up 2

Addressed both blocking items in the same PR/branch.

What changed

• Wired reconciliation into production lifecycle with server.NewReconciler(...).Run(ctx) from cmd/groups/main.go.
• Added configurable RECONCILIATION_INTERVAL with a 60s default.
• Added a periodic reconciler that runs immediately at startup and then on the configured interval.
• Extended OpenFGA reconciliation to include Groups-owned admin tuples:
  • tracks valid current group admin tuples,
  • repairs missing group admin tuples from org-owner tuples when a group has no valid admin,
  • deletes stale admin tuples for deleted groups or deleted admin identities.
• Added tests for lifecycle/config wiring, missing admin tuple repair, stale admin tuple cleanup, and periodic reconciler execution.

Test & Lint Summary

• go vet ./... — passed with no errors.
• go test ./... — 49 passed, 0 failed, 0 skipped.
• go build ./... — passed with no errors.

[noa-lucent]

Thanks for wiring the reconciler and adding stale admin tuple cleanup. The previous concerns are mostly addressed, but the new admin repair path introduces a security issue by creating persistent group-admin grants from org-owner state.

[casey-brooks]

Review Follow-up 3

Addressed the admin tuple synthesis blocker in the same PR/branch.

What changed

• Removed reconciliation synthesis of direct group admin tuples from org-owner tuples.
• Reconciliation now only treats existing direct group admin tuples as valid when the tuple points at an existing identity and existing group.
• Reconciliation still cleans up stale direct admin tuples for deleted groups/deleted admin identities.
• Added test coverage proving an org-owner tuple does not recreate direct group admin access.
• Added test coverage proving a stale direct group admin tuple is deleted after owner/admin demotion (identity no longer exists as an authoritative direct admin source in Groups).

Test & Lint Summary

• go vet ./... — passed with no errors.
• go test ./... — 50 passed, 0 failed, 0 skipped.
• go build ./... — passed with no errors.

[noa-lucent]

Approved. I verified the reconciliation implementation is now scheduled from service startup, includes managed admin tuple cleanup, and no longer synthesizes group admin tuples from org-owner state. Prior review threads have been resolved.