Merge pull request #802 from tdurieux/t3code/recover-owner-credentials

fix: recover conflicting credentials by verifying the recorded owner

docs/credential-encryption.md

@@ -379,3 +379,50 @@ identities never trigger automatic archiving.
 Once archival completes, rerun credential migration with `--prefer-owner-token`,
 then follow the verification/enforcement steps above. Start only a release that
 understands the archived status.
+
+### Conflicting resource tokens when the owner has no credential
+
+`--prefer-owner-token` cannot resolve conflicting repository tokens when neither
+`users.accessTokens.github` nor an encrypted credential exists. Use the opt-in
+`--recover-owner-tokens` option to validate resource tokens against that user's
+existing `externalIDs.github`. It never changes repository ownership or archives
+repositories. Only one distinct token authenticating the recorded owner is
+accepted; revoked and other users' tokens cannot be selected. Existing encrypted
+credentials and user tokens still take precedence with `--prefer-owner-token`.
+
+After updating the code and rebuilding `anonymous_github`, preview:
+
+```bash
+docker compose run --rm --no-deps -T --entrypoint node anonymous_github \
+  build/scripts/migrate-credentials.js --prefer-owner-token --recover-owner-tokens
+```
+
+With all application writers stopped, create the verified encrypted credentials
+first, retaining plaintext until the results have been reviewed:
+
+```bash
+docker compose run --rm --no-deps -T --entrypoint node anonymous_github \
+  build/scripts/migrate-credentials.js --prefer-owner-token --recover-owner-tokens \
+  --apply --maintenance
+```
+
+Then rerun the preview and follow the legacy-removal, verification and enforcement
+steps above. Do not regenerate encryption keys between runs.
+
+Recovery contacts GitHub even in preview mode. Requests are sequential, paced at
+least 250 ms apart, with a bounded cache keyed by token hashes. Network failures,
+rate limits and server errors halt the run (`halted: true`); earlier completed
+owners may already have been migrated in apply mode. Rerunning is safe.
+
+Unresolved owners are reported once in the `users` collection:
+
+- `missing_or_invalid_owner_github_id`: no usable GitHub ID recorded on the user.
+- `no_valid_owner_token`: no candidate authenticates that owner.
+- `multiple_valid_owner_tokens`: several distinct tokens authenticate the owner;
+  the script does not guess which token or scope is appropriate.
+- `unsupported_github_identity`: a response cannot establish a personal identity.
+
+These owners retain their legacy tokens, including with `--remove-legacy`.
+Have the owner sign in again to establish a fresh authoritative credential, or
+review their records manually. Do not bulk-archive owned repositories solely
+because credential recovery failed.

src/core/migrate-credentials.ts

@@ -1,5 +1,7 @@
 import { mongo } from "mongoose";
 import { createTokenCipher, EncryptedToken } from "./credential-crypto";
+import { createOwnerCredentialRecovery } from "./recover-owner-credential";
+import { IdentityResult } from "./recover-repository-owners";
 
 type Cipher = ReturnType<typeof createTokenCipher>;
 const resources = ["anonymizedrepositories", "anonymizedgists", "anonymizedpullrequests"];
@@ -8,6 +10,8 @@ export interface MigrationOptions {
   apply?: boolean;
   removeLegacy?: boolean;
   preferOwnerToken?: boolean;
+  recoverOwnerTokens?: boolean;
+  identify?: (token: string) => Promise<IdentityResult>;
   batchSize?: number;
   report?: (event: { collection: string; id: string; issue: string }) => void;
 }
