Open
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces six new public PostgreSQL functions for retrieving organizational metrics with built-in authorization controls and caching. Functions compute per-app metrics, global aggregated metrics, and total metrics across specified date ranges, validating access permissions and leveraging cache mechanisms to optimize queries. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Auth as Authorization
participant DB as Database
participant Cache as Metrics Cache
participant Compute as Computation
Client->>Auth: Request metrics (org_id, date range)
alt Service Role
Auth-->>DB: Allow (bypass)
else Regular User
Auth->>DB: Check permissions & org
DB-->>Auth: Validate rights
alt Unauthorized
Auth-->>Client: Deny (log event)
else Authorized
Auth-->>DB: Proceed
end
end
DB->>Cache: Check cache validity
alt Cache Valid & Recent
Cache-->>Client: Return cached metrics
else Cache Stale or Missing
DB->>Compute: Seed metrics
Compute->>Cache: Store results
Cache-->>Client: Return metrics
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260226100000_fix_metrics_rpcs_org_authorization.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260226100000_fix_metrics_rpcs_org_authorization.sql (1)
1-3: Consider adding explicit GRANT statements to document access intent.The migration creates public functions without explicit
GRANTorREVOKEstatements. While PostgreSQL defaults grant EXECUTE to PUBLIC for new functions and the authorization is enforced internally, explicit grants improve clarity and make the security model auditable.📝 Example: Explicit grants at end of migration
-- Explicitly grant to authenticated and anon for RPC access GRANT EXECUTE ON FUNCTION public.get_app_metrics(uuid) TO authenticated, anon; GRANT EXECUTE ON FUNCTION public.get_app_metrics(uuid, date, date) TO authenticated, anon; GRANT EXECUTE ON FUNCTION public.get_global_metrics(uuid) TO authenticated, anon; GRANT EXECUTE ON FUNCTION public.get_global_metrics(uuid, date, date) TO authenticated, anon; GRANT EXECUTE ON FUNCTION public.get_total_metrics(uuid) TO authenticated, anon; GRANT EXECUTE ON FUNCTION public.get_total_metrics(uuid, date, date) TO authenticated, anon;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260226100000_fix_metrics_rpcs_org_authorization.sql` around lines 1 - 3, Add explicit GRANT/REVOKE statements for the created RPC functions to document and lock down access intent: for each function created (get_app_metrics(uuid), get_app_metrics(uuid, date, date), get_global_metrics(uuid), get_global_metrics(uuid, date, date), get_total_metrics(uuid), get_total_metrics(uuid, date, date)) add GRANT EXECUTE ON FUNCTION ... TO authenticated, anon (or the appropriate roles) at the end of the migration and consider REVOKE EXECUTE ON FUNCTION ... FROM PUBLIC if you want to remove the default public execute privilege so the authorization model is auditable and explicit.
🤖 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/20260226100000_fix_metrics_rpcs_org_authorization.sql`:
- Around line 173-208: Remove the redundant authorization block that duplicates
get_app_metrics' checks: delete the v_is_service_role/v_user_id check and the IF
NOT v_is_service_role ... END IF; block (including the calls to
public.get_identity, public.check_min_rights, public.pg_log and the early
RETURN) so this function simply calls public.get_app_metrics and returns its
aggregated results; keep the initial v_is_service_role assignment only if used
elsewhere, otherwise remove its declaration too.
---
Nitpick comments:
In `@supabase/migrations/20260226100000_fix_metrics_rpcs_org_authorization.sql`:
- Around line 1-3: Add explicit GRANT/REVOKE statements for the created RPC
functions to document and lock down access intent: for each function created
(get_app_metrics(uuid), get_app_metrics(uuid, date, date),
get_global_metrics(uuid), get_global_metrics(uuid, date, date),
get_total_metrics(uuid), get_total_metrics(uuid, date, date)) add GRANT EXECUTE
ON FUNCTION ... TO authenticated, anon (or the appropriate roles) at the end of
the migration and consider REVOKE EXECUTE ON FUNCTION ... FROM PUBLIC if you
want to remove the default public execute privilege so the authorization model
is auditable and explicit.
Comment on lines
+173
to
+208
| BEGIN | ||
| v_is_service_role := ((SELECT auth.jwt() ->> 'role') = 'service_role') OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres'); | ||
|
|
||
| IF NOT v_is_service_role THEN | ||
| v_user_id := public.get_identity('{read,upload,write,all}'::public.key_mode[]); | ||
|
|
||
| IF v_user_id IS NULL OR NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| v_user_id, | ||
| p_org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| PERFORM public.pg_log('deny: NO_RIGHTS', jsonb_build_object('org_id', p_org_id, 'uid', v_user_id, 'rpc', 'get_global_metrics')); | ||
| RETURN; | ||
| END IF; | ||
| END IF; | ||
|
|
||
| RETURN QUERY | ||
| SELECT | ||
| metrics.date, | ||
| SUM(metrics.mau)::bigint AS mau, | ||
| SUM(metrics.storage)::bigint AS storage, | ||
| SUM(metrics.bandwidth)::bigint AS bandwidth, | ||
| SUM(metrics.get)::bigint AS get, | ||
| SUM(metrics.fail)::bigint AS fail, | ||
| SUM(metrics.install)::bigint AS install, | ||
| SUM(metrics.uninstall)::bigint AS uninstall | ||
| FROM | ||
| public.get_app_metrics(p_org_id, p_start_date, p_end_date) AS metrics | ||
| GROUP BY | ||
| metrics.date | ||
| ORDER BY | ||
| metrics.date; | ||
| END; | ||
| $function$; |
Contributor
There was a problem hiding this comment.
Redundant authorization check causes double denial logging.
This function performs authorization at lines 176-189, then calls get_app_metrics at line 202 which performs the same authorization check again. When access is denied, pg_log is invoked twice for a single request, creating confusing audit trails.
Consider removing the authorization block here since get_app_metrics already enforces it, or mark this function SECURITY DEFINER and use an internal helper that skips auth.
♻️ Suggested fix: Remove redundant auth check
CREATE OR REPLACE FUNCTION public.get_global_metrics(
p_org_id uuid,
p_start_date date,
p_end_date date
) RETURNS TABLE (
date date,
mau bigint,
storage bigint,
bandwidth bigint,
get bigint,
fail bigint,
install bigint,
uninstall bigint
) LANGUAGE plpgsql
SET
search_path = '' AS $function$
-DECLARE
- v_user_id uuid;
- v_is_service_role boolean;
BEGIN
- v_is_service_role := ((SELECT auth.jwt() ->> 'role') = 'service_role') OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres');
-
- IF NOT v_is_service_role THEN
- v_user_id := public.get_identity('{read,upload,write,all}'::public.key_mode[]);
-
- IF v_user_id IS NULL OR NOT public.check_min_rights(
- 'read'::public.user_min_right,
- v_user_id,
- p_org_id,
- NULL::character varying,
- NULL::bigint
- ) THEN
- PERFORM public.pg_log('deny: NO_RIGHTS', jsonb_build_object('org_id', p_org_id, 'uid', v_user_id, 'rpc', 'get_global_metrics'));
- RETURN;
- END IF;
- END IF;
-
RETURN QUERY
SELECT
metrics.date,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BEGIN | |
| v_is_service_role := ((SELECT auth.jwt() ->> 'role') = 'service_role') OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres'); | |
| IF NOT v_is_service_role THEN | |
| v_user_id := public.get_identity('{read,upload,write,all}'::public.key_mode[]); | |
| IF v_user_id IS NULL OR NOT public.check_min_rights( | |
| 'read'::public.user_min_right, | |
| v_user_id, | |
| p_org_id, | |
| NULL::character varying, | |
| NULL::bigint | |
| ) THEN | |
| PERFORM public.pg_log('deny: NO_RIGHTS', jsonb_build_object('org_id', p_org_id, 'uid', v_user_id, 'rpc', 'get_global_metrics')); | |
| RETURN; | |
| END IF; | |
| END IF; | |
| RETURN QUERY | |
| SELECT | |
| metrics.date, | |
| SUM(metrics.mau)::bigint AS mau, | |
| SUM(metrics.storage)::bigint AS storage, | |
| SUM(metrics.bandwidth)::bigint AS bandwidth, | |
| SUM(metrics.get)::bigint AS get, | |
| SUM(metrics.fail)::bigint AS fail, | |
| SUM(metrics.install)::bigint AS install, | |
| SUM(metrics.uninstall)::bigint AS uninstall | |
| FROM | |
| public.get_app_metrics(p_org_id, p_start_date, p_end_date) AS metrics | |
| GROUP BY | |
| metrics.date | |
| ORDER BY | |
| metrics.date; | |
| END; | |
| $function$; | |
| BEGIN | |
| RETURN QUERY | |
| SELECT | |
| metrics.date, | |
| SUM(metrics.mau)::bigint AS mau, | |
| SUM(metrics.storage)::bigint AS storage, | |
| SUM(metrics.bandwidth)::bigint AS bandwidth, | |
| SUM(metrics.get)::bigint AS get, | |
| SUM(metrics.fail)::bigint AS fail, | |
| SUM(metrics.install)::bigint AS install, | |
| SUM(metrics.uninstall)::bigint AS uninstall | |
| FROM | |
| public.get_app_metrics(p_org_id, p_start_date, p_end_date) AS metrics | |
| GROUP BY | |
| metrics.date | |
| ORDER BY | |
| metrics.date; | |
| END; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260226100000_fix_metrics_rpcs_org_authorization.sql`
around lines 173 - 208, Remove the redundant authorization block that duplicates
get_app_metrics' checks: delete the v_is_service_role/v_user_id check and the IF
NOT v_is_service_role ... END IF; block (including the calls to
public.get_identity, public.check_min_rights, public.pg_log and the early
RETURN) so this function simply calls public.get_app_metrics and returns its
aggregated results; keep the initial v_is_service_role assignment only if used
elsewhere, otherwise remove its declaration too.
|
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 (AI generated)
supabase/migrations/20260226100000_fix_metrics_rpcs_org_authorization.sqlto enforce organization membership checks on metrics RPC endpoints and prevent anonymous cross-tenant usage telemetry disclosure.get_app_metrics,get_global_metrics, andget_total_metricsexecution paths so requests now require valid read rights for the requested org and log denied attempts.Test plan (AI generated)
bun lint:backendanonkey and verify unauthorized org IDs no longer return cross-tenant rows.Screenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Summary by CodeRabbit