fix(plugin-auth): a correct-password sign-in refused by an organization IP list no longer counts toward the account lockout

.changeset/31251-org-ip-refusal-lockout-counter.md

@@ -0,0 +1,10 @@
+---
+'@objectstack/plugin-auth': patch
+---
+
+fix(plugin-auth): a correct-password sign-in refused afterwards, by an organization's allowed IP ranges, a ban or an unverified address, no longer counts toward the account lockout
+
+Clause-②: no
+Metadata-protocol: none
+
+The ADR-0069 D2 lockout counter read every refused `/sign-in/email` as a wrong password. Three refusals come only after the password verified: an organization's own IP list (`403 IP_NOT_ALLOWED`, judged when the new session is scoped to it), a ban (`403 BANNED_USER`, judged when the session is created) and an address the deployment requires verified (`403 EMAIL_NOT_VERIFIED`). Each raised the count, so an account holder with the right password could lock their own account: a member signing in from outside the organization's network was then refused `ACCOUNT_LOCKED` from inside it, a lifted ban left a lock behind, and a person waiting for their verification mail was locked out once it arrived. These three refusals now leave the counter as it stands, neither counting nor clearing it. A wrong password still counts in every case, and a successful sign-in still clears the count. The refusals' status, code and message and their `login_failed` audit rows are unchanged.

packages/plugins/plugin-auth/src/account-lockout-resolved-account.test.ts

@@ -21,6 +21,11 @@
 // The account events (`login_failed` / `account_locked`) name the account the
 // same resolution names, so the ledger and the lock never disagree.
 //
+// Only a refused CREDENTIAL moves the counter. A sign-in whose password passed
+// and that a policy then refused (the organization's allowed IP ranges, judged
+// when the session is scoped to it; a ban; an address the deployment requires
+// verified) neither counts nor clears: the last two `describe`s below.
+//
 // ## Why the server here is the real one
 //
 // "Which account does this identifier name" is answered by the credential
@@ -56,7 +61,8 @@ const PASSWORD = 'Corr3ct!Horse-Battery';
 const WRONG_PASSWORD = 'Wr0ng!Guess-Zebra-7731';
 const THRESHOLD = 3;
 
-function deployment() {
+/** `extra` is merged into the manager's config (a plugin switch, an e-mail rule). */
+function deployment(extra: Record<string, unknown> = {}) {
   const engine = createMemoryEngine();
   const events: AuthSessionAuditEventInput[] = [];
   const manager = new AuthManager({
@@ -65,6 +71,7 @@ function deployment() {
     dataEngine: engine,
     lockoutThreshold: THRESHOLD,
     lockoutDurationMinutes: 15,
+    ...extra,
     getAuditSink: () => ({
       recordAuthEvent: async (event: AuthSessionAuditEventInput) => {
         events.push(event);
@@ -74,13 +81,14 @@ function deployment() {
   return { engine, manager, events };
 }
 
-const post = (manager: AuthManager, path: string, body: unknown) =>
+const post = (manager: AuthManager, path: string, body: unknown, remoteAddress?: string) =>
   manager.handleRequest(
     new Request(`${BASE}${path}`, {
       method: 'POST',
       headers: { 'Content-Type': 'application/json' },
       body: JSON.stringify(body),
     }),
+    remoteAddress ? { remoteAddress } : undefined,
   );
 
 async function signUp(manager: AuthManager): Promise<void> {
@@ -226,3 +234,128 @@ describe('ADR-0069 D2: the lockout counts and holds the account the credential c
     expect((await signIn(manager, EMAIL, PASSWORD)).status).toBe(200);
   });
 });
+
+/** The organization the account belongs to, and the one network its own list admits. */
+const ORGANIZATION = 'org_lockout_policy';
+const INSIDE = '10.1.0.7';
+const OUTSIDE = '10.2.0.7';
+const NOT_ADMITTED = { status: 403, code: 'IP_NOT_ALLOWED' };
+
+/**
+ * {@link deployment}, with the account a member of an organization whose own
+ * allowed IP ranges admit only 10.1.0.0/16. The deployment sets no list of its
+ * own, so the organization's list is judged where the sign-in's session is
+ * scoped to it — after the password passed.
+ */
+async function organizationListDeployment() {
+  const { engine, manager, events } = deployment();
+  await signUp(manager);
+  const userId = String(accountRow(engine).id);
+  // Whatever membership the sign-up settled is replaced by the one under test.
+  engine.tables.set('sys_member', (engine.tables.get('sys_member') ?? []).filter((m: any) => m.user_id !== userId));
+  await engine.insert('sys_member', { user_id: userId, organization_id: ORGANIZATION, role: 'member', created_at: new Date() });
+  manager.setOrganizationAccessPolicySource({
+    read: async (id) => (id === ORGANIZATION ? { allowed_ip_ranges: '10.1.0.0/16' } : {}),
+    anyOrganizationRequiresMfa: async () => false,
+  });
+  return { engine, manager, events };
+}
+
+const signInFrom = (manager: AuthManager, password: string, remoteAddress: string) =>
+  post(manager, '/sign-in/email', { email: EMAIL, password }, remoteAddress);
+
+describe('ADR-0069 D2: a correct password the organization\'s IP list refuses neither counts nor clears', () => {
+  it('refusals past the threshold lock nothing, and the account signs in from inside the list', async () => {
+    const { engine, manager, events } = await organizationListDeployment();
+    events.length = 0;
+
+    for (let i = 0; i < THRESHOLD; i++) {
+      expect(await outcome(await signInFrom(manager, PASSWORD, OUTSIDE)), `attempt ${i + 1}`).toEqual(NOT_ADMITTED);
+      expect(Number(accountRow(engine).failed_login_count ?? 0), `attempt ${i + 1}`).toBe(0);
+    }
+    expect(isLocked(accountRow(engine))).toBe(false);
+    expect(events.some((e) => e.action === 'account_locked')).toBe(false);
+
+    const home = await signInFrom(manager, PASSWORD, INSIDE);
+    expect(home.status, await home.clone().text()).toBe(200);
+    expect(Number(accountRow(engine).failed_login_count ?? 0)).toBe(0);
+  });
+
+  it('the refusal leaves a count earned by wrong passwords where it stands', async () => {
+    const { engine, manager } = await organizationListDeployment();
+    for (let i = 0; i < THRESHOLD - 1; i++) {
+      expect(await outcome(await signInFrom(manager, WRONG_PASSWORD, INSIDE))).toEqual(REFUSED);
+    }
+    expect(Number(accountRow(engine).failed_login_count)).toBe(THRESHOLD - 1);
+
+    expect(await outcome(await signInFrom(manager, PASSWORD, OUTSIDE))).toEqual(NOT_ADMITTED);
+    expect(Number(accountRow(engine).failed_login_count)).toBe(THRESHOLD - 1);
+    expect(isLocked(accountRow(engine))).toBe(false);
+  });
+
+  it('a wrong password counts from either side of the list, and a correct one from inside clears', async () => {
+    const { engine, manager } = await organizationListDeployment();
+
+    // From outside the credential refuses first: the list is judged only once a session is created.
+    expect(await outcome(await signInFrom(manager, WRONG_PASSWORD, OUTSIDE))).toEqual(REFUSED);
+    expect(Number(accountRow(engine).failed_login_count)).toBe(1);
+    expect(await outcome(await signInFrom(manager, WRONG_PASSWORD, INSIDE))).toEqual(REFUSED);
+    expect(Number(accountRow(engine).failed_login_count)).toBe(2);
+
+    expect((await signInFrom(manager, PASSWORD, INSIDE)).status).toBe(200);
+    expect(Number(accountRow(engine).failed_login_count)).toBe(0);
+
+    for (let i = 0; i < THRESHOLD; i++) {
+      expect(await outcome(await signInFrom(manager, WRONG_PASSWORD, OUTSIDE))).toEqual(REFUSED);
+    }
+    expect(isLocked(accountRow(engine))).toBe(true);
+    expect(await outcome(await signInFrom(manager, PASSWORD, INSIDE))).toEqual(LOCKED);
+  });
+});
+
+describe('ADR-0069 D2: a correct password refused for a ban or an unverified address neither counts nor clears', () => {
+  it('a banned account: wrong passwords count, the correct one is refused BANNED_USER and moves nothing', async () => {
+    const { engine, manager, events } = deployment({ plugins: { admin: true } });
+    await signUp(manager);
+    accountRow(engine).banned = true;
+    for (let i = 0; i < THRESHOLD - 1; i++) {
+      expect(await outcome(await signIn(manager, EMAIL, WRONG_PASSWORD))).toEqual(REFUSED);
+    }
+    expect(Number(accountRow(engine).failed_login_count)).toBe(THRESHOLD - 1);
+    events.length = 0;
+
+    for (let i = 0; i < THRESHOLD; i++) {
+      expect(await outcome(await signIn(manager, EMAIL, PASSWORD)), `attempt ${i + 1}`)
+        .toEqual({ status: 403, code: 'BANNED_USER' });
+      expect(Number(accountRow(engine).failed_login_count), `attempt ${i + 1}`).toBe(THRESHOLD - 1);
+    }
+    expect(isLocked(accountRow(engine))).toBe(false);
+    expect(events.some((e) => e.action === 'account_locked')).toBe(false);
+
+    // Lifted, the ban leaves no lock behind: the correct password signs in and clears the count.
+    accountRow(engine).banned = false;
+    expect((await signIn(manager, EMAIL, PASSWORD)).status).toBe(200);
+    expect(Number(accountRow(engine).failed_login_count)).toBe(0);
+  });
+
+  it('an unverified address: wrong passwords count, the correct one is refused EMAIL_NOT_VERIFIED and moves nothing', async () => {
+    const { engine, manager } = deployment({ emailAndPassword: { enabled: true, requireEmailVerification: true } });
+    await signUp(manager);
+    accountRow(engine).email_verified = false;
+    for (let i = 0; i < THRESHOLD - 1; i++) {
+      expect(await outcome(await signIn(manager, EMAIL, WRONG_PASSWORD))).toEqual(REFUSED);
+    }
+    expect(Number(accountRow(engine).failed_login_count)).toBe(THRESHOLD - 1);
+
+    for (let i = 0; i < THRESHOLD; i++) {
+      expect(await outcome(await signIn(manager, EMAIL, PASSWORD)), `attempt ${i + 1}`)
+        .toEqual({ status: 403, code: 'EMAIL_NOT_VERIFIED' });
+      expect(Number(accountRow(engine).failed_login_count), `attempt ${i + 1}`).toBe(THRESHOLD - 1);
+    }
+    expect(isLocked(accountRow(engine))).toBe(false);
+
+    // One more wrong password still reaches the threshold and locks.
+    expect(await outcome(await signIn(manager, EMAIL, WRONG_PASSWORD))).toEqual(REFUSED);
+    expect(isLocked(accountRow(engine))).toBe(true);
+  });
+});

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

@@ -42,8 +42,9 @@
 
 import { codeSelfServiceRefusal } from './account-self-service-refusal-code.js';
 import type { AuthManager } from './auth-manager.js';
-import { SIGN_IN_EMAIL_PATH } from './auth-session-audit.js';
+import { refusalCodeOf, SIGN_IN_EMAIL_PATH } from './auth-session-audit.js';
 import { narrowDirectoryForExternalCaller } from './external-directory-door.js';
+import { SESSION_SCOPING_REFUSAL } from './organization-access-policy.js';
 import { refuseIntrospectionForBannedSubject } from './oauth-token-read-ban-refusal.js';
 import { rotateCallerBearerOnImpersonation } from './impersonation-bearer-rotation.js';
 import { stampInvitationDeliveryOutcome } from './invitation-delivery-outcome.js';
@@ -89,6 +90,48 @@ function changeEmailFrom(user: unknown): { email: string; name?: string; id?: st
   };
 }
 
+/**
+ * better-auth's admin plugin refuses a banned user's new session with this
+ * code, from its `session.create.before` hook (`ADMIN_ERROR_CODES.BANNED_USER`
+ * in `better-auth/dist/plugins/admin/error-codes.mjs`). The package exports no
+ * public path to that table, so the code is named here, once.
+ */
+const VENDOR_BANNED_USER_CODE = 'BANNED_USER';
+
+/**
+ * ADR-0069 D2 — the `/sign-in/email` refusals that come AFTER the password
+ * passed, which the lockout counter neither counts nor clears: only a refused
+ * credential counts toward a lock. better-auth's `signInEmail` throws
+ * `INVALID_EMAIL_OR_PASSWORD` for a wrong password before it reaches either
+ * place these are raised, so a wrong password never carries one of them to
+ * the counter:
+ *
+ *  - `EMAIL_NOT_VERIFIED` (`BASE_ERROR_CODES`), thrown by `signInEmail` itself
+ *    right after the password verified, when the deployment requires a
+ *    verified address;
+ *  - the organization's own allowed IP ranges (`SESSION_SCOPING_REFUSAL`),
+ *    judged where the new session is scoped to it (`refuseUnadmittedScoping`,
+ *    at session creation);
+ *  - a ban ({@link VENDOR_BANNED_USER_CODE}), judged at session creation too.
+ *
+ * A function, read when a sign-in is judged, rather than a set built when this
+ * module loads: `organization-access-policy.ts` imports `auth-manager.ts`,
+ * which reaches this module, so on that import order its constant is not yet
+ * initialised while this module evaluates; and better-auth is loaded lazily,
+ * as everywhere in this package.
+ */
+async function refusedAfterTheCredential(returned: unknown): Promise<boolean> {
+  const code = refusalCodeOf(returned);
+  if (code === undefined) return false;
+  const { BASE_ERROR_CODES } = await import('@better-auth/core/error');
+  const after: readonly string[] = [
+    BASE_ERROR_CODES.EMAIL_NOT_VERIFIED.code,
+    SESSION_SCOPING_REFUSAL.code,
+    VENDOR_BANNED_USER_CODE,
+  ];
+  return after.includes(code);
+}
+
 /** The body of `createAuthInstance`'s `hooks.after` arrow; the comments there and in it are its contract. */
 export async function globalAfterHook(this: AfterHookHost, ctx: any) {
   // ── #8243: impersonation must actually take effect for a bearer ──
@@ -149,6 +192,9 @@ export async function globalAfterHook(this: AfterHookHost, ctx: any) {
   // better-auth catches an INVALID_EMAIL_OR_PASSWORD APIError and runs
   // the after-hook with it on `ctx.context.returned`; a success leaves
   // the session payload there. Count failures, reset on success.
+  // A refusal `refusedAfterTheCredential` names is neither: the password
+  // passed and a policy then refused the sign-in, so the counter is left
+  // as it stands. Its `login_failed` row is still written below.
   if (ctx?.path === SIGN_IN_EMAIL_PATH) {
     const email = typeof ctx?.body?.email === 'string' ? ctx.body.email : '';
     if (email) {
@@ -159,7 +205,9 @@ export async function globalAfterHook(this: AfterHookHost, ctx: any) {
       } catch {
         succeeded = !(ctx?.context?.returned instanceof Error);
       }
-      const lockStamped = await this.recordSignInOutcome(email, succeeded);
+      const lockStamped = !succeeded && await refusedAfterTheCredential(ctx?.context?.returned)
+        ? undefined
+        : await this.recordSignInOutcome(email, succeeded);
       // A refused sign-in writes `login_failed`, and the one that
       // stamped the lockout also writes `account_locked`. A successful
       // one is recorded as `login` by the session hook, unchanged.