diff --git a/docs/credential-encryption.md b/docs/credential-encryption.md index 1ded306..0043530 100644 --- a/docs/credential-encryption.md +++ b/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. diff --git a/src/core/migrate-credentials.ts b/src/core/migrate-credentials.ts index 30a7edd..9e60f4f 100644 --- a/src/core/migrate-credentials.ts +++ b/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; const resources = ["anonymizedrepositories", "anonymizedgists", "anonymizedpullrequests"]; @@ -8,6 +10,8 @@ export interface MigrationOptions { apply?: boolean; removeLegacy?: boolean; preferOwnerToken?: boolean; + recoverOwnerTokens?: boolean; + identify?: (token: string) => Promise; 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(); 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 }); diff --git a/src/core/recover-owner-credential.ts b/src/core/recover-owner-credential.ts new file mode 100644 index 0000000..549235d --- /dev/null +++ b/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(); + let nextRequest = 0; + return async (githubId: unknown, candidates: Iterable): Promise => { + 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" }; + }; +} diff --git a/src/scripts/migrate-credentials.ts b/src/scripts/migrate-credentials.ts index 0ec44c2..590848c 100644 --- a/src/scripts/migrate-credentials.ts +++ b/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"); diff --git a/test/recover-owner-credential.test.js b/test/recover-owner-credential.test.js new file mode 100644 index 0000000..0b9846d --- /dev/null +++ b/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"); + }); +});