mirror of
https://github.com/fosrl/pangolin.git
synced 2026-10-14 08:39:05 +02:00
fix(org): clean up every site when deleting an org
deleteOrgById fetched the org's sites with .limit(1), so the per-site cleanup loop ran for at most one site: the WireGuard deletePeer call, the Newt terminate collection, and cleanupSiteAssociations were all skipped for every other site in the org. Drop the limit so every site is loaded, and mirror deleteSite.ts in the loop: run cleanupSiteAssociations for newt sites so clients of other orgs do not keep stale associations for the deleted sites. Covered by a standalone regression test that seeds two wireguard and two newt sites plus a stale association-cache row, runs deleteOrgById against a stubbed exit node, and asserts all four sites were cleaned. On the previous code the test fails with exactly the reported symptom: one peer removal instead of two. Fixes #3863
This commit is contained in:
choksi2212
@@ -0,0 +1,139 @@
|
||||
// Test child for deleteOrg.test.ts. Boots the real sqlite driver against a
|
||||
// fresh database in the temp workdir (cwd), seeds an org with two wireguard
|
||||
// sites and two newt sites, runs deleteOrgById against a local HTTP stand-in
|
||||
// for the exit node, and prints one JSON line of observations.
|
||||
// The app boots with `import "./extendZod"` before anything else; zod
|
||||
// schemas in the import chain use .openapi(), so do the same here.
|
||||
import "@server/extendZod";
|
||||
import http from "http";
|
||||
import { deleteOrgById } from "@server/lib/deleteOrg";
|
||||
import {
|
||||
db,
|
||||
exitNodes,
|
||||
orgs,
|
||||
sites,
|
||||
newts,
|
||||
newtSessions,
|
||||
clientSitesAssociationsCache
|
||||
} from "@server/db";
|
||||
import { sql } from "drizzle-orm";
|
||||
|
||||
const peerRemovals: string[] = [];
|
||||
const server = http.createServer((req, res) => {
|
||||
let body = "";
|
||||
req.on("data", (c) => (body += c));
|
||||
req.on("end", () => {
|
||||
if (req.method === "DELETE") {
|
||||
if (!req.url) {
|
||||
res.writeHead(200, { "Content-Type": "application/json" });
|
||||
res.end(JSON.stringify({ status: "ok" }));
|
||||
return;
|
||||
}
|
||||
const url = new URL(req.url, "http://localhost");
|
||||
const pubKey = url.searchParams.get("public_key");
|
||||
if (pubKey !== null) {
|
||||
peerRemovals.push(pubKey);
|
||||
}
|
||||
}
|
||||
res.writeHead(200, { "Content-Type": "application/json" });
|
||||
res.end(JSON.stringify({ status: "ok" }));
|
||||
});
|
||||
});
|
||||
|
||||
server.listen(0, "127.0.0.1", async () => {
|
||||
const addr = server.address();
|
||||
if (addr === null || typeof addr === "string") {
|
||||
throw new Error("failed to bind the stub exit node");
|
||||
}
|
||||
const port = addr.port;
|
||||
try {
|
||||
await db.insert(exitNodes).values({
|
||||
exitNodeId: 1,
|
||||
name: "en",
|
||||
address: "10.0.0.1",
|
||||
endpoint: "10.0.0.1:51820",
|
||||
publicKey: "enKey",
|
||||
listenPort: 51820,
|
||||
reachableAt: "http://127.0.0.1:" + port,
|
||||
type: "wireguard"
|
||||
});
|
||||
await db.insert(orgs).values({
|
||||
orgId: "org1",
|
||||
name: "Org",
|
||||
settingsJitModeLimit: 1
|
||||
});
|
||||
for (let i = 1; i <= 2; i++) {
|
||||
await db.insert(sites).values({
|
||||
siteId: i,
|
||||
orgId: "org1",
|
||||
niceId: String(i),
|
||||
name: "wg" + i,
|
||||
pubKey: "wgKey" + i,
|
||||
exitNodeId: 1,
|
||||
type: "wireguard"
|
||||
});
|
||||
}
|
||||
for (let i = 3; i <= 4; i++) {
|
||||
await db.insert(sites).values({
|
||||
siteId: i,
|
||||
orgId: "org1",
|
||||
niceId: String(i),
|
||||
name: "newt" + i,
|
||||
pubKey: "newtPub" + i,
|
||||
publicKey: "newtSession" + i,
|
||||
type: "newt"
|
||||
});
|
||||
await db.insert(newts).values({
|
||||
newtId: "newtId" + i,
|
||||
secretHash: "h" + i,
|
||||
dateCreated: "2026-01-01",
|
||||
siteId: i
|
||||
});
|
||||
await db.insert(newtSessions).values({
|
||||
sessionId: "sess" + i,
|
||||
newtId: "newtId" + i,
|
||||
expiresAt: 4102444800
|
||||
});
|
||||
}
|
||||
// A stale association-cache row pinned to one of the newt sites, the
|
||||
// kind cleanupSiteAssociations exists to clear.
|
||||
await db.insert(clientSitesAssociationsCache).values({
|
||||
clientId: 99,
|
||||
siteId: 3,
|
||||
isRelayed: false
|
||||
});
|
||||
|
||||
const result = await deleteOrgById("org1");
|
||||
|
||||
const row = (await db.get(
|
||||
sql`select
|
||||
(select count(*) from sites) as sites,
|
||||
(select count(*) from "newt") as newts,
|
||||
(select count(*) from "newtSession") as sessions,
|
||||
(select count(*) from clientSitesAssociationsCache where siteId = 3) as assoc`
|
||||
)) as {
|
||||
sites: number;
|
||||
newts: number;
|
||||
sessions: number;
|
||||
assoc: number;
|
||||
};
|
||||
|
||||
console.log(
|
||||
"OBSERVATIONS " +
|
||||
JSON.stringify({
|
||||
peerRemovals: peerRemovals.slice().sort(),
|
||||
terminatedNewtIds: result.deletedNewtIds.slice().sort(),
|
||||
sitesLeft: Number(row.sites),
|
||||
newtsLeft: Number(row.newts),
|
||||
sessionsLeft: Number(row.sessions),
|
||||
associationRowsLeft: Number(row.assoc)
|
||||
})
|
||||
);
|
||||
server.close();
|
||||
process.exit(0);
|
||||
} catch (e: any) {
|
||||
console.error("CHILD_FAIL", e && e.message ? e.message : String(e));
|
||||
server.close();
|
||||
process.exit(1);
|
||||
}
|
||||
});
|
||||
@@ -0,0 +1,185 @@
|
||||
import { execFileSync } from "child_process";
|
||||
import fs from "fs";
|
||||
import os from "os";
|
||||
import path from "path";
|
||||
import { fileURLToPath } from "url";
|
||||
import { assertEquals } from "@test/assert";
|
||||
|
||||
const here = path.dirname(fileURLToPath(import.meta.url));
|
||||
const repoRoot = path.resolve(here, "..", "..");
|
||||
|
||||
const CHILD_SCRIPT_PATH = path.join(
|
||||
repoRoot,
|
||||
"server",
|
||||
"lib",
|
||||
"deleteOrg.childTest.ts"
|
||||
);
|
||||
|
||||
function runChild(workdir: string): {
|
||||
exitCode: number;
|
||||
observations: any;
|
||||
output: string;
|
||||
} {
|
||||
const tsconfig = ["tsconfig.json", "tsconfig.oss.json"]
|
||||
.map((file) => path.join(repoRoot, file))
|
||||
.find((file) => fs.existsSync(file));
|
||||
if (!tsconfig) {
|
||||
throw new Error("No tsconfig found for @server path aliases");
|
||||
}
|
||||
const tsxCli = path.join(
|
||||
repoRoot,
|
||||
"node_modules",
|
||||
"tsx",
|
||||
"dist",
|
||||
"cli.mjs"
|
||||
);
|
||||
if (!fs.existsSync(tsxCli)) {
|
||||
throw new Error("tsx is not installed; run npm ci first");
|
||||
}
|
||||
try {
|
||||
const output = execFileSync(
|
||||
process.execPath,
|
||||
[tsxCli, "--tsconfig", tsconfig, CHILD_SCRIPT_PATH],
|
||||
{
|
||||
cwd: workdir,
|
||||
timeout: 120000,
|
||||
encoding: "utf8",
|
||||
env: { ...process.env, NODE_ENV: "test" }
|
||||
}
|
||||
);
|
||||
const m = output.match(/OBSERVATIONS (.*)/);
|
||||
if (!m) {
|
||||
throw new Error("child did not print observations: " + output);
|
||||
}
|
||||
return { exitCode: 0, observations: JSON.parse(m[1]), output };
|
||||
} catch (error: any) {
|
||||
const output = String(error?.stdout ?? "");
|
||||
return { exitCode: 1, observations: null, output };
|
||||
}
|
||||
}
|
||||
|
||||
function createTestEnvironment(): string {
|
||||
for (const generated of ["server/build.ts", "server/db/index.ts"]) {
|
||||
if (!fs.existsSync(path.join(repoRoot, generated))) {
|
||||
throw new Error(
|
||||
`Missing ${generated}; run npm run set:oss && npm run set:sqlite first`
|
||||
);
|
||||
}
|
||||
}
|
||||
const workdir = fs.mkdtempSync(
|
||||
path.join(os.tmpdir(), "pangolin-org-cleanup-")
|
||||
);
|
||||
fs.mkdirSync(path.join(workdir, "config", "db"), { recursive: true });
|
||||
fs.copyFileSync(
|
||||
path.join(repoRoot, "config", "config.example.yml"),
|
||||
path.join(workdir, "config", "config.yml")
|
||||
);
|
||||
fs.symlinkSync(
|
||||
path.join(repoRoot, "server"),
|
||||
path.join(workdir, "server"),
|
||||
process.platform === "win32" ? "junction" : "dir"
|
||||
);
|
||||
fs.symlinkSync(
|
||||
path.join(repoRoot, "test"),
|
||||
path.join(workdir, "test"),
|
||||
process.platform === "win32" ? "junction" : "dir"
|
||||
);
|
||||
fs.symlinkSync(
|
||||
path.join(repoRoot, "node_modules"),
|
||||
path.join(workdir, "node_modules"),
|
||||
process.platform === "win32" ? "junction" : "dir"
|
||||
);
|
||||
return workdir;
|
||||
}
|
||||
|
||||
function applyMigrations(workdir: string) {
|
||||
const tsconfig = ["tsconfig.json", "tsconfig.oss.json"]
|
||||
.map((file) => path.join(repoRoot, file))
|
||||
.find((file) => fs.existsSync(file));
|
||||
if (!tsconfig) {
|
||||
throw new Error("No tsconfig found for @server path aliases");
|
||||
}
|
||||
const tsxCli = path.join(
|
||||
repoRoot,
|
||||
"node_modules",
|
||||
"tsx",
|
||||
"dist",
|
||||
"cli.mjs"
|
||||
);
|
||||
// CI applies migrations with `npm run db:push` before booting the app;
|
||||
// server/db/migrate.ts is the drizzle migrator that script invokes.
|
||||
const migrationsScript = path.join(repoRoot, "server", "db", "migrate.ts");
|
||||
execFileSync(
|
||||
process.execPath,
|
||||
[tsxCli, "--tsconfig", tsconfig, migrationsScript],
|
||||
{
|
||||
cwd: workdir,
|
||||
timeout: 120000,
|
||||
encoding: "utf8",
|
||||
env: { ...process.env, NODE_ENV: "test" }
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
function testOrgCleanupCoversEverySite() {
|
||||
console.log("Running deleteOrgById per-site cleanup test...");
|
||||
const workdir = createTestEnvironment();
|
||||
applyMigrations(workdir);
|
||||
|
||||
const { exitCode, observations, output } = runChild(workdir);
|
||||
if (exitCode !== 0) {
|
||||
throw new Error("deleteOrgById child run failed: " + output);
|
||||
}
|
||||
|
||||
// The exact bug from the issue: with a single-row sites fetch, only one
|
||||
// of the four sites gets its side effects. All wireguard peers must be
|
||||
// removed from the exit node...
|
||||
assertEquals(
|
||||
JSON.stringify(observations.peerRemovals),
|
||||
JSON.stringify(["wgKey1", "wgKey2"]),
|
||||
"deleteOrgById must remove every wireguard site's peer from its exit node"
|
||||
);
|
||||
// ...and every newt connector must be collected for the terminate message.
|
||||
assertEquals(
|
||||
JSON.stringify(observations.terminatedNewtIds),
|
||||
JSON.stringify(["newtId3", "newtId4"]),
|
||||
"deleteOrgById must collect every newt id so all connectors get the terminate message"
|
||||
);
|
||||
// DB rows must still cascade to nothing left.
|
||||
assertEquals(observations.sitesLeft, 0, "all site rows must be gone");
|
||||
assertEquals(observations.newtsLeft, 0, "all newt rows must be gone");
|
||||
assertEquals(
|
||||
observations.sessionsLeft,
|
||||
0,
|
||||
"all newt session rows must be gone"
|
||||
);
|
||||
|
||||
// The gap the issue calls out separately: cleanupSiteAssociations must
|
||||
// run for newt sites (deleteSite.ts already does this), so the stale
|
||||
// association cache rows for these sites must be cleared.
|
||||
assertEquals(
|
||||
observations.associationRowsLeft,
|
||||
0,
|
||||
"newt site cleanup must clear clientSitesAssociationsCache rows"
|
||||
);
|
||||
|
||||
console.log("deleteOrgById per-site cleanup test passed");
|
||||
}
|
||||
|
||||
function main() {
|
||||
let failed = 0;
|
||||
for (const test of [testOrgCleanupCoversEverySite]) {
|
||||
try {
|
||||
test();
|
||||
} catch (error) {
|
||||
failed++;
|
||||
console.error(`FAIL: ${(error as Error).message}`);
|
||||
}
|
||||
}
|
||||
if (failed > 0) {
|
||||
process.exit(1);
|
||||
}
|
||||
console.log("deleteOrg tests complete");
|
||||
}
|
||||
|
||||
main();
|
||||
+19
-15
@@ -23,6 +23,7 @@ import { sendToClient } from "#dynamic/routers/ws";
|
||||
import { deletePeer } from "@server/routers/gerbil/peers";
|
||||
import { OlmErrorCodes } from "@server/routers/olm/error";
|
||||
import { sendTerminateClient } from "@server/routers/client/terminate";
|
||||
import { cleanupSiteAssociations } from "@server/lib/rebuildClientAssociations";
|
||||
import { usageService } from "./billing/usageService";
|
||||
import { LimitId } from "./billing";
|
||||
|
||||
@@ -55,8 +56,7 @@ export async function deleteOrgById(
|
||||
const orgSites = await db
|
||||
.select()
|
||||
.from(sites)
|
||||
.where(eq(sites.orgId, orgId))
|
||||
.limit(1);
|
||||
.where(eq(sites.orgId, orgId));
|
||||
|
||||
const orgClients = await db
|
||||
.select()
|
||||
@@ -73,20 +73,24 @@ export async function deleteOrgById(
|
||||
|
||||
await db.transaction(async (trx) => {
|
||||
for (const site of orgSites) {
|
||||
if (site.pubKey) {
|
||||
if (site.type == "wireguard") {
|
||||
if (site.type == "wireguard") {
|
||||
if (site.pubKey) {
|
||||
await deletePeer(site.exitNodeId!, site.pubKey);
|
||||
} else if (site.type == "newt") {
|
||||
const [deletedNewt] = await trx
|
||||
.delete(newts)
|
||||
.where(eq(newts.siteId, site.siteId))
|
||||
.returning();
|
||||
if (deletedNewt) {
|
||||
deletedNewtIds.push(deletedNewt.newtId);
|
||||
await trx
|
||||
.delete(newtSessions)
|
||||
.where(eq(newtSessions.newtId, deletedNewt.newtId));
|
||||
}
|
||||
}
|
||||
} else if (site.type == "newt") {
|
||||
// Same per-site cleanup deleteSite.ts performs, so that
|
||||
// removing an org does not leave other orgs' clients
|
||||
// holding stale associations for these sites.
|
||||
await cleanupSiteAssociations(site, trx);
|
||||
const [deletedNewt] = await trx
|
||||
.delete(newts)
|
||||
.where(eq(newts.siteId, site.siteId))
|
||||
.returning();
|
||||
if (deletedNewt) {
|
||||
deletedNewtIds.push(deletedNewt.newtId);
|
||||
await trx
|
||||
.delete(newtSessions)
|
||||
.where(eq(newtSessions.newtId, deletedNewt.newtId));
|
||||
}
|
||||
}
|
||||
logger.info(`Deleting site ${site.siteId}`);
|
||||
|
||||
Reference in New Issue
Block a user