Skip to content

Commit d1fc695

Browse files
committed
fix(auth): make org session policy and cookie-cache version cache-coherent
The session-policy enforcement added in #5862 split an org's `securityPolicyVersion` and `sessionPolicySettings` across two independent process-local caches. On a multi-replica deployment a pod could observe the bumped version while still holding the pre-save policy, and that skew silently undid a just-saved tightening: 1. Saving a policy eagerly clamps every member session in SQL. Shortening `expires_at` makes Better Auth's refresh predicate (`expiresAt - expiresIn + updateAge <= now`) unconditionally true, so the save itself marks the whole org due for refresh. 2. The version bump expires the session-data cookie, forcing the DB path. 3. The refresh proposes `now + 30d` and `session.update.before` re-clamps it — but against the stale policy, so it passed through unclamped. 4. With `expiresAt` back at 30 days the predicate is false again, so nothing re-clamped that session for another ~24h. Both fields live on the same `organization` row, so they are now read and cached as one record: a pod that still sees version N also serves policy N and forces no cookie mismatch, removing the skew by construction rather than patching one direction of it. Session CREATE additionally reads through the cache — unlike a refresh it is not already preceded by a version-mismatch read, so a stale record would otherwise mint a full-length session. Also in this change: - A failed entitlement read no longer disables enforcement. Stored bounds are only writable by an entitled org, so their presence is the source of truth when the plan check is unavailable; `resolveOrganizationEnterprisePlan` propagates errors so "downgraded" and "check failed" stay distinguishable. This matches the reasoning already documented on the PII-redaction path. - A failed org-record read serves the last known-good record instead of falling back to defaults, which would have dropped the org's bounds entirely. - A failed membership lookup in `session.create.before` no longer skips the clamp; it falls back to the cached membership. - Both caches are bounded (expired entries first, then least-recently-refreshed) — they previously grew one entry per distinct org/user for the process life. - The cookie-cache version fn short-circuits when organizations are disabled, so those deployments pay no lookup on the authenticated hot path. - Idle-timeout copy now states the real guarantee: activity is recorded at most once per 24h, so sign-out lands between (value - 24) and (value) hours after last use, never later. Tests: new coverage for the org-record collapse, last-known-good behavior, entitlement failure, the impersonation exemption, and the previously untested revoke route (authz branches, self-token and impersonator exclusions).
1 parent f43b52c commit d1fc695

12 files changed

Lines changed: 889 additions & 152 deletions

File tree

apps/docs/content/docs/en/platform/enterprise/session-policies.mdx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ Use this to enforce periodic re-authentication — for example, a value of `168`
2727

2828
### Idle timeout
2929

30-
Signs a member out after this many hours without activity. Activity extends the session, so members who use Sim regularly stay signed in; dormant sessions expire.
30+
Signs a member out after a period without activity. Activity extends the session, so members who use Sim regularly stay signed in; dormant sessions expire.
31+
32+
Activity is recorded at most once per 24 hours, so the actual sign-out lands between (value − 24) and (value) hours after a member's last use — never later. A value of `48` means a dormant session ends somewhere between 24 and 48 hours after last activity.
3133

3234
Accepts 48 to 8760 hours. The 48-hour minimum exists because session activity is recorded at most once per day — a shorter window could sign out members who are actively working.
3335

@@ -72,5 +74,10 @@ Use this after a security incident, an offboarding wave, or before tightening a
7274
answer:
7375
'Session activity is recorded at most once per 24 hours for performance. The minimum is twice that window so a member who stays active always registers activity before the idle limit can expire their session.',
7476
},
77+
{
78+
question: 'Why was a member signed out earlier than the idle timeout I set?',
79+
answer:
80+
'Because activity is recorded at most once per 24 hours, the last recorded activity can be up to a day older than a member\'s actual last use. Sign-out therefore lands between (value − 24) and (value) hours after they stop working. It never lands later than the value you set, so the policy is never exceeded.',
81+
},
7582
]}
7683
/>

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,10 @@ const { mockIsEnterprise, mockEagerClamp, mockRecordAudit } = vi.hoisted(() => (
2121

2222
vi.mock('@/lib/auth/session-policy', () => ({
2323
eagerClampOrgSessions: mockEagerClamp,
24-
invalidateSessionPolicyCache: vi.fn(),
2524
}))
2625

2726
vi.mock('@/lib/auth/security-policy', () => ({
28-
invalidateSecurityPolicyVersionCache: vi.fn(),
27+
invalidateOrgSecurityCache: vi.fn(),
2928
}))
3029

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

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ 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 { invalidateSecurityPolicyVersionCache } from '@/lib/auth/security-policy'
13-
import { eagerClampOrgSessions, invalidateSessionPolicyCache } from '@/lib/auth/session-policy'
12+
import { invalidateOrgSecurityCache } from '@/lib/auth/security-policy'
13+
import { eagerClampOrgSessions } from '@/lib/auth/session-policy'
1414
import { isOrganizationOnEnterprisePlan } from '@/lib/billing/core/subscription'
1515
import { isBillingEnabled } from '@/lib/core/config/env-flags'
1616
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -160,8 +160,7 @@ export const PUT = withRouteHandler(
160160
return NextResponse.json({ error: 'Organization not found' }, { status: 404 })
161161
}
162162

163-
invalidateSessionPolicyCache(organizationId)
164-
invalidateSecurityPolicyVersionCache(organizationId)
163+
invalidateOrgSecurityCache(organizationId)
165164

166165
logger.info('Updated organization session policy', { organizationId })
167166

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { member, organization, session as sessionTable } from '@sim/db/schema'
5+
import {
6+
authMockFns,
7+
createMockRequest,
8+
dbChainMockFns,
9+
queueTableRows,
10+
resetDbChainMock,
11+
resetEnvFlagsMock,
12+
setEnvFlags,
13+
} from '@sim/testing'
14+
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
15+
16+
const { mockIsEnterprise, mockRecordAudit, mockInvalidateOrgSecurityCache } = vi.hoisted(() => ({
17+
mockIsEnterprise: vi.fn(),
18+
mockRecordAudit: vi.fn(),
19+
mockInvalidateOrgSecurityCache: vi.fn(),
20+
}))
21+
22+
vi.mock('@/lib/auth/security-policy', () => ({
23+
invalidateOrgSecurityCache: mockInvalidateOrgSecurityCache,
24+
}))
25+
26+
vi.mock('@/lib/billing/core/subscription', () => ({
27+
isOrganizationOnEnterprisePlan: mockIsEnterprise,
28+
}))
29+
30+
vi.mock('@sim/audit', () => ({
31+
recordAudit: mockRecordAudit,
32+
AuditAction: { ORGANIZATION_SESSIONS_REVOKED: 'organization.sessions.revoked' },
33+
AuditResourceType: { ORGANIZATION: 'organization' },
34+
}))
35+
36+
import { POST } from '@/app/api/organizations/[id]/sessions/revoke/route'
37+
38+
const mockGetSession = authMockFns.mockGetSession
39+
40+
const ORG_ID = 'org-1'
41+
const CALLER_TOKEN = 'tok-caller'
42+
const routeContext = { params: Promise.resolve({ id: ORG_ID }) }
43+
44+
interface MockCondition {
45+
type: string
46+
left?: unknown
47+
right?: unknown
48+
column?: unknown
49+
conditions?: MockCondition[]
50+
}
51+
52+
/** Flattens the nested `and(...)` the route builds into a flat condition list. */
53+
function flattenConditions(condition: MockCondition): MockCondition[] {
54+
if (condition.type !== 'and') return [condition]
55+
return (condition.conditions ?? []).flatMap(flattenConditions)
56+
}
57+
58+
/**
59+
* The condition list passed to the session DELETE's `.where(...)`. Identified by
60+
* its `inArray` over `session.user_id` rather than by call position, since the
61+
* version-bump UPDATE issues its own `.where(...)` on the same shared spy.
62+
*/
63+
function deleteConditions(): MockCondition[] {
64+
expect(dbChainMockFns.delete).toHaveBeenCalledWith(sessionTable)
65+
const match = dbChainMockFns.where.mock.calls
66+
.map(([arg]) => flattenConditions(arg as MockCondition))
67+
.find((conditions) =>
68+
conditions.some(
69+
(condition) => condition.type === 'inArray' && condition.column === sessionTable.userId
70+
)
71+
)
72+
expect(match).toBeDefined()
73+
return match as MockCondition[]
74+
}
75+
76+
function authenticateAs(overrides: Record<string, unknown> = {}) {
77+
mockGetSession.mockResolvedValue({
78+
user: { id: 'user-1', name: 'Admin', email: 'admin@acme.dev' },
79+
session: { token: CALLER_TOKEN, ...overrides },
80+
})
81+
}
82+
83+
beforeAll(() => {
84+
setEnvFlags({ isBillingEnabled: true })
85+
})
86+
87+
afterAll(resetEnvFlagsMock)
88+
89+
describe('org sessions revoke route', () => {
90+
beforeEach(() => {
91+
vi.clearAllMocks()
92+
resetDbChainMock()
93+
authenticateAs()
94+
mockIsEnterprise.mockResolvedValue(true)
95+
})
96+
97+
describe('authorization', () => {
98+
it('returns 401 when unauthenticated', async () => {
99+
mockGetSession.mockResolvedValue(null)
100+
const response = await POST(createMockRequest('POST'), routeContext)
101+
expect(response.status).toBe(401)
102+
expect(dbChainMockFns.delete).not.toHaveBeenCalled()
103+
})
104+
105+
it('returns 403 for non-members', async () => {
106+
queueTableRows(member, [])
107+
const response = await POST(createMockRequest('POST'), routeContext)
108+
expect(response.status).toBe(403)
109+
expect(dbChainMockFns.delete).not.toHaveBeenCalled()
110+
})
111+
112+
it('returns 403 for members without an admin role', async () => {
113+
queueTableRows(member, [{ role: 'member' }])
114+
const response = await POST(createMockRequest('POST'), routeContext)
115+
expect(response.status).toBe(403)
116+
expect(dbChainMockFns.delete).not.toHaveBeenCalled()
117+
})
118+
119+
it('returns 403 for non-enterprise organizations', async () => {
120+
queueTableRows(member, [{ role: 'owner' }])
121+
mockIsEnterprise.mockResolvedValue(false)
122+
const response = await POST(createMockRequest('POST'), routeContext)
123+
expect(response.status).toBe(403)
124+
expect(dbChainMockFns.delete).not.toHaveBeenCalled()
125+
})
126+
127+
it('returns 404 when the organization does not exist', async () => {
128+
queueTableRows(member, [{ role: 'owner' }])
129+
queueTableRows(organization, [])
130+
const response = await POST(createMockRequest('POST'), routeContext)
131+
expect(response.status).toBe(404)
132+
expect(dbChainMockFns.delete).not.toHaveBeenCalled()
133+
})
134+
135+
it('allows admins as well as owners', async () => {
136+
queueTableRows(member, [{ role: 'admin' }])
137+
queueTableRows(organization, [{ name: 'Acme' }])
138+
dbChainMockFns.returning.mockResolvedValueOnce([{ id: 's-1' }])
139+
const response = await POST(createMockRequest('POST'), routeContext)
140+
expect(response.status).toBe(200)
141+
})
142+
})
143+
144+
describe('revocation', () => {
145+
beforeEach(() => {
146+
queueTableRows(member, [{ role: 'owner' }])
147+
queueTableRows(organization, [{ name: 'Acme' }])
148+
})
149+
150+
it('reports the revoked count, bumps the version, and invalidates the cache', async () => {
151+
dbChainMockFns.returning.mockResolvedValueOnce([{ id: 's-1' }, { id: 's-2' }])
152+
153+
const response = await POST(createMockRequest('POST'), routeContext)
154+
155+
expect(response.status).toBe(200)
156+
expect(await response.json()).toEqual({ success: true, data: { revokedSessions: 2 } })
157+
expect(dbChainMockFns.set).toHaveBeenCalledWith(
158+
expect.objectContaining({ securityPolicyVersion: expect.anything() })
159+
)
160+
expect(mockInvalidateOrgSecurityCache).toHaveBeenCalledWith(ORG_ID)
161+
expect(mockRecordAudit).toHaveBeenCalledWith(
162+
expect.objectContaining({
163+
action: 'organization.sessions.revoked',
164+
resourceId: ORG_ID,
165+
metadata: { revokedSessions: 2 },
166+
})
167+
)
168+
})
169+
170+
it("never deletes the caller's own session", async () => {
171+
dbChainMockFns.returning.mockResolvedValueOnce([])
172+
await POST(createMockRequest('POST'), routeContext)
173+
174+
expect(deleteConditions()).toContainEqual(
175+
expect.objectContaining({ type: 'ne', right: CALLER_TOKEN })
176+
)
177+
})
178+
179+
it('spares impersonation sessions', async () => {
180+
dbChainMockFns.returning.mockResolvedValueOnce([])
181+
await POST(createMockRequest('POST'), routeContext)
182+
183+
expect(deleteConditions()).toContainEqual(
184+
expect.objectContaining({ type: 'isNull', column: sessionTable.impersonatedBy })
185+
)
186+
})
187+
188+
it('scopes the delete to members of the organization', async () => {
189+
dbChainMockFns.returning.mockResolvedValueOnce([])
190+
await POST(createMockRequest('POST'), routeContext)
191+
192+
expect(deleteConditions()).toContainEqual(
193+
expect.objectContaining({ type: 'inArray', column: sessionTable.userId })
194+
)
195+
})
196+
197+
it("also spares the impersonator's own sessions when the caller is impersonating", async () => {
198+
authenticateAs({ impersonatedBy: 'platform-admin-1' })
199+
dbChainMockFns.returning.mockResolvedValueOnce([])
200+
201+
await POST(createMockRequest('POST'), routeContext)
202+
203+
expect(deleteConditions()).toContainEqual(
204+
expect.objectContaining({
205+
type: 'ne',
206+
left: sessionTable.userId,
207+
right: 'platform-admin-1',
208+
})
209+
)
210+
})
211+
212+
it('adds no impersonator exclusion for an ordinary caller', async () => {
213+
dbChainMockFns.returning.mockResolvedValueOnce([])
214+
await POST(createMockRequest('POST'), routeContext)
215+
216+
const userIdExclusions = deleteConditions().filter(
217+
(condition) => condition.type === 'ne' && condition.left === sessionTable.userId
218+
)
219+
expect(userIdExclusions).toHaveLength(0)
220+
})
221+
222+
it('singularizes the audit description for a single session', async () => {
223+
dbChainMockFns.returning.mockResolvedValueOnce([{ id: 's-1' }])
224+
await POST(createMockRequest('POST'), routeContext)
225+
226+
expect(mockRecordAudit).toHaveBeenCalledWith(
227+
expect.objectContaining({ description: 'Revoked 1 member session' })
228+
)
229+
})
230+
})
231+
})

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 { invalidateSecurityPolicyVersionCache } from '@/lib/auth/security-policy'
11+
import { invalidateOrgSecurityCache } 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-
invalidateSecurityPolicyVersionCache(organizationId)
108+
invalidateOrgSecurityCache(organizationId)
109109

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

apps/sim/ee/session-policy/components/session-policy-settings.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ export function SessionPolicySettings({ organizationId }: SessionPolicySettingsP
190190
<HourField
191191
id='idle-timeout-hours'
192192
title='Idle timeout (hours)'
193-
hint={`Sessions expire after this many hours without activity. Minimum ${MIN_IDLE_TIMEOUT_HOURS} hours.`}
193+
hint={`Sessions expire within this many hours of a member's last activity. Activity is recorded at most once per 24 hours, so members may be signed out up to a day earlier than the value you set — never later. Minimum ${MIN_IDLE_TIMEOUT_HOURS} hours.`}
194194
value={idleTimeoutHours}
195195
onChange={setIdleTimeoutHours}
196196
/>

apps/sim/lib/auth/auth.ts

Lines changed: 36 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -661,40 +661,54 @@ export const auth = betterAuth({
661661
}
662662
}
663663

664+
// Resolved separately from the clamp below: when this lookup fails we
665+
// still clamp (falling back to the cached membership) rather than
666+
// minting a full-length session off a transient read error.
667+
let membershipOrgId: string | null | undefined
664668
try {
665-
// Find the first organization this user is a member of
666669
const members = await db
667670
.select({ organizationId: schema.member.organizationId })
668671
.from(schema.member)
669672
.where(eq(schema.member.userId, session.userId))
670673
.limit(1)
671-
672-
if (members.length > 0) {
673-
logger.info('Found organization for user', {
674-
userId: session.userId,
675-
organizationId: members[0].organizationId,
676-
})
677-
678-
const expiresAt = await clampExpiryForSession(session, members[0].organizationId)
679-
680-
return {
681-
data: {
682-
...session,
683-
expiresAt,
684-
activeOrganizationId: members[0].organizationId,
685-
},
686-
}
687-
}
688-
logger.info('No organizations found for user', {
674+
membershipOrgId = members[0]?.organizationId ?? null
675+
logger.info(
676+
membershipOrgId ? 'Found organization for user' : 'No organizations found for user',
677+
{ userId: session.userId, organizationId: membershipOrgId ?? undefined }
678+
)
679+
} catch (error) {
680+
logger.error('Error resolving organization for new session; using cached membership', {
681+
error,
689682
userId: session.userId,
690683
})
691-
return { data: session }
684+
membershipOrgId = undefined
685+
}
686+
687+
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+
})
695+
return {
696+
data: {
697+
...session,
698+
expiresAt,
699+
...(membershipOrgId ? { activeOrganizationId: membershipOrgId } : {}),
700+
},
701+
}
692702
} catch (error) {
693-
logger.error('Error setting active organization', {
703+
logger.error('Error clamping new session to org policy', {
694704
error,
695705
userId: session.userId,
696706
})
697-
return { data: session }
707+
return {
708+
data: membershipOrgId
709+
? { ...session, activeOrganizationId: membershipOrgId }
710+
: session,
711+
}
698712
}
699713
},
700714
},

0 commit comments

Comments
 (0)