[codex] Fix RBAC API key password policy gate#2080
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces public.check_min_rights (SECURITY DEFINER) to resolve effective org scope, load API-key identity, enforce org-level 2FA and password-policy gates, delegate to legacy checks when RBAC disabled, or call rbac_check_permission_direct for RBAC; updates function ownership/permissions and adds pgTAP tests for satisfied and stale RBAC-v2 API-key password-policy scenarios. ChangesAPI Key RBAC and Password Policy Authorization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
adamsardo
left a comment
There was a problem hiding this comment.
I reviewed the RBAC API-key/password-policy path. The implementation direction makes sense, but I think this needs one negative regression before merge.
The new check_min_rights branch intentionally skips its local 2FA/password-policy block for API-key-only calls (v_apikey IS NOT NULL AND user_id IS NULL) and relies on rbac_check_permission_direct to enforce the API-key owner's password policy later. The added pgTAP case only proves the happy path where user_password_compliance already contains the matching policy hash.
Could you add a denial assertion for the same RBAC v2 API key after removing/staling that compliance row? For example, with the same capgkey, delete the user_password_compliance row or change password_policy_config, then assert the app RLS query returns 0 rows and/or public.check_min_rights('read', NULL, org_id, app_id, NULL) returns false. That would lock down the actual security invariant this patch is trying to preserve, not just the allowed path.
|
Addressed the review by adding the negative RBAC v2 API-key/password-policy regression: after deleting the owner's user_password_compliance row, the same capgkey now asserts both the app RLS query returns 0 rows and check_min_rights returns false. CI is green on the latest commit. |
|



Summary (AI generated)
check_min_rightsso RBAC-managed API keys can be resolved before password-policy and 2FA gates run.appsRLS read using an RBAC v2 API key on an org with password policy enabled.Motivation (AI generated)
RBAC v2 API keys have
mode = NULL, soget_identity_org_appid()can returnNULLbeforerbac_check_permission_direct()resolves the key. That made password-policy enforcement reject valid CLI/API-key requests withCHECK_MIN_RIGHTS_PASSWORD_POLICY_ENFORCEMENT.Business Impact (AI generated)
This restores CLI/API-key access for organizations using RBAC v2 and password policies, avoiding false authorization failures for valid automation keys.
Test Plan (AI generated)
bun lintbun run cli:build && vue-tsc --noEmitbun scripts/supabase-worktree.ts test db supabase/tests/49_test_apikey_oracle_rpc_permissions.sqlsupabase/tests/17_test_prevent_admin_privilege_escalation.sqlduringSELECT my_tests()Summary by CodeRabbit
Bug Fixes
Tests
UI