Closed
Conversation
New organizations now use the RBAC permission system (use_new_rbac = true) by default instead of the legacy org_users.user_right system. Existing organizations are unaffected. Tests are updated to explicitly mark legacy orgs with use_new_rbac = false and add equivalent RBAC coverage. - Migration: ALTER TABLE orgs ALTER COLUMN use_new_rbac SET DEFAULT true - organization-api.test.ts: existing org pinned to legacy; new RBAC describe block covers GET/PUT org, GET/POST/DELETE members with role_bindings verification - password-policy.test.ts: legacy org pinned; RBAC variant added - private-error-cases.test.ts: test org pinned to legacy (error ordering differs between RBAC and legacy for non-existent apps) - SQL tests 37 and 40: org INSERTs now explicit use_new_rbac = false - seed.sql: seed orgs pinned to legacy - Fix rbac_check_permission_direct: use v_effective_user_id in RBAC path (was using p_user_id which is NULL for API key auth) - Fix invite_user_to_org_rbac: create role_binding directly after insert Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- fix_rbac_check_effective_user.sql: replace raw `WHERE key = p_apikey` with `find_apikey_by_value(p_apikey)` in the apikey principal lookup so hashed keys are correctly resolved when checking RBAC permissions (regression from 20260228000300_fix_apikey_hashed_lookup.sql) - password-policy.test.ts: assert insert errors for stripe_info, orgs, and org_users in the RBAC password-policy test so setup failures surface immediately rather than silently producing a false-positive result Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR refactors build endpoint authorization to validate app_id after fetching the build request, implements new RBAC permission check functions in the database, enables RBAC by default for new organizations, and adds comprehensive test coverage for both legacy and RBAC authentication paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Build Endpoint
participant DB as Database
participant RBAC as RBAC Engine
participant Builder as External Builder
Client->>API: POST /build/cancel/:jobId
API->>DB: Fetch build_request by builder_job_id
DB-->>API: Return buildRequest
alt buildRequest.app_id mismatch
API-->>Client: 401 Unauthorized (app_id mismatch)
else app_id matches
API->>RBAC: Check permission on buildRequest.app_id
alt Permission denied
RBAC-->>API: Permission denied
API-->>Client: 401 Unauthorized
else Permission granted
RBAC-->>API: Permission granted
API->>DB: Update build_requests status using buildRequest.app_id
DB-->>API: Update confirmed
API->>Builder: POST /cancel/:jobId
Builder-->>API: Cancel result
API-->>Client: Return result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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/20260302185011_fix_rbac_check_effective_user.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: supabase/migrations/20260302000000_rbac_default_for_new_orgs.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 |
![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: 8d38db8fce
ℹ️ 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".
Comment on lines
102
to
106
| const { data: buildRequest, error: buildRequestError } = await supabase | ||
| .from('build_requests') | ||
| .select('app_id') | ||
| .eq('builder_job_id', jobId) | ||
| .eq('app_id', appId) | ||
| .maybeSingle() |
There was a problem hiding this comment.
This lookup now uses maybeSingle() on builder_job_id alone, so if build_requests contains two rows with the same job ID (allowed today because there is no unique constraint on that column), Supabase returns a multi-row error and /build/start fails with internal_error before authorization logic can run; the same pattern was introduced in /build/cancel. This turns a recoverable duplicate-data condition into a hard failure for legitimate users, so the query should either include app_id disambiguation or handle multiple matches explicitly before deciding authorization.
Useful? React with 👍 / 👎.
feat: make RBAC the default for newly created organizations
…/capgo into riderx/fix-attached-issue
|
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/password-policy.test.ts (1)
514-590:⚠️ Potential issue | 🟠 MajorSwitch the new tests to
it.concurrent()to match test-runner policy.The newly added tests use
it(...)inside a.test.tsfile.As per coding guidelines: `ALL test files run in parallel; use it.concurrent() instead of it() to run tests within the same file in parallel for faster CI/CD`.⚡ Proposed fix
- it('check_min_rights respects password policy (legacy mode)', async () => { + it.concurrent('check_min_rights respects password policy (legacy mode)', async () => { - it('check_min_rights respects password policy (RBAC mode)', async () => { + it.concurrent('check_min_rights respects password policy (RBAC mode)', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/password-policy.test.ts` around lines 514 - 590, Two new tests ("check_min_rights respects password policy (legacy mode)" and "check_min_rights respects password policy (RBAC mode)") use it(...) but must run concurrently per test-runner policy; change both invocations from it(...) to it.concurrent(...). Locate the two test cases by their exact titles and replace the test declaration calls so they run in parallel (no other code changes needed).supabase/seed.sql (1)
868-872:⚠️ Potential issue | 🟡 MinorKeep
use_new_rbacdeterministic in the upsert branch.When
public.reset_and_seed_app_datahits an existing org,use_new_rbacis not updated, so mode can drift between runs.🔧 Proposed fix
ON CONFLICT (id) DO UPDATE SET customer_id = EXCLUDED.customer_id, management_email = EXCLUDED.management_email, name = EXCLUDED.name, + use_new_rbac = EXCLUDED.use_new_rbac, updated_at = NOW();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/seed.sql` around lines 868 - 872, The upsert conflict branch fails to update the use_new_rbac column, allowing its value to drift; modify the ON CONFLICT ... DO UPDATE SET clause (the upsert for the org row used by public.reset_and_seed_app_data) to explicitly set use_new_rbac = EXCLUDED.use_new_rbac (or the intended deterministic expression) alongside customer_id, management_email, name, and updated_at so the RBAC mode is always updated on upsert.
🧹 Nitpick comments (2)
supabase/functions/_backend/public/build/start.ts (1)
130-157: Consider extracting this auth guard into a shared helper withcancelBuild.The “resolve build_request → enforce app match → app-scoped
checkPermission” sequence is duplicated in both public build actions. A shared helper would reduce future security drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/build/start.ts` around lines 130 - 157, The auth checks for start builds duplicate the same "resolve build_request → enforce app match → app-scoped checkPermission" logic found in cancelBuild; extract that sequence into a shared helper (e.g., authorizeBuildAction or ensureBuildPermission) which takes the context c, buildRequest (or jobId/appId) and required permission string, performs the app_id match check (using buildRequest.app_id vs appId), calls checkPermission(c, 'app.build_native', { appId: buildRequest.app_id }), logs via cloudlogErr with the same fields (requestId, job_id, app_id/requested_app_id, user_id) and throws simpleError on failure, and replace the duplicated blocks in start.ts and the cancelBuild implementation with calls to this helper, returning the resolved boundAppId for downstream use.tests/build-job-scope.test.ts (1)
191-209: ESLint: Test title should begin with lowercase.The linter prefers test descriptions to start with a lowercase letter. Consider rephrasing or suppressing if HTTP method prefixes are a project convention.
🔧 Optional fix
- it.concurrent('POST /build/start/:jobId denies cross-app jobId with allowed app_id', async () => { + it.concurrent('denies cross-app jobId with allowed app_id on POST /build/start/:jobId', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/build-job-scope.test.ts` around lines 191 - 209, Rename the test title string used in the it.concurrent call so it begins with a lowercase letter (e.g., change "POST /build/start/:jobId denies cross-app jobId with allowed app_id" to "post /build/start/:jobId denies cross-app jobId with allowed app_id") to satisfy the ESLint rule; locate the it.concurrent invocation and update its first argument (the test description) accordingly (or if you prefer to keep the exact casing, add an inline ESLint suppression immediately above the it.concurrent statement).
🤖 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/20260302185011_fix_rbac_check_effective_user.sql`:
- Around line 261-272: Replace the vulnerable SELECT-then-INSERT logic with an
atomic upsert so concurrent invites won't conflict: perform INSERT INTO
public.org_users (user_id, org_id, user_right, rbac_role_name) VALUES
(invited_user.id, invite_user_to_org_rbac.org_id, invite_right,
invite_user_to_org_rbac.role_name) ON CONFLICT (user_id, org_id) DO NOTHING
RETURNING id INTO current_record (or use GET DIAGNOSTICS row_count to detect if
an insert happened); then if current_record IS NOT NULL (or row_count = 0)
return 'ALREADY_INVITED' else continue — reference the variables invited_user,
invite_user_to_org_rbac, current_record and the public.org_users table when
making the change.
In `@tests/build-job-scope.test.ts`:
- Around line 22-35: fetchBuildRequestState calls supabase.from(...).eq('id',
buildRequestId) but buildRequestId is typed string | null; add a guard at the
start of fetchBuildRequestState to check buildRequestId !== null (or throw a
descriptive error) before calling getSupabaseClient() and .eq, so .eq always
receives a string; reference fetchBuildRequestState and buildRequestId and
ensure the type-checker is satisfied (early return or throw) rather than using a
non-null assertion.
In `@tests/organization-api.test.ts`:
- Line 739: The describe title string in the test suite "RBAC mode -
organization member operations" violates the test/prefer-lowercase-title rule;
update the describe call (the describe(...) invocation that begins with the
title "RBAC mode - organization member operations") to use a lowercase title
(e.g., "rbac mode - organization member operations") so the title is fully
lowercase and satisfies ESLint.
- Around line 739-876: The tests use shared mutable fixtures (ORG_ID_RBAC,
beforeAll/afterAll) and plain it() which can race when tests run in parallel;
change each it(...) to it.concurrent(...) and refactor setup so each test
creates and cleans a dedicated org and related records (replace global
ORG_ID_RBAC usage with a per-test unique org id/name generated inside the test
or a per-test helper), move the org/stripe/org_users/role_bindings insertions
and deletions into per-test setup/teardown (beforeEach/afterEach or inline in
the test) and ensure cleanup removes role_bindings/org_users/orgs/stripe_info
created for that test (references: beforeAll, afterAll, ORG_ID_RBAC,
USER_ADMIN_EMAIL, the organization/members test cases and the DB queries to
role_bindings/org_users/stripe_info).
---
Outside diff comments:
In `@supabase/seed.sql`:
- Around line 868-872: The upsert conflict branch fails to update the
use_new_rbac column, allowing its value to drift; modify the ON CONFLICT ... DO
UPDATE SET clause (the upsert for the org row used by
public.reset_and_seed_app_data) to explicitly set use_new_rbac =
EXCLUDED.use_new_rbac (or the intended deterministic expression) alongside
customer_id, management_email, name, and updated_at so the RBAC mode is always
updated on upsert.
In `@tests/password-policy.test.ts`:
- Around line 514-590: Two new tests ("check_min_rights respects password policy
(legacy mode)" and "check_min_rights respects password policy (RBAC mode)") use
it(...) but must run concurrently per test-runner policy; change both
invocations from it(...) to it.concurrent(...). Locate the two test cases by
their exact titles and replace the test declaration calls so they run in
parallel (no other code changes needed).
---
Nitpick comments:
In `@supabase/functions/_backend/public/build/start.ts`:
- Around line 130-157: The auth checks for start builds duplicate the same
"resolve build_request → enforce app match → app-scoped checkPermission" logic
found in cancelBuild; extract that sequence into a shared helper (e.g.,
authorizeBuildAction or ensureBuildPermission) which takes the context c,
buildRequest (or jobId/appId) and required permission string, performs the
app_id match check (using buildRequest.app_id vs appId), calls
checkPermission(c, 'app.build_native', { appId: buildRequest.app_id }), logs via
cloudlogErr with the same fields (requestId, job_id, app_id/requested_app_id,
user_id) and throws simpleError on failure, and replace the duplicated blocks in
start.ts and the cancelBuild implementation with calls to this helper, returning
the resolved boundAppId for downstream use.
In `@tests/build-job-scope.test.ts`:
- Around line 191-209: Rename the test title string used in the it.concurrent
call so it begins with a lowercase letter (e.g., change "POST
/build/start/:jobId denies cross-app jobId with allowed app_id" to "post
/build/start/:jobId denies cross-app jobId with allowed app_id") to satisfy the
ESLint rule; locate the it.concurrent invocation and update its first argument
(the test description) accordingly (or if you prefer to keep the exact casing,
add an inline ESLint suppression immediately above the it.concurrent statement).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
supabase/functions/_backend/public/build/cancel.tssupabase/functions/_backend/public/build/start.tssupabase/migrations/20260302000000_rbac_default_for_new_orgs.sqlsupabase/migrations/20260302185011_fix_rbac_check_effective_user.sqlsupabase/schemas/prod.sqlsupabase/seed.sqlsupabase/tests/37_test_check_min_rights_2fa_enforcement.sqlsupabase/tests/40_test_password_policy_enforcement.sqltests/build-job-scope.test.tstests/organization-api.test.tstests/password-policy.test.tstests/private-error-cases.test.ts
Comment on lines
+261
to
+272
| IF invited_user IS NOT NULL THEN | ||
| SELECT public.org_users.id INTO current_record | ||
| FROM public.org_users | ||
| WHERE public.org_users.user_id = invited_user.id | ||
| AND public.org_users.org_id = invite_user_to_org_rbac.org_id; | ||
|
|
||
| IF current_record IS NOT NULL THEN | ||
| RETURN 'ALREADY_INVITED'; | ||
| ELSE | ||
| INSERT INTO public.org_users (user_id, org_id, user_right, rbac_role_name) | ||
| VALUES (invited_user.id, invite_user_to_org_rbac.org_id, invite_right, invite_user_to_org_rbac.role_name); | ||
|
|
Contributor
There was a problem hiding this comment.
Make member invitation insert idempotent to avoid race failures.
This SELECT-then-INSERT sequence is vulnerable to concurrent invites for the same user/org, which can throw errors or create duplicate state depending on constraints.
🔒 Proposed fix
DECLARE
org record;
invited_user record;
current_record record;
current_tmp_user record;
role_id uuid;
legacy_right public.user_min_right;
invite_right public.user_min_right;
api_key_text text;
v_granted_by uuid;
+ v_rows_affected integer;
BEGIN
@@
- INSERT INTO public.org_users (user_id, org_id, user_right, rbac_role_name)
- VALUES (invited_user.id, invite_user_to_org_rbac.org_id, invite_right, invite_user_to_org_rbac.role_name);
+ INSERT INTO public.org_users (user_id, org_id, user_right, rbac_role_name)
+ VALUES (invited_user.id, invite_user_to_org_rbac.org_id, invite_right, invite_user_to_org_rbac.role_name)
+ ON CONFLICT DO NOTHING;
+
+ GET DIAGNOSTICS v_rows_affected = ROW_COUNT;
+ IF v_rows_affected = 0 THEN
+ RETURN 'ALREADY_INVITED';
+ END IF;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260302185011_fix_rbac_check_effective_user.sql` around
lines 261 - 272, Replace the vulnerable SELECT-then-INSERT logic with an atomic
upsert so concurrent invites won't conflict: perform INSERT INTO
public.org_users (user_id, org_id, user_right, rbac_role_name) VALUES
(invited_user.id, invite_user_to_org_rbac.org_id, invite_right,
invite_user_to_org_rbac.role_name) ON CONFLICT (user_id, org_id) DO NOTHING
RETURNING id INTO current_record (or use GET DIAGNOSTICS row_count to detect if
an insert happened); then if current_record IS NOT NULL (or row_count = 0)
return 'ALREADY_INVITED' else continue — reference the variables invited_user,
invite_user_to_org_rbac, current_record and the public.org_users table when
making the change.
Comment on lines
+22
to
+35
| const fetchBuildRequestState = async () => { | ||
| const supabase = getSupabaseClient() | ||
| const { data, error } = await supabase | ||
| .from('build_requests') | ||
| .select('status, app_id, last_error') | ||
| .eq('id', buildRequestId) | ||
| .single() | ||
|
|
||
| if (error) { | ||
| throw error | ||
| } | ||
|
|
||
| return data | ||
| } |
Contributor
There was a problem hiding this comment.
Fix TypeScript error: buildRequestId may be null.
The pipeline fails because buildRequestId is typed as string | null but .eq() expects a string. Add a guard to satisfy the type checker.
🔧 Proposed fix
const fetchBuildRequestState = async () => {
+ if (!buildRequestId) {
+ throw new Error('buildRequestId not initialized')
+ }
const supabase = getSupabaseClient()
const { data, error } = await supabase
.from('build_requests')
.select('status, app_id, last_error')
.eq('id', buildRequestId)
.single()🧰 Tools
🪛 GitHub Actions: Run tests
[error] 27-27: TS2345: Argument of type 'string | null' is not assignable to parameter of type 'string'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/build-job-scope.test.ts` around lines 22 - 35, fetchBuildRequestState
calls supabase.from(...).eq('id', buildRequestId) but buildRequestId is typed
string | null; add a guard at the start of fetchBuildRequestState to check
buildRequestId !== null (or throw a descriptive error) before calling
getSupabaseClient() and .eq, so .eq always receives a string; reference
fetchBuildRequestState and buildRequestId and ensure the type-checker is
satisfied (early return or throw) rather than using a non-null assertion.
| const nameRbac = `RBAC Test Organization ${globalIdRbac}` | ||
| const customerIdRbac = `cus_test_rbac_${ORG_ID_RBAC}` | ||
|
|
||
| describe('RBAC mode - organization member operations', () => { |
Contributor
There was a problem hiding this comment.
Lowercase the describe title to satisfy ESLint.
Line 739 currently violates test/prefer-lowercase-title.
🔧 Proposed fix
-describe('RBAC mode - organization member operations', () => {
+describe('rbac mode - organization member operations', () => {📝 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
| describe('RBAC mode - organization member operations', () => { | |
| describe('rbac mode - organization member operations', () => { |
🧰 Tools
🪛 ESLint
[error] 739-739: describes should begin with lowercase
(test/prefer-lowercase-title)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/organization-api.test.ts` at line 739, The describe title string in the
test suite "RBAC mode - organization member operations" violates the
test/prefer-lowercase-title rule; update the describe call (the describe(...)
invocation that begins with the title "RBAC mode - organization member
operations") to use a lowercase title (e.g., "rbac mode - organization member
operations") so the title is fully lowercase and satisfies ESLint.
Comment on lines
+739
to
+876
| describe('RBAC mode - organization member operations', () => { | ||
| beforeAll(async () => { | ||
| const { error: stripeError } = await getSupabaseClient().from('stripe_info').insert({ | ||
| customer_id: customerIdRbac, | ||
| status: 'succeeded', | ||
| product_id: 'prod_LQIregjtNduh4q', | ||
| subscription_id: `sub_${globalIdRbac}`, | ||
| trial_at: new Date(Date.now() + 15 * 24 * 60 * 60 * 1000).toISOString(), | ||
| is_good_plan: true, | ||
| }) | ||
| if (stripeError) | ||
| throw stripeError | ||
|
|
||
| const { error } = await getSupabaseClient().from('orgs').insert({ | ||
| id: ORG_ID_RBAC, | ||
| name: nameRbac, | ||
| management_email: TEST_EMAIL, | ||
| created_by: USER_ID, | ||
| customer_id: customerIdRbac, | ||
| use_new_rbac: true, // Explicitly RBAC — tests the RBAC permission path | ||
| }) | ||
| if (error) | ||
| throw error | ||
|
|
||
| // The generate_org_user_on_org_create trigger creates org_users(super_admin) | ||
| // and role_bindings(org_super_admin) for created_by automatically. | ||
| }) | ||
|
|
||
| afterAll(async () => { | ||
| await getSupabaseClient().from('role_bindings').delete().eq('org_id', ORG_ID_RBAC) | ||
| await getSupabaseClient().from('org_users').delete().eq('org_id', ORG_ID_RBAC) | ||
| await getSupabaseClient().from('orgs').delete().eq('id', ORG_ID_RBAC) | ||
| await getSupabaseClient().from('stripe_info').delete().eq('customer_id', customerIdRbac) | ||
| }) | ||
|
|
||
| it('[GET] /organization - get RBAC org by id', async () => { | ||
| const response = await fetch(`${BASE_URL}/organization?orgId=${ORG_ID_RBAC}`, { | ||
| headers, | ||
| }) | ||
| expect(response.status).toBe(200) | ||
| const type = z.object({ id: z.string(), name: z.string() }) | ||
| const safe = type.safeParse(await response.json()) | ||
| expect(safe.success).toBe(true) | ||
| expect(safe.data).toEqual({ id: ORG_ID_RBAC, name: nameRbac }) | ||
| }) | ||
|
|
||
| it('[GET] /organization/members - returns members via role_bindings (RBAC path)', async () => { | ||
| const response = await fetch(`${BASE_URL}/organization/members?orgId=${ORG_ID_RBAC}`, { | ||
| headers, | ||
| }) | ||
| expect(response.status).toBe(200) | ||
| const type = z.array(z.object({ | ||
| uid: z.string(), | ||
| email: z.string(), | ||
| image_url: z.string(), | ||
| role: z.string(), | ||
| })) | ||
| const safe = type.safeParse(await response.json()) | ||
| expect(safe.success).toBe(true) | ||
|
|
||
| const testUser = safe.data?.find(m => m.uid === USER_ID) | ||
| expect(testUser).toBeTruthy() | ||
| expect(testUser?.email).toBe(USER_EMAIL) | ||
| expect(testUser?.role).toBe('super_admin') | ||
| }) | ||
|
|
||
| it('[PUT] /organization - update RBAC org name', async () => { | ||
| const updatedName = `RBAC Updated ${new Date().toISOString()}` | ||
| const response = await fetch(`${BASE_URL}/organization`, { | ||
| headers, | ||
| method: 'PUT', | ||
| body: JSON.stringify({ orgId: ORG_ID_RBAC, name: updatedName }), | ||
| }) | ||
| expect(response.status).toBe(200) | ||
| const type = z.object({ id: z.uuid(), data: z.any() }) | ||
| const safe = type.safeParse(await response.json()) | ||
| expect(safe.success).toBe(true) | ||
| expect(safe.data?.id).toBe(ORG_ID_RBAC) | ||
| }) | ||
|
|
||
| it('[POST] /organization/members - add member in RBAC mode (sync trigger creates role_binding)', async () => { | ||
| const response = await fetch(`${BASE_URL}/organization/members`, { | ||
| headers, | ||
| method: 'POST', | ||
| body: JSON.stringify({ | ||
| orgId: ORG_ID_RBAC, | ||
| email: USER_ADMIN_EMAIL, | ||
| invite_type: 'read', | ||
| }), | ||
| }) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| const { data: userData } = await getSupabaseClient().from('users').select().eq('email', USER_ADMIN_EMAIL).single() | ||
| expect(userData).toBeTruthy() | ||
|
|
||
| // Verify org_users entry exists | ||
| const { data: orgUser } = await getSupabaseClient().from('org_users').select().eq('org_id', ORG_ID_RBAC).eq('user_id', userData!.id).single() | ||
| expect(orgUser).toBeTruthy() | ||
| expect(orgUser?.user_right).toBe('invite_read') | ||
|
|
||
| // Verify role_binding was created by sync trigger | ||
| const { data: binding } = await getSupabaseClient().from('role_bindings').select().eq('principal_type', 'user').eq('principal_id', userData!.id).eq('org_id', ORG_ID_RBAC) | ||
| expect(binding).toBeTruthy() | ||
| expect(binding!.length).toBeGreaterThan(0) | ||
|
|
||
| // Cleanup | ||
| await getSupabaseClient().from('org_users').delete().eq('org_id', ORG_ID_RBAC).eq('user_id', userData!.id) | ||
| }) | ||
|
|
||
| it('[DELETE] /organization/members - remove member in RBAC mode cleans up role_bindings', async () => { | ||
| const { data: userData } = await getSupabaseClient().from('users').select().eq('email', USER_ADMIN_EMAIL).single() | ||
| expect(userData).toBeTruthy() | ||
|
|
||
| // Add member (sync trigger creates role_binding) | ||
| await getSupabaseClient().from('org_users').insert({ | ||
| org_id: ORG_ID_RBAC, | ||
| user_id: userData!.id, | ||
| user_right: 'read', | ||
| }) | ||
|
|
||
| const { data: bindingsBefore } = await getSupabaseClient().from('role_bindings').select().eq('principal_type', 'user').eq('principal_id', userData!.id).eq('org_id', ORG_ID_RBAC) | ||
| expect(bindingsBefore!.length).toBeGreaterThan(0) | ||
|
|
||
| const response = await fetch(`${BASE_URL}/organization/members?orgId=${ORG_ID_RBAC}&email=${USER_ADMIN_EMAIL}`, { | ||
| headers, | ||
| method: 'DELETE', | ||
| }) | ||
| expect(response.status).toBe(200) | ||
|
|
||
| // org_users removed | ||
| const { data: orgUserAfter } = await getSupabaseClient().from('org_users').select().eq('org_id', ORG_ID_RBAC).eq('user_id', userData!.id) | ||
| expect(orgUserAfter).toHaveLength(0) | ||
|
|
||
| // role_bindings also cleaned up | ||
| const { data: bindingsAfter } = await getSupabaseClient().from('role_bindings').select().eq('principal_type', 'user').eq('principal_id', userData!.id).eq('org_id', ORG_ID_RBAC) | ||
| expect(bindingsAfter).toHaveLength(0) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Use it.concurrent() for these new tests and isolate mutable fixtures per test.
This block adds new it(...) tests in a .test.ts file and mutates shared org/member state, which conflicts with the parallel-test guideline and introduces order coupling.
⚡ Suggested direction
- it('[GET] /organization - get RBAC org by id', async () => {
+ it.concurrent('[GET] /organization - get RBAC org by id', async () => {
- it('[POST] /organization/members - add member in RBAC mode (sync trigger creates role_binding)', async () => {
+ it.concurrent('[POST] /organization/members - add member in RBAC mode (sync trigger creates role_binding)', async () => {Also prefer creating/cleaning a dedicated org per test case (or via a per-test helper) instead of sharing ORG_ID_RBAC across the whole block.
🧰 Tools
🪛 ESLint
[error] 739-739: describes should begin with lowercase
(test/prefer-lowercase-title)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/organization-api.test.ts` around lines 739 - 876, The tests use shared
mutable fixtures (ORG_ID_RBAC, beforeAll/afterAll) and plain it() which can race
when tests run in parallel; change each it(...) to it.concurrent(...) and
refactor setup so each test creates and cleans a dedicated org and related
records (replace global ORG_ID_RBAC usage with a per-test unique org id/name
generated inside the test or a per-test helper), move the
org/stripe/org_users/role_bindings insertions and deletions into per-test
setup/teardown (beforeEach/afterEach or inline in the test) and ensure cleanup
removes role_bindings/org_users/orgs/stripe_info created for that test
(references: beforeAll, afterAll, ORG_ID_RBAC, USER_ADMIN_EMAIL, the
organization/members test cases and the DB queries to
role_bindings/org_users/stripe_info).
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
2 participants
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)
Fixed cross-tenant authorization in build start and build cancel endpoints by resolving the build request from builder_job_id, enforcing app_id equality before permission checks, and running the app.build_native check against the resolved owning app before calling the builder service. The build actions now update using the resolved app_id, and related log messages were updated to avoid ambiguous context; bun.lock also includes the current configVersion metadata.
Motivation (AI generated)
This closes a path where a valid app permission could be paired with a victim job_id to start or cancel foreign builds.
Business Impact (AI generated)
This blocks cross-tenant build sabotage and unauthorized compute actions, reducing security risk without changing public API contracts.
Test Plan (AI generated)
Summary by CodeRabbit
New Features
Bug Fixes