Skip to content

Commit 3b76628

Browse files
committed
refactor(auth): drop LOC that did not earn its place
Line-by-line pass over the final diff. No behavior change; every item is either a comment that had drifted from the code or state the code did not need. - The create hook's comment claimed a failed membership read still clamps "via the cached membership". Since `getMemberOrganizationId` now throws, that only holds when the cache is warm. Say what the code actually does: `null` means confirmed org-less, `undefined` means could-not-tell, and the catch branches on which one it got. - `isPolicyEnforced(orgId, hasBounds)` took a boolean parameter that inverted its own name — it returned `true` for an org with no policy at all. Split into a plain early return for the no-bounds case and `isEntitledToEnforce`, which now only answers the question its name asks. - The revoke route aliased a transaction result to rename one field; destructure it instead. - The policy UPDATE returned `id` purely as an existence check that the version column it also returns already provides. - Two test names still described invalidating a cache that is now published to.
1 parent b467685 commit 3b76628

6 files changed

Lines changed: 20 additions & 24 deletions

File tree

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,7 @@ export const PUT = withRouteHandler(
150150
updatedAt: new Date(),
151151
})
152152
.where(eq(organization.id, organizationId))
153-
.returning({
154-
id: organization.id,
155-
securityPolicyVersion: organization.securityPolicyVersion,
156-
})
153+
.returning({ securityPolicyVersion: organization.securityPolicyVersion })
157154
if (!row) return null
158155
await eagerClampOrgSessions(organizationId, merged, tx)
159156
return row

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ describe('org sessions revoke route', () => {
149149
queueTableRows(organization, [{ name: 'Acme' }])
150150
})
151151

152-
it('reports the revoked count, bumps the version, and invalidates the cache', async () => {
152+
it('reports the revoked count, bumps the version, and publishes it', async () => {
153153
dbChainMockFns.returning
154154
.mockResolvedValueOnce([{ id: 's-1' }, { id: 's-2' }])
155155
.mockResolvedValueOnce([{ securityPolicyVersion: 5 }])

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ export const POST = withRouteHandler(
8181
// Delete and version bump commit atomically: a bump failure must roll the
8282
// delete back, or members would stay authenticated from the cookie cache
8383
// for up to 24h with their DB sessions already gone.
84-
const revokeResult = await db.transaction(async (tx) => {
84+
const { revoked, version } = await db.transaction(async (tx) => {
8585
const deleted = await tx
8686
.delete(sessionTable)
8787
.where(
@@ -104,12 +104,9 @@ export const POST = withRouteHandler(
104104
.set({ securityPolicyVersion: sql`${organization.securityPolicyVersion} + 1` })
105105
.where(eq(organization.id, organizationId))
106106
.returning({ securityPolicyVersion: organization.securityPolicyVersion })
107-
return { deleted, version: bumped?.securityPolicyVersion }
107+
return { revoked: deleted, version: bumped?.securityPolicyVersion }
108108
})
109-
const revoked = revokeResult.deleted
110-
if (revokeResult.version !== undefined) {
111-
setSecurityPolicyVersion(organizationId, revokeResult.version)
112-
}
109+
if (version !== undefined) setSecurityPolicyVersion(organizationId, version)
113110

114111
logger.info('Revoked organization sessions', {
115112
organizationId,

apps/sim/lib/auth/auth.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -661,9 +661,10 @@ 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.
664+
// Resolved separately from the clamp below so the two failures stay
665+
// distinguishable: `null` means "confirmed not in an org", while
666+
// `undefined` means "could not tell". The clamp retries through the
667+
// membership cache, and the catch below branches on which one it got.
667668
let membershipOrgId: string | null | undefined
668669
try {
669670
const members = await db
@@ -677,7 +678,7 @@ export const auth = betterAuth({
677678
{ userId: session.userId, organizationId: membershipOrgId ?? undefined }
678679
)
679680
} catch (error) {
680-
logger.error('Error resolving organization for new session; using cached membership', {
681+
logger.error('Could not resolve organization for new session', {
681682
error,
682683
userId: session.userId,
683684
})

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ describe('security policy', () => {
3333
})
3434

3535
describe('getSecurityPolicyVersion', () => {
36-
it('caches the version and re-reads after invalidation', async () => {
36+
it('caches the version and serves a newly published one without a re-read', async () => {
3737
const orgId = nextOrgId()
3838
queueTableRows(organization, [{ version: 3 }])
3939

apps/sim/lib/auth/session-policy.ts

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,10 @@ const NO_POLICY: ResolvedSessionPolicy = {
3030
* downgrade. `isOrganizationOnEnterprisePlan` swallows its errors and returns
3131
* `false`, which is correct for feature gating (fail closed) but would silently
3232
* stop ENFORCING here — the fail-open trap the PII-redaction path documents in
33-
* `lib/logs/execution/logger.ts`. Only reached when bounds are actually stored,
34-
* so non-enterprise orgs never pay for it.
33+
* `lib/logs/execution/logger.ts`. Callers only reach this when bounds are
34+
* actually stored, so non-enterprise orgs never pay for it.
3535
*/
36-
async function isPolicyEnforced(organizationId: string, hasBounds: boolean): Promise<boolean> {
37-
if (!hasBounds) return true
36+
async function isEntitledToEnforce(organizationId: string): Promise<boolean> {
3837
try {
3938
return await resolveOrganizationEnterprisePlan(organizationId)
4039
} catch (error) {
@@ -78,13 +77,15 @@ export async function getSessionPolicy(
7877
.limit(1)
7978

8079
const settings = row?.settings ?? {}
81-
const hasBounds = Boolean(settings.maxSessionHours || settings.idleTimeoutHours)
82-
if (!(await isPolicyEnforced(organizationId, hasBounds))) return NO_POLICY
83-
84-
return {
80+
const bounds: ResolvedSessionPolicy = {
8581
maxSessionHours: settings.maxSessionHours ?? null,
8682
idleTimeoutHours: settings.idleTimeoutHours ?? null,
8783
}
84+
85+
if (!bounds.maxSessionHours && !bounds.idleTimeoutHours) return NO_POLICY
86+
if (!(await isEntitledToEnforce(organizationId))) return NO_POLICY
87+
88+
return bounds
8889
}
8990

9091
/**

0 commit comments

Comments
 (0)