test: split is_admin and is_platform_admin behavior#1777
Conversation
|
Warning Rate limit exceeded
⌛ 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 (5)
📝 WalkthroughWalkthroughReplaces backend RPC calls from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant App as App Server
participant Supabase as Supabase Client
participant DB as Database
Client->>App: request requiring admin/platform-admin check
App->>Supabase: rpc('is_platform_admin', { userid? })
Supabase->>DB: invoke is_platform_admin(userid)
DB->>DB: verify MFA, read admin_users from secrets, consult role_bindings
DB-->>Supabase: boolean (is_platform_admin) or error
Supabase-->>App: { data: boolean } or error
App-->>Client: allow or deny based on result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab5ff9c9b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| is_admin_legacy := COALESCE(admin_ids_jsonb ? userid::text, false); | ||
|
|
||
| RETURN is_admin_legacy; |
There was a problem hiding this comment.
Changing is_admin(userid) to return only the legacy vault lookup means RBAC-only platform_super_admin users now evaluate as non-admin, but the web guard still calls is_admin (src/services/supabase.ts → src/modules/auth.ts) to set main.isAdmin before allowing /admin routes. In practice, users who are valid platform admins via role bindings (and now pass backend checks that were switched to is_platform_admin) will be redirected away from admin pages, so this commit introduces a user-facing authorization regression for RBAC-managed admins.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/is-admin-functions.test.ts (1)
40-40: Considerit.concurrent()with per-test client for parallelism.The tests currently use
it()with a sharedclientacquired inbeforeEach. While this works correctly with transaction isolation, the shared client prevents parallel execution.To enable parallelism, each test could acquire its own client:
♻️ Suggested refactor for concurrent execution
- it('keeps is_admin legacy-only even when RBAC is enabled', async () => { + it.concurrent('keeps is_admin legacy-only even when RBAC is enabled', async () => { + const client = await pool.connect() + const query = (text: string, values: Array<string | boolean> = []) => client.query(text, values) + try { + await query('BEGIN') const legacyAdmin = randomUUID() // ... rest of test + await query('ROLLBACK') + } finally { + client.release() + } })Based on learnings: "Design all tests for parallel execution; use
it.concurrent()instead ofit()to maximize parallelism".Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/is-admin-functions.test.ts` at line 40, Refactor the test "keeps is_admin legacy-only even when RBAC is enabled" to use per-test clients and allow parallel execution by changing the spec to use it.concurrent() instead of it(), removing reliance on a shared client from beforeEach; inside the test acquire/initialize/cleanup its own client (mirroring the current setup logic) so tests like the one at line 75 can run concurrently without shared state, and ensure teardown/transaction rollback is handled within the test scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/admin_stats.ts`:
- Line 83: The TypeScript build is failing because the new Supabase RPC
is_platform_admin was added in a migration but the generated DB types weren't
updated; run the type generation command (bun types) to regenerate Supabase
Database types so supabaseClient.rpc('is_platform_admin') is recognized, then
commit the updated types; after regenerating, re-run the TypeScript build and
fix any remaining type errors related to is_platform_admin or other new RPCs.
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql`:
- Around line 80-89: The no-argument function public.is_admin() must be marked
SECURITY DEFINER like the other wrapper public.is_platform_admin() and the
underlying public.is_admin(uuid) to ensure it runs with the intended security
context; update the CREATE OR REPLACE FUNCTION public.is_admin() declaration to
include SECURITY DEFINER so the wrapper executes with the definer's privileges
while still delegating to public.is_admin((SELECT auth.uid())).
- Around line 52-91: Add OWNER and GRANT statements for both
public.is_admin(userid uuid) and public.is_admin() after their CREATE FUNCTION
blocks: set the function owner to the same role used by the is_platform_admin
functions and then GRANT EXECUTE ON FUNCTION public.is_admin(uuid) TO
authenticated, anon; and GRANT EXECUTE ON FUNCTION public.is_admin() TO
authenticated, anon; ensure these are applied to the exact function signatures
so RPC calls from authenticated and anon succeed while preserving the existing
SECURITY DEFINER behavior.
In `@supabase/tests/07_auth_functions.sql`:
- Around line 71-76: Conflict: SQL test expects is_platform_admin() to be false
while the TypeScript test expects true; update the TypeScript test to align with
the migration. Update the test in tests/is-admin-functions.test.ts to assert
that is_platform_admin() returns false for a user with a platform_super_admin
role binding (matching the SQL test expectation), and ensure any helper/mocking
logic that sets admin_users secret check remains unchanged so the TypeScript
assertion mirrors the SQL test's behavior.
---
Nitpick comments:
In `@tests/is-admin-functions.test.ts`:
- Line 40: Refactor the test "keeps is_admin legacy-only even when RBAC is
enabled" to use per-test clients and allow parallel execution by changing the
spec to use it.concurrent() instead of it(), removing reliance on a shared
client from beforeEach; inside the test acquire/initialize/cleanup its own
client (mirroring the current setup logic) so tests like the one at line 75 can
run concurrently without shared state, and ensure teardown/transaction rollback
is handled within the test scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 279e673c-91d7-4c8d-88cd-6a9cce291e5c
📒 Files selected for processing (8)
supabase/functions/_backend/private/admin_credits.tssupabase/functions/_backend/private/admin_stats.tssupabase/functions/_backend/private/log_as.tssupabase/functions/_backend/public/replication.tssupabase/migrations/20260311000000_split_is_admin_platform_admin.sqlsupabase/tests/07_auth_functions.sqlsupabase/tests/35_test_is_admin_rbac.sqltests/is-admin-functions.test.ts
| // Verify user is admin | ||
| const supabaseClient = useSupabaseClient(c, authToken) | ||
| const { data: isAdmin, error: adminError } = await supabaseClient.rpc('is_admin') | ||
| const { data: isAdmin, error: adminError } = await supabaseClient.rpc('is_platform_admin') |
There was a problem hiding this comment.
TypeScript types not regenerated after migration — build failure.
The is_platform_admin RPC function was added in the migration but the Supabase TypeScript types haven't been regenerated. This causes the TS2345 error across all files using this RPC.
Run bun types after the migration to regenerate the Database types, which will add is_platform_admin to the allowed RPC function union.
As per coding guidelines: "Run bun types after schema changes in migrations to regenerate TypeScript types for Supabase".
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 83-83: TS2345: Argument of type '"is_platform_admin"' is not assignable to parameter of type '"accept_invitation_to_org" | "apply_usage_overage" | "audit_logs_allowed_orgs" | "calculate_credit_cost" | "check_min_rights" | "check_min_rights_legacy" | "check_min_rights_legacy_no_password_policy" | ... 267 more | "verify_mfa"'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/functions/_backend/private/admin_stats.ts` at line 83, The
TypeScript build is failing because the new Supabase RPC is_platform_admin was
added in a migration but the generated DB types weren't updated; run the type
generation command (bun types) to regenerate Supabase Database types so
supabaseClient.rpc('is_platform_admin') is recognized, then commit the updated
types; after regenerating, re-run the TypeScript build and fix any remaining
type errors related to is_platform_admin or other new RPCs.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/modules/auth.ts (1)
10-10: Use the existing~/services/supabaseimport here.This adds a second path to the same frontend module and breaks the repo import convention. Fold
getPlansandisPlatformAdmininto the aliased import on Line 7 instead.Proposed fix
-import { getLocalConfig, useSupabase } from '~/services/supabase' +import { getLocalConfig, getPlans, isPlatformAdmin, useSupabase } from '~/services/supabase' import { sendEvent } from '~/services/tracking' import { useMainStore } from '~/stores/main' -import { getPlans, isPlatformAdmin } from './../services/supabase'As per coding guidelines, "Import using
~/alias forsrc/directory in frontend code instead of relative paths".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/auth.ts` at line 10, Replace the relative import of getPlans and isPlatformAdmin with the existing aliased import from '~/services/supabase' so all exports come from the same module path; locate where the current aliased import of '~/services/supabase' is used in this file and add getPlans and isPlatformAdmin to that import list (removing the separate "./../services/supabase" import) to conform to the project's import alias convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/auth.ts`:
- Around line 161-163: The async platform-admin check currently runs in
background (isPlatformAdmin(main.auth?.id).then(...)) so the auth guard proceeds
before resolution and can allow /admin to render; update both occurrences (the
isPlatformAdmin(...) usages around the current block and the similar block at
the later occurrence) to await the call inside the surrounding async guard
function, wrap it in try/catch, set main.isAdmin = await
isPlatformAdmin(main.auth?.id) on success, and on error set main.isAdmin = false
(and optionally log the error) so failures deny access cleanly instead of
breaking navigation.
In `@src/services/supabase.ts`:
- Around line 351-360: The wrapper is short-circuiting when userid is omitted;
update isPlatformAdmin to call the RPC without arguments instead of returning
false so the auth-bound SQL variant can run. Modify the logic in
isPlatformAdmin: call useSupabase().rpc('is_platform_admin') with no params when
userid is undefined and call useSupabase().rpc('is_platform_admin', { userid })
when provided, await the result, throw on error (throw new
Error(error.message)), and return the RPC result (e.g., data ?? false). Ensure
you still use .single() if the RPC returns a single scalar.
In `@tests/is-admin-functions.test.ts`:
- Around line 40-48: Rename the test titled "keeps is_admin legacy-only even
when RBAC is enabled" to accurately reflect the implemented scenario (e.g.,
"returns true for legacy admin when RBAC is disabled"), keep the DB update that
sets use_new_rbac = false as-is, and change the test declaration from it(...) to
it.concurrent(...) so it runs in parallel; update the test title string where
this spec uses legacyAdmin and nonAdmin identifiers.
- Around line 75-115: Replace the synchronous Jest test declaration with a
concurrent one: change the test call using it(...) to it.concurrent(...) for the
test that starts with "keeps is_platform_admin tied to admin_users (no RBAC role
check)" in tests/is-admin-functions.test.ts so that the block that inserts into
public.role_bindings and runs the queries (the async test function) runs in
parallel with other tests; ensure the test remains async and returns the same
assertions (no other logic changes).
---
Nitpick comments:
In `@src/modules/auth.ts`:
- Line 10: Replace the relative import of getPlans and isPlatformAdmin with the
existing aliased import from '~/services/supabase' so all exports come from the
same module path; locate where the current aliased import of
'~/services/supabase' is used in this file and add getPlans and isPlatformAdmin
to that import list (removing the separate "./../services/supabase" import) to
conform to the project's import alias convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 743330d7-a052-40ba-a8c0-65bd18ce7474
📒 Files selected for processing (6)
src/modules/auth.tssrc/services/supabase.tssrc/types/supabase.types.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260311000000_split_is_admin_platform_admin.sqltests/is-admin-functions.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/migrations/20260311000000_split_is_admin_platform_admin.sql
| export async function isPlatformAdmin(userid?: string): Promise<boolean> { | ||
| if (!userid) | ||
| return false | ||
| const { data, error } = await useSupabase() | ||
| .rpc('is_platform_admin', { userid }) | ||
| .single() | ||
| if (error) | ||
| throw new Error(error.message) | ||
|
|
||
| return data ?? false |
There was a problem hiding this comment.
Support the zero-arg RPC variant here.
is_platform_admin now exists with both () and ({ userid }) forms, but this wrapper hard-returns false when userid is omitted. That makes isPlatformAdmin() a false negative for authenticated platform admins instead of delegating to the auth-bound SQL wrapper.
Proposed fix
export async function isPlatformAdmin(userid?: string): Promise<boolean> {
- if (!userid)
- return false
- const { data, error } = await useSupabase()
- .rpc('is_platform_admin', { userid })
- .single()
+ const rpc = userid
+ ? useSupabase().rpc('is_platform_admin', { userid })
+ : useSupabase().rpc('is_platform_admin')
+ const { data, error } = await rpc.single()
if (error)
throw new Error(error.message)
return data ?? false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/supabase.ts` around lines 351 - 360, The wrapper is
short-circuiting when userid is omitted; update isPlatformAdmin to call the RPC
without arguments instead of returning false so the auth-bound SQL variant can
run. Modify the logic in isPlatformAdmin: call
useSupabase().rpc('is_platform_admin') with no params when userid is undefined
and call useSupabase().rpc('is_platform_admin', { userid }) when provided, await
the result, throw on error (throw new Error(error.message)), and return the RPC
result (e.g., data ?? false). Ensure you still use .single() if the RPC returns
a single scalar.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260311000000_split_is_admin_platform_admin.sql (1)
51-75:is_admin(userid uuid)andis_platform_admin(userid uuid)have identical implementations.Both functions perform the exact same logic: MFA verification followed by checking membership in the
admin_usersvault secret. While the PR aims to split these for future divergence, having identical implementations may cause confusion about when to use which function.Consider adding a brief code comment clarifying the intended future distinction, or confirming this duplication is intentional for the PR's scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql` around lines 51 - 75, The two functions is_admin(userid uuid) and is_platform_admin(userid uuid) are currently identical; add a brief explanatory comment above each function (or at the top of the file) clarifying that this duplication is intentional for now and describing the planned future divergence (e.g., that is_platform_admin will later check a different vault key or role), or alternatively consolidate into a shared helper if duplication is not intended; reference the function names is_admin and is_platform_admin so reviewers can find and verify the added comment or refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql`:
- Around line 43-47: Remove the inadvertent anon grant on the parameterized
function public.is_platform_admin(userid uuid): revoke or delete the GRANT ALL
... TO "anon" line so that only the no-argument wrapper function
public.is_platform_admin() (which performs auth checks) remains granted to
anon/authenticated as intended; ensure consistency with the is_admin(userid
uuid) treatment so no inner function that accepts a userid is exposed to the
anon role.
---
Nitpick comments:
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql`:
- Around line 51-75: The two functions is_admin(userid uuid) and
is_platform_admin(userid uuid) are currently identical; add a brief explanatory
comment above each function (or at the top of the file) clarifying that this
duplication is intentional for now and describing the planned future divergence
(e.g., that is_platform_admin will later check a different vault key or role),
or alternatively consolidate into a shared helper if duplication is not
intended; reference the function names is_admin and is_platform_admin so
reviewers can find and verify the added comment or refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13e6e21e-1283-4e2a-95d7-92261e6fed4a
📒 Files selected for processing (1)
supabase/migrations/20260311000000_split_is_admin_platform_admin.sql
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/supabase.ts`:
- Around line 351-355: The isPlatformAdmin helper should be a zero-argument
frontend RPC because useSupabase() is the anon-key client and passing a userid
attempts the service-role overload (causing privilege errors); remove the userid
parameter and the conditional branch in isPlatformAdmin so it always calls
useSupabase().rpc('is_platform_admin') with no params, update the function
signature to export async function isPlatformAdmin(): Promise<boolean>, and move
any UUID-based checks to a backend/service-role endpoint instead of calling this
helper from the browser.
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql`:
- Around line 43-46: The zero-arg wrapper functions public.is_platform_admin()
and public.is_admin() still have EXECUTE granted to PUBLIC by default; add
matching REVOKE ALL ON FUNCTION public.is_platform_admin() FROM PUBLIC; and
REVOKE ALL ON FUNCTION public.is_admin() FROM PUBLIC; immediately before the
corresponding GRANT statements (the places where you already revoke and grant
the uuid overloads) so the wrapper overloads are deny-by-default before granting
to "authenticated" or "service_role".
- Around line 13-24: The function is_platform_admin() still mirrors is_admin()
by gating on verify_mfa() and reading the legacy vault.decrypted_secrets
'admin_users' secret, so the new split never takes effect; update
is_platform_admin() to remove or bypass the MFA+secret lookup and instead
consult the RBAC source used by isPlatformAdmin (e.g., query the same
role/permission table or call the same helper that isPlatformAdmin uses),
leaving verify_mfa() and the 'admin_users' secret lookup in is_admin() only;
reference is_platform_admin, is_admin, isPlatformAdmin, verify_mfa(), and
vault.decrypted_secrets/admin_users when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7229ff45-3758-4882-b136-c35ae80e2276
📒 Files selected for processing (5)
AGENTS.mdsrc/modules/auth.tssrc/services/supabase.tssupabase/migrations/20260311000000_split_is_admin_platform_admin.sqltests/is-admin-functions.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/is-admin-functions.test.ts
- src/modules/auth.ts
| SELECT public.verify_mfa() INTO mfa_verified; | ||
| IF NOT mfa_verified THEN | ||
| RETURN false; | ||
| END IF; | ||
|
|
||
| SELECT decrypted_secret::jsonb INTO admin_ids_jsonb | ||
| FROM vault.decrypted_secrets | ||
| WHERE name = 'admin_users'; | ||
|
|
||
| is_platform_admin_from_secret := COALESCE(admin_ids_jsonb ? userid::text, false); | ||
|
|
||
| RETURN is_platform_admin_from_secret; |
There was a problem hiding this comment.
is_platform_admin() is still the same legacy check as is_admin().
These two bodies still do the same MFA gate and the same admin_users secret lookup, so the split this PR is introducing never actually happens. Since the auth flow now routes platform-admin checks through isPlatformAdmin, RBAC-only platform admins will still be rejected until is_platform_admin reads the RBAC source instead of the legacy secret.
Also applies to: 61-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260311000000_split_is_admin_platform_admin.sql` around
lines 13 - 24, The function is_platform_admin() still mirrors is_admin() by
gating on verify_mfa() and reading the legacy vault.decrypted_secrets
'admin_users' secret, so the new split never takes effect; update
is_platform_admin() to remove or bypass the MFA+secret lookup and instead
consult the RBAC source used by isPlatformAdmin (e.g., query the same
role/permission table or call the same helper that isPlatformAdmin uses),
leaving verify_mfa() and the 'admin_users' secret lookup in is_admin() only;
reference is_platform_admin, is_admin, isPlatformAdmin, verify_mfa(), and
vault.decrypted_secrets/admin_users when making the change.
|
1 participant
Summary (AI generated)
Motivation (AI generated)
Business Impact (AI generated)
Test Plan (AI generated)
Generated with AI
Summary by CodeRabbit
New Features
Tests
Documentation