fix: rearm owner alerts across recovery actions safely

test/owner-alert-actions.test.js

@@ -0,0 +1,65 @@
+const { expect } = require("chai");
+require("ts-node/register/transpile-only");
+const notifications = require("../src/core/owner-notifications");
+const utils = require("../src/server/routes/route-utils");
+const app = require("../src/core/github-app");
+const db = require("../src/server/database");
+const github = require("../src/core/source/GitHubRepository");
+const Resources = require("../src/core/model/anonymizedRepositories/anonymizedRepositories.model").default;
+const Sources = require("../src/core/model/repositories/repositories.model").default;
+const Users = require("../src/core/model/users/users.model").default;
+const repositoryRouter = require("../src/server/routes/repository-private").default;
+const appRouter = require("../src/server/routes/github-app").githubAppRouter;
+const handler = (router, path) => router.stack.find(layer => layer.route?.path === path).route.stack.at(-1).handle;
+
+describe("owner actions rearm access alerts", () => {
+  let restores, resets;
+  const owner = "507f1f77bcf86cd799439011";
+  function stub(object, key, value) {
+    const old = object[key]; restores.push(() => { object[key] = old; }); object[key] = value;
+  }
+  beforeEach(() => {
+    restores = []; resets = [];
+    stub(utils, "getUser", async () => ({ id: owner, model: { id: owner } }));
+    stub(utils, "handleError", error => { throw error; });
+    stub(notifications, "resetOwnerAccessAlerts", async (...args) => { resets.push(args); });
+  });
+  afterEach(() => { while (restores.length) restores.pop()(); });
+
+  for (const isOwner of [true, false]) it(`extension ${isOwner ? "rearms for its owner" : "does not rearm for an admin"}`, async () => {
+    const id = isOwner ? owner : "admin";
+    stub(utils, "getUser", async () => ({ id, model: { id }, isAdmin: !isOwner }));
+    const repo = { owner: { id: owner }, status: "ready", model: { _id: "resource", options: {} } };
+    stub(utils, "getRepo", async () => repo);
+    stub(Resources, "updateOne", () => ({ exec: async () => ({ matchedCount: 1 }) }));
+    await handler(repositoryRouter, "/:repoId/extend")({}, { json() {} });
+    expect(resets).to.deep.equal(isOwner ? [[owner, { kind: "repository", id: "resource" }]] : []);
+  });
+
+  it("clears the previous owner's claim atomically when a repository is claimed", async () => {
+    stub(db, "getRepository", async () => ({ repoId: "resource", model: { source: { repositoryId: "source" } } }));
+    stub(app, "selectRepositoryAccess", async () => ({ token: "token", binding: { kind: "oauth" } }));
+    stub(github, "getRepositoryFromGitHub", async () => ({ id: 42 }));
+    stub(Sources, "findById", async () => ({ externalId: 42 }));
+    let write;
+    stub(Resources, "updateOne", (_filter, update) => ({ collation: async () => { write = update; } }));
+    await handler(repositoryRouter, "/claim")({ body: { repoId: "resource", repoUrl: "https://github.com/owner/repo" }, query: {} }, { send() {} });
+    expect(write.$set.owner).to.equal(owner);
+    expect(write.$set.accessAlertGeneration).to.be.a("string");
+    expect(write.$unset).to.have.property("accessAlertClaimedAt");
+  });
+
+  for (const action of ["install", "request", "unverified"]) it(`handles ${action} installation completion without premature resets`, async () => {
+    stub(app, "userInstallations", async () => [{ id: 42 }]);
+    stub(Users, "updateOne", async () => ({}));
+    const req = { session: {
+      githubInstallFlow: { ownerId: owner, state: "state", expires: Date.now() + 60000, returnTo: "/connections" },
+      save: callback => callback(),
+    }, isAuthenticated: () => true, query: { state: "state", setup_action: action, installation_id: action === "unverified" ? "99" : "42" } };
+    let failure;
+    try { await handler(appRouter, "/app/setup")(req, { redirect() {} }); } catch (error) { failure = error; }
+    expect(resets).to.deep.equal(action === "install" ? [[owner]] : []);
+    if (action === "unverified") expect(failure?.message).to.equal("github_app_access_required");
+    else expect(failure).to.equal(undefined);
+  });
+});