@@ -17,13 +21,14 @@ export async function migrateCredentials(db: mongo.Db, cipher: Cipher, options:
   const credentials = db.collection("credentials");
   const users = db.collection("users");
   const batchSize = options.batchSize || 100;
-  const counts = { owners: 0, created: 0, removed: 0, issues: 0 };
+  const counts = { owners: 0, created: 0, removed: 0, issues: 0, halted: false };
+  const recover = createOwnerCredentialRecovery(options.identify);
   const issue = (collection: string, id: unknown, reason: string) => {
     counts.issues++;
     options.report?.({ collection, id: String(id), issue: reason });
   };
   if (options.apply) await credentials.createIndex({ ownerId: 1, provider: 1 }, { unique: true });
-  for await (const user of users.find({}, { projection: { accessTokens: 1, accessTokenDates: 1, status: 1 } }).batchSize(batchSize)) {
+  for await (const user of users.find({}, { projection: { accessTokens: 1, accessTokenDates: 1, status: 1, externalIDs: 1 } }).batchSize(batchSize)) {
     counts.owners++;
     const ownerId = user._id;
     const existing = await credentials.findOne({ ownerId, provider: "github" });
@@ -35,11 +40,14 @@ export async function migrateCredentials(db: mongo.Db, cipher: Cipher, options:
     }
     const ownerToken = user.accessTokens?.github;
     const authoritative = !!(selected || (typeof ownerToken === "string" && ownerToken));
+    const recovering = options.recoverOwnerTokens && !authoritative && user.status !== "removed";
+    const candidates = new Set<string>();
     const inspect = (value: unknown, collection: string, id: unknown) => {
       if (value === undefined || value === null || value === "") return;
       if (typeof value !== "string") {
         issue(collection, id, "malformed_token"); valid = false; return;
       }
+      if (recovering) { candidates.add(value); return; }
       if (!selected) selected = value;
       else if (selected !== value && !(options.preferOwnerToken && authoritative)) {
         issue(collection, id, "conflicting_token"); valid = false;
@@ -55,6 +63,15 @@ export async function migrateCredentials(db: mongo.Db, cipher: Cipher, options:
       }
     }
     if (!valid) continue;
+    if (recovering && candidates.size) {
+      const result = await recover(user.externalIDs?.github, candidates);
+      if ("issue" in result) {
+        issue("users", ownerId, result.issue);
+        if (result.halt) { counts.halted = true; return counts; }
+        continue;
+      }
+      selected = result.token;
+    }
     // Removed accounts must never regain credentials during backfill.
     if (user.status === "removed") {
       if (options.apply && options.removeLegacy) await credentials.deleteMany({ ownerId });

src/core/recover-owner-credential.ts

@@ -0,0 +1,35 @@
+import { createHash } from "crypto";
+import { identifyGitHubToken, IdentityResult } from "./recover-repository-owners";
+
+type Result = { token: string } | { issue: string; halt?: boolean };
+
+/** A resource token is usable only when it authenticates the already recorded owner. */
+export function createOwnerCredentialRecovery(identify = identifyGitHubToken) {
+  const cache = new Map<string, IdentityResult>();
+  let nextRequest = 0;
+  return async (githubId: unknown, candidates: Iterable<string>): Promise<Result> => {
+    const expected = String(githubId ?? "");
+    if (!/^[1-9][0-9]*$/.test(expected)) return { issue: "missing_or_invalid_owner_github_id" };
+    let selected: string | undefined;
+    for (const token of new Set(candidates)) {
+      const hash = createHash("sha256").update(token).digest("hex");
+      let identity = cache.get(hash);
+      if (!identity) {
+        await new Promise(resolve => setTimeout(resolve, Math.max(0, nextRequest - Date.now())));
+        nextRequest = Date.now() + 250;
+        try { identity = await identify(token); }
+        catch { return { issue: "github_request_failed", halt: true }; }
+        if (cache.size >= 10000) cache.delete(cache.keys().next().value!);
+        cache.set(hash, identity);
+      }
+      if ("issue" in identity) {
+        if (identity.issue === "invalid_or_revoked_token") continue;
+        return identity;
+      }
+      if (identity.githubId !== expected) continue;
+      if (selected) return { issue: "multiple_valid_owner_tokens" };
+      selected = token;
+    }
+    return selected ? { token: selected } : { issue: "no_valid_owner_token" };
+  };
+}

src/scripts/migrate-credentials.ts

@@ -7,7 +7,7 @@ import { migrateCredentials, verifyCredentials, enforceCredentialStorage } from
 async function main() {
   const args = new Set(process.argv.slice(2));
   for (const arg of args) {
-    if (!["--apply", "--remove-legacy", "--prefer-owner-token", "--maintenance", "--verify", "--enforce"].includes(arg)) {
+    if (!["--apply", "--remove-legacy", "--prefer-owner-token", "--recover-owner-tokens", "--maintenance", "--verify", "--enforce"].includes(arg)) {
       throw new Error("Unknown migration option");
     }
   }
@@ -28,6 +28,7 @@ async function main() {
     const result = await migrateCredentials(db, cipher, {
       apply: args.has("--apply"), removeLegacy: args.has("--remove-legacy"),
       preferOwnerToken: args.has("--prefer-owner-token"),
+      recoverOwnerTokens: args.has("--recover-owner-tokens"),
       report: event => process.stdout.write(JSON.stringify(event) + "\n"),
     });
     process.stdout.write(JSON.stringify(result) + "\n");

test/recover-owner-credential.test.js

@@ -0,0 +1,112 @@
+const { expect } = require("chai");
+require("ts-node/register/transpile-only");
+const { createOwnerCredentialRecovery } = require("../src/core/recover-owner-credential");
+const { migrateCredentials } = require("../src/core/migrate-credentials");
+const { createTokenCipher } = require("../src/core/credential-crypto");
+
+describe("owner credential recovery", function () {
+  this.timeout(5000);
+  it("rejects a shared admin token and deduplicates the owner's token", async () => {
+    const calls = [];
+    const recover = createOwnerCredentialRecovery(async token => {
+      calls.push(token);
+      return { githubId: token === "admin" ? "9" : "42" };
+    });
+    expect(await recover(42, ["admin", "owner", "owner"])).to.deep.equal({ token: "owner" });
+    expect(await recover("9", ["admin", "owner"])).to.deep.equal({ token: "admin" });
+    expect(calls).to.deep.equal(["admin", "owner"]);
+  });
+  it("does not choose between distinct valid owner tokens", async () => {
+    const recover = createOwnerCredentialRecovery(async () => ({ githubId: "42" }));
+    expect(await recover("42", ["old", "new"])).to.deep.equal({ issue: "multiple_valid_owner_tokens" });
+  });
+  it("ignores revoked tokens but does not recover a mismatched identity", async () => {
+    const recover = createOwnerCredentialRecovery(async token => token === "revoked"
+      ? { issue: "invalid_or_revoked_token" } : { githubId: "9" });
+    expect(await recover("42", ["revoked", "admin"])).to.deep.equal({ issue: "no_valid_owner_token" });
+  });
+  it("does not call GitHub without a usable recorded ID", async () => {
+    const recover = createOwnerCredentialRecovery(async () => { throw new Error("must not call"); });
+    for (const id of [undefined, null, "", "login", 0, -1]) {
+      expect(await recover(id, ["token"])).to.deep.equal({ issue: "missing_or_invalid_owner_github_id" });
+    }
+  });
+  it("halts on a failed request even after a matching token", async () => {
+    const recover = createOwnerCredentialRecovery(async token => token === "owner"
+      ? { githubId: "42" } : { issue: "github_http_403", halt: true });
+    expect(await recover("42", ["owner", "unknown"])).to.deep.equal({ issue: "github_http_403", halt: true });
+  });
+  it("redacts thrown request errors", async () => {
+    const recover = createOwnerCredentialRecovery(async () => { throw new Error("secret"); });
+    expect(await recover("42", ["secret"])).to.deep.equal({ issue: "github_request_failed", halt: true });
+  });
+});
+
+// Exercise the migration's write/cleanup decisions independently of a Mongo server.
+function fixture() {
+  const user = { _id: "owner", status: "active", externalIDs: { github: "42" } };
+  const rows = [{ _id: "repo", owner: "owner", source: { accessToken: "owner-token" }, accessToken: "admin-token" }];
+  const writes = [];
+  let credential;
+  const cursor = values => ({ batchSize() { return this; }, async *[Symbol.asyncIterator]() { yield* values; } });
+  const db = { collection(name) {
+    return {
+      find: () => cursor(name === "users" ? [user] : name === "anonymizedrepositories" ? rows : []),
+      findOne: async () => name === "users" ? user : credential,
+      createIndex: async () => {},
+      updateOne: async (filter, update) => {
+        writes.push({ name, update });
+        if (name === "credentials") credential = update.$setOnInsert;
+        return { modifiedCount: 1 };
+      },
+      updateMany: async (filter, update) => { writes.push({ name, update }); return { modifiedCount: 1 }; },
+    };
+  } };
+  return { db, user, rows, writes, credential: () => credential };
+}
+
+describe("migration owner recovery decisions", function () {
+  const cipher = createTokenCipher(JSON.stringify({ test: Buffer.alloc(32, 7).toString("base64") }), "test");
+  const identify = async token => ({ githubId: token === "owner-token" ? "42" : "9" });
+  it("previews without writes, then encrypts the matching token without changing ownership", async () => {
+    const f = fixture();
+    const options = { recoverOwnerTokens: true, preferOwnerToken: true, identify };
+    expect((await migrateCredentials(f.db, cipher, options)).created).to.equal(1);
+    expect(f.writes).to.deep.equal([]);
+    const result = await migrateCredentials(f.db, cipher, { ...options, apply: true, removeLegacy: true });
+    expect(result.issues).to.equal(0);
+    expect(cipher.decrypt(f.credential().encryptedToken, "owner", "github")).to.equal("owner-token");
+    expect(f.writes.filter(w => w.name !== "credentials").every(w => !!w.update.$unset)).to.equal(true);
+    expect(f.rows[0].owner).to.equal("owner");
+  });
+  it("leaves all tokens untouched on ambiguity", async () => {
+    const f = fixture();
+    const events = [];
+    const result = await migrateCredentials(f.db, cipher, {
+      recoverOwnerTokens: true, apply: true, removeLegacy: true,
+      identify: async () => ({ githubId: "42" }), report: e => events.push(e),
+    });
+    expect(result.issues).to.equal(1);
+    expect(events[0].issue).to.equal("multiple_valid_owner_tokens");
+    expect(f.writes).to.deep.equal([]);
+    expect(JSON.stringify(events)).not.to.include("owner-token");
+  });
+  it("halts without cleaning the current owner on transient failures", async () => {
+    const f = fixture();
+    const result = await migrateCredentials(f.db, cipher, {
+      recoverOwnerTokens: true, apply: true, removeLegacy: true,
+      identify: async () => ({ issue: "github_http_429", halt: true }),
+    });
+    expect(result.halted).to.equal(true);
+    expect(f.writes).to.deep.equal([]);
+  });
+  it("preserves the authoritative user token without making GitHub requests", async () => {
+    const f = fixture();
+    f.user.accessTokens = { github: "authoritative" };
+    await migrateCredentials(f.db, cipher, {
+      recoverOwnerTokens: true, preferOwnerToken: true, apply: true,
+      identify: async () => { throw new Error("must not call"); },
+    });
+    expect(cipher.decrypt(f.credential().encryptedToken, "owner", "github")).to.equal("authoritative");
+  });
+});