Repository navigation
feat(redis-dep): PR 4/5 — delete permission cache, collapse permissions-cached into permissions - #1041
Merged
Conversation
…ssions-cached into permissions
Redis deprecation PR 4 of 5. Collapses the hybrid L1/L2 permission cache layer
into a single canonical permissions module that goes straight to Postgres.
- Delete packages/lib/src/services/permission-cache.ts (523 lines) and
packages/lib/src/permissions/permissions-cached.ts (618 lines).
- Fold getUserAccessLevel, canUser{View,Edit,Share,Delete}Page, getUserDriveAccess,
getUserDrivePermissions, getBatchPagePermissions, and DrivePermissionLevel into
packages/lib/src/permissions/permissions.ts with unchanged signatures.
- getBatchPagePermissions becomes a single-CTE Drizzle query joining pages →
drives → drive_members (ADMIN, acceptedAt IS NOT NULL) → page_permissions
(unexpired expires_at) in one DB round-trip. Returns an entry for every
input pageId — trashed / expired / inaccessible rows resolve to all-false.
- Remove invalidateUserPermissions / invalidateDrivePermissions / permissionCache
call sites in permission-mutations, member invite/remove routes, and
memory-monitor's emergency cleanup path.
- Delete the GET /api/permissions/batch cache-stats handler (POST keeps working;
route now derives accessible count from canView instead of map.size since the
map is no longer pre-filtered).
- Update @pagespace/lib/server, @pagespace/lib, and the permissions index to
re-export from the canonical module. Drop ./permissions-cached and
./services/permission-cache subpath entries from packages/lib/package.json.
- Update @pagespace/lib/permissions-cached → @pagespace/lib/permissions imports
in realtime, processor, and their test mocks; drop the alias in
apps/processor/vitest.config.ts.
- Add src/permissions/__tests__/batch-page-permissions.test.ts covering owner,
accepted ADMIN member, unaccepted invite, unexpired/expired explicit grants,
trashed pages, inaccessible pageIds, non-existent pageIds, mixed batches, and
fail-closed on DB failure. Delete the superseded cache-trust-boundaries,
permissions-cached, and permission-cache test files and their references in
scripts/test-security.sh, .github/workflows/security.yml, and knip.json.
See tasks/redis-deprecation.md § "PR 4 — Delete the permission cache" for the
full spec.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR removes the cached permissions system (permissions-cached.ts, permission-cache.ts, and related services) and consolidates all permission lookups to use the non-cached permissions module. The change includes updating imports across all applications, removing cache invalidation calls from mutation endpoints, and deleting related test suites. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/app/api/permissions/batch/route.ts (1)
50-55:⚠️ Potential issue | 🟡 MinorStale
cacheHitsfield in empty-array response contradicts the new stats shape.The docblock (Line 31) and the non-empty path (Lines 93-98) now return
{ total, accessible, denied, processingTimeMs }, but this early-return branch still returns the legacy{ total, accessible, cacheHits }. Since this PR removes the permission cache entirely,cacheHitsis stale and misleading, and the response shape is now inconsistent between empty and non-empty inputs — clients (and the test at Line 120) will see differing keys.Proposed fix
if (pageIds.length === 0) { return NextResponse.json({ permissions: {}, - stats: { total: 0, accessible: 0, cacheHits: 0 } + stats: { total: 0, accessible: 0, denied: 0, processingTimeMs: 0 } }); }The corresponding assertion in
apps/web/src/app/api/permissions/batch/__tests__/route.test.ts(Line 120) should be updated accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/permissions/batch/route.ts` around lines 50 - 55, The early-return in the batch permissions route returns the old stats shape with cacheHits which is now removed; update the branch that checks if (pageIds.length === 0) in route.ts to return stats matching the new shape used elsewhere ({ total: 0, accessible: 0, denied: 0, processingTimeMs: 0 }) and adjust the corresponding test assertion in apps/web/src/app/api/permissions/batch/__tests__/route.test.ts to expect denied and processingTimeMs instead of cacheHits.apps/web/src/app/api/permissions/batch/__tests__/route.test.ts (1)
112-122:⚠️ Potential issue | 🟡 MinorTest pins the stale
cacheHitscontract.This assertion locks in the legacy
{ total, accessible, cacheHits: 0 }response for the empty-pageIds branch, which is inconsistent with the new{ total, accessible, denied, processingTimeMs }shape used everywhere else in this route (see Lines 93-98 ofroute.tsand the docblock at Line 31). If the route is fixed to emit the new shape consistently, this expectation needs to be updated in lockstep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/permissions/batch/__tests__/route.test.ts` around lines 112 - 122, The test "should return empty permissions for empty pageIds array" pins the old stats shape with cacheHits; update the assertion in this test (the block using createPostRequest and POST) to expect the new stats structure — include permissions: {} and stats with total: 0, accessible: 0, denied: 0, and processingTimeMs validated as a number (use the test matcher for any Number), and remove any expectation for cacheHits.apps/realtime/src/per-event-auth.ts (1)
11-14:⚠️ Potential issue | 🟡 MinorUpdate the stale cache wording.
The import now targets the canonical non-cached permissions module, but these comments still describe cache stale windows / “cache or DB”. That can confuse future security reviews of the realtime write-auth path.
📝 Proposed wording update
- * Stale-window analysis after bypassCache fix: - * - Write operations (per-event auth): 0s — always hits DB directly - * - Room joins: up to 60s (kick handler provides immediate eviction) - * - API routes: up to 60s (read operations, acceptable risk) + * Stale-window analysis after permission-cache removal: + * - Write operations (per-event auth): 0s — direct permission read + * - Room joins/API routes use the centralized permissions module; kick handling + * remains defense-in-depth for connected sockets. ... - * This goes directly to the permission system (cache or DB) to verify current access. + * This goes through the centralized permissions module to verify current access.Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/src/per-event-auth.ts` around lines 11 - 14, Update the comment block that begins "Stale-window analysis after bypassCache fix" and the similar wording around lines referencing the per-event auth path (mentions "per-event auth", "cache or DB", and stale windows) to clarify that the import now points to the canonical non-cached permissions module; remove references to cache staleness or "cache or DB" ambiguity and explicitly state that write operations use the non-cached permissions module and hit the DB directly (and adjust the room joins/API routes wording if present to reflect their separate behavior). Locate and edit the comment text in per-event-auth.ts (the top "Stale-window analysis..." block and the similar block at ~100-103) to use precise wording like "uses canonical non-cached permissions module — write operations hit DB directly" instead of any cache/stale-window phrasing.
🧹 Nitpick comments (4)
apps/web/src/app/api/permissions/batch/__tests__/route.test.ts (1)
204-223: Busy-wait for >500ms makes this test slow and flaky on CI.Spinning on
Date.now()for 510ms per run adds real wall-clock time and is sensitive to CI scheduler jitter. Prefer stubbingDate.now(or thestartTime/endTimereadings) viavi.useFakeTimers()/vi.spyOn(Date, 'now')so the slow-path branch is exercised deterministically without actually sleeping.Sketch
const nowSpy = vi.spyOn(Date, 'now'); nowSpy.mockReturnValueOnce(0).mockReturnValueOnce(501); mockGetBatchPagePermissions.mockResolvedValue(new Map()); // ... invoke POST ... nowSpy.mockRestore();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/permissions/batch/__tests__/route.test.ts` around lines 204 - 223, The test currently busy-waits inside mockGetBatchPagePermissions which makes CI slow and flaky; instead spy on Date.now (e.g., vi.spyOn(Date, 'now')) and mock its sequential returns to simulate a >500ms duration (first call = start, second call = start+501), have mockGetBatchPagePermissions resolve immediately (new Map()) and then call POST; after assertions restore the spy. Update the test to use vi.spyOn(Date, 'now').mockReturnValueOnce(...).mockReturnValueOnce(...) around the POST invocation and call .mockRestore() afterwards so the slow-path branch is exercised deterministically.packages/lib/src/index.ts (1)
28-30: Nit: redundant named type re-export.
export * from './permissions/permissions'on line 29 already re-exportsDrivePermissionLevel, so the explicitexport typeon line 30 is a no-op. Safe to drop, though harmless to leave for discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/index.ts` around lines 28 - 30, The file re-exports everything from './permissions/permissions' and then redundantly re-exports the type DrivePermissionLevel; remove the explicit "export type { DrivePermissionLevel }" line to avoid the no-op duplication. Locate the re-export block in packages/lib/src/index.ts (the lines exporting from './permissions/permissions' and the explicit DrivePermissionLevel type) and delete the explicit type export, leaving the existing "export * from './permissions/permissions';" to provide the type.packages/lib/src/permissions/__tests__/batch-page-permissions.test.ts (1)
266-280: Optional: also assert the failure is logged.Fail-closed behavior is well-covered, but asserting
loggers.api.error(mocked at line 36) is called on DB rejection would lock in the observability contract alongside the deny-map return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/permissions/__tests__/batch-page-permissions.test.ts` around lines 266 - 280, The test should also assert that a DB rejection is logged: after stubbing db.select to return the chain that ultimately rejects (the existing mocked where/leftJoin chain) and calling getBatchPagePermissions(USER, ['p1','p2']), add an expectation that the mocked logger (loggers.api.error) was called (or calledWith an Error/DB down message) to ensure the failure is observable; locate the logger mock from the test setup and add expect(loggers.api.error).toHaveBeenCalled() (or toHaveBeenCalledWith(expect.objectContaining({ message: 'DB down' })) as appropriate) alongside the existing assertions.packages/lib/src/permissions/permissions.ts (1)
485-572:bypassCacheis a pure no-op here — document clearly or drop in the final PR.
getUserDriveAccessacceptsbypassCachepurely for signature parity; it’s never destructured or consulted. The JSDoc at line 483 does note this, which is good. Consider annotating the param@deprecatedso the compatibility shim can be removed cleanly in PR 5/5 once all consumers have migrated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/permissions/permissions.ts` around lines 485 - 572, The options.bypassCache parameter on getUserDriveAccess is a no-op kept only for signature parity; update the function's JSDoc to mark options.bypassCache as `@deprecated` and call out it is unused so it can be removed in a future PR, and (optionally) add a short inline comment next to the options type ({ silent?: boolean; bypassCache?: boolean }) indicating it's a compatibility shim to make future removal safe; reference getUserDriveAccess and the options/bypassCache symbols so reviewers can find and remove the shim later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/permissions/batch/route.ts`:
- Around line 85-91: Update the route.ts docblock for the batch permissions
response to explicitly state that the returned permissions object only includes
entries where canView === true (denied pages are omitted) and that denied pages
are instead counted in stats.denied; reference the variables/structures in the
implementation (permissionsMap, permissions object, accessibleCount and stats)
and enumerate the response shape fields: permissions (map of viewable page
entries) and stats (total, accessible, denied, processingTimeMs) so callers
understand they must check canView before using a permissions entry.
In `@packages/lib/src/permissions/permissions.ts`:
- Around line 582-661: getUserDrivePermissions treats any drive_members row as
active membership and thus grants pending-invite users admin/edit rights; update
the membership query inside getUserDrivePermissions to require acceptedAt not be
null (same filter used in getBatchPagePermissions) by adding an
and(isNotNull(driveMembers.acceptedAt)) to the where clause so only accepted
memberships set isAdmin/canEdit true; ensure the change references
driveMembers.role and driveMembers.acceptedAt in the membership selection and
keeps existing role->isAdmin/isMember logic.
---
Outside diff comments:
In `@apps/realtime/src/per-event-auth.ts`:
- Around line 11-14: Update the comment block that begins "Stale-window analysis
after bypassCache fix" and the similar wording around lines referencing the
per-event auth path (mentions "per-event auth", "cache or DB", and stale
windows) to clarify that the import now points to the canonical non-cached
permissions module; remove references to cache staleness or "cache or DB"
ambiguity and explicitly state that write operations use the non-cached
permissions module and hit the DB directly (and adjust the room joins/API routes
wording if present to reflect their separate behavior). Locate and edit the
comment text in per-event-auth.ts (the top "Stale-window analysis..." block and
the similar block at ~100-103) to use precise wording like "uses canonical
non-cached permissions module — write operations hit DB directly" instead of any
cache/stale-window phrasing.
In `@apps/web/src/app/api/permissions/batch/__tests__/route.test.ts`:
- Around line 112-122: The test "should return empty permissions for empty
pageIds array" pins the old stats shape with cacheHits; update the assertion in
this test (the block using createPostRequest and POST) to expect the new stats
structure — include permissions: {} and stats with total: 0, accessible: 0,
denied: 0, and processingTimeMs validated as a number (use the test matcher for
any Number), and remove any expectation for cacheHits.
In `@apps/web/src/app/api/permissions/batch/route.ts`:
- Around line 50-55: The early-return in the batch permissions route returns the
old stats shape with cacheHits which is now removed; update the branch that
checks if (pageIds.length === 0) in route.ts to return stats matching the new
shape used elsewhere ({ total: 0, accessible: 0, denied: 0, processingTimeMs: 0
}) and adjust the corresponding test assertion in
apps/web/src/app/api/permissions/batch/__tests__/route.test.ts to expect denied
and processingTimeMs instead of cacheHits.
---
Nitpick comments:
In `@apps/web/src/app/api/permissions/batch/__tests__/route.test.ts`:
- Around line 204-223: The test currently busy-waits inside
mockGetBatchPagePermissions which makes CI slow and flaky; instead spy on
Date.now (e.g., vi.spyOn(Date, 'now')) and mock its sequential returns to
simulate a >500ms duration (first call = start, second call = start+501), have
mockGetBatchPagePermissions resolve immediately (new Map()) and then call POST;
after assertions restore the spy. Update the test to use vi.spyOn(Date,
'now').mockReturnValueOnce(...).mockReturnValueOnce(...) around the POST
invocation and call .mockRestore() afterwards so the slow-path branch is
exercised deterministically.
In `@packages/lib/src/index.ts`:
- Around line 28-30: The file re-exports everything from
'./permissions/permissions' and then redundantly re-exports the type
DrivePermissionLevel; remove the explicit "export type { DrivePermissionLevel }"
line to avoid the no-op duplication. Locate the re-export block in
packages/lib/src/index.ts (the lines exporting from './permissions/permissions'
and the explicit DrivePermissionLevel type) and delete the explicit type export,
leaving the existing "export * from './permissions/permissions';" to provide the
type.
In `@packages/lib/src/permissions/__tests__/batch-page-permissions.test.ts`:
- Around line 266-280: The test should also assert that a DB rejection is
logged: after stubbing db.select to return the chain that ultimately rejects
(the existing mocked where/leftJoin chain) and calling
getBatchPagePermissions(USER, ['p1','p2']), add an expectation that the mocked
logger (loggers.api.error) was called (or calledWith an Error/DB down message)
to ensure the failure is observable; locate the logger mock from the test setup
and add expect(loggers.api.error).toHaveBeenCalled() (or
toHaveBeenCalledWith(expect.objectContaining({ message: 'DB down' })) as
appropriate) alongside the existing assertions.
In `@packages/lib/src/permissions/permissions.ts`:
- Around line 485-572: The options.bypassCache parameter on getUserDriveAccess
is a no-op kept only for signature parity; update the function's JSDoc to mark
options.bypassCache as `@deprecated` and call out it is unused so it can be
removed in a future PR, and (optionally) add a short inline comment next to the
options type ({ silent?: boolean; bypassCache?: boolean }) indicating it's a
compatibility shim to make future removal safe; reference getUserDriveAccess and
the options/bypassCache symbols so reviewers can find and remove the shim later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67e3a456-1bae-40ca-acb2-17c73f813ec9
📒 Files selected for processing (42)
.github/workflows/security.ymlapps/atlas/src/architecture-data.tsapps/processor/src/services/__tests__/authorization.test.tsapps/processor/src/services/__tests__/rbac-delete.test.tsapps/processor/src/services/authorization.tsapps/processor/src/services/rbac.tsapps/processor/vitest.config.tsapps/realtime/src/__tests__/index.test.tsapps/realtime/src/__tests__/per-event-auth.test.tsapps/realtime/src/__tests__/rooms.test.tsapps/realtime/src/index.tsapps/realtime/src/per-event-auth.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/invite/route.tsapps/web/src/app/api/permissions/batch/__tests__/route.test.tsapps/web/src/app/api/permissions/batch/route.tsknip.jsonpackages/lib/package.jsonpackages/lib/src/__tests__/cross-tenant-escalation.test.tspackages/lib/src/__tests__/multi-tenant-isolation.test.tspackages/lib/src/__tests__/permission-cache.test.tspackages/lib/src/__tests__/permissions-cached.test.tspackages/lib/src/index.tspackages/lib/src/permissions/__tests__/batch-page-permissions.test.tspackages/lib/src/permissions/__tests__/cache-trust-boundaries.test.tspackages/lib/src/permissions/__tests__/permission-mutations-unit.test.tspackages/lib/src/permissions/__tests__/permissions-cached.test.tspackages/lib/src/permissions/index.tspackages/lib/src/permissions/permission-mutations.tspackages/lib/src/permissions/permissions-cached.tspackages/lib/src/permissions/permissions.tspackages/lib/src/repositories/__tests__/enforced-file-repository.test.tspackages/lib/src/repositories/enforced-file-repository.tspackages/lib/src/server.tspackages/lib/src/services/__tests__/validated-service-token.test.tspackages/lib/src/services/memory-monitor.tspackages/lib/src/services/permission-cache.tspackages/lib/src/services/validated-service-token.tspackages/lib/vitest.config.tsscripts/test-security.sh
💤 Files with no reviewable changes (15)
- apps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
- packages/lib/vitest.config.ts
- packages/lib/src/services/memory-monitor.ts
- knip.json
- packages/lib/src/permissions/tests/permission-mutations-unit.test.ts
- .github/workflows/security.yml
- apps/web/src/app/api/drives/[driveId]/members/[userId]/tests/route.test.ts
- packages/lib/package.json
- packages/lib/src/tests/permissions-cached.test.ts
- packages/lib/src/permissions/tests/permissions-cached.test.ts
- packages/lib/src/tests/permission-cache.test.ts
- packages/lib/src/permissions/tests/cache-trust-boundaries.test.ts
- scripts/test-security.sh
- packages/lib/src/services/permission-cache.ts
- packages/lib/src/permissions/permissions-cached.ts
Address review feedback on 15ff1de (PR 4/5 of the Redis-deprecation series) without changing scope: - Align /api/permissions/batch response shape — empty-pageIds branch now returns { total, accessible, denied, processingTimeMs } matching the non-empty branch; cacheHits field dropped. - Fix admin-acceptance divergence: every ADMIN lookup and drive membership predicate in permissions.ts now filters on acceptedAt IS NOT NULL, matching the invariant already enforced by getBatchPagePermissions. Applies to getUserAccessLevel, isDriveOwnerOrAdmin, isUserDriveMember, getUserAccessiblePagesInDrive, getUserAccessiblePagesInDriveWithDetails, getUserDriveAccess, and getUserDrivePermissions. Regression test added to batch-page-permissions.test.ts. - Drop bypassCache option from getUserAccessLevel, canUser*Page, and getUserDriveAccess now that there is no cache to bypass. Strip the option from 13 call sites across apps/web and apps/realtime plus their test assertions; rewrite the stale-window comment in apps/realtime/src/per-event-auth.ts for the direct-to-Postgres design. - Remove redundant export type { DrivePermissionLevel } from packages/lib/src/index.ts (already covered by export *). - Update architecture docs (2.0-architecture/2.2-backend/permissions.md and 2.3-shared/lib-package.md) to describe the single-module, direct-to-Postgres design. Prefix the old permission-cache threat model with a historical notice. Typecheck clean across @pagespace/lib, web, realtime, processor; vitest suites all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chet Addresses remaining review feedback on #1041 and fixes the failing Unit Tests check. - Add src/permissions/__tests__/drive-permissions.test.ts covering the getUserDrivePermissions function body (owner / accepted ADMIN / MEMBER / VIEWER / missing drive / unaccepted invite / silent-mode debug logs / DB error fail-closed / acceptedAt filter invariant) plus the getUserDriveAccess silent-mode debug branches. These functions live in permissions.ts after PR 4 collapsed the cached wrapper, but their mock-based unit tests had been deleted along with permissions-cached.test.ts — restoring coverage. - Mirror the test-level integration-test exclude list into the coverage `exclude` list in packages/lib/vitest.config.ts. Integration tests that never execute in the unit run (cross-tenant-escalation, permission-mutations, zero-trust-boundaries, …) were being counted as 0%-covered files, pulling the global functions metric below the 88% ratchet threshold. Global functions coverage now climbs from 87.51% → 88.52% with no real behavior change. - Make the slow-path warn test deterministic by spying on Date.now instead of busy-waiting >500ms in the mock — removes 0.5s of wall-clock time and the CI scheduler flakiness it introduced. - Assert loggers.api.error is called on getBatchPagePermissions DB failure — locks in the observability contract alongside the pre-seeded deny-map return. - Expand the POST /api/permissions/batch docblock to spell out that the permissions map only contains viewable entries, that denied pages are omitted and reflected in stats.denied, and that the empty-input branch returns the same stats shape (processingTimeMs: 0). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Review follow-ups — comprehensive resolutionAddressing every item from the CodeRabbit review (inline comments + outside-diff potential issues + nitpicks) plus the failing Unit Tests check. Two follow-up commits on the branch: 15034c5 and 0635dae. Code-change items (potential issues)
Nitpicks
CI
Docs
Verification |
Owner
Author
Wave-2 Cross-PR Coordination Audit — PR 4 sectionAuditor: point-guard orchestrator. Reviewer: Eric Elliott. Static checks — PASS
C3 — signature-collision resolutionMaster has both PR 4 picked the validated Every one of the six collapsed names ( C2 — cross-cutting callers verifiedAll of Composed integration — PASSLocal dry-merge
VerdictPASS — safe to merge as the third and final wave-2 PR (after #1043, #1042). |
3 of 4 tasks
10 tasks
4 tasks
This branch was previously deployed
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
Redis-deprecation PR 4 of 5. Collapses the hybrid L1 memory + L2 Redis permission cache into a single canonical
permissions.tsmodule backed by direct Postgres reads — every request is now a DB round-trip (1 for point lookups, 1 CTE for batches).packages/lib/src/services/permission-cache.ts(523 lines) andpackages/lib/src/permissions/permissions-cached.ts(618 lines); the exported surface moves verbatim intopermissions.ts.getBatchPagePermissionsbecomes a single Drizzle CTE joiningpages → drives → drive_members (ADMIN, acceptedAt IS NOT NULL) → page_permissions (unexpired)in one statement — no per-page fan-out. Trashed / expired / inaccessible pageIds resolve to all-false so callers can readmap.get(id)?.canViewsafely.invalidateUserPermissions/invalidateDrivePermissions/getPermissionCacheStatsand theGET /api/permissions/batchstats handler. Member invite/remove routes,permission-mutations.ts, andmemory-monitor.tslose their cache-invalidation calls.@pagespace/lib/permissions-cached→@pagespace/lib/permissionsinrealtime,processor, and their test mocks; drops the subpath frompackages/lib/package.jsonand the alias fromapps/processor/vitest.config.ts.See
tasks/redis-deprecation.md§ "PR 4" for the wave-level plan.Review follow-ups (commits
15034c5a,0635dae5)Post-initial-review improvements on top of the PR 4 spec:
permissions.ts(getUserAccessLevel,isDriveOwnerOrAdmin,isUserDriveMember,getUserAccessiblePagesInDrive,getUserAccessiblePagesInDriveWithDetails,getUserDriveAccess,getUserDrivePermissions) now filters onacceptedAt IS NOT NULL, matching the invariant already enforced bygetBatchPagePermissions. Single-page and batch paths can no longer disagree on unaccepted invites.bypassCacheoption dropped entirely fromgetUserAccessLevel,canUser{View,Edit,Share,Delete}Page, andgetUserDriveAccess. 13 call sites and 4 test assertions updated.POST /api/permissions/batchempty-pageIds early return now emits the same{ total, accessible, denied, processingTimeMs }shape as the non-empty branch (previously emitted stalecacheHits). Docblock rewritten to spell out that thepermissionsmap only contains viewable entries.docs/2.0-architecture/2.2-backend/permissions.mdanddocs/2.0-architecture/2.3-shared/lib-package.mdnow describe the single-module, direct-to-Postgres design.docs/security/permission-cache-threat-model.mdprefixed with a historical notice.packages/lib/src/permissions/__tests__/drive-permissions.test.ts(18 tests) exercisinggetUserDrivePermissionsandgetUserDriveAccesssilent-mode branches directly — their previous mock-based unit coverage lived in the deletedpermissions-cached.test.ts. Mirrored the test-level integration-testexcludelist into the coverageexcludelist inpackages/lib/vitest.config.tsso integration tests that never execute in the unit run stop counting as 0%-covered files. Global functions coverage climbs from 87.51% → 88.52%, clearing the 88% ratchet threshold.export type { DrivePermissionLevel }inpackages/lib/src/index.ts. Made the slow-path warn test deterministic viavi.spyOn(Date, 'now')(no more 500ms busy-wait on CI). Addedloggers.api.errorassertion to thegetBatchPagePermissionsDB-failure test to pin the observability contract.Pre-merge audit
Acceptance-criteria greps
Requirements checklist (per
tasks/redis-deprecation.md§ PR 4)permission-cache.ts(523 lines) — file removedpermissions-cached.ts(618 lines) — file removedgetUserAccessLevel,canUser{View,Edit,Share,Delete}Page,getUserDriveAccess,getUserDrivePermissions,getBatchPagePermissions,getDriveIdsForUser,isDriveOwnerOrAdmin,isUserDriveMember,PermissionLevel,DrivePermissionLevelall live atpackages/lib/src/permissions/permissions.ts.bypassCache?no-op shim dropped in follow-up — unused, not deferrable.getBatchPagePermissionssingle-CTE — onedb.select(...).from(pages).leftJoin(drives)...leftJoin(driveMembers)...leftJoin(pagePermissions)...where(inArray(...))— no per-page fan-out. Regression test assertsdb.selectis called exactly once and thatisNotNull('acceptedAt')is part of the predicate.permission-mutations.ts,members/invite/route.ts,members/[userId]/route.ts,memory-monitor.tsemergency cleanup all cleared.GET /api/permissions/batch— POST only;stats.accessiblenow derives fromcanView === true.@pagespace/lib/serverbarrel —server.tsre-exports from./permissions/permissions;packages/lib/package.jsondrops./permissions-cachedand./services/permission-cachesubpath exports + typesVersions.batch-page-permissions.test.ts+ newdrive-permissions.test.tscover owner, accepted ADMIN member, accepted MEMBER, VIEWER role, unaccepted invite fall-through, unexpired vs expired explicit grant, trashed page, inaccessible pageId, non-existent pageId, mixed batches, fail-closed on DB failure, single-round-trip guard rail, and the sharedisNotNull(acceptedAt)invariant.Pattern propagation
from '@pagespace/lib/permissions-cached'/from '../permissions/permissions-cached'→from '@pagespace/lib/permissions'/from '../permissions/permissions'index.ts,per-event-auth.ts, 3 realtime test files; processorrbac.ts,authorization.ts, 2 processor test files; repository + service source + tests)rg 'permissions-cached|permission-cache'inpackages+apps→ zero matchesDeferred items
Verification
Test plan
@pagespace/lib,web,realtime,processortest suites.pnpm test:securitygreen post-merge on staging (requires Postgres)./api/pages/[pageId],/api/inbox,/api/search— per the spec, index-tune Postgres if a hot path regresses (do NOT re-add the cache).🤖 Generated with Claude Code