test/owner-notifications.test.js

@@ -11,10 +11,10 @@ describe("owner access notifications", () => {
   const ownerId = "507f1f77bcf86cd799439011";
   const resource = { kind: "repository", id: "507f1f77bcf86cd799439012" };
   const other = { kind: "repository", id: "507f1f77bcf86cd799439013" };
-  let owner, sent, claims, rows;
+  let owner, sent, claims, rows, generations;
   beforeEach(() => {
     owner = { status: "active", emails: [{ email: "owner@example.com", default: true }] };
-    sent = []; claims = []; rows = new Map();
+    sent = []; claims = []; rows = new Map(); generations = new Map();
     stub(config, "RESEND_API_KEY", "test-resend-key");
     stub(config, "EMAIL_FROM", "alerts@example.com");
     stub(config, "APP_HOSTNAME", "anonymous.example.com");
@@ -22,15 +22,23 @@ describe("owner access notifications", () => {
     Object.defineProperty(User.db, "readyState", { configurable: true, value: 1 });
     restores.push(() => { delete User.db.readyState; });
     stub(User.db, "collection", name => ({
+      findOne: async filter => {
+        const key = `${name}:${filter.owner}:${filter._id}`;
+        if (!generations.has(key)) generations.set(key, undefined);
+        return { accessAlertGeneration: generations.get(key) };
+      },
       updateOne: async (filter, update) => {
         claims.push(filter);
         const key = `${name}:${filter.owner}:${filter._id}`;
-        if (rows.has(key)) return { modifiedCount: 0 };
+        if (rows.has(key) || generations.get(key) !== (typeof filter.accessAlertGeneration === "string" ? filter.accessAlertGeneration : undefined)) return { modifiedCount: 0 };
         rows.set(key, update.$set.accessAlertClaimedAt);
         return { modifiedCount: 1 };
       },
-      updateMany: async filter => {
-        for (const key of rows.keys()) if (key.startsWith(`${name}:${filter.owner}:`) && (!filter._id || key.endsWith(`:${filter._id}`))) rows.delete(key);
+      updateMany: async (filter, update) => {
+        for (const key of generations.keys()) if (key.startsWith(`${name}:${filter.owner}:`) && (!filter._id || key.endsWith(`:${filter._id}`))) {
+          rows.delete(key);
+          generations.set(key, update.$set.accessAlertGeneration);
+        }
       },
     }));
     stub(global, "fetch", async (url, options) => { sent.push({ url, options, body: JSON.parse(options.body) }); return { ok: true }; });
@@ -71,6 +79,25 @@ describe("owner access notifications", () => {
     expect(sent).to.have.length(3);
   });
 
+  it("rejects a pre-action failure paused during owner lookup", async () => {
+    let resume, entered;
+    const waiting = new Promise(resolve => { entered = resolve; });
+    stub(User, "findOne", () => ({ lean: async () => {
+      entered();
+      await new Promise(resolve => { resume = resolve; });
+      return owner;
+    } }));
+    const oldFailure = notifyOwnerAccessProblem(ownerId, "token_expired", resource);
+    await waiting;
+    await notifications.resetOwnerAccessAlerts(ownerId, resource);
+    resume();
+    await oldFailure;
+    expect(sent).to.have.length(0);
+    stub(User, "findOne", () => ({ lean: async () => owner }));
+    await notifyOwnerAccessProblem(ownerId, "token_expired", resource);
+    expect(sent).to.have.length(1);
+  });
+
   it("reconnecting resets all of the owner's resource types", async () => {
     const resources = [resource, { ...resource, kind: "pull-request" }, { ...resource, kind: "gist" }];
     for (const item of resources) await notifyOwnerAccessProblem(ownerId, "token_expired", item);