feat: Add SpiceDB authorization sync for project lifecycle #1916
Conversation
Wrap project create operations in two-phase commit with SpiceDB sync so authorization relationships are created atomically with DB records. On delete, clean up SpiceDB relationships with graceful fallback. Mock SpiceDB functions in integration tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f89f186 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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 Key Findings | Risk: Medium
🟠⚠️ Major (3) 🟠⚠️
🟠 1) projectFull.ts:132-189 "Two-phase commit" naming overstates consistency guarantees
Issue: The code comments claim "two-phase commit" where "If SpiceDB sync fails, both DB transactions will rollback automatically." This is architecturally misleading. Calling an external gRPC service (SpiceDB) within a database transaction does not make the external call part of that transaction. The pattern provides fail-fast semantics (SpiceDB failure before commit prevents DB commit), but not true atomicity.
Why: If SpiceDB sync succeeds but a subsequent operation fails (or the DB commit itself fails for any reason), the SpiceDB relationships remain — they cannot be rolled back. This could leave orphaned authorization state. Additionally, there's no compensation mechanism to clean up SpiceDB if something goes wrong after the syncProjectToSpiceDb call returns successfully.
Fix: Consider either:
- Rename the pattern in comments to clarify actual guarantees (e.g., "fail-fast consistency: SpiceDB failure prevents DB commit")
- Move SpiceDB sync after the transaction commits, with explicit rollback logic (delete the project) if SpiceDB fails
- Accept eventual consistency with a background reconciliation job
Refs:
🟠 2) projectFull.ts:204-206 gRPC error detection heuristic may misclassify errors
Issue: The check error?.metadata !== undefined && typeof error?.code === 'number' is intended to detect gRPC errors from SpiceDB, but this heuristic is fragile. Many JavaScript errors have numeric code properties, and various libraries attach metadata properties. If a non-SpiceDB error matches this pattern, users receive the misleading message about SpiceDB authorization failure.
Why: False positives would show "Failed to set up project authorization" when the actual error is unrelated to SpiceDB. False negatives would let SpiceDB errors slip through to generic error handling, potentially exposing internal error details.
Fix: Consider more specific detection:
// Check for gRPC status code range (0-16) and required properties
const isGrpcError = (
error?.metadata !== undefined &&
typeof error?.code === 'number' &&
error?.code >= 0 &&
error?.code <= 16 &&
typeof error?.details === 'string'
);Or wrap syncProjectToSpiceDb in its own try-catch to distinguish SpiceDB errors explicitly.
Refs:
🟠 3) projectFull.ts vs projects.ts Inconsistent SpiceDB error handling between sibling routes
Issue: This PR introduces a stricter consistency model (rollback on SpiceDB failure) for projectFull.ts, while the sibling projects.ts route uses graceful fallback (log warning, continue). Both endpoints handle project creation but with different failure semantics.
Why: A customer using POST /manage/tenants/{id}/projects gets different behavior than one using POST /manage/tenants/{id}/project-full. With the former, SpiceDB failures are logged but creation succeeds. With the latter, SpiceDB failures roll back the entire operation. This behavioral divergence creates confusing API semantics.
Fix: Either:
- Align
projectFull.tsto use the same graceful fallback asprojects.ts - Update
projects.tsto use the new stricter pattern - If divergence is intentional, document why the two routes have different failure semantics
Refs:
// Findings posted as inline comments:
- 🟠 Major:
projectFull.ts:473Variable shadowing — isCreate redeclared
🟡 Minor (2) 🟡
🟡 1) projectFull.test.ts:9-19 No test coverage for SpiceDB failure scenarios
Issue: The integration tests mock all SpiceDB functions to return success. The critical error handling logic (gRPC error detection, transaction rollback) has no test coverage.
Why: If the rollback-on-failure logic has bugs (e.g., SpiceDB call executes outside transaction scope), it won't be caught by tests. The PR's core consistency guarantee is untested.
Fix: Add at least one test that configures the mock to throw and verifies DB rollback:
it('should rollback DB when SpiceDB sync fails during create', async () => {
vi.mocked(syncProjectToSpiceDb).mockRejectedValueOnce(
Object.assign(new Error('SpiceDB connection failed'), { metadata: {}, code: 14 })
);
// ... verify 500 response and project NOT in DB
});Refs:
🟡 2) projectFull.ts:176-184 External service call inside open transaction extends lock duration
Issue: The SpiceDB gRPC call occurs while two database transactions are held open. SpiceDB involves network I/O with variable latency (DNS, TLS, potential retries). During this time, both databases hold locks, increasing contention under load.
Why: If SpiceDB is slow or times out, both databases are blocked. This is the "pessimistic lock held across async boundary" anti-pattern that can cause cascading contention.
Fix: Consider moving SpiceDB sync after the transactions commit. Pattern:
const project = await runDbClient.transaction(...);
await syncProjectToSpiceDb(...); // After commit
return project;If SpiceDB then fails, implement explicit rollback logic (delete the just-created project).
Refs:
// Findings posted as inline comments:
- 🟡 Minor:
projectFull.ts:178Silent skip when userId is falsy - 🟡 Minor:
projectFull.ts:582-593Standardize error key in log
💭 Consider (2) 💭
💭 1) projectFull.ts:179-183 No timeout or retry for SpiceDB calls
Issue: SpiceDB gRPC calls lack explicit timeout configuration and retry logic for transient failures.
Why: Could hang indefinitely or fail on brief network blips.
Fix: Configure gRPC deadline (5-10s) and consider 2-3 retries with exponential backoff for creates.
💭 2) sync.ts SpiceDB sync for API key/system users may create orphaned relationships
Issue: When userId is apikey:... or system, the sync creates relationships for non-existent SpiceDB users.
Why: Could pollute SpiceDB with invalid relationships.
Fix: Add explicit handling: skip SpiceDB sync entirely for non-user principals, or only create the project-to-org link.
💡 APPROVE WITH SUGGESTIONS
Summary: This PR adds valuable SpiceDB authorization sync for project lifecycle, addressing a real gap in the authorization system. The core implementation is solid. The main concerns are: (1) the "two-phase commit" naming overstates the consistency guarantees — clarifying the actual semantics would help future maintainers, (2) the gRPC error detection heuristic could be more robust, and (3) the behavioral divergence from the sibling projects.ts route should be intentional and documented. None of these are blocking, but addressing them would strengthen the implementation.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
projectFull.ts:574-593 |
Asymmetric consistency (DELETE graceful, CREATE strict) | Intentional design choice documented in code comments |
client.ts:21-33 |
SpiceDB client lacks health monitoring | Pre-existing — not introduced by this PR |
projectFull.ts:207-215 |
Missing correlation ID in logs | Valid but very minor — logger may already include request context |
projectFull.test.ts |
Test mock completely bypasses SpiceDB | Expected behavior for API/DB layer tests; documented in PR |
projects.test.ts |
Sibling test doesn't mock SpiceDB | Pre-existing — consequence of existing graceful fallback pattern |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-architecture |
5 | 2 | 0 | 0 | 0 | 0 | 3 |
pr-review-security-iam |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-sre |
6 | 0 | 1 | 0 | 0 | 0 | 5 |
pr-review-errors |
5 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-standards |
3 | 1 | 0 | 0 | 1 | 0 | 1 |
pr-review-tests |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-consistency |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
| Total | 30 | 6 | 2 | 0 | 3 | 0 | 19 |
| } | ||
|
|
||
| // Handle SpiceDB sync failures for creates - transactions already rolled back | ||
| const isCreate = c.get('isProjectCreate') ?? false; |
There was a problem hiding this comment.
🟠 MAJOR Variable shadowing: isCreate redeclared in catch block
Issue: The variable isCreate is already declared at line 405 and is redeclared identically here in the catch block.
Why: While functionally harmless (same value), this duplication is unnecessary and could confuse maintainers if the logic diverges in the future.
Fix: (1-click apply)
| const isCreate = c.get('isProjectCreate') ?? false; | |
| // Handle SpiceDB sync failures for creates - transactions already rolled back | |
| if (c.get('isProjectCreate')) { |
Refs:
|
|
||
| // Phase 2: Sync to SpiceDB (still within transaction scope) | ||
| // If this fails, both transactions will rollback automatically | ||
| if (userId) { |
There was a problem hiding this comment.
🟡 MINOR Silent skip when userId is falsy — no logging
Issue: When userId is not set, the SpiceDB sync is silently skipped without any log entry.
Why: If a project is created in a code path where userId is undefined (e.g., certain test scenarios or future code changes), the project will exist but have no authorization relationships. The silent skip makes this difficult to debug.
Fix: Add explicit logging:
if (userId) {
await syncProjectToSpiceDb({
tenantId,
projectId: validatedProjectData.id,
creatorUserId: userId,
});
} else {
logger.warn(
{ tenantId, projectId: validatedProjectData.id },
'Skipping SpiceDB sync: no userId available - project may have no explicit owner'
);
}Refs:
- Same pattern at line 440 in PUT handler
| } catch (spiceDbError) { | ||
| // Log but don't fail - the project data is already deleted | ||
| // This could leave orphaned auth relationships, but won't affect functionality | ||
| logger.warn( | ||
| { | ||
| spiceDbError, | ||
| tenantId, | ||
| projectId, | ||
| }, | ||
| 'Failed to remove project from SpiceDB - orphaned auth relationships may remain' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR Standardize error key and add remediation guidance
Issue: The SpiceDB error is logged as spiceDbError instead of the conventional error key, which may cause inconsistent log parsing.
Why: Log aggregation tools that key on error may not detect this failure. Operations teams also need guidance on remediation.
Fix:
| } catch (spiceDbError) { | |
| // Log but don't fail - the project data is already deleted | |
| // This could leave orphaned auth relationships, but won't affect functionality | |
| logger.warn( | |
| { | |
| spiceDbError, | |
| tenantId, | |
| projectId, | |
| }, | |
| 'Failed to remove project from SpiceDB - orphaned auth relationships may remain' | |
| ); | |
| } | |
| } catch (spiceDbError) { | |
| // Log but don't fail - the project data is already deleted | |
| // This could leave orphaned auth relationships, but won't affect functionality | |
| logger.warn( | |
| { | |
| error: spiceDbError, | |
| tenantId, | |
| projectId, | |
| }, | |
| 'Failed to remove project from SpiceDB - orphaned auth relationships may remain. Manual cleanup may be required if this persists.' | |
| ); | |
| } |
Refs:
1 participant
Summary
Split from #1836 — this PR contains only the SpiceDB/authz synchronization work.
Test plan
Made with Cursor