address comments

Author: icecrasher321

apps/realtime/src/middleware/permissions.test.ts

@@ -365,12 +365,52 @@ describe('checkWorkflowOperationPermission', () => {
     }
   })
 
-  it('falls back to the last known role on a transient DB error', async () => {
+  it('falls back to the join-time role on a transient DB error when nothing is cached yet', async () => {
     mockAuthorize.mockRejectedValue(new Error('db unavailable'))
 
     const result = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'write')
 
     expect(result.allowed).toBe(true)
     expect(result.role).toBe('write')
   })
+
+  it('preserves a recorded revocation through a later transient DB error', async () => {
+    vi.useFakeTimers()
+    try {
+      // First check records the revocation (null) in the cache
+      mockAuthorize.mockResolvedValue({ allowed: false, workspacePermission: null })
+      const first = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'admin')
+      expect(first.allowed).toBe(false)
+      expect(first.role).toBeNull()
+
+      // TTL expires, then the DB blips on the next re-validation. The stale join-time
+      // role ('admin') must NOT resurrect access — the recorded revocation wins.
+      vi.advanceTimersByTime(31_000)
+      mockAuthorize.mockRejectedValue(new Error('db unavailable'))
+
+      const second = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'admin')
+      expect(second.allowed).toBe(false)
+      expect(second.role).toBeNull()
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
+  it('uses the last cached role (not the join-time role) on a transient DB error', async () => {
+    vi.useFakeTimers()
+    try {
+      mockAuthorize.mockResolvedValue({ allowed: true, workspacePermission: 'write' })
+      await checkWorkflowOperationPermission(userId, workflowId, 'update', 'read')
+
+      vi.advanceTimersByTime(31_000)
+      mockAuthorize.mockRejectedValue(new Error('db unavailable'))
+
+      // fallbackRole is 'read', but the last recorded decision was 'write' — use that
+      const result = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'read')
+      expect(result.allowed).toBe(true)
+      expect(result.role).toBe('write')
+    } finally {
+      vi.useRealTimers()
+    }
+  })
 })

apps/realtime/src/middleware/permissions.ts

@@ -122,8 +122,10 @@ function purgeExpiredRoles(now: number): void {
  * table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod.
  *
  * Returns `null` when the user genuinely has no access (removed/revoked). On a transient
- * DB failure it falls back to `fallbackRole` so a blip does not block legitimate editors
- * — the subsequent persist would fail on its own if the database is truly unavailable.
+ * DB failure it reuses the last recorded decision for this (user, workflow) — including a
+ * previously recorded revocation (`null`) — and only falls back to `fallbackRole` when no
+ * decision has been recorded yet, so a blip neither blocks legitimate editors nor
+ * resurrects already-revoked access.
  */
 export async function resolveCurrentWorkflowRole(
   userId: string,
@@ -154,7 +156,12 @@ export async function resolveCurrentWorkflowRole(
       `Failed to re-validate role for user ${userId} on workflow ${workflowId}; using last known role`,
       error
     )
-    return fallbackRole
+    // Prefer the last recorded decision — even if expired, and even if it is `null` for an
+    // already-revoked user — so a recorded revocation survives a transient DB failure
+    // instead of reverting to the stale join-time role. Only trust `fallbackRole` when
+    // nothing has been recorded for this (user, workflow) yet.
+    const lastKnown = roleCache.get(key)
+    return lastKnown !== undefined ? lastKnown.role : fallbackRole
   }
 }