-
-
Notifications
You must be signed in to change notification settings - Fork 127
Split is_admin platform-admin check #1776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,7 +80,7 @@ app.post('/', middlewareAuth, async (c) => { | |
|
|
||
| // 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') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript build fails: Same issue as in 🧰 Tools🪛 GitHub Actions: Run tests[error] 83-83: vue-tsc: TS2345: Argument of type '"is_platform_admin"' is not assignable to parameter of type union of permissions. 🤖 Prompt for AI Agents |
||
|
|
||
| if (adminError) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'is_admin_error', error: adminError }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ app.post('/', middlewareAuth, async (c) => { | |
| const supabaseAdmin = await useSupabaseAdmin(c) | ||
| 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') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript build fails: Same issue as the other files — run 🧰 Tools🪛 GitHub Actions: Run tests[error] 30-30: vue-tsc: TS2345: Argument of type '"is_platform_admin"' is not assignable to parameter of type union of permissions. 🤖 Prompt for AI Agents |
||
| if (adminError) { | ||
| throw simpleError('is_admin_error', 'Is admin error', { adminError }) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,7 +175,7 @@ async function validateReplicationAccess(c: ReplicationContext) { | |
| }) | ||
|
|
||
| const userClient = supabaseClient(c, authorization) | ||
| const { data: isAdmin, error: adminError } = await userClient.rpc('is_admin') | ||
| const { data: isAdmin, error: adminError } = await userClient.rpc('is_platform_admin') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript build fails: Same issue as the other files — run 🧰 Tools🪛 GitHub Actions: Run tests[error] 178-178: vue-tsc: TS2345: Argument of type '"is_platform_admin"' is not assignable to parameter of type union of permissions. 🤖 Prompt for AI Agents |
||
| if (adminError) { | ||
| cloudlogErr({ requestId: c.get('requestId'), message: 'replication_is_admin_error', error: adminError }) | ||
| throw quickError(500, 'is_admin_error', 'Unable to verify admin rights') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| -- Split platform admin detection from is_admin | ||
| CREATE OR REPLACE FUNCTION public.is_platform_admin(userid uuid) | ||
| RETURNS boolean | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = '' | ||
| AS $$ | ||
| DECLARE | ||
| admin_ids_jsonb jsonb; | ||
| is_admin_legacy boolean := false; | ||
| mfa_verified boolean; | ||
| rbac_enabled boolean; | ||
| has_platform_admin boolean := false; | ||
| BEGIN | ||
| 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_admin_legacy := COALESCE(admin_ids_jsonb ? userid::text, false); | ||
|
|
||
| SELECT use_new_rbac INTO rbac_enabled | ||
| FROM public.rbac_settings | ||
| WHERE id = 1; | ||
|
|
||
| IF COALESCE(rbac_enabled, false) THEN | ||
| SELECT EXISTS ( | ||
| SELECT 1 | ||
| FROM public.role_bindings rb | ||
| JOIN public.roles r ON r.id = rb.role_id | ||
| WHERE rb.principal_type = public.rbac_principal_user() | ||
| AND rb.principal_id = userid | ||
| AND rb.scope_type = public.rbac_scope_platform() | ||
| AND r.name = public.rbac_role_platform_super_admin() | ||
| ) INTO has_platform_admin; | ||
| END IF; | ||
|
|
||
| RETURN is_admin_legacy OR has_platform_admin; | ||
| END; | ||
| $$; | ||
|
|
||
| ALTER FUNCTION public.is_platform_admin(userid uuid) OWNER TO "postgres"; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.is_platform_admin() | ||
| RETURNS boolean | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = '' | ||
| AS $$ | ||
| BEGIN | ||
| RETURN public.is_platform_admin((SELECT auth.uid())); | ||
| END; | ||
| $$; | ||
|
|
||
| ALTER FUNCTION public.is_platform_admin() OWNER TO "postgres"; | ||
|
|
||
| GRANT ALL ON FUNCTION public.is_platform_admin(userid uuid) TO "anon"; | ||
| GRANT ALL ON FUNCTION public.is_platform_admin(userid uuid) TO "authenticated"; | ||
| GRANT ALL ON FUNCTION public.is_platform_admin(userid uuid) TO "service_role"; | ||
| GRANT ALL ON FUNCTION public.is_platform_admin() TO "anon"; | ||
| GRANT ALL ON FUNCTION public.is_platform_admin() TO "authenticated"; | ||
| GRANT ALL ON FUNCTION public.is_platform_admin() TO "service_role"; | ||
|
|
||
| COMMENT ON FUNCTION public.is_platform_admin(uuid) IS 'Checks if a user is a platform admin. In RBAC mode, accepts legacy vault list or platform_super_admin role. In legacy mode, only accepts legacy vault list. Always requires MFA.'; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.is_admin(userid uuid) | ||
| RETURNS boolean | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = '' | ||
| AS $$ | ||
| DECLARE | ||
| admin_ids_jsonb jsonb; | ||
| is_admin_legacy boolean; | ||
| mfa_verified boolean; | ||
| BEGIN | ||
| 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_admin_legacy := COALESCE(admin_ids_jsonb ? userid::text, false); | ||
|
|
||
| RETURN is_admin_legacy; | ||
|
Comment on lines
+90
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This change makes Useful? React with 👍 / 👎. |
||
| END; | ||
| $$; | ||
|
|
||
| COMMENT ON FUNCTION public.is_admin(uuid) IS 'Checks if a user is listed in legacy platform admin secret. Always requires MFA.'; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.is_admin() | ||
| RETURNS boolean | ||
| LANGUAGE plpgsql | ||
| SET | ||
| search_path = '' | ||
| AS $$ | ||
| BEGIN | ||
| RETURN public.is_admin((SELECT auth.uid())); | ||
| END; | ||
| $$; | ||
|
|
||
| COMMENT ON FUNCTION public.is_admin() IS 'Legacy platform admin helper. Checks if the current user is listed in admin_users.'; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,116 @@ | ||||||||||||||||||||||
| import type { PoolClient } from 'pg' | ||||||||||||||||||||||
| import { randomUUID } from 'node:crypto' | ||||||||||||||||||||||
| import { Pool } from 'pg' | ||||||||||||||||||||||
| import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest' | ||||||||||||||||||||||
| import { POSTGRES_URL } from './test-utils.ts' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| describe('is_admin / is_platform_admin SQL functions', () => { | ||||||||||||||||||||||
| let pool: Pool | ||||||||||||||||||||||
| let client: PoolClient | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const query = (text: string, values: Array<string | boolean> = []) => { | ||||||||||||||||||||||
| return client.query(text, values) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| beforeAll(() => { | ||||||||||||||||||||||
| pool = new Pool({ connectionString: POSTGRES_URL }) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| beforeEach(async () => { | ||||||||||||||||||||||
| client = await pool.connect() | ||||||||||||||||||||||
| await query('BEGIN') | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| afterEach(async () => { | ||||||||||||||||||||||
| if (!client) | ||||||||||||||||||||||
| return | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| await query('ROLLBACK') | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| finally { | ||||||||||||||||||||||
| client.release() | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| afterAll(async () => { | ||||||||||||||||||||||
| await pool.end() | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| it('keeps is_admin legacy-only even when RBAC is enabled', async () => { | ||||||||||||||||||||||
| const legacyAdmin = randomUUID() | ||||||||||||||||||||||
| const nonAdmin = randomUUID() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| await query(` | ||||||||||||||||||||||
| UPDATE public.rbac_settings | ||||||||||||||||||||||
| SET use_new_rbac = false | ||||||||||||||||||||||
| WHERE id = 1; | ||||||||||||||||||||||
| `) | ||||||||||||||||||||||
|
Comment on lines
+44
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 46 disables the mode this test claims to cover. The title says "even when RBAC is enabled", but Line 46 sets Suggested fix- SET use_new_rbac = false
+ SET use_new_rbac = true📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| await query(` | ||||||||||||||||||||||
| UPDATE vault.decrypted_secrets | ||||||||||||||||||||||
| SET decrypted_secret = ( | ||||||||||||||||||||||
| COALESCE(decrypted_secret::jsonb, '{}'::jsonb) | ||||||||||||||||||||||
| || $1::jsonb | ||||||||||||||||||||||
| )::text | ||||||||||||||||||||||
| WHERE name = 'admin_users'; | ||||||||||||||||||||||
| `, [JSON.stringify({ [legacyAdmin]: true })]) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const legacy = await query( | ||||||||||||||||||||||
| 'SELECT public.is_admin($1::uuid) as is_admin, public.is_platform_admin($1::uuid) as is_platform_admin', | ||||||||||||||||||||||
| [legacyAdmin], | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const regular = await query( | ||||||||||||||||||||||
| 'SELECT public.is_admin($1::uuid) as is_admin, public.is_platform_admin($1::uuid) as is_platform_admin', | ||||||||||||||||||||||
| [nonAdmin], | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
Comment on lines
+59
to
+67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add coverage for the current-user/MFA path, not just the Both cases only call Based on learnings, "Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows". Also applies to: 101-109 🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(legacy.rows[0].is_admin).toBe(true) | ||||||||||||||||||||||
| expect(legacy.rows[0].is_platform_admin).toBe(true) | ||||||||||||||||||||||
| expect(regular.rows[0].is_admin).toBe(false) | ||||||||||||||||||||||
| expect(regular.rows[0].is_platform_admin).toBe(false) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| it('moves RBAC platform super-admin checks into is_platform_admin only', async () => { | ||||||||||||||||||||||
| const rbacUserId = randomUUID() | ||||||||||||||||||||||
| const normalUserId = randomUUID() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| await query(` | ||||||||||||||||||||||
| UPDATE public.rbac_settings | ||||||||||||||||||||||
| SET use_new_rbac = true | ||||||||||||||||||||||
| WHERE id = 1; | ||||||||||||||||||||||
| `) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| await query(` | ||||||||||||||||||||||
| INSERT INTO public.role_bindings ( | ||||||||||||||||||||||
| principal_type, | ||||||||||||||||||||||
| principal_id, | ||||||||||||||||||||||
| role_id, | ||||||||||||||||||||||
| scope_type, | ||||||||||||||||||||||
| granted_by | ||||||||||||||||||||||
| ) VALUES ( | ||||||||||||||||||||||
| public.rbac_principal_user(), | ||||||||||||||||||||||
| $1::uuid, | ||||||||||||||||||||||
| (SELECT id FROM public.roles WHERE name = public.rbac_role_platform_super_admin()), | ||||||||||||||||||||||
| public.rbac_scope_platform(), | ||||||||||||||||||||||
| $1::uuid | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| `, [rbacUserId]) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const rbacUserResults = await query( | ||||||||||||||||||||||
| 'SELECT public.is_admin($1::uuid) as is_admin, public.is_platform_admin($1::uuid) as is_platform_admin', | ||||||||||||||||||||||
| [rbacUserId], | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const regularUserResults = await query( | ||||||||||||||||||||||
| 'SELECT public.is_admin($1::uuid) as is_admin, public.is_platform_admin($1::uuid) as is_platform_admin', | ||||||||||||||||||||||
| [normalUserId], | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| expect(rbacUserResults.rows[0].is_admin).toBe(false) | ||||||||||||||||||||||
| expect(rbacUserResults.rows[0].is_platform_admin).toBe(true) | ||||||||||||||||||||||
| expect(regularUserResults.rows[0].is_admin).toBe(false) | ||||||||||||||||||||||
| expect(regularUserResults.rows[0].is_platform_admin).toBe(false) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript build fails:
is_platform_adminnot in generated types.The pipeline failure indicates the Supabase TypeScript types haven't been regenerated after adding the new SQL function. The RPC name
"is_platform_admin"is not recognized in the type union.Run
bun typesafter the migration to regenerate the TypeScript types, as per the coding guidelines for schema changes.🧰 Tools
🪛 GitHub Actions: Run tests
[error] 41-41: vue-tsc: TS2345: Argument of type '"is_platform_admin"' is not assignable to parameter of type union of permissions.
🤖 Prompt for AI Agents