nextcloud: repair the account only on a 401, never reset the throttle (issue 1253, review 2)

Resetting Nextcloud's brute-force record for the docker gateway cleared it for
every client behind that address: someone guessing passwords could drive the
throttle to 429 and have the module's poll wipe it. A throttle is no evidence
the password is wrong, so a 429 is now backed off only, and the back-off
outlasts the throttle. The account's existence is read from user:info's exit
status, since user:list pages.

modules/nextcloud/client.ts

@@ -185,7 +185,9 @@ export class NextcloudClient {
       // occ writes many refusals ("user not found") to stdout with an empty stderr: say both.
       const detail = [err?.stderr, err?.stdout].map((x) => String(x ?? "").trim()).filter(Boolean).join("\n")
         || String(err?.message ?? "").trim();
-      throw new Error(detail || `occ produced no output — is the ${this.container} container running?`);
+      // The exit status rides along, so a caller can tell occ's own answer from a failure.
+      throw Object.assign(new Error(detail || `occ produced no output — is the ${this.container} container running?`),
+        { status: typeof err?.status === "number" ? err.status : undefined });
     }
   }
 
@@ -265,49 +267,40 @@ export class NextcloudClient {
    * The image creates NEXTCLOUD_ADMIN_USER only when it installs a fresh instance, so an instance
    * the mesh adopted never has it, and a secret made anew never reaches it. Only the module's own
    * account (MESH_ACCOUNT) is touched: under any other name it writes nothing and throws
-   * LoginRefusedError. Through occ: whether the account exists is read from user:list's
-   * membership; a missing one is added, an existing one gets its password set to the secret. It
-   * gets no admin group: the shares poll needs no admin rights (least privilege). The password
-   * goes by OC_PASS (`--password-from-env`), never on an argv. Then the brute-force record of the
-   * addresses the module's calls arrive from is reset, so the first good login is not held behind
-   * the throttle. A failing occ write is thrown as LoginRefusedError carrying occ's own message.
+   * LoginRefusedError. Through occ: whether the account exists is read from `user:info`, whose
+   * exit status 1 with "not found" means missing (user:list pages, so it is not asked); a missing
+   * one is added, an existing one gets its password set to the secret. It gets no admin group: the
+   * shares poll needs no admin rights (least privilege). The password goes by OC_PASS
+   * (`--password-from-env`), never on an argv. Nextcloud's brute-force record is left alone: it is
+   * shared by every client behind the same address, and the caller's back-off outlasts it. A
+   * failing occ is thrown as LoginRefusedError carrying occ's own message.
    */
-  ensureAdminAccount(): { account: "created" | "password-set"; throttleReset: boolean } {
+  ensureAdminAccount(): "created" | "password-set" {
     if (this.adminUser !== MESH_ACCOUNT) {
       throw new LoginRefusedError(401, this.adminUser, `not repaired: only ${MESH_ACCOUNT}, the module's own account, is`);
     }
     const pass = { env: { OC_PASS: this.adminPassword } };
-    let account: "created" | "password-set";
     try {
-      if (!this.listUsers().some((u) => u.uid === this.adminUser)) {
+      if (!this.accountExists()) {
         this.occ(["user:add", "--password-from-env", "--display-name=Mesh administration", this.adminUser], pass);
-        account = "created";
-      } else {
-        this.occ(["user:resetpassword", "--password-from-env", this.adminUser], pass);
-        account = "password-set";
+        return "created";
       }
+      this.occ(["user:resetpassword", "--password-from-env", this.adminUser], pass);
+      return "password-set";
     } catch (err) {
       throw new LoginRefusedError(401, this.adminUser, `repair failed: ${String((err as Error)?.message ?? err).split("\n")[0]}`);
     }
-    let throttleReset = true;
-    try {
-      for (const address of this.callerAddresses()) this.occ(["security:bruteforce:reset", address]);
-    } catch {
-      throttleReset = false;
-    }
-    return { account, throttleReset };
   }
 
-  /**
-   * The addresses the module's calls reach Nextcloud from, as Nextcloud sees them: the gateway of
-   * each network the container is on (a published port arrives from it; the access log shows
-   * 172.17.0.1 on the default bridge).
-   */
-  callerAddresses(): string[] {
-    const out = execFileSync("docker", ["inspect", "-f", "{{range .NetworkSettings.Networks}}{{.Gateway}} {{end}}", this.container], {
-      encoding: "utf8", timeout: 30_000,
-    });
-    return out.split(/\s+/).filter(Boolean);
+  /** Whether the account exists: occ user:info answers it, exit 1 and "not found" when missing. */
+  private accountExists(): boolean {
+    try {
+      this.occ(["user:info", this.adminUser, "--output=json"]);
+      return true;
+    } catch (err: any) {
+      if (err?.status === 1 && /not found/i.test(String(err?.message ?? ""))) return false;
+      throw err;
+    }
   }
 
   private fetchShares(): Promise<Response> {
@@ -319,15 +312,15 @@ export class NextcloudClient {
   }
 
   /**
-   * List every share, over the OCS Sharing API as the mesh's own account. A refused login (401) or
-   * a throttle (429) makes the account right once (ensureAdminAccount) and asks again; a refusal
-   * or a throttle after that throws LoginRefusedError, so the caller backs off (novox/hq issue 1253).
+   * List every share, over the OCS Sharing API as the mesh's own account. A refused login (401)
+   * makes the account right once (ensureAdminAccount) and asks again; a refusal after that, or
+   * any throttle (429), throws LoginRefusedError, so the caller backs off (novox/hq issue 1253).
    */
   async listShares(): Promise<NextcloudShare[]> {
     let res = await this.fetchShares();
-    // A throttle (429) follows refusals, so it gets the same one repair: the repair goes by occ,
-    // never by a login, and the throttle then wears off under the caller's back-off.
-    if ((res.status === 401 || res.status === 429) && !this.accountMadeRight) {
+    // Only a refused login (401) is repaired. A throttle (429) is no evidence the password is
+    // wrong (anyone guessing passwords from the same address causes it): it is backed off only.
+    if (res.status === 401 && !this.accountMadeRight) {
       await res.text();
       this.accountMadeRight = true;
       this.ensureAdminAccount();

modules/nextcloud/test/admin-login.test.ts

@@ -16,10 +16,7 @@ const REFUSED_XML = `<?xml version="1.0"?>\n<ocs>\n <meta>\n  <status>failure</s
 const SHARES = { ocs: { data: [{ id: 7, path: "/Photos", share_type: 3, share_with: "", uid_owner: "jochens" }] } };
 
 function client(user = "mesh-admin"): any {
-  const c = new (NextcloudClient as any)("nextcloud", "http://127.0.0.1:20003", user, PASSWORD);
-  // The address Nextcloud sees the module's calls arrive from (docker inspect in the real one).
-  mock.method(c, "callerAddresses", () => ["172.17.0.1"]);
-  return c;
+  return new (NextcloudClient as any)("nextcloud", "http://127.0.0.1:20003", user, PASSWORD);
 }
 
 /** A stand-in Nextcloud: answers `statuses` in turn (the last one repeating) and counts the asks. */
@@ -41,25 +38,26 @@ function standInOcc(accountExists: boolean, opts: { failWrite?: string } = {}) {
   const impl = (args: string[], occOpts: { env?: Record<string, string> } = {}): string => {
     calls.push(args);
     envs.push(occOpts.env);
-    if (args[0] === "user:list") {
-      return JSON.stringify(accountExists ? { jochens: "Jochen", "mesh-admin": "Mesh administration" } : { jochens: "Jochen" });
+    if (args[0] === "user:info") {
+      // As the real occ() throws it: occ's stdout as the message, its exit status alongside.
+      if (!accountExists) throw Object.assign(new Error("user not found"), { status: 1 });
+      return "{}";
     }
     if (["user:add", "user:resetpassword"].includes(args[0])) {
       if (opts.failWrite) throw new Error(opts.failWrite);
       return "";
     }
-    if (args[0] === "security:bruteforce:reset") return "";
     throw new Error("unexpected occ call: " + args.join(" "));
   };
   return { calls, envs, impl };
 }
 
-/** The occ calls that write: anything but a read of the user list. */
-const writes = (calls: string[][]) => calls.filter((a) => a[0] !== "user:list");
+/** The occ calls that write: anything but a read of the account. */
+const writes = (calls: string[][]) => calls.filter((a) => a[0] !== "user:info");
 
 // 1253/AC1: given an adopted instance without the mesh's account, when Nextcloud refuses the
 // login, then the account is added (no admin group) with the module's own secret, by OC_PASS and
-// never on an argv, the throttle of the caller's address is reset, and the shares are listed.
+// never on an argv, and the shares are listed; Nextcloud's brute-force record is left alone.
 test("a refused login adds the missing account, password by environment, and lists the shares", async () => {
   const c = client();
   const occ = standInOcc(false);
@@ -74,7 +72,7 @@ test("a refused login adds the missing account, password by environment, and lis
     assert.ok(add, "user:add ran");
     assert.ok(add.includes("--password-from-env") && add.includes("mesh-admin"));
     assert.ok(!add.some((a) => a.includes("group")), "no admin group: least privilege");
-    assert.deepEqual(occ.calls.at(-1), ["security:bruteforce:reset", "172.17.0.1"]);
+    assert.deepEqual(occ.calls.map((a) => a[0]), ["user:info", "user:add"]);
     assert.equal(occ.envs[occ.calls.indexOf(add)]?.OC_PASS, PASSWORD);
     assert.ok(!occ.calls.flat().some((a) => a.includes(PASSWORD)), "the password is on no argv");
   } finally {
@@ -83,7 +81,7 @@ test("a refused login adds the missing account, password by environment, and lis
 });
 
 // 1253/AC2: given the account exists with another password, when Nextcloud refuses the login,
-// then its password is set to the module's own secret, in no group, and the throttle is reset.
+// then its password is set to the module's own secret, in no group.
 test("a refused login resets an existing account's password, in no group", async () => {
   const c = client();
   const occ = standInOcc(true);
@@ -91,7 +89,7 @@ test("a refused login resets an existing account's password, in no group", async
   const f = mock.method(globalThis, "fetch", standInFetch([401, 200]).impl);
   try {
     await c.listShares();
-    assert.deepEqual(occ.calls.map((a) => a[0]), ["user:list", "user:resetpassword", "security:bruteforce:reset"]);
+    assert.deepEqual(occ.calls.map((a) => a[0]), ["user:info", "user:resetpassword"]);
     assert.equal(occ.envs[1]?.OC_PASS, PASSWORD);
     assert.ok(!occ.calls.flat().some((a) => a.includes(PASSWORD)));
   } finally {
@@ -124,17 +122,33 @@ test("a refusal after the repair throws LoginRefusedError once-repaired, short a
   }
 });
 
-// 1253/AC3b: a throttle (429) follows refusals: it gets the same one repair, and a throttle
-// that stays throws LoginRefusedError.
-test("a throttled login (429) is repaired once and then throws LoginRefusedError", async () => {
+// 1253/AC3b: a throttle (429) is no evidence the password is wrong: two cycles of 429 then 200
+// run no occ write, and the 429s are LoginRefusedError for the back-off.
+test("a throttle (429) is backed off, never repaired", async () => {
   const c = client();
   const occ = standInOcc(true);
   mock.method(c, "occ", occ.impl);
-  const f = mock.method(globalThis, "fetch", standInFetch([429]).impl);
+  const f = mock.method(globalThis, "fetch", standInFetch([429, 200, 429, 200]).impl);
   try {
-    await assert.rejects(c.listShares(), (err: any) => err instanceof LoginRefusedError && err.status === 429);
-    await assert.rejects(c.listShares(), LoginRefusedError);
-    assert.equal(occ.calls.filter((a) => a[0] === "user:resetpassword").length, 1);
+    for (let cycle = 0; cycle < 2; cycle++) {
+      await assert.rejects(c.listShares(), (err: any) => err instanceof LoginRefusedError && err.status === 429);
+      assert.equal((await c.listShares()).length, 1);
+    }
+    assert.deepEqual(occ.calls, [], "no occ call at all, so no write");
+  } finally {
+    f.mock.restore();
+  }
+});
+
+// 1253/AC3c: a 401 runs exactly one repair.
+test("a 401 runs exactly one repair", async () => {
+  const c = client();
+  const occ = standInOcc(true);
+  mock.method(c, "occ", occ.impl);
+  const f = mock.method(globalThis, "fetch", standInFetch([401, 200]).impl);
+  try {
+    await c.listShares();
+    assert.equal(writes(occ.calls).length, 1);
   } finally {
     f.mock.restore();
   }
@@ -252,7 +266,7 @@ test("a failing repair carries occ's message into LoginRefusedError", async () =
 });
 
 /** A fake docker on PATH: answers occ like Nextcloud (user:info's refusal on stdout, exit 1). */
-function fakeDocker(users: Record<string, string>): { dir: string; record: string } {
+function fakeDocker(): { dir: string; record: string } {
   const dir = mkdtempSync(join(tmpdir(), "nextcloud-1253-docker-"));
   const record = join(dir, "record");
   writeFileSync(join(dir, "docker"), [
@@ -260,8 +274,6 @@ function fakeDocker(users: Record<string, string>): { dir: string; record: strin
     `printf '%s\\n' "$*" >> ${record}`,
     'case "$*" in',
     "  *' occ user:info '*) echo 'user not found'; exit 1 ;;",
-    `  *' occ user:list '*) printf '%s' '${JSON.stringify(users)}' ;;`,
-    "  inspect*) echo '172.17.0.1 ' ;;",
     "esac",
     "exit 0",
     "",
@@ -279,15 +291,15 @@ function withPath<T>(dir: string, fn: () => T): T {
 // 1253/AC9: occ's own refusal on stdout with an empty stderr reaches the error, and the real
 // occ() through a docker answering like Nextcloud makes the missing account.
 test("occ says a refusal printed on stdout, and the real occ makes the missing account", () => {
-  const docker = fakeDocker({ jochens: "Jochen" });
+  const docker = fakeDocker();
   const c = new (NextcloudClient as any)("nextcloud", "http://127.0.0.1:20003", "mesh-admin", PASSWORD);
   withPath(docker.dir, () => {
     assert.throws(() => c.occ(["user:info", "mesh-admin", "--output=json"]), /user not found/);
     const did = c.ensureAdminAccount();
-    assert.deepEqual(did, { account: "created", throttleReset: true });
+    assert.equal(did, "created");
   });
   const lines = readFileSync(docker.record, "utf8").trim().split("\n");
   assert.ok(lines.some((l) => l.endsWith("php occ user:add --password-from-env --display-name=Mesh administration mesh-admin")), lines.join("\n"));
-  assert.ok(lines.some((l) => l.endsWith("php occ security:bruteforce:reset 172.17.0.1")), lines.join("\n"));
+  assert.ok(!lines.some((l) => l.includes("bruteforce")), "the brute-force record is left alone");
   assert.ok(!lines.join("\n").includes(PASSWORD));
 });