feat: tenant-scoped composite project IDs in SpiceDB#1996
feat: tenant-scoped composite project IDs in SpiceDB#1996omar-inkeep wants to merge 4 commits intomainfrom
Conversation
- Migrate SpiceDB project IDs from plain format (project:{projectId}) to
tenant-scoped composite format (project:{tenantId}/{projectId})
- Add migration script (pnpm spicedb:migrate-ids) with dry-run and --apply modes
- Detect and repair missing project→organization links in SpiceDB
- Update permissions, sync, and middleware to use composite project IDs
- Update tests for new tenant-scoped ID format
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
runAuth.ts:245-248Missing requiredtenantIdparameter incanUseProjectStrictcall breaks Slack JWT authorization
🟠⚠️ Major (3) 🟠⚠️
🟠 1) permissions.ts, sync.ts Silent failures when parsing composite SpiceDB IDs
files: packages/agents-core/src/auth/authz/permissions.ts:152-159, permissions.ts:177-184, sync.ts:366-374
Issue: Multiple locations use try { ... } catch { return []; } patterns that silently swallow errors when parsing composite SpiceDB project IDs via fromSpiceDbProjectId(). There is no logging when IDs fail to parse.
Why: If SpiceDB returns malformed IDs (during migration, data corruption, or bugs), users silently lose access to projects with no error trail. This makes debugging authorization issues extremely difficult in production. The listAccessibleProjectIds, listUsableProjectIds, and listUserProjectMembershipsInSpiceDb functions all have this pattern.
Fix: Add logging in catch blocks to aid debugging while preserving graceful degradation:
} catch (error) {
console.warn(`Failed to parse SpiceDB project ID: ${id}`, error);
return [];
}Consider using a structured logger if available in this module.
Refs:
🟠 2) permissions.test.ts Missing test coverage for listUsableProjectIds cross-tenant filtering
Issue: The listUsableProjectIds function includes tenant filtering logic that silently drops IDs from other tenants, but there is no unit test verifying this security-critical behavior. The similar function listAccessibleProjectIds has a test for cross-tenant filtering (lines 235-248), but listUsableProjectIds does not.
Why: If the tenant filtering logic were accidentally removed or broken, users could see projects from other tenants in API responses (e.g., the availableAgents endpoint). This is a cross-tenant data exposure risk.
Fix: Add a test case to permissions.test.ts:
describe('listUsableProjectIds', () => {
it('should filter out projects from other tenants', async () => {
mockLookupResources.mockResolvedValue([
'test-tenant/project-1',
'other-tenant/project-2',
'test-tenant/project-3',
]);
const result = await listUsableProjectIds({
userId: 'user-1',
tenantId: 'test-tenant',
});
expect(result).toEqual(['project-1', 'project-3']);
});
});Refs:
- permissions.ts:165-184 — function implementation
- permissions.test.ts:235-248 — existing test for listAccessibleProjectIds
🟠 3) config.ts Missing unit tests for core ID transformation functions
Issue: No unit tests exist for toSpiceDbProjectId and fromSpiceDbProjectId functions. These are the core ID transformation functions that the entire tenant-scoped authorization system depends on.
Why: If the composite ID format parsing/generation logic breaks (wrong separator, incorrect substring handling), all SpiceDB operations would silently fail or produce incorrect results. The fromSpiceDbProjectId function throws on invalid format, but this error path is untested.
Fix: Add tests (consider creating config.test.ts):
describe('toSpiceDbProjectId', () => {
it('should compose tenant-scoped ID', () => {
expect(toSpiceDbProjectId('tenant-abc', 'proj-123')).toBe('tenant-abc/proj-123');
});
});
describe('fromSpiceDbProjectId', () => {
it('should parse tenant-scoped ID', () => {
const result = fromSpiceDbProjectId('tenant-abc/proj-123');
expect(result).toEqual({ tenantId: 'tenant-abc', projectId: 'proj-123' });
});
it('should throw on invalid format', () => {
expect(() => fromSpiceDbProjectId('invalid-no-slash')).toThrow('Invalid SpiceDB project ID format');
});
it('should handle projectId containing separator', () => {
const result = fromSpiceDbProjectId('tenant/project/with/slashes');
expect(result).toEqual({ tenantId: 'tenant', projectId: 'project/with/slashes' });
});
});Refs:
Inline Comments:
- 🟠 Major:
permissions.ts:152-159Silent failure when parsing composite SpiceDB IDs (see inline for details)
🟡 Minor (2) 🟡
🟡 1) sync-spicedb.sh:249 Typo in migration command documentation
Issue: Comment references pnpm spicedb:migrate-ids:apply but the actual command is pnpm spicedb:migrate-ids --apply.
Why: Operators copy-pasting will get "command not found".
Fix: Change to pnpm spicedb:migrate-ids --apply.
Inline Comments:
- 🟡 Minor:
sync-spicedb.sh:249Typo in migration command (see inline for 1-click fix)
💭 Consider (2) 💭
💭 1) migrate-spicedb-project-ids.ts Database connection cleanup
Issue: The migration script creates a database client but doesn't explicitly close the connection before process.exit(0).
Why: While process.exit() will terminate regardless, explicit cleanup is best practice for scripts that may be run repeatedly during dry-run testing.
Fix: Consider adding connection cleanup in a finally block if the Drizzle client exposes a close method.
💭 2) .changeset/ Missing changeset for agents-core public API change
Issue: This PR adds new public exports (toSpiceDbProjectId, fromSpiceDbProjectId) to @inkeep/agents-core's authz subpath.
Why: Per AGENTS.md, public surface changes to published packages should have a changeset.
Fix: pnpm bump patch --pkg agents-core "Add tenant-scoped SpiceDB project ID utilities" (or minor if these are considered new features).
🚫 REQUEST CHANGES
Summary: This PR makes important security improvements by introducing tenant-scoped composite project IDs in SpiceDB, but there is one critical blocking bug: the canUseProjectStrict call in trySlackUserJwtAuth (runAuth.ts:245-248) is missing the newly required tenantId parameter. This will break all Slack work app authenticated requests. Please fix this before merging.
Additionally, the silent error handling in ID parsing functions should be addressed to improve debuggability, and test coverage for the cross-tenant filtering logic should be added given the security-sensitive nature of these changes.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
migrate-spicedb-project-ids.ts |
Migration doesn't preserve user role relationships on old IDs | Upon review, the script correctly deletes OLD plain-ID relationships — if user roles exist on composite IDs (created by sync functions), they're preserved. The migration only fixes org links, which is the stated purpose. |
permissions.test.ts |
Missing test for canUseProjectStrict |
Lower priority — middleware integration tests cover the happy path, and the CRITICAL bug needs to be fixed first anyway. |
sync.test.ts |
Missing test for revokeAllProjectMemberships tenant prefix |
Lower priority — the behavior change is correct, testing is nice-to-have. |
sync.test.ts |
Missing test for listUserProjectMembershipsInSpiceDb filtering |
Lower priority — membership listing is lower risk than permission checks. |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-errors |
5 | 1 | 0 | 0 | 0 | 0 | 4 |
pr-review-tests |
5 | 2 | 0 | 0 | 0 | 0 | 3 |
pr-review-devops |
5 | 0 | 2 | 0 | 1 | 0 | 2 |
pr-review-standards |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-consistency |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-types |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 20 | 3 | 2 | 0 | 2 | 0 | 13 |
Note: Multiple reviewers independently identified the same CRITICAL bug (missing tenantId in runAuth.ts), demonstrating high confidence in this finding. Findings were deduplicated and merged.
| return compositeIds.flatMap((id) => { | ||
| try { | ||
| const parsed = fromSpiceDbProjectId(id); | ||
| return parsed.tenantId === params.tenantId ? [parsed.projectId] : []; | ||
| } catch { | ||
| return []; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR Silent failure when parsing composite SpiceDB IDs
Issue: The catch block silently returns an empty array for any parsing error, with no logging. This catches ALL exceptions, not just parsing errors from fromSpiceDbProjectId.
Why: If SpiceDB returns malformed IDs (e.g., during migration or data corruption), users silently lose access to projects without any indication why. Debugging becomes extremely difficult because there's no log of which IDs failed to parse.
Fix: Add logging for parse failures to aid debugging:
return compositeIds.flatMap((id) => {
try {
const parsed = fromSpiceDbProjectId(id);
return parsed.tenantId === params.tenantId ? [parsed.projectId] : [];
} catch (error) {
// Log but don't throw - graceful degradation
console.warn(`Failed to parse SpiceDB project ID: ${id}`, error);
return [];
}
});Refs:
- Same pattern exists at permissions.ts:177-184 (
listUsableProjectIds) - sync.ts:366-374 (
listUserProjectMembershipsInSpiceDb)
|
📚 Documentation Review No documentation updates needed for this PR. The tenant-scoped composite project ID changes are internal to SpiceDB authorization and don't affect user-facing APIs, configurations, or behaviors. The migration script ( |
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
runAuth.ts:245-248Missing requiredtenantIdparameter incanUseProjectStrictcall breaks Slack work app authorization
🕐 Pending Recommendations (4)
Prior issues from the previous review that remain unaddressed:
- 🟠
permissions.ts:152-159Silent failure when parsing composite SpiceDB IDs — same pattern inpermissions.ts:177-184andsync.ts:366-374 - 🟠
permissions.test.tsMissing test coverage forlistUsableProjectIdscross-tenant filtering - 🟠
config.tsMissing unit tests for core ID transformation functions (toSpiceDbProjectId,fromSpiceDbProjectId) - 💭
.changeset/Consider adding changeset foragents-corepublic API addition
✅ Resolved Since Last Review
- 🟡
sync-spicedb.sh:249— Typo in migration command documentation (fixed via 1-click suggestion ✓)
🚫 REQUEST CHANGES
Summary: The delta since the last review fixed the documentation typo (thanks!), but the CRITICAL bug remains: trySlackUserJwtAuth in runAuth.ts:245-248 is still missing the required tenantId parameter. This will cause all Slack work app authenticated requests to fail because the SpiceDB permission check will use an incorrectly formatted project ID.
The previous review's inline comment on this file appears to not have posted correctly, so I've added it again with a 1-click suggestion to fix it.
Discarded (0)
No new findings to discard in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 0 | 0 | 1 | 4 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 4 | 0 |
Note: Delta review — subagent reviewers not dispatched for 1-line documentation fix. Critical bug from prior review verified as still present.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (4) Pending | Risk: Medium
✅ Resolved Since Last Review
🔴 CRITICAL bug fixed: The missing tenantId parameter in trySlackUserJwtAuth (runAuth.ts:245-248) has been addressed. The fix correctly passes tenantId: payload.tenantId to canUseProjectStrict, ensuring Slack work app authenticated requests use the tenant-scoped composite project ID format.
The fix is minimal and correct — no new issues introduced.
🕐 Pending Recommendations (4)
Prior issues from previous reviews that remain unaddressed:
- 🟠
permissions.ts:152-159Silent failure when parsing composite SpiceDB IDs — same pattern inpermissions.ts:177-184andsync.ts:366-374. Add logging to catch blocks for debuggability. - 🟠
permissions.test.tsMissing test coverage forlistUsableProjectIdscross-tenant filtering (security-critical behavior) - 🟠
config.tsMissing unit tests for core ID transformation functions (toSpiceDbProjectId,fromSpiceDbProjectId) - 💭
.changeset/Consider adding changeset foragents-corepublic API addition (toSpiceDbProjectId,fromSpiceDbProjectIdexports)
💡 APPROVE WITH SUGGESTIONS
Summary: The CRITICAL blocking bug is now fixed — Slack work app authorization will work correctly with the new tenant-scoped composite project IDs. 🎉
The pending recommendations above are quality/observability improvements that would strengthen this security-sensitive change but are not blocking. The silent error handling in ID parsing functions and missing test coverage for cross-tenant filtering are worth addressing before or shortly after merge to ensure production debuggability and maintain confidence in the tenant isolation guarantees.
Discarded (0)
No new findings to discard in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 4 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 4 | 0 |
Note: Delta review — the only change was a 1-line fix for the CRITICAL bug identified in the prior review. No subagent reviewers dispatched as the fix is straightforward and correct.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (4) Pending | Risk: Medium
Delta Analysis
This review was triggered by a pull_request:synchronize event. The 10 commits since the last review (81618863c4a8..HEAD) are entirely from a merge of main into this branch — bringing in unrelated work (Slack work app fixes, changesets, version bumps).
No substantive changes to PR code. The only change to PR files is an automated version bump in packages/agents-core/package.json (0.47.5 → 0.48.2).
🕐 Pending Recommendations (4)
Prior issues from previous reviews that remain unaddressed:
- 🟠
permissions.ts:152-159Silent failure when parsing composite SpiceDB IDs — same pattern inpermissions.ts:177-184andsync.ts:366-374. Add logging to catch blocks for debuggability. - 🟠
permissions.test.tsMissing test coverage forlistUsableProjectIdscross-tenant filtering (security-critical behavior) - 🟠
config.tsMissing unit tests for core ID transformation functions (toSpiceDbProjectId,fromSpiceDbProjectId) - 💭
.changeset/Consider adding changeset foragents-corepublic API addition (toSpiceDbProjectId,fromSpiceDbProjectIdexports)
✅ Previously Resolved
- 🔴
runAuth.ts:245-248— CRITICAL missingtenantIdparameter (fixed in commit81618863) - 🟡
sync-spicedb.sh:249— Typo in migration command documentation (fixed via 1-click suggestion)
💡 APPROVE WITH SUGGESTIONS
Summary: No new code changes in this delta — just a merge from main. The previous review's approval stands. 🎉
The pending recommendations above are quality/observability improvements that would strengthen this security-sensitive change but are not blocking. The silent error handling in ID parsing functions and missing test coverage for cross-tenant filtering are worth addressing before or shortly after merge.
Discarded (0)
No new findings — delta contained no substantive PR changes.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 4 | 0 |
Note: No subagent reviewers dispatched — delta contained only a merge from main with no substantive changes to PR code.
1 participant
Summary
Migrates SpiceDB project IDs from plain format (
project:{projectId}) to tenant-scoped composite format (project:{tenantId}/{projectId}) to prevent cross-tenant collisions and fix missing organization links.Changes
pnpm spicedb:migrate-ids): Deletes old plain-ID relationships and ensures every project has the correctorganizationlink on its composite ID--applyto write changestoSpiceDbProjectId(tenantId, projectId)everywhereprojectAccess,runAuth, and routes updated for composite IDsHow to run the migration