feat(auth): add Purchaser group + carve execute/approve-any/retry-any out of admin wildcard (closes #923) - #924
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 4 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds a system-managed Purchaser group, carves execute/approve-any/retry-any on purchases out of admin:* in backend auth, seeds Purchaser and system_managed metadata in the DB, updates frontend permission helpers/generated constants, and applies gating/UI notices and tests that reflect the new Purchaser semantics. ChangesAuthorization Model & Carve-Outs
Frontend Permission Model & Code Generation
Frontend Action Authorization and User-Facing Gating
Sequence Diagram(s)sequenceDiagram
participant User
participant canAccess
participant effectivePermissions
participant adminCarvedOuts
participant userGroups
User->>canAccess: check execute:purchases
alt with effectivePermissions loaded
canAccess->>effectivePermissions: lookup admin:*
effectivePermissions-->>canAccess: found admin:*
canAccess->>adminCarvedOuts: is execute:purchases carved-out?
adminCarvedOuts-->>canAccess: yes, carved out from admin:*
canAccess->>effectivePermissions: require exact execute:purchases entry
effectivePermissions-->>canAccess: entry found or not
else without effectivePermissions (fallback)
canAccess->>adminCarvedOuts: is execute:purchases carved-out?
adminCarvedOuts-->>canAccess: yes, carved out
canAccess->>userGroups: isPurchaser() check
userGroups-->>canAccess: user in Purchaser group?
end
canAccess-->>User: grant or deny based on Purchaser membership
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
force-pushed
the
feat/923-purchaser-group
branch
from
June 3, 2026 10:53
b1aea61 to
7a40be8
Compare
Member
Author
|
Rebased on Resolved conflicts in:
Security-sensitive merge note: PR #922's new The merged
Tests added under
Tests under
Stash-verified the new effective-permissions guard: removing Also fixed one em-dash in a code comment in Verification:
@coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/recommendations.ts (1)
2861-2863:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-sync purchase visibility after the box is already mounted.
mountBottomActionBox()snapshotspurchaseBtn.hiddenand the no-Purchaser banner once, then returns early on every later render. If the current user gains Purchaser membership in-session,updateBottomActionBox()never unhides the Purchase CTA or removes the notice. Since the new copy explicitly sends people to Settings → Users to fix access, this leaves the page stale until a full reload.Also applies to: 2904-2919, 3018-3132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/recommendations.ts` around lines 2861 - 2863, mountBottomActionBox() currently returns early if the DOM element 'recommendations-action-box' already exists, which prevents re-syncing purchaseBtn.hidden and the "no-Purchaser" banner when membership changes in-session; remove the early return (or explicitly call updateBottomActionBox()) so that on subsequent renders you re-evaluate and set purchase CTA visibility and banner state (inspect and update purchaseBtn.hidden, the no-Purchaser banner element, and any cached snapshot logic inside mountBottomActionBox/updateBottomActionBox) so membership changes are applied without a full reload; apply the same fix pattern to the other duplicated blocks referenced (the similar logic around lines ~2904-2919 and ~3018-3132).
🧹 Nitpick comments (4)
frontend/src/__tests__/permissions.test.ts (1)
243-287: ⚡ Quick winAdd one non-seeded custom-group case for the carved-out verbs.
These new cases prove the seeded Purchaser group works, but they still don't exercise the contract described in
permissions.tsfor a user who getsexecute/approve-any/retry-any:purchasesfrom some other custom group. A fixture with a non-PURCHASER_GROUP_IDgroup plus those expliciteffectivePermissionswould lock in the intended behavior and catch regressions around the helper mismatch above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/permissions.test.ts` around lines 243 - 287, Add a new test that mirrors the "Purchaser explicit grants" case but uses a non-seeded custom group id instead of PURCHASER_GROUP_ID to ensure carved-out verbs work when provided by any group; use mockUserWithGroups with a custom group id and an effectivePermissions array containing execute/approve-any/retry-any:purchases (and the four view:purchases/plans/recommendations/history as needed), then assert canAccess allows those spending and view verbs and denies admin/*, delete:plans, view:users, cancel-any:purchases; this will exercise the permission contract in permissions.ts and the canAccess helper for non-PURCHASER_GROUP_ID sources.frontend/src/__tests__/history-approve-button.test.ts (1)
68-77: ⚡ Quick winAdd the admin-without-Purchaser regression case here.
This fixture now only preserves migrated
admin + Purchaserbehaviour. The new contract infrontend/src/history.tsis that admin alone should not get approve-any, so this suite is missing the negative path that would catch a reintroduction ofisAdmin()here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/history-approve-button.test.ts` around lines 68 - 77, Add a negative fixture for an admin without Purchaser to cover the regression: declare a new test constant (e.g. ADMIN_NO_PURCH) with id/email and groups containing only ADMINISTRATORS_GROUP_ID (no PURCHASER_GROUP_ID), and update the test(s) that currently reference ADMIN_USER/REG_USER to include a case that uses ADMIN_NO_PURCH (via setupDOM or the existing test harness) asserting that this user does NOT receive approve-any/purchases permissions; reference ADMIN_USER, ADMIN_NO_PURCH, REG_USER, OTHER_UUID and setupDOM to locate where to add the new fixture and test case.frontend/src/__tests__/recommendations-permissions.test.ts (1)
68-77: ⚡ Quick winPlease cover the split-admin UI state explicitly.
mockUser('admin')now means “admin + Purchaser”, so this suite no longer checks the new frontend contract for admin-only sessions: Purchase hidden, Create Plan still visible, and the Purchaser notice shown. That is the highest-risk regression for this screen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/recommendations-permissions.test.ts` around lines 68 - 77, Current tests use mockUser('admin') which sets both ADMINISTRATORS_GROUP_ID and PURCHASER_GROUP_ID, so they miss the admin-only UI state; add an explicit test that simulates an admin without Purchaser by calling mockUserWithGroups (or extend mockUser to accept a special flag) and set state.getCurrentUser to return { id: 'u', email: 'u@example.com', groups: [ADMINISTRATORS_GROUP_ID] }; assert Purchase button/section is hidden, Create Plan remains visible, and the Purchaser notice is shown to cover the split-admin UI contract.frontend/src/__tests__/history-retry-button.test.ts (1)
74-77: ⚡ Quick winAdd the admin-without-Purchaser regression case here.
This suite now only exercises migrated
admin + Purchaserbehaviour. The carve-out infrontend/src/history.tsis that admin alone should lose retry-any, so please add the negative case; otherwise switching back toisAdmin()would still leave these tests green.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/history-retry-button.test.ts` around lines 74 - 77, Add a negative test case for an admin user that does NOT have the Purchaser group: define a new fixture (e.g., ADMIN_USER_NO_PURCHASER or ADMIN_USER_WITHOUT_PURCHASER) using the same id/email but with groups: [ADMINISTRATORS_GROUP_ID] only, and add assertions mirroring the existing positive case that verify the retry-any:purchases permission is NOT present for that user; update the test suite in history-retry-button.test.ts to exercise this case so the behavior in history.ts (admin without Purchaser should lose retry-any) is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/history.ts`:
- Around line 423-430: The UI is using isPurchaser() to gate "any" approve/retry
paths which diverges from backend auth; update canApprovePendingRow (and the
analogous checks at the other mentioned blocks) to call canAccess('approve-any',
'purchases') for the approve-any branch (and canAccess('retry-any', 'purchases')
for the retry-any branch) instead of isPurchaser(), keep the existing early
returns (status and getCurrentUser) intact, and ensure any banner/readonly
message is derived from those same canAccess checks so the UI uses the exact
capability checks the backend expects.
In `@frontend/src/permissions.ts`:
- Around line 106-118: isPurchaser() currently only checks PURCHASER_GROUP_ID
via state.getCurrentUser().user.groups; change it to derive from the user's
effectivePermissions when present: fetch user = state.getCurrentUser(), return
false if no user, then if user.effectivePermissions is defined check for any of
the purchase verbs ('execute:purchases', 'approve-any:purchases',
'retry-any:purchases') via includes/Set and return true if present; if
effectivePermissions is missing fall back to the existing
Array.isArray(user.groups) && user.groups.includes(PURCHASER_GROUP_ID) behavior;
keep function name isPurchaser() and ensure callers relying on canAccess() are
unchanged.
In `@frontend/src/recommendations.ts`:
- Around line 2907-2919: The banner's visibility is using isPurchaser() while
the Purchase CTA uses canAccess('execute','purchases'), causing mismatches;
update the banner condition to mirror the CTA by replacing the isPurchaser()
check with canAccess('execute', 'purchases') (negated) so the noPurchaseBanner
is only shown when !canAccess('execute', 'purchases'); keep the existing
noPurchaseBanner creation (className, role, textContent, append to box) and
remove the old isPurchaser() branch to ensure consistent behavior with the
`#bulk-purchase-btn` permission logic.
In `@internal/auth/types.go`:
- Around line 318-319: The GroupPurchaser constant currently uses "purchaser"
but the DB seed in 000058_seed_purchaser_group.up.sql inserts "Purchaser",
causing mismatches; update the GroupPurchaser constant in internal/auth/types.go
to exactly "Purchaser" to match the seeded group name (and run/adjust any tests
or comparisons that assume the old lowercase value). Ensure references to
GroupPurchaser throughout the codebase remain correct after the value change.
In `@internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql`:
- Around line 24-42: The INSERT currently uses a blanket "ON CONFLICT DO
NOTHING" which hides the case where another row already has the name "Purchaser"
but a different id, causing the seeded UUID
('00000000-0000-5000-8000-000000000005') to be missing and skipping the admin
backfill; change the migration to explicitly detect a name collision and fail
instead of silently skipping: implement a conditional/transactional check that
if a row exists with name = 'Purchaser' and id <>
'00000000-0000-5000-8000-000000000005' then RAISE EXCEPTION (or otherwise
abort), otherwise perform the INSERT with ON CONFLICT (id) DO NOTHING so only
the intended id conflict is tolerated; ensure this touches the same INSERT into
groups and preserves the admin backfill logic that relies on
DefaultPurchaserGroupID.
---
Outside diff comments:
In `@frontend/src/recommendations.ts`:
- Around line 2861-2863: mountBottomActionBox() currently returns early if the
DOM element 'recommendations-action-box' already exists, which prevents
re-syncing purchaseBtn.hidden and the "no-Purchaser" banner when membership
changes in-session; remove the early return (or explicitly call
updateBottomActionBox()) so that on subsequent renders you re-evaluate and set
purchase CTA visibility and banner state (inspect and update purchaseBtn.hidden,
the no-Purchaser banner element, and any cached snapshot logic inside
mountBottomActionBox/updateBottomActionBox) so membership changes are applied
without a full reload; apply the same fix pattern to the other duplicated blocks
referenced (the similar logic around lines ~2904-2919 and ~3018-3132).
---
Nitpick comments:
In `@frontend/src/__tests__/history-approve-button.test.ts`:
- Around line 68-77: Add a negative fixture for an admin without Purchaser to
cover the regression: declare a new test constant (e.g. ADMIN_NO_PURCH) with
id/email and groups containing only ADMINISTRATORS_GROUP_ID (no
PURCHASER_GROUP_ID), and update the test(s) that currently reference
ADMIN_USER/REG_USER to include a case that uses ADMIN_NO_PURCH (via setupDOM or
the existing test harness) asserting that this user does NOT receive
approve-any/purchases permissions; reference ADMIN_USER, ADMIN_NO_PURCH,
REG_USER, OTHER_UUID and setupDOM to locate where to add the new fixture and
test case.
In `@frontend/src/__tests__/history-retry-button.test.ts`:
- Around line 74-77: Add a negative test case for an admin user that does NOT
have the Purchaser group: define a new fixture (e.g., ADMIN_USER_NO_PURCHASER or
ADMIN_USER_WITHOUT_PURCHASER) using the same id/email but with groups:
[ADMINISTRATORS_GROUP_ID] only, and add assertions mirroring the existing
positive case that verify the retry-any:purchases permission is NOT present for
that user; update the test suite in history-retry-button.test.ts to exercise
this case so the behavior in history.ts (admin without Purchaser should lose
retry-any) is covered.
In `@frontend/src/__tests__/permissions.test.ts`:
- Around line 243-287: Add a new test that mirrors the "Purchaser explicit
grants" case but uses a non-seeded custom group id instead of PURCHASER_GROUP_ID
to ensure carved-out verbs work when provided by any group; use
mockUserWithGroups with a custom group id and an effectivePermissions array
containing execute/approve-any/retry-any:purchases (and the four
view:purchases/plans/recommendations/history as needed), then assert canAccess
allows those spending and view verbs and denies admin/*, delete:plans,
view:users, cancel-any:purchases; this will exercise the permission contract in
permissions.ts and the canAccess helper for non-PURCHASER_GROUP_ID sources.
In `@frontend/src/__tests__/recommendations-permissions.test.ts`:
- Around line 68-77: Current tests use mockUser('admin') which sets both
ADMINISTRATORS_GROUP_ID and PURCHASER_GROUP_ID, so they miss the admin-only UI
state; add an explicit test that simulates an admin without Purchaser by calling
mockUserWithGroups (or extend mockUser to accept a special flag) and set
state.getCurrentUser to return { id: 'u', email: 'u@example.com', groups:
[ADMINISTRATORS_GROUP_ID] }; assert Purchase button/section is hidden, Create
Plan remains visible, and the Purchaser notice is shown to cover the split-admin
UI contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f27348dc-e2f9-40f3-b981-ee0b187454da
📒 Files selected for processing (18)
cmd/gen-permissions/main.gofrontend/src/__tests__/history-approve-button.test.tsfrontend/src/__tests__/history-retry-button.test.tsfrontend/src/__tests__/permissions.test.tsfrontend/src/__tests__/recommendations-permissions.test.tsfrontend/src/api/types.tsfrontend/src/history.tsfrontend/src/permissions.generated.tsfrontend/src/permissions.tsfrontend/src/recommendations.tsfrontend/src/styles/components.cssfrontend/src/users/userActions.tsinternal/auth/store_postgres.gointernal/auth/store_postgres_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/database/postgres/migrations/000058_seed_purchaser_group.down.sqlinternal/database/postgres/migrations/000058_seed_purchaser_group.up.sql
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Address the five Major findings from CodeRabbit's pass on PR #924 so the frontend/backend authorization contract for the carved-out money-spending verbs (execute / approve-any / retry-any on purchases) stays consistent end-to-end. F1 (migration 000058): the seed used a bare `ON CONFLICT DO NOTHING` which treated "row with seeded UUID already exists" and "some other row already owns the name Purchaser" the same way. The latter case left DefaultPurchaserGroupID absent and silently broke the admin backfill. Detect a name-collision under a different UUID and RAISE EXCEPTION; narrow the conflict target to (id) so only the intended re-run is tolerated. F2 (internal/auth/types.go): `GroupPurchaser` constant was the lowercase string "purchaser" while the seeded row carries "Purchaser". Any name-based lookup would have missed the row. Align the constant with the seeded literal and document the contract in the comment. F3 (frontend/src/recommendations.ts): the no-Purchaser banner was predicated on `isPurchaser()` while the Purchase CTA used `canAccess('execute', 'purchases')`. Custom roles that hold execute:purchases via a non-seeded group saw both a live Purchase button AND the contradictory "you can view but not execute" notice. Drive the banner from the same predicate as the CTA. F4 (frontend/src/permissions.ts): `isPurchaser()` only checked PURCHASER_GROUP_ID membership, so the helper denied users who hold the carved-out verbs via custom groups. Extend it to consult effectivePermissions when present (true if any of the three carved-out verbs is granted), keeping the seeded-group fallback for the loading window so it agrees with canAccess()'s carve-out path. F5 (frontend/src/history.ts): canApprovePendingRow / canRetryFailedRow and the read-only banner all hard-coded isPurchaser(). A user granted approve-any:purchases or retry-any:purchases via another group was rejected by the UI even though the backend allowed the action. Gate each path on the exact canAccess(verb, 'purchases') the buttons need, and derive the banner from the same predicate so UI and enforcement stay in lockstep. Regression coverage: * permissions.test.ts: new isPurchaser() suite (seeded-group fallback, explicit-grant via execute / approve-any / retry-any in effectivePermissions, admin-without-explicit-carve-out denied, empty-effective-permissions denied, null-user denied) + custom-group carve-out coverage in the canAccess suite. * history-approve-button.test.ts: admin-without-Purchaser MUST NOT see Approve on rows they did not create (catches a future regression to isAdmin() / isPurchaser() group-only gates). * history-retry-button.test.ts: matching admin-without-Purchaser retry-any negative case. * recommendations-permissions.test.ts: admin-without-Purchaser hides Purchase, keeps Create Plan, shows the no-Purchaser banner; custom non-seeded group with explicit execute:purchases shows Purchase and no banner (F3 + F4 together).
cristim
force-pushed
the
feat/923-purchaser-group
branch
from
June 3, 2026 13:10
7a40be8 to
b108e58
Compare
Member
Author
Addressed CR pass-1 findings (5 Major + 4 nitpick regression tests)Rebased the branch onto the latest F1 — migration 000058 silent skip on name collision ( F2 — F3 — recommendations.ts banner predicate drift ( F4 — F5 — history.ts hard-coded Regression coverage (CR nitpicks 1-4)
Pre-fix stash-verifyStashed only Out-of-diff finding (mountBottomActionBox cache)CodeRabbit's "Re-sync purchase visibility after the box is already mounted" note ( Verification
@coderabbitai review |
Contributor
|
The stash-verify on F4 and the 79/0 jest result give good confidence in the regression coverage. Regarding the Kicking off an incremental review of commit 🧠 Learnings used✅ Action performedReview finished.
|
Member
Author
|
Followed up the out-of-diff finding (mountBottomActionBox re-sync gap) as #928 with full repro + acceptance criteria. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/__tests__/permissions.test.ts (1)
27-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUUID collision:
STD_GIDequalsPURCHASER_GROUP_ID.
STD_GIDis defined as'00000000-0000-5000-8000-000000000005', which is the same UUID as the Purchaser group. This causes tests usingSTD_GIDto inadvertently grant Purchaser membership during the fallback path (wheneffectivePermissionsis absent). Consider using a distinct UUID for the standard-user fixture (e.g.,'00000000-0000-5000-8000-000000000002'which is the actual Standard Users group from migration 000057).Proposed fix
const ADMIN_GID = ADMINISTRATORS_GROUP_ID; -const STD_GID = '00000000-0000-5000-8000-000000000005'; +const STD_GID = '00000000-0000-5000-8000-000000000002'; const RO_GID = '00000000-0000-5000-8000-000000000006';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/permissions.test.ts` around lines 27 - 28, STD_GID in the test fixtures is colliding with the real Purchaser group ID and causing accidental Purchaser membership when effectivePermissions is missing; change the STD_GID constant (used in permissions.test.ts) to a distinct UUID (for example '00000000-0000-5000-8000-000000000002' or another unique value) so it no longer equals PURCHASER_GROUP_ID, and update any tests that rely on STD_GID to use the new constant; ensure tests that exercise the fallback path (where effectivePermissions is absent) still assert expected non-Purchaser behavior after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/__tests__/permissions.test.ts`:
- Around line 27-28: STD_GID in the test fixtures is colliding with the real
Purchaser group ID and causing accidental Purchaser membership when
effectivePermissions is missing; change the STD_GID constant (used in
permissions.test.ts) to a distinct UUID (for example
'00000000-0000-5000-8000-000000000002' or another unique value) so it no longer
equals PURCHASER_GROUP_ID, and update any tests that rely on STD_GID to use the
new constant; ensure tests that exercise the fallback path (where
effectivePermissions is absent) still assert expected non-Purchaser behavior
after the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cf4c9db-df4a-4970-853c-7b2c6753eeb2
📒 Files selected for processing (18)
cmd/gen-permissions/main.gofrontend/src/__tests__/history-approve-button.test.tsfrontend/src/__tests__/history-retry-button.test.tsfrontend/src/__tests__/permissions.test.tsfrontend/src/__tests__/recommendations-permissions.test.tsfrontend/src/api/types.tsfrontend/src/history.tsfrontend/src/permissions.generated.tsfrontend/src/permissions.tsfrontend/src/recommendations.tsfrontend/src/styles/components.cssfrontend/src/users/userActions.tsinternal/auth/store_postgres.gointernal/auth/store_postgres_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/database/postgres/migrations/000058_seed_purchaser_group.down.sqlinternal/database/postgres/migrations/000058_seed_purchaser_group.up.sql
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/styles/components.css
- frontend/src/api/types.ts
- frontend/src/permissions.generated.ts
- internal/auth/store_postgres_test.go
- cmd/gen-permissions/main.go
- frontend/src/users/userActions.ts
- internal/auth/types_test.go
- internal/auth/store_postgres.go
- internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql
- internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql
- internal/auth/types.go
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Address the five Major findings from CodeRabbit's pass on PR #924 so the frontend/backend authorization contract for the carved-out money-spending verbs (execute / approve-any / retry-any on purchases) stays consistent end-to-end. F1 (migration 000058): the seed used a bare `ON CONFLICT DO NOTHING` which treated "row with seeded UUID already exists" and "some other row already owns the name Purchaser" the same way. The latter case left DefaultPurchaserGroupID absent and silently broke the admin backfill. Detect a name-collision under a different UUID and RAISE EXCEPTION; narrow the conflict target to (id) so only the intended re-run is tolerated. F2 (internal/auth/types.go): `GroupPurchaser` constant was the lowercase string "purchaser" while the seeded row carries "Purchaser". Any name-based lookup would have missed the row. Align the constant with the seeded literal and document the contract in the comment. F3 (frontend/src/recommendations.ts): the no-Purchaser banner was predicated on `isPurchaser()` while the Purchase CTA used `canAccess('execute', 'purchases')`. Custom roles that hold execute:purchases via a non-seeded group saw both a live Purchase button AND the contradictory "you can view but not execute" notice. Drive the banner from the same predicate as the CTA. F4 (frontend/src/permissions.ts): `isPurchaser()` only checked PURCHASER_GROUP_ID membership, so the helper denied users who hold the carved-out verbs via custom groups. Extend it to consult effectivePermissions when present (true if any of the three carved-out verbs is granted), keeping the seeded-group fallback for the loading window so it agrees with canAccess()'s carve-out path. F5 (frontend/src/history.ts): canApprovePendingRow / canRetryFailedRow and the read-only banner all hard-coded isPurchaser(). A user granted approve-any:purchases or retry-any:purchases via another group was rejected by the UI even though the backend allowed the action. Gate each path on the exact canAccess(verb, 'purchases') the buttons need, and derive the banner from the same predicate so UI and enforcement stay in lockstep. Regression coverage: * permissions.test.ts: new isPurchaser() suite (seeded-group fallback, explicit-grant via execute / approve-any / retry-any in effectivePermissions, admin-without-explicit-carve-out denied, empty-effective-permissions denied, null-user denied) + custom-group carve-out coverage in the canAccess suite. * history-approve-button.test.ts: admin-without-Purchaser MUST NOT see Approve on rows they did not create (catches a future regression to isAdmin() / isPurchaser() group-only gates). * history-retry-button.test.ts: matching admin-without-Purchaser retry-any negative case. * recommendations-permissions.test.ts: admin-without-Purchaser hides Purchase, keeps Create Plan, shows the no-Purchaser banner; custom non-seeded group with explicit execute:purchases shows Purchase and no banner (F3 + F4 together).
cristim
force-pushed
the
feat/923-purchaser-group
branch
from
June 3, 2026 14:31
b108e58 to
a4e1cb4
Compare
Member
Author
|
Rebased on
Addressed CR pass-2 findings:
CR pass-1 findings (all already addressed in earlier commits, confirmed stale):
Verification: @coderabbitai review |
Contributor
|
The STD_GID UUID fix ( Kicking off an incremental review of the post-rebase commits now. 🧠 Learnings used✅ Action performedReview finished.
|
… out of admin wildcard (closes #923) Implements separation of duties for money-spending operations. The three carved verbs (execute:purchases, approve-any:purchases, retry-any:purchases) are removed from the admin:* wildcard in HasPermission; a user must hold them via explicit group membership (the new Purchaser group or any custom group that grants them). Migration 000058 seeds the Purchaser group (UUID 000000000005, system_managed) and auto-assigns existing Administrators-group members so upgrades preserve current behavior. Adds system_managed column to the groups table. Frontend: canAccess respects the carve-out; isPurchaser() checks Purchaser group membership; info banners on Recommendations and History pages for sessions lacking execute:purchases; first-run dialog for admins not yet in the Purchaser group.
…hase execution The Purchaser-group carve-out (issue #923) removed execute:purchases, approve-any:purchases, and retry-any:purchases from the admin:* wildcard. Four tests were written before that contract existed and expected admin-alone membership to grant those verbs. - permissions.test.ts: split the "Administrators group member passes all checks" test into two -- admin-alone now asserts execute:purchases === false (regression guard for the carve-out), admin+Purchaser asserts all three carved-out verbs pass - recommendations-permissions.test.ts: mockUser('admin') now includes Purchaser group membership, matching the auto-migration path for existing admins - history-approve-button.test.ts: ADMIN_USER gains Purchaser membership so approve-any:purchases is granted (approve button visible on all pending rows) - history-retry-button.test.ts: ADMIN_USER gains Purchaser membership so retry-any:purchases is granted (retry button visible on all failed rows)
Address the five Major findings from CodeRabbit's pass on PR #924 so the frontend/backend authorization contract for the carved-out money-spending verbs (execute / approve-any / retry-any on purchases) stays consistent end-to-end. F1 (migration 000058): the seed used a bare `ON CONFLICT DO NOTHING` which treated "row with seeded UUID already exists" and "some other row already owns the name Purchaser" the same way. The latter case left DefaultPurchaserGroupID absent and silently broke the admin backfill. Detect a name-collision under a different UUID and RAISE EXCEPTION; narrow the conflict target to (id) so only the intended re-run is tolerated. F2 (internal/auth/types.go): `GroupPurchaser` constant was the lowercase string "purchaser" while the seeded row carries "Purchaser". Any name-based lookup would have missed the row. Align the constant with the seeded literal and document the contract in the comment. F3 (frontend/src/recommendations.ts): the no-Purchaser banner was predicated on `isPurchaser()` while the Purchase CTA used `canAccess('execute', 'purchases')`. Custom roles that hold execute:purchases via a non-seeded group saw both a live Purchase button AND the contradictory "you can view but not execute" notice. Drive the banner from the same predicate as the CTA. F4 (frontend/src/permissions.ts): `isPurchaser()` only checked PURCHASER_GROUP_ID membership, so the helper denied users who hold the carved-out verbs via custom groups. Extend it to consult effectivePermissions when present (true if any of the three carved-out verbs is granted), keeping the seeded-group fallback for the loading window so it agrees with canAccess()'s carve-out path. F5 (frontend/src/history.ts): canApprovePendingRow / canRetryFailedRow and the read-only banner all hard-coded isPurchaser(). A user granted approve-any:purchases or retry-any:purchases via another group was rejected by the UI even though the backend allowed the action. Gate each path on the exact canAccess(verb, 'purchases') the buttons need, and derive the banner from the same predicate so UI and enforcement stay in lockstep. Regression coverage: * permissions.test.ts: new isPurchaser() suite (seeded-group fallback, explicit-grant via execute / approve-any / retry-any in effectivePermissions, admin-without-explicit-carve-out denied, empty-effective-permissions denied, null-user denied) + custom-group carve-out coverage in the canAccess suite. * history-approve-button.test.ts: admin-without-Purchaser MUST NOT see Approve on rows they did not create (catches a future regression to isAdmin() / isPurchaser() group-only gates). * history-retry-button.test.ts: matching admin-without-Purchaser retry-any negative case. * recommendations-permissions.test.ts: admin-without-Purchaser hides Purchase, keeps Create Plan, shows the no-Purchaser banner; custom non-seeded group with explicit execute:purchases shows Purchase and no banner (F3 + F4 together).
cristim
force-pushed
the
feat/923-purchaser-group
branch
from
June 3, 2026 21:17
a4e1cb4 to
ba8f8d2
Compare
Member
Author
|
Rebased onto the latest Clean rebase (no content conflicts); it picks up #936's fix to Verified locally on the rebased head: Note: not re-pinging CodeRabbit this round because the org-level CR usage is currently rate-limited (billing); this PR was already CR-clean before the rebase and the rebase introduced no code changes to its own commits, so there is nothing new on the diff for CR to review. Will request a fresh CR pass once org CR credits are restored. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
…000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet.
Merged
5 tasks
cristim
added a commit
that referenced
this pull request
Jun 4, 2026
…ect gate (closes #940) (#941) * fix(api/purchases): drop removed session.Role shortcut in execute-direct gate (closes #940) PR #912 (#907) removed the Role field from api.Session, leaving authorizeSessionExecuteDirect with a dead `if session.Role == "admin"` reference that prevented the base branch from compiling. Remove the three-line shortcut and update the gate-logic doc comment to match the sibling authorizeSessionApprove/authorizeSessionCancel pattern: admins pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin,*} wildcard permission. Also fix all test Session struct literals that still set Role (now an unknown field), update the admin-role-shortcut test to properly stub the HasPermissionAPI path, and add three regression tests: - AdminGroupViaExecuteAny: confirms admin users still pass via execute-any - NoGrant: confirms non-admin without execute-any/own gets 403 - NilAuth: confirms nil auth component returns 500 (fail-closed) * fix(db): renumber duplicate migration 000058_seed_purchaser_group to 000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet.
Merged
7 tasks
This was referenced Jun 4, 2026
cristim
added a commit
that referenced
this pull request
Jun 4, 2026
…ple bootstrap from migration step (closes #945) (#947) * fix(api/purchases): drop removed session.Role shortcut in execute-direct gate (closes #940) PR #912 (#907) removed the Role field from api.Session, leaving authorizeSessionExecuteDirect with a dead `if session.Role == "admin"` reference that prevented the base branch from compiling. Remove the three-line shortcut and update the gate-logic doc comment to match the sibling authorizeSessionApprove/authorizeSessionCancel pattern: admins pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin,*} wildcard permission. Also fix all test Session struct literals that still set Role (now an unknown field), update the admin-role-shortcut test to properly stub the HasPermissionAPI path, and add three regression tests: - AdminGroupViaExecuteAny: confirms admin users still pass via execute-any - NoGrant: confirms non-admin without execute-any/own gets 403 - NilAuth: confirms nil auth component returns 500 (fail-closed) * fix(db): renumber duplicate migration 000058_seed_purchaser_group to 000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet. * fix(db/migrate): admin-upsert SQL parity with post-057 schema + decouple bootstrap from migration step (closes #945) Migration 000057 dropped the users.role column and sessions.role column (PR #912). The Go bootstrap functions in migrate.go were not updated at the time, leaving three SQL statements that reference the dropped column: - ensureAdminUser: INSERT INTO users included `role` in column list / VALUES - ensureAdminUserWithPassword: same INSERT pattern - assignAdminGroupAndWarn: two queries used WHERE role = 'admin' This caused every Lambda cold start to fail the admin-upsert step with "column role of relation users does not exist (SQLSTATE 42703)". The admin-upsert failure also masked a second deployment gap: migration 000058 (purchase_executions direct-execute audit columns) was missing from the deployed image, causing all purchase-execution reads/writes to fail with column-not-found errors. The image rebuild (ops action) will apply 000058 on the next cold start. Changes: - Remove `role` from INSERT column list and VALUES in both ensureAdminUser and ensureAdminUserWithPassword - Replace `WHERE role = 'admin'` in assignAdminGroupAndWarn with `WHERE (group_ids IS NULL OR cardinality(group_ids) = 0)`; post-057, the users_min_one_group CHECK makes this a permanent no-op so the backfill path is defence-in-depth for rollback scenarios only - Decouple admin-bootstrap failure from RunMigrations return value: if m.Up() succeeds but ensureAdminUser fails, RunMigrations now logs a WARNING and returns nil so the health endpoint correctly reports migration success and the app boots cleanly - Update ensure_admin_user_test.go: the pre-057 drift scenario (empty group_ids on an existing admin row) is structurally impossible post-057 due to the CHECK constraint; replace with an idempotency test that verifies re-running RunMigrations on an already-correct admin row is a no-op - Update migrate_security_integration_test.go: the same drift simulation (INSERT with role + empty group_ids after migrations) is impossible post-057; test now exercises the "Admin user created" log path instead * fix(db/migrate): address CR feedback on PR #947 - Scope assignAdminGroupAndWarn to the bootstrap admin email only; the previous UPDATE/COUNT targeted all users with empty group_ids, which would assign the Administrators group to non-admin users in pre-057 or rollback states (CR finding, major severity) - Remove dropped `role = 'admin'` predicate from test SQL in the operator-customisation sub-case; the column was dropped by migration 000057 so the WHERE clause would always return zero rows on the current schema (CR outside-diff finding) - Deferred: frontend 000058->000059 comment update is out of PR scope; filed as issue #955
5 tasks
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
… follow-up) (#972) PR #924's test-fix wave updated history-approve-button.test.ts, history-retry-button.test.ts, recommendations-permissions.test.ts, and permissions.test.ts, but missed three suites that also broke under the #923 carve-out contract: - execute-mode-toggle.test.ts: mockUser('admin') produced { role: 'admin' } with no groups array. The toggle check calls canAccess('execute-any', 'purchases') which falls back to isAdmin() (not a carved-out verb), and isAdmin() requires ADMINISTRATORS_GROUP_ID in groups. Without groups the toggle was never rendered. - purchase-execution-toast.test.ts: the jest.mock('../recommendations') factory was missing getExecuteMode and clearExecuteMode, both of which app.ts calls in handleExecutePurchase (added in the #289 / #924 wave). Every single-record test threw TypeError at line 328 of app.ts before reaching the toast assertions. - recommendations.test.ts: the default state mock used groups: ['...000000000001'] (Administrators only). After #923, the execute-mode toggle checks canAccess('execute-any', 'purchases') which calls isAdmin() -- not a carved-out verb -- so admin-group membership alone caused the toggle to render. The three tests that assert the approval-required note ('shows purchase summary', 'modal body carries the approval-required explanation', 'approval-required note renders with its dedicated class') saw the toggle instead of the note. Fixes applied: 1. execute-mode-toggle: import ADMINISTRATORS_GROUP_ID + PURCHASER_GROUP_ID from permissions; mockUser('admin') now produces groups: [ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID]. 2. purchase-execution-toast: add getExecuteMode (returns '') and clearExecuteMode to the recommendations mock factory. 3. recommendations: the three approval-required note tests override getCurrentUser to a non-admin user (groups: []) and restore the admin user after each test to prevent mock state from leaking into sibling describe blocks.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
00000000-0000-5000-8000-000000000005) that holds the three money-spending verbs:execute:purchases,approve-any:purchases,retry-any:purchases.admin:*wildcard inHasPermissionand on the frontendcanAccess. An admin who is not in the Purchaser group is denied these verbs.system_managedcolumn togroupstable, and auto-assigns all current Administrators-group members to the Purchaser group so upgrade preserves behavior.Carve-out rationale
cancel-anystays on admin (cleanup, no money out). The three carved verbs all spend money; requiring explicit group membership for each means a compromised admin account alone cannot drain commitments.Migration plan
system_managedcolumn; INSERT Purchaser group; UPDATE users to join all existing Administrators-group members.Test plan
TestAdminWildcardCarveOuts-- admin:* returns false for all 3 carved verbs, true for cancel-any and other admin verbsTestPurchaserGroupCoversExecutePurchases-- Purchaser group alone grants execute/approve-any/retry-anyTestAdminWithoutPurchaserCannotExecutePurchases-- admin-only context denied execute:purchasesTestAdminAndPurchaserCanExecutePurchases-- both groups together grants everythingTestDefaultPermissions/DefaultPurchaserPermissions contains carved verbs and view grantsgo build ./...cleango test ./internal/auth/...-- 513 passedgo test ./internal/api/...-- 1385 passednpx tsc --noEmit-- no errorsSummary by CodeRabbit
New Features
Chores
Style