fix(rbac): allow app insert returning#1774
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 (2)
📝 WalkthroughWalkthroughThis PR updates the SELECT RLS policy on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
supabase/migrations/20260310103000_fix_apps_select_rls_returning.sql (1)
36-47: OR condition has performance implications but is justified here.The coding guidelines recommend avoiding OR conditions that hurt query performance. However, this approach is reasonable given:
- Short-circuit evaluation means only one branch typically evaluates fully
- The alternative (two policies) would have the same OR semantics implicitly
- The fix is necessary for INSERT...RETURNING visibility
Consider documenting this tradeoff in the migration comment block for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260310103000_fix_apps_select_rls_returning.sql` around lines 36 - 47, Add a brief migration comment above the OR condition containing public.check_min_rights(...) / public.get_identity_org_appid(...) that explains the performance tradeoff: note that OR conditions can hurt query performance but are justified here because short-circuit evaluation limits work, splitting into two policies would preserve the same semantics, and this form is required to fix INSERT...RETURNING visibility; mention that this is intentional to aid future maintainers.supabase/tests/45_test_apps_insert_returning_rbac.sql (1)
29-50: Consider adding a test for app-only permission holders.This test validates org-level super_admin access (branch 1 of the new policy). Adding a companion test for a user with only app-scoped permissions would ensure branch 2 continues working correctly, providing full coverage of both OR branches.
Would you like me to draft an additional test case for a user with only app-level read permissions?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/45_test_apps_insert_returning_rbac.sql` around lines 29 - 50, Add a companion test alongside the existing SELECT lives_ok block that verifies the INSERT ... RETURNING path for a user/session that only has app-scoped permissions: duplicate the current INSERT INTO public.apps ... RETURNING app_id test but change the session context to the app-scoped principal (use a different current_setting key such as current_setting('test.apps_returning_rbac_app_holder')::uuid or the session setup used elsewhere for app-only tests), and update the assertion message to something like 'RBAC app-scoped permission holder can create an app with INSERT ... RETURNING' so branch 2 of the policy is covered; ensure the new test runs under a role that has only app-level permissions.
🤖 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/20260310103000_fix_apps_select_rls_returning.sql`:
- Around line 26-35: The org-scoped branch currently calls
get_identity_org_allowed which ignores limited_to_apps and lets RBAC org-level
keys read all apps; change the branch to use get_identity_org_appid so app
restrictions are enforced (i.e., call
get_identity_org_appid('{read,upload,write,all}'::public.key_mode[], owner_org,
app_id_or_self) and pass the relevant app_id context), and ensure
check_min_rights is invoked with a non-NULL app_id when the row has an app_id so
limited_to_apps is applied; if INSERT...RETURNING caused the original
self-reference problem, refactor the policy to resolve the self-reference (e.g.,
compute the app_id via a stable function or use NEW.app_id in a safe way) rather
than bypassing limited_to_apps.
---
Nitpick comments:
In `@supabase/migrations/20260310103000_fix_apps_select_rls_returning.sql`:
- Around line 36-47: Add a brief migration comment above the OR condition
containing public.check_min_rights(...) / public.get_identity_org_appid(...)
that explains the performance tradeoff: note that OR conditions can hurt query
performance but are justified here because short-circuit evaluation limits work,
splitting into two policies would preserve the same semantics, and this form is
required to fix INSERT...RETURNING visibility; mention that this is intentional
to aid future maintainers.
In `@supabase/tests/45_test_apps_insert_returning_rbac.sql`:
- Around line 29-50: Add a companion test alongside the existing SELECT lives_ok
block that verifies the INSERT ... RETURNING path for a user/session that only
has app-scoped permissions: duplicate the current INSERT INTO public.apps ...
RETURNING app_id test but change the session context to the app-scoped principal
(use a different current_setting key such as
current_setting('test.apps_returning_rbac_app_holder')::uuid or the session
setup used elsewhere for app-only tests), and update the assertion message to
something like 'RBAC app-scoped permission holder can create an app with INSERT
... RETURNING' so branch 2 of the policy is covered; ensure the new test runs
under a role that has only app-level permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23aedcf0-0e54-4208-9064-b868b1ac1818
📒 Files selected for processing (2)
supabase/migrations/20260310103000_fix_apps_select_rls_returning.sqlsupabase/tests/45_test_apps_insert_returning_rbac.sql
|
1 participant
Summary (AI generated)
public.appsRLS so RBAC users can create an app throughINSERT ... RETURNINGorg read OR app readMotivation (AI generated)
RBAC organizations could fail CLI onboarding when the app creation step used a direct insert with
RETURNING. The insert check passed, but the row could not be read back in the same statement snapshot, causing a generic RLS error.Business Impact (AI generated)
This restores CLI onboarding for RBAC customers who need to create their first app, reducing support load and avoiding drop-off during activation.
Test Plan (AI generated)
INSERT ... RETURNINGbunx supabase test db --local supabase/tests/00-supabase_test_helpers.sql supabase/tests/45_test_apps_insert_returning_rbac.sqlGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests