Skip to content

Commit 7303cbf

Browse files
claude[bot]claude
andauthored
fix(plugin-auth): SCIM active:false disables the account again — route the vendor's reconcileUser hook to the platform ban write (#14360) (#14540)
* fix(plugin-auth): route @better-auth/scim's identity.reconcileUser to the platform ban write SCIM active:false revoked sessions and wrote nothing on stable @better-auth/scim (the vendor's ban write left the package in 1.7.0); sys_user.banned was never set and a local-password user signed straight back in. Wire identity.reconcileUser into the scim() options and route it to the shared ban/unban write in admin-ban-endpoints.ts, inside the SCIM transaction; the engine-level last-administrator guard judges it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * test(plugin-auth): pin the refused-deactivation residual the dead SCIM transaction scope leaves (#14522) The (c) face keeps the account enabled and the SCIM 403 shape; the vendor's own scimUser.active write surviving the refusal is pinned as observed and attributed to the adapter's SCIM transaction scoping never engaging on 1.7.2, so the fix for that seam flips the pin deliberately. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * fix(plugin-auth): make a deactivated principal's expiring ban permanent; move the shared ban write off the barrel Contract review round 1 (#14360): a SCIM active:false over an administrator's TIMED ban left the expiry in place, and the vendor's session hook auto-lifts an expired ban - so the principal was re-admitted while the IdP still held them deactivated. The hook now clears banExpires on that row (reason and banned untouched). The shared write moves to the package-internal user-ban-write.ts (not re-exported from index.ts), so no new public symbol ships and the changeset drops to patch. Two faces added: the expiring-ban overlap and POST /Users with active:false. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7286dd5 commit 7303cbf

5 files changed

Lines changed: 913 additions & 10 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
SCIM `active: false` disables the account again. Stable `@better-auth/scim` (1.7.0+) no longer writes the admin plugin's `banned` column itself — it hands the aggregate lifecycle state to an optional host callback, `identity.reconcileUser`, and only revokes sessions. `plugin-auth` passed no `identity` member, so an identity provider deactivating a user revoked sessions and wrote nothing: `sys_user.banned` stayed false and a user holding a local password signed straight back in. `AuthManager` now implements the callback and routes it to the platform's own ban write: `active: false` bans the user (reason `Deactivated via SCIM`, no expiry) — and makes an administrator's existing EXPIRING ban permanent (`banExpires` cleared, the administrator's reason kept), because the vendor's session hook auto-lifts an expired ban and would otherwise admit a principal the identity provider still holds deactivated — and the vendor's `BANNED_USER` sign-in refusal applies; `POST /Users` with `active: false` provisions the account disabled. `active: true` lifts a ban that carries that reason — an administrator's ban (any other reason) is not the identity provider's to lift, so an attribute sync never re-admits a user banned for cause; the one documented collision is an administrator who types the reason `Deactivated via SCIM` themselves, which produces a ban the identity provider can lift. The last-LOCAL-credential guard the `/admin/ban-user` mount re-runs is deliberately not applied on the SCIM path: an identity-provider deprovision can disable the last password-holding account while non-administrator SSO users remain. The break-glass last-administrator guard (ADR-0024 D5.2) judges the write at the engine, so deactivating the last administrator through SCIM is refused with a 403 SCIM error and the account stays active. A SCIM `DELETE /Users/{id}` — which on 1.7.2 tombstones the source rather than deleting the user — now leaves that account disabled too. No new public symbol: the shared write lives in a package-internal module.

packages/plugins/plugin-auth/src/admin-ban-endpoints.ts

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,9 @@
5151
* The writes mirror better-auth's own handlers field for field — `banned` /
5252
* `banReason` / `banExpires` / `updatedAt`, then `deleteUserSessions` — so a
5353
* banned user is signed out and refused at sign-in by the vendor's OWN session
54-
* hook (`BANNED_USER`), which is untouched. The default ban reason is
54+
* hook (`BANNED_USER`), which is untouched. The write itself lives in the
55+
* package-internal `user-ban-write.ts` (#14360), shared with the SCIM
56+
* deprovisioning hook in `auth-manager.ts` — one write, two callers. The default ban reason is
5557
* `'No reason'` because ObjectStack configures no `defaultBanReason`.
5658
*
5759
* ⚠️ Shadowing a vendor route detaches every better-auth hook keyed on its
@@ -72,6 +74,7 @@ import {
7274
type CredentialAccountAdapter,
7375
} from './last-local-credential.js';
7476
import type { AdminActor, EndpointResult } from './admin-user-endpoints.js';
77+
import { applyUserBan, applyUserUnban } from './user-ban-write.js';
7578

7679
/**
7780
* Minimal better-auth `$context` surface these two routes touch. Mirrors what
@@ -161,11 +164,9 @@ export async function runAdminBanUser(
161164
};
162165
}
163166

164-
await ctx.internalAdapter.updateUser(userId, {
165-
banned: true,
167+
await applyUserBan(ctx.internalAdapter, userId, {
166168
banReason,
167169
...(banExpires ? { banExpires } : {}),
168-
updatedAt: new Date(),
169170
});
170171
// Sign the banned user out everywhere, exactly as the vendor handler does.
171172
await ctx.internalAdapter.deleteUserSessions(userId);
@@ -197,12 +198,7 @@ export async function runAdminUnbanUser(
197198
const ctx = await deps.getAuthContext();
198199
if (!(await ctx.internalAdapter.findUserById(userId))) return notFound();
199200

200-
await ctx.internalAdapter.updateUser(userId, {
201-
banned: false,
202-
banReason: null,
203-
banExpires: null,
204-
updatedAt: new Date(),
205-
});
201+
await applyUserUnban(ctx.internalAdapter, userId);
206202

207203
return { status: 200, body: { success: true, data: { userId, banned: false } } };
208204
}

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import type { Auth, BetterAuthOptions } from 'better-auth';
4+
import type { SCIMIdentityState, SCIMTransactionContext } from '@better-auth/scim';
45
// better-auth value imports (betterAuth + plugins) are deferred via dynamic
56
// import() in getOrCreateAuth() / buildPluginList() so that disabled plugins
67
// never get loaded into the process. See Stage 2F (RSS investigation).
@@ -101,6 +102,11 @@ import {
101102
LAST_LOCAL_CREDENTIAL_CODE,
102103
LAST_LOCAL_CREDENTIAL_MESSAGE,
103104
} from './last-local-credential.js';
105+
import {
106+
applyUserBan,
107+
applyUserUnban,
108+
SCIM_DEACTIVATION_BAN_REASON,
109+
} from './user-ban-write.js';
104110
import {
105111
PHONE_SMS_TOPICS,
106112
builtinPhoneSmsBody,
@@ -3276,6 +3282,16 @@ export class AuthManager {
32763282
return verifyScimBearerToken(engine as never, secret, input.token);
32773283
},
32783284
},
3285+
// [#14360] The host half of `active`: stable @better-auth/scim
3286+
// writes no `banned` itself any more (the 1.6.x coupling left the
3287+
// package in 1.7.0) — it hands the aggregate lifecycle state to
3288+
// this callback inside the SCIM transaction and only revokes
3289+
// sessions. Routed to the platform's own ban write; the break-glass
3290+
// last-administrator guard judges it at the engine. See
3291+
// `reconcileScimUserLifecycle` for the contract and the measurement.
3292+
identity: {
3293+
reconcileUser: (state, context) => this.reconcileScimUserLifecycle(state, context),
3294+
},
32793295
});
32803296
});
32813297
}
@@ -4840,6 +4856,140 @@ export class AuthManager {
48404856
return auth.api;
48414857
}
48424858

4859+
/**
4860+
* [#14360] `identity.reconcileUser` — the host half of SCIM `active`.
4861+
*
4862+
* `@better-auth/scim` 1.7.0 removed its own `banned` write (1.6.30 mapped
4863+
* `active` onto the admin plugin's ban and refused a deactivation without
4864+
* that plugin; on the installed 1.7.2 the substring `ban` occurs zero times
4865+
* in the package) and replaced it with this optional callback: the vendor
4866+
* computes the user's AGGREGATE lifecycle state — `active` is true while
4867+
* any participating SCIM source says so — inside the request's
4868+
* transaction, calls the host, and then revokes the user's sessions when
4869+
* the state is inactive (`dist/index.mjs`, the identity facade's
4870+
* `reconcileUser`). Without a host implementation an IdP's `active: false`
4871+
* revoked sessions and wrote nothing: `sys_user.banned` stayed false and a
4872+
* local-password user signed straight back in, while ADR-0071, the
4873+
* generated docs and the #13816 refusal all asserted the ban.
4874+
*
4875+
* This method restores declared = enforced by routing the state to the
4876+
* platform's OWN ban write (`admin-ban-endpoints.ts`):
4877+
*
4878+
* - `active: false` on a row that is not banned ⇒ `applyUserBan` with
4879+
* `SCIM_DEACTIVATION_BAN_REASON` and no expiry. The vendor's
4880+
* `session.create` hook (`BANNED_USER`) then refuses sign-in — the same
4881+
* enforcement the admin ban has, because it is the same write. On a row
4882+
* that is ALREADY banned with an expiry (an administrator's timed ban),
4883+
* the deactivation makes that ban permanent — `banExpires` is cleared,
4884+
* `banned` and the administrator's reason are left untouched — because
4885+
* the vendor's session hook auto-lifts an expired ban and would admit a
4886+
* principal the IdP still holds deactivated, and this callback is not
4887+
* re-invoked until the IdP mutates that user again.
4888+
* - `active: true` on a row banned WITH that reason ⇒ `applyUserUnban`.
4889+
* A ban carrying any other reason was placed by an administrator and is
4890+
* not the IdP's to lift: an attribute sync (every SCIM PUT carries
4891+
* `active: true`) must not silently re-admit a user banned for cause.
4892+
* Known collision, documented rather than reserved: an administrator
4893+
* who types the reason `Deactivated via SCIM` on the admin mount
4894+
* produces a ban this rule reads as the IdP's, so an `active: true`
4895+
* lifts it. Reserving the string on the admin mount would change that
4896+
* surface, which is not this hook's to do.
4897+
* - Anything else is a no-op. The callback is contractually idempotent
4898+
* ("Implementations must be idempotent") and the vendor invokes it on
4899+
* EVERY user mutation, so a PATCH that changes only `displayName`
4900+
* touches no ban column.
4901+
*
4902+
* A consequence worth stating: on 1.7.2 a SCIM `DELETE /Users/{id}` no
4903+
* longer deletes the better-auth user (the vendor tombstones the source);
4904+
* it leaves the user with no active source, so this callback disables the
4905+
* account. Re-provisioning through the tombstone re-links the same user,
4906+
* the state turns active, and the SCIM ban is lifted by the second bullet.
4907+
*
4908+
* The break-glass last-administrator guard (ADR-0024 D5.2, #5892) is an
4909+
* ENGINE `beforeUpdate` hook on `sys_user`, so it judges this write exactly
4910+
* as it judges the admin mount's: deactivating the last administrator
4911+
* throws its 403 `PERMISSION_DENIED`, the adapter rethrows it as an
4912+
* `APIError`, the vendor re-throws `APIError`s unchanged out of this
4913+
* callback (`runSCIMApplicationCallback`, measured on 1.7.2 — any other
4914+
* throw becomes a SCIM 500 "SCIM identity reconciliation failed" carrying
4915+
* the original as `cause`), and the IdP receives a SCIM error with
4916+
* `status: "403"` and the guard's own explanation. The ban is ONE write,
4917+
* so it never half-lands: the account stays enabled and nothing is
4918+
* skipped silently.
4919+
*
4920+
* ⚠️ What does NOT roll back today: the vendor runs this callback inside
4921+
* `runWithTransaction`, which on this adapter is a real engine transaction
4922+
* only while `scimRequestScope` is set — and that scope, stamped inside
4923+
* `verifyBearerToken`, is not observed at write time on 1.7.2 (measured:
4924+
* zero `engine.transaction` calls across a SCIM POST + PATCH; #14522). So
4925+
* the vendor's own `scimUser.active = false` write, made before this
4926+
* callback, survives a refusal and the SCIM resource reads inactive while
4927+
* the account is enabled. #14522 owns that seam; the #14360 suite pins the
4928+
* residual so its fix flips the pin deliberately.
4929+
*
4930+
* Deliberately NOT applied here: the last-LOCAL-credential guard the admin
4931+
* mount re-runs (`isLastLocalCredentialHolder`). That guard protects the
4932+
* password escape hatch from an administrator's click; on this path the
4933+
* identity provider is the authority for the user it deprovisions, and
4934+
* keeping a departed user's password alive because it happened to be the
4935+
* last one is the wrong direction for a deprovisioning contract. 1.6.x
4936+
* never applied it on the SCIM path either — the vendor wrote the column
4937+
* straight through the adapter.
4938+
*
4939+
* Every read and write goes through `context.database` — the adapter the
4940+
* vendor bound to its transaction — never through an `internalAdapter`
4941+
* resolved outside it, so the moment #14522 makes that transaction real,
4942+
* the ban commits or rolls back with the SCIM mutation it belongs to.
4943+
*/
4944+
private async reconcileScimUserLifecycle(
4945+
state: SCIMIdentityState,
4946+
context: SCIMTransactionContext,
4947+
): Promise<void> {
4948+
const db = context.database;
4949+
const user = await db.findOne<{ banned?: unknown; banReason?: unknown; banExpires?: unknown }>({
4950+
model: 'user',
4951+
where: [{ field: 'id', value: state.userId }],
4952+
});
4953+
if (!user) {
4954+
// The vendor holds a `scimSubject` for this user inside the same
4955+
// transaction, so a missing row is an invariant break, not a state to
4956+
// reconcile. Thrown, not logged: the vendor turns it into a SCIM 500
4957+
// and rolls the mutation back — a deactivation that cannot find its
4958+
// account must not report success.
4959+
throw new Error(
4960+
`[auth] SCIM identity reconciliation: better-auth user '${state.userId}' has no sys_user row`,
4961+
);
4962+
}
4963+
const writer = {
4964+
updateUser: (id: string, data: Record<string, unknown>) =>
4965+
db.update({ model: 'user', where: [{ field: 'id', value: id }], update: data }),
4966+
};
4967+
const banned = user.banned === true;
4968+
if (!state.active) {
4969+
if (banned) {
4970+
// Already disabled — by an earlier SCIM pass or by an administrator.
4971+
// An administrator's TIMED ban is made permanent: the vendor's session
4972+
// hook auto-lifts an expired ban, and nothing re-invokes this callback
4973+
// until the IdP mutates the user again — so left alone, the expiry
4974+
// would re-admit a principal the IdP still holds deactivated. The
4975+
// reason stays the administrator's; only the expiry goes.
4976+
if (user.banExpires !== null && user.banExpires !== undefined) {
4977+
await writer.updateUser(state.userId, { banExpires: null, updatedAt: new Date() });
4978+
}
4979+
return;
4980+
}
4981+
await applyUserBan(writer, state.userId, {
4982+
banReason: SCIM_DEACTIVATION_BAN_REASON,
4983+
banExpires: null,
4984+
});
4985+
return;
4986+
}
4987+
if (!banned) return;
4988+
// An administrator's ban is not the IdP's to lift.
4989+
if (user.banReason !== SCIM_DEACTIVATION_BAN_REASON) return;
4990+
await applyUserUnban(writer, state.userId);
4991+
}
4992+
48434993
/**
48444994
* Get the underlying better-auth context for low-level operations such as
48454995
* `internalAdapter.createAccount` / `password.hash`.

0 commit comments

Comments
 (0)