feat(api,recs): execute-any/own permission gate on direct execute (closes #289)#803
Conversation
closes #289) Add two new RBAC verbs (execute-any:purchases, execute-own:purchases) that let privileged users bypass the approval email and execute purchases immediately from the dashboard. - auth/types.go: ActionExecuteOwn + ActionExecuteAny constants (not in DefaultUserPermissions -- opt-in only) - config/types.go: three nullable audit fields on PurchaseExecution (executed_by_user_id, executed_at, pre_approval_skip_reason) - migration 000057: ALTER TABLE purchase_executions ADD COLUMN for the three audit fields + partial index on direct-execute rows - store_postgres.go: extend INSERT, all SELECT RETURNING, and scanExecutionRows to carry the three new columns - handler_purchases.go: authorizeSessionExecuteDirect (fail-closed, nil-safe), directExecutePurchase (stamps audit fields, delegates to ApproveAndExecute), and execute_mode="direct" branch in executePurchase - handler_purchases_test.go: 4 new scenarios (no-permission 403, execute-any admin 200, execute-own owner 200, execute-own non-owner 403) - store_postgres_pgxmock_test.go: updated column/row fixtures for new columns
…ions
Show a radio-group toggle ("Send for Approval" / "Execute Now") in the
purchase modal when the session holds execute-any:purchases or
execute-own:purchases; plain approval note otherwise.
- permissions.ts: add 'execute-own' and 'execute-any' to Action union type
- api/purchases.ts: accept optional executeMode param, send execute_mode
in POST body
- api/types.ts: add direct_execute?: boolean to PurchaseResult
- recommendations.ts: currentExecuteMode state, getExecuteMode /
clearExecuteMode exports, conditional toggle rendering with warning
callout in openPurchaseModal
- app.ts: read getExecuteMode() in handleExecutePurchase, confirmation
dialog copy + toast vary by mode, clearExecuteMode on success
- __tests__/execute-mode-toggle.test.ts: 6 tests covering toggle
visibility by role, default state, radio-change state, and button
label updates
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
PR #802 (issue #742) also claimed migration 000057 -- the two PRs were dispatched in parallel and each independently verified "000057 is free". Whichever lands second would trip the pre-commit migration-collision hook. Per project memory `project_migration_number_collisions.md`, renumber the lower-priority PR via `git mv` to the next free slot. - 000057_purchase_executions_direct_execute_audit.up.sql -> 000058_* - 000057_purchase_executions_direct_execute_audit.down.sql -> 000058_* - internal/config/types.go: three "Migration 000057" comment refs -> "Migration 000058" Verified 000058 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration.
Three PRs were dispatched in parallel and each independently picked the same "next free" migration number 000057. PR #802 (universal-plans cleanup) took 000057, PR #803 (execute-permissions) renumbered to 000058 in a follow-up, and this PR (#804, revocation) now takes 000059. - 000057_purchase_history_revocation.up.sql -> 000059_* - 000057_purchase_history_revocation.down.sql -> 000059_* - internal/config/store_postgres.go: comment ref "migration 000057" -> "migration 000059" - migration file headers updated Verified 000059 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration. Per `project_migration_number_collisions.md`.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…se complexity on PR #803
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Three PRs were dispatched in parallel and each independently picked the same "next free" migration number 000057. PR #802 (universal-plans cleanup) took 000057, PR #803 (execute-permissions) renumbered to 000058 in a follow-up, and this PR (#804, revocation) now takes 000059. - 000057_purchase_history_revocation.up.sql -> 000059_* - 000057_purchase_history_revocation.down.sql -> 000059_* - internal/config/store_postgres.go: comment ref "migration 000057" -> "migration 000059" - migration file headers updated Verified 000059 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration. Per `project_migration_number_collisions.md`.
|
_pr-iterate: CR has not delivered a substantive review on this PR despite 5 PR otherwise merge-ready:
Independent permission-bypass audit (manual, in lieu of CR):
Re-invoke |
|
I'll kick off a full review pass now to give the PR a proper automated look. (╯°□°)╯ ᵒʰ ᶰᵒ... credits! → 🐇 ✅ back in action 🧠 Learnings used✅ Actions performedFull review triggered. |
…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.
…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.
Three PRs were dispatched in parallel and each independently picked the same "next free" migration number 000057. PR #802 (universal-plans cleanup) took 000057, PR #803 (execute-permissions) renumbered to 000058 in a follow-up, and this PR (#804, revocation) now takes 000059. - 000057_purchase_history_revocation.up.sql -> 000059_* - 000057_purchase_history_revocation.down.sql -> 000059_* - internal/config/store_postgres.go: comment ref "migration 000057" -> "migration 000059" - migration file headers updated Verified 000059 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration. Per `project_migration_number_collisions.md`.
Three PRs were dispatched in parallel and each independently picked the same "next free" migration number 000057. PR #802 (universal-plans cleanup) took 000057, PR #803 (execute-permissions) renumbered to 000058 in a follow-up, and this PR (#804, revocation) now takes 000059. - 000057_purchase_history_revocation.up.sql -> 000059_* - 000057_purchase_history_revocation.down.sql -> 000059_* - internal/config/store_postgres.go: comment ref "migration 000057" -> "migration 000059" - migration file headers updated Verified 000059 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration. Per `project_migration_number_collisions.md`.
…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
Three PRs were dispatched in parallel and each independently picked the same "next free" migration number 000057. PR #802 (universal-plans cleanup) took 000057, PR #803 (execute-permissions) renumbered to 000058 in a follow-up, and this PR (#804, revocation) now takes 000059. - 000057_purchase_history_revocation.up.sql -> 000059_* - 000057_purchase_history_revocation.down.sql -> 000059_* - internal/config/store_postgres.go: comment ref "migration 000057" -> "migration 000059" - migration file headers updated Verified 000059 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration. Per `project_migration_number_collisions.md`.
Three PRs were dispatched in parallel and each independently picked the same "next free" migration number 000057. PR #802 (universal-plans cleanup) took 000057, PR #803 (execute-permissions) renumbered to 000058 in a follow-up, and this PR (#804, revocation) now takes 000059. - 000057_purchase_history_revocation.up.sql -> 000059_* - 000057_purchase_history_revocation.down.sql -> 000059_* - internal/config/store_postgres.go: comment ref "migration 000057" -> "migration 000059" - migration file headers updated Verified 000059 is free on origin/feat/multicloud-web-frontend and no other open PR introduces a clashing migration. Per `project_migration_number_collisions.md`.
Summary
Implements #289: permission-gated direct purchase execute via two new RBAC verbs.
execute-any:purchases-- holder can directly execute any purchase, bypassing the approval email.execute-own:purchases-- holder can directly execute purchases they themselves submitted.DefaultUserPermissions; granting them is an explicit opt-in by a superadmin.Backend (commit
45f0fed)auth/types.go:ActionExecuteOwn+ActionExecuteAnyconstants.config/types.go: three nullable audit fields onPurchaseExecution(executed_by_user_id,executed_at,pre_approval_skip_reason).ALTER TABLE purchase_executions ADD COLUMN IF NOT EXISTSfor the three audit columns + partial indexidx_executions_direct_execute.store_postgres.go: INSERT, all SELECT RETURNING clauses, andscanExecutionRowsupdated to carry the new columns.handler_purchases.go:authorizeSessionExecuteDirect(fail-closed, nil-safe),directExecutePurchase(stamps audit fields, delegates toApproveAndExecute), andexecute_mode="direct"branch inexecutePurchase.Frontend (commit
d85d9e3)permissions.ts:'execute-own'and'execute-any'added to theActionunion.api/purchases.ts: optionalexecuteModeparam forwarded asexecute_modein POST body.api/types.ts:direct_execute?: booleanonPurchaseResult.recommendations.ts:currentExecuteModemodule state withgetExecuteMode/clearExecuteModeexports;openPurchaseModalconditionally renders the radio-group toggle + warning callout for privileged sessions, plain approval note otherwise.app.ts: reads execute mode inhandleExecutePurchase; confirmation copy, toast, and button label vary by mode.execute-mode-toggle.test.ts.Test plan
go test ./internal/api/ ./internal/auth/ ./internal/config/-- all pass (2407 tests).frontend: jest --testPathPattern='execute-mode-toggle'-- 6 pass.frontend: jest --testPathPattern='recommendations-permissions'-- 5 pass.go build ./...clean.npx tsc --noEmitclean.go run ./cmd/gen-permissions-- no diff topermissions.generated.ts(new verbs not inDefaultUserPermissions).execute_mode:"direct"appears in POST body only when toggle is on.execute_mode=directis posted.direct_execute: truein response and skips approval email.