Skip to content

Commit d8f09bf

Browse files
committed
refactor(auth): read the session policy fresh instead of caching it
Follows the pattern the two sibling enterprise-settings call sites already establish. `lib/logs/execution/logger.ts` and `lib/workflows/executor/execution-core.ts` both resolve org enterprise settings with a single indexed read at enforcement time and carry explicit comments rejecting a cached/re-checked resolution, because a stale or failed read there silently skips the control. The session policy is the same shape of thing, and it is not a hot path: it is read on sign-in and on a sliding session refresh, which the 24h cookie cache limits to roughly once per user per day. Caching it bought nothing and cost the correctness bug this branch set out to fix — so drop the cache rather than build machinery to keep two caches coherent. Better Auth also documents `cookieCache.version` as a static string bumped at deploy and offers no cross-instance invalidation, so the version TTL is inherent to the design while the policy cache never was. This removes, rather than fixes, the whole class of problems the previous revision introduced: no merged record, no cache-bypass flag, no last-known-good fallback, no entitlement cache, no destructive-invalidation window, and no ordering hazard between concurrent readers. `security-policy.ts` is back to caching only the cookie-cache version — which genuinely is per-request — plus the membership lookup. Failure posture is now chosen per call site instead of globally fail-open: - `getSessionPolicy` and `getMemberOrganizationId` propagate read errors. - The session UPDATE hook refuses to EXTEND on a failed read, keeping the member's current expiry rather than granting a fresh 30 days the policy may forbid. - The session CREATE hook allows the sign-in but logs that the session went unclamped — refusing sign-ins would turn a read blip into an org-wide outage. - `getSessionCookieCacheVersion` still never throws; it runs on every request, and its fallback only costs a cookie mismatch and a database session read. Kept from the previous revision: the entry caps with low-water-mark eviction, the organizations-disabled short-circuit, the propagating enterprise-plan resolver, and the corrected idle-timeout copy.
1 parent 825959b commit d8f09bf

9 files changed

Lines changed: 212 additions & 330 deletions

File tree

apps/sim/app/api/organizations/[id]/session-policy/route.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ vi.mock('@/lib/auth/session-policy', () => ({
2424
}))
2525

2626
vi.mock('@/lib/auth/security-policy', () => ({
27-
invalidateOrgSecurityCache: vi.fn(),
27+
invalidateSecurityPolicyVersionCache: vi.fn(),
2828
}))
2929

3030
vi.mock('@/lib/billing/core/subscription', () => ({

apps/sim/app/api/organizations/[id]/session-policy/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { type NextRequest, NextResponse } from 'next/server'
99
import { updateOrganizationSessionPolicyContract } from '@/lib/api/contracts/organization'
1010
import { parseRequest, validationErrorResponse } from '@/lib/api/server'
1111
import { getSession } from '@/lib/auth'
12-
import { invalidateOrgSecurityCache } from '@/lib/auth/security-policy'
12+
import { invalidateSecurityPolicyVersionCache } from '@/lib/auth/security-policy'
1313
import { eagerClampOrgSessions } from '@/lib/auth/session-policy'
1414
import { isOrganizationOnEnterprisePlan } from '@/lib/billing/core/subscription'
1515
import { isBillingEnabled } from '@/lib/core/config/env-flags'
@@ -160,7 +160,7 @@ export const PUT = withRouteHandler(
160160
return NextResponse.json({ error: 'Organization not found' }, { status: 404 })
161161
}
162162

163-
invalidateOrgSecurityCache(organizationId)
163+
invalidateSecurityPolicyVersionCache(organizationId)
164164

165165
logger.info('Updated organization session policy', { organizationId })
166166

apps/sim/app/api/organizations/[id]/sessions/revoke/route.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@ import {
1313
} from '@sim/testing'
1414
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
1515

16-
const { mockIsEnterprise, mockRecordAudit, mockInvalidateOrgSecurityCache } = vi.hoisted(() => ({
16+
const { mockIsEnterprise, mockRecordAudit, mockInvalidateVersionCache } = vi.hoisted(() => ({
1717
mockIsEnterprise: vi.fn(),
1818
mockRecordAudit: vi.fn(),
19-
mockInvalidateOrgSecurityCache: vi.fn(),
19+
mockInvalidateVersionCache: vi.fn(),
2020
}))
2121

2222
vi.mock('@/lib/auth/security-policy', () => ({
23-
invalidateOrgSecurityCache: mockInvalidateOrgSecurityCache,
23+
invalidateSecurityPolicyVersionCache: mockInvalidateVersionCache,
2424
}))
2525

2626
vi.mock('@/lib/billing/core/subscription', () => ({
@@ -157,7 +157,7 @@ describe('org sessions revoke route', () => {
157157
expect(dbChainMockFns.set).toHaveBeenCalledWith(
158158
expect.objectContaining({ securityPolicyVersion: expect.anything() })
159159
)
160-
expect(mockInvalidateOrgSecurityCache).toHaveBeenCalledWith(ORG_ID)
160+
expect(mockInvalidateVersionCache).toHaveBeenCalledWith(ORG_ID)
161161
expect(mockRecordAudit).toHaveBeenCalledWith(
162162
expect.objectContaining({
163163
action: 'organization.sessions.revoked',

apps/sim/app/api/organizations/[id]/sessions/revoke/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { type NextRequest, NextResponse } from 'next/server'
88
import { revokeOrganizationSessionsContract } from '@/lib/api/contracts/organization'
99
import { parseRequest } from '@/lib/api/server'
1010
import { getSession } from '@/lib/auth'
11-
import { invalidateOrgSecurityCache } from '@/lib/auth/security-policy'
11+
import { invalidateSecurityPolicyVersionCache } from '@/lib/auth/security-policy'
1212
import { isOrganizationOnEnterprisePlan } from '@/lib/billing/core/subscription'
1313
import { isBillingEnabled } from '@/lib/core/config/env-flags'
1414
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -105,7 +105,7 @@ export const POST = withRouteHandler(
105105
.where(eq(organization.id, organizationId))
106106
return deleted
107107
})
108-
invalidateOrgSecurityCache(organizationId)
108+
invalidateSecurityPolicyVersionCache(organizationId)
109109

110110
logger.info('Revoked organization sessions', {
111111
organizationId,

apps/sim/lib/auth/auth.ts

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -685,13 +685,7 @@ export const auth = betterAuth({
685685
}
686686

687687
try {
688-
// Session creation is rare and, unlike a sliding refresh, is not
689-
// already preceded by a cookie-version mismatch read — so it must
690-
// read the policy fresh or a just-tightened policy could be missed
691-
// for the life of the session.
692-
const expiresAt = await clampExpiryForSession(session, membershipOrgId, {
693-
bypassPolicyCache: true,
694-
})
688+
const expiresAt = await clampExpiryForSession(session, membershipOrgId)
695689
return {
696690
data: {
697691
...session,
@@ -703,7 +697,10 @@ export const auth = betterAuth({
703697
},
704698
}
705699
} catch (error) {
706-
logger.error('Error clamping new session to org policy', {
700+
// Allow the sign-in: refusing it would turn a policy-read blip into
701+
// an org-wide outage. The session keeps Better Auth's default
702+
// expiry and the next refresh clamps it.
703+
logger.error('Error clamping new session to org policy; session not clamped', {
707704
error,
708705
userId: session.userId,
709706
})
@@ -728,11 +725,22 @@ export const auth = betterAuth({
728725
if (!data.expiresAt) return { data }
729726
const current = ctx?.context?.session?.session
730727
if (!current) return { data }
731-
const expiresAt = await clampExpiryForSession({
732-
...current,
733-
expiresAt: new Date(data.expiresAt),
734-
})
735-
return { data: { ...data, expiresAt } }
728+
try {
729+
const expiresAt = await clampExpiryForSession({
730+
...current,
731+
expiresAt: new Date(data.expiresAt),
732+
})
733+
return { data: { ...data, expiresAt } }
734+
} catch (error) {
735+
// Refuse to EXTEND when the policy cannot be read: keeping the
736+
// session's current expiry leaves the member signed in without
737+
// granting them a fresh 30 days a policy might have forbidden.
738+
logger.error('Error re-clamping refreshed session; leaving expiry unchanged', {
739+
error,
740+
userId: current.userId,
741+
})
742+
return { data: { ...data, expiresAt: current.expiresAt } }
743+
}
736744
},
737745
},
738746
},

apps/sim/lib/auth/security-policy.test.ts

Lines changed: 35 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,10 @@ import {
1212
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
1313
import {
1414
getMemberOrganizationId,
15-
getOrgSecurityRecord,
1615
getSecurityPolicyVersion,
1716
getSessionCookieCacheVersion,
1817
invalidateMembershipCache,
19-
invalidateOrgSecurityCache,
18+
invalidateSecurityPolicyVersionCache,
2019
} from '@/lib/auth/security-policy'
2120

2221
/** Module-level caches persist across cases, so every case uses a fresh id. */
@@ -33,104 +32,37 @@ describe('security policy', () => {
3332
setEnvFlags({ isOrganizationsEnabled: true })
3433
})
3534

36-
describe('getOrgSecurityRecord', () => {
37-
it('serves the version and the session policy from ONE row read', async () => {
35+
describe('getSecurityPolicyVersion', () => {
36+
it('caches the version and re-reads after invalidation', async () => {
3837
const orgId = nextOrgId()
39-
queueTableRows(organization, [
40-
{ version: 7, sessionPolicySettings: { maxSessionHours: 8, idleTimeoutHours: null } },
41-
])
38+
queueTableRows(organization, [{ version: 3 }])
4239

43-
const record = await getOrgSecurityRecord(orgId)
44-
45-
expect(record.version).toBe(7)
46-
expect(record.sessionPolicySettings).toEqual({ maxSessionHours: 8, idleTimeoutHours: null })
40+
expect(await getSecurityPolicyVersion(orgId)).toBe(3)
41+
expect(await getSecurityPolicyVersion(orgId)).toBe(3)
4742
expect(dbChainMockFns.select).toHaveBeenCalledTimes(1)
48-
})
4943

50-
it('reads the row once for a version lookup, then serves the policy from cache', async () => {
51-
const orgId = nextOrgId()
52-
queueTableRows(organization, [
53-
{ version: 3, sessionPolicySettings: { maxSessionHours: 12, idleTimeoutHours: null } },
54-
])
55-
56-
// This is the coherence guarantee: whatever version a caller observed,
57-
// the policy it goes on to read came from that same row. Two independent
58-
// caches previously allowed a bumped version to pair with a stale policy.
59-
const version = await getSecurityPolicyVersion(orgId)
60-
const { sessionPolicySettings } = await getOrgSecurityRecord(orgId)
61-
62-
expect(version).toBe(3)
63-
expect(sessionPolicySettings).toEqual({ maxSessionHours: 12, idleTimeoutHours: null })
64-
expect(dbChainMockFns.select).toHaveBeenCalledTimes(1)
44+
invalidateSecurityPolicyVersionCache(orgId)
45+
queueTableRows(organization, [{ version: 4 }])
46+
expect(await getSecurityPolicyVersion(orgId)).toBe(4)
6547
})
6648

67-
it('re-reads after invalidation', async () => {
68-
const orgId = nextOrgId()
69-
queueTableRows(organization, [{ version: 1, sessionPolicySettings: null }])
70-
await getOrgSecurityRecord(orgId)
71-
72-
invalidateOrgSecurityCache(orgId)
73-
queueTableRows(organization, [
74-
{ version: 2, sessionPolicySettings: { maxSessionHours: 4, idleTimeoutHours: null } },
75-
])
76-
77-
const record = await getOrgSecurityRecord(orgId)
78-
expect(record.version).toBe(2)
79-
expect(record.sessionPolicySettings).toEqual({ maxSessionHours: 4, idleTimeoutHours: null })
80-
expect(dbChainMockFns.select).toHaveBeenCalledTimes(2)
49+
it('returns the default without querying for an org-less session', async () => {
50+
expect(await getSecurityPolicyVersion(null)).toBe(1)
51+
expect(dbChainMockFns.select).not.toHaveBeenCalled()
8152
})
8253

83-
it('re-reads when the caller bypasses the cache', async () => {
84-
const orgId = nextOrgId()
85-
queueTableRows(organization, [{ version: 1, sessionPolicySettings: null }])
86-
await getOrgSecurityRecord(orgId)
87-
88-
queueTableRows(organization, [
89-
{ version: 2, sessionPolicySettings: { maxSessionHours: 6, idleTimeoutHours: null } },
90-
])
91-
const record = await getOrgSecurityRecord(orgId, { bypassCache: true })
92-
93-
expect(record.sessionPolicySettings).toEqual({ maxSessionHours: 6, idleTimeoutHours: null })
94-
expect(dbChainMockFns.select).toHaveBeenCalledTimes(2)
54+
it('defaults an unknown organization to version 1', async () => {
55+
queueTableRows(organization, [])
56+
expect(await getSecurityPolicyVersion(nextOrgId())).toBe(1)
9557
})
9658

97-
it('serves the last known-good record when a refresh fails', async () => {
98-
const orgId = nextOrgId()
99-
queueTableRows(organization, [
100-
{ version: 5, sessionPolicySettings: { maxSessionHours: 8, idleTimeoutHours: null } },
101-
])
102-
await getOrgSecurityRecord(orgId)
103-
104-
invalidateOrgSecurityCache(orgId)
105-
// A read failure must not silently drop the org's bounds — that would
106-
// disable a security control on a transient database blip.
107-
dbChainMockFns.limit.mockImplementationOnce(() => {
108-
throw new Error('connection reset')
109-
})
110-
111-
// Nothing cached for this org after invalidation, so it falls to defaults.
112-
const cold = await getOrgSecurityRecord(orgId)
113-
expect(cold).toEqual({ version: 1, sessionPolicySettings: null })
114-
115-
queueTableRows(organization, [
116-
{ version: 5, sessionPolicySettings: { maxSessionHours: 8, idleTimeoutHours: null } },
117-
])
118-
await getOrgSecurityRecord(orgId)
59+
it('falls back to the default when the read fails', async () => {
11960
dbChainMockFns.limit.mockImplementationOnce(() => {
12061
throw new Error('connection reset')
12162
})
122-
123-
const warm = await getOrgSecurityRecord(orgId, { bypassCache: true })
124-
expect(warm.version).toBe(5)
125-
expect(warm.sessionPolicySettings).toEqual({ maxSessionHours: 8, idleTimeoutHours: null })
126-
})
127-
128-
it('defaults an unknown organization to version 1 with no policy', async () => {
129-
queueTableRows(organization, [])
130-
expect(await getOrgSecurityRecord(nextOrgId())).toEqual({
131-
version: 1,
132-
sessionPolicySettings: null,
133-
})
63+
// Erring low only costs a cookie mismatch and a database session read —
64+
// it can never suppress a revocation the way a stale-high value would.
65+
expect(await getSecurityPolicyVersion(nextOrgId())).toBe(1)
13466
})
13567
})
13668

@@ -152,14 +84,21 @@ describe('security policy', () => {
15284
queueTableRows(member, [{ organizationId: 'org-b' }])
15385
expect(await getMemberOrganizationId(userId)).toBe('org-b')
15486
})
87+
88+
it('propagates a read failure so callers can tell it apart from "no org"', async () => {
89+
dbChainMockFns.limit.mockImplementationOnce(() => {
90+
throw new Error('connection reset')
91+
})
92+
await expect(getMemberOrganizationId(nextUserId())).rejects.toThrow('connection reset')
93+
})
15594
})
15695

15796
describe('getSessionCookieCacheVersion', () => {
15897
it('embeds the org id so two orgs never share a version string', async () => {
15998
const userId = nextUserId()
16099
const orgId = nextOrgId()
161100
queueTableRows(member, [{ organizationId: orgId }])
162-
queueTableRows(organization, [{ version: 4, sessionPolicySettings: null }])
101+
queueTableRows(organization, [{ version: 4 }])
163102

164103
expect(await getSessionCookieCacheVersion({ userId })).toBe(`${orgId}:4`)
165104
})
@@ -169,6 +108,14 @@ describe('security policy', () => {
169108
expect(await getSessionCookieCacheVersion({ userId: nextUserId() })).toBe('none')
170109
})
171110

111+
it('never throws on a failed membership read', async () => {
112+
dbChainMockFns.limit.mockImplementationOnce(() => {
113+
throw new Error('connection reset')
114+
})
115+
// Runs on every authenticated request — a throw here would 500 the app.
116+
expect(await getSessionCookieCacheVersion({ userId: nextUserId() })).toBe('none')
117+
})
118+
172119
it('costs no lookups when organizations are disabled for the deployment', async () => {
173120
setEnvFlags({ isOrganizationsEnabled: false })
174121
expect(await getSessionCookieCacheVersion({ userId: nextUserId() })).toBe('none')

0 commit comments

Comments
 (0)