Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .changeset/forced-password-change-new-password.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
'@objectstack/plugin-auth': patch
'@object-ui/app-shell': patch
'@object-ui/i18n': patch
---

feat(plugin-auth): a forced password change now requires a password different from the current one

Clause-②: no (narrowing)
Metadata-protocol: none

<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, export or stored shape is removed, renamed or re-shaped, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is a credential door's accept set: `/change-password` and `/reset-password` stop accepting the current password as the new one while an administrator's forced change is outstanding, and the remedy is to choose a different password, never a rewrite of anyone's code or metadata. No exported declaration is removed or narrowed (not runtime-interface-only or type-surface-only); every bumped package publishes (not unpublished); no ADR-0087 id is named or touched (not registered or already-registered). -->

**BREAKING** (an accept-set narrowing), shipped as `patch`: the metadata protocol does not move, so the level is `patch` and this banner is the carrier.

When an administrator issues a password and asks for it to be changed at next sign-in (`sys_user.must_change_password`, set by Create User, Set Password and Import Users), the change now has to move the user to a new password.

**FROM.** With the forced-change flag set and the default `passwordHistoryCount` (0), a `/change-password` or `/reset-password` whose new password equals the current one completed and cleared the flag, so the user kept the password the administrator issued. With `passwordHistoryCount` above 0, a `/change-password` carrying a wrong current password and a recycled new one answered `400 PASSWORD_REUSE`.

**TO.** While the forced change is outstanding, `/change-password` and `/reset-password` refuse the current password as the new one with `400 PASSWORD_REUSE` ("Choose a new password that is different from your temporary password."), whatever `passwordHistoryCount` is, and the flag stays set until a different password is chosen. On `/change-password` a reuse refusal of either kind (the forced change's and the `passwordHistoryCount` ring's) is given only after the current password is confirmed: a request whose current password is wrong answers the endpoint's own `INVALID_PASSWORD` whatever the new password is, including a recycled one at a history depth above 0, which answered `PASSWORD_REUSE` before. A confirmed request reusing the current or a recent password still answers `PASSWORD_REUSE`. `/reset-password` carries no current password; its token is the authority, and the forced refusal applies to it directly.

**For clients.** A client that completes a forced change by sending the issued password back as the new one (an onboarding script, a test harness) must now send a different password.

Nothing else changes: with no forced change outstanding, which passwords count as reuse is still decided by `passwordHistoryCount` exactly as before. If the flag cannot be read, the request goes on as though no forced change were outstanding and the failure is logged through the auth manager's durability-degradation channel.

The Console's forced-change screen says the same thing in the user's language (all ten locale packs) when the new password matches the one typed as current, and sends no request.
12 changes: 10 additions & 2 deletions packages/plugins/plugin-auth/src/auth-manager-before-hook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,12 @@ export async function globalBeforeHook(
// change/reset only (sign-up has no prior history). Reuses
// better-auth's native password.verify — no bespoke crypto. Stashes
// the old hash so the after-hook appends it to the bounded ring on
// success.
// success. While an administrator's forced change is outstanding the
// current password is refused here whatever the history depth. The
// change door hands over its current-password claim, and the claim gate
// sits above the history ring as well as the forced change: no
// `PASSWORD_REUSE` of either kind is given until that claim is confirmed
// (see the method's docblock).
if (
candidate &&
(ctx?.path === '/reset-password' || ctx?.path === '/change-password')
Expand All @@ -189,7 +194,10 @@ export async function globalBeforeHook(
ctx.context.__osPwChangeUserId = userId;
const pw = ctx?.context?.password;
const verify = typeof pw?.verify === 'function' ? pw.verify.bind(pw) : undefined;
const oldHash = await this.assertPasswordNotReused(userId, candidate, verify);
const currentPassword = ctx?.path === '/change-password'
? (typeof ctx?.body?.currentPassword === 'string' ? ctx.body.currentPassword : '')
: undefined;
const oldHash = await this.assertPasswordNotReused(userId, candidate, verify, currentPassword);
if (oldHash !== undefined) ctx.context.__osPwHistory = { userId, oldHash };
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,18 +303,73 @@ export async function resolvePasswordChangeUserId(this: Pick<PasswordSessionPoli
return undefined;
}

/**
* The refusal a forced password change answers when the new password is the
* one the administrator issued. The code is `PASSWORD_REUSE` — the condition
* IS reuse of the current password — and the message names the case the user
* is in, which the history ring's generic message does not.
*/
export const FORCED_ROTATION_REUSE_MESSAGE =
'Choose a new password that is different from your temporary password.';

/**
* Is an administrator's forced password change (`sys_user.must_change_password`,
* issued by the admin create-user, set-user-password and import-users doors)
* outstanding for `userId`? Read as the system; the column is `readonly`, not
* `internal`, so the row carries it. Fails open (false) on a lookup error, as
* every read in `assertPasswordNotReused` does, but never silently: the
* failure is reported through the host's durability-degradation channel (the
* one `stampPasswordChangedAt` reports its write through), because the
* request then looks normal while a security control did not run.
*/
async function isForcedRotationPending(
host: Pick<PasswordSessionPolicyHost, 'logDurabilityDegradation'>,
engine: any,
userId: string,
context: unknown,
): Promise<boolean> {
try {
const u = await engine.findOne('sys_user', {
where: { id: userId },
fields: ['must_change_password'],
context,
});
return u?.must_change_password === true || u?.must_change_password === 1;
} catch (e) {
host.logDurabilityDegradation(
'AuthManager: the sys_user forced-change flag (must_change_password) could NOT be read '
+ 'for a password change or reset — the request goes on as though no forced change were '
+ 'outstanding, so the check that a forced change moves the user to a new password did not '
+ 'run for it, and nothing else looks wrong. Remedy: make the sys_user read succeed; the '
+ 'next change or reset reads the flag again.',
e,
{ object: 'sys_user', fields: ['must_change_password'] },
);
return false;
}
}

/** The body of `AuthManager#assertPasswordNotReused`; the method's docblock there is its contract. */
export async function assertPasswordNotReused(
this: Pick<PasswordSessionPolicyHost, 'config' | 'getDataEngine' | 'parseHashes'>,
this: Pick<PasswordSessionPolicyHost, 'config' | 'getDataEngine' | 'logDurabilityDegradation' | 'parseHashes'>,
userId: string,
candidate: string,
verify?: (data: { password: string; hash: string }) => Promise<boolean>,
currentPassword?: string,
): Promise<string | undefined> {
const count = Math.floor(Number(this.config.passwordHistoryCount) || 0);
if (count <= 0 || typeof verify !== 'function') return undefined;
if (typeof verify !== 'function') return undefined;
const engine = this.getDataEngine();
if (!engine) return undefined;
const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] };
// A forced change exists to move the user OFF a password the administrator
// knows, so while it is outstanding the current password is refused as the
// new one whatever `passwordHistoryCount` says (0, the default, included).
// Every door that clears the flag (`stampPasswordChangedAt`, from the
// after-hook on `/change-password` and `/reset-password`) passes through
// here first.
const forced = await isForcedRotationPending(this, engine, userId, SYSTEM_CTX);
if (count <= 0 && !forced) return undefined;
let account: any;
try {
account = await engine.findOne('sys_account', {
Expand All @@ -338,11 +393,29 @@ export async function assertPasswordNotReused(
}
if (!account?.id) return undefined;
const currentHash = typeof account.password === 'string' ? account.password : '';
const matches = async (password: string, hash: string) => {
try { return await verify({ password, hash }); } catch { return false; }
};
// `/change-password` carries the caller's claim of the current password, and
// better-auth confirms that claim only AFTER this hook. Every reuse refusal
// below — the forced change's and the history ring's — is given only once
// the current password is confirmed; an unconfirmed claim is left to the
// endpoint's own refusal (`INVALID_PASSWORD`). `/reset-password` carries no
// claim (`currentPassword` undefined): its token is the caller's authority.
const claimHolds = currentPassword === undefined
|| (!!currentHash && await matches(currentPassword, currentHash));
if (!claimHolds) return undefined;
if (forced && currentHash && await matches(candidate, currentHash)) {
const { APIError } = await import('better-auth/api');
throw new APIError('BAD_REQUEST', {
message: FORCED_ROTATION_REUSE_MESSAGE,
code: 'PASSWORD_REUSE',
});
}
if (count <= 0) return undefined;
const compareList = [currentHash, ...this.parseHashes(account.previous_password_hashes)].filter(Boolean);
for (const h of compareList) {
let match = false;
try { match = await verify({ password: candidate, hash: h }); } catch { match = false; }
if (match) {
if (await matches(candidate, h)) {
const { APIError } = await import('better-auth/api');
throw new APIError('BAD_REQUEST', {
message: `For security you can't reuse one of your last ${count} passwords. Please choose a different one.`,
Expand Down
26 changes: 24 additions & 2 deletions packages/plugins/plugin-auth/src/auth-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4382,11 +4382,33 @@ describe('AuthManager', () => {
expect((m as any).parseHashes('["a","b"]')).toEqual(['a', 'b']);
});

it('is a no-op when history depth is 0', async () => {
it('is a no-op when history depth is 0 and no forced change is outstanding', async () => {
const engine = makeEngine(acct());
const m = mgr(engine, { passwordHistoryCount: 0 });
await expect((m as any).assertPasswordNotReused('u1', 'current', stubVerify)).resolves.toBeUndefined();
expect(engine.findOne).not.toHaveBeenCalled();
// The one read is the forced-change flag on `sys_user`; with none
// outstanding the credential row is never read.
expect(engine.findOne.mock.calls.map((c: any[]) => c[0])).toEqual(['sys_user']);
});

it('reports a failed forced-change flag read through the durability channel and fails open', async () => {
const engine = makeEngine(acct());
engine.findOne.mockImplementation(async (o: string) => {
if (o === 'sys_user') throw new Error('sys_user read refused');
return null;
});
const logger = { warn: vi.fn(), info: vi.fn() };
const m = mgr(engine, { passwordHistoryCount: 0, logger });
logger.warn.mockClear();
// Fail open: treated as no forced change, so depth 0 accepts.
await expect((m as any).assertPasswordNotReused('u1', 'current', stubVerify)).resolves.toBeUndefined();
// ...but never silently: exactly one report naming the unread flag.
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.warn.mock.calls[0][1]).toMatchObject({
object: 'sys_user',
fields: ['must_change_password'],
error: 'sys_user read refused',
});
});

it('rejects reuse of the CURRENT password', async () => {
Expand Down
21 changes: 19 additions & 2 deletions packages/plugins/plugin-auth/src/auth-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5443,20 +5443,37 @@ export class AuthManager {
* native `password.verify` (passed in) rather than re-hashing. Returns the
* current hash (for the after-hook to append) when the candidate is fresh, or
* undefined when the feature is off / nothing to compare.
*
* While an administrator's forced change (`sys_user.must_change_password`)
* is outstanding, a candidate matching the CURRENT password is refused with
* `PASSWORD_REUSE` whatever `passwordHistoryCount` is, the default 0
* included: completing the change clears the flag, and it must not clear
* while the user keeps the password the administrator issued.
* `currentPassword` is the caller's claim of the current password on
* `/change-password` (`''` when the body carries none). The claim gate sits
* above BOTH reuse refusals, not only the forced change's: the history
* ring's `PASSWORD_REUSE` is also given only once the claim verifies against
* the current hash, so a wrong current password gets the endpoint's own
* `INVALID_PASSWORD` whatever the new password is. Omitted on a door that
* carries no claim (`/reset-password`, whose token is the authority). A
* failed read of the forced-change flag fails open (treated as not set) and
* is reported through `logDurabilityDegradation`.
*/
private assertPasswordNotReused(
userId: string,
candidate: string,
verify?: (data: { password: string; hash: string }) => Promise<boolean>,
currentPassword?: string,
): Promise<string | undefined> {
// [#25224 cut 3] The body moved to `auth-manager-password-session-policy.ts` (see `PasswordSessionPolicyHost`).
const readConfig = () => this.config;
const host: Pick<PasswordSessionPolicyHost, 'config' | 'getDataEngine' | 'parseHashes'> = {
const host: Pick<PasswordSessionPolicyHost, 'config' | 'getDataEngine' | 'logDurabilityDegradation' | 'parseHashes'> = {
get config() { return readConfig(); },
getDataEngine: (...a) => this.getDataEngine(...a),
logDurabilityDegradation: (...a) => this.logDurabilityDegradation(...a),
parseHashes: (...a) => this.parseHashes(...a),
};
return assertPasswordNotReused.call(host, userId, candidate, verify);
return assertPasswordNotReused.call(host, userId, candidate, verify, currentPassword);
}

/**
Expand Down
Loading
Loading