mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 00:05:49 +02:00
fix(server): read existing grants before seeding role defaults (#16365)
`insertMissingPrincipalGrants` now reads the existing permission keys first and inserts only missing grants. In the steady state no INSERT runs, so cloud-tenant auth resync no longer waits on row locks held by concurrent grant edits until the statement timeout (Sentry PAPERCLIP-BACKEND-3S). Co-Authored-By: Paperclip <Paperclip-Paperclip@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
3 files changed
+196
-4
No files matched your search
@@ -0,0 +1,158 @@
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import { and, eq } from "drizzle-orm";
|
||||
import { companies, createDb, principalPermissionGrants } from "@paperclipai/db";
|
||||
import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
startEmbeddedPostgresTestDatabase,
|
||||
} from "./helpers/embedded-postgres.js";
|
||||
import { grantsForHumanRole } from "../services/company-member-roles.js";
|
||||
import {
|
||||
ensureHumanRoleDefaultGrants,
|
||||
insertMissingPrincipalGrants,
|
||||
} from "../services/principal-access-compatibility.js";
|
||||
|
||||
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
||||
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
||||
|
||||
if (!embeddedPostgresSupport.supported) {
|
||||
console.warn(
|
||||
`Skipping embedded Postgres principal grant tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`,
|
||||
);
|
||||
}
|
||||
|
||||
describeEmbeddedPostgres("insertMissingPrincipalGrants", () => {
|
||||
let db!: ReturnType<typeof createDb>;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
|
||||
beforeAll(async () => {
|
||||
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-principal-grants-");
|
||||
db = createDb(tempDb.connectionString);
|
||||
}, 20_000);
|
||||
|
||||
afterEach(async () => {
|
||||
await db.delete(principalPermissionGrants);
|
||||
await db.delete(companies);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await tempDb?.cleanup();
|
||||
});
|
||||
|
||||
async function createCompany() {
|
||||
return db
|
||||
.insert(companies)
|
||||
.values({
|
||||
name: `Grant Seeding ${randomUUID()}`,
|
||||
issuePrefix: `GS${randomUUID().slice(0, 6).toUpperCase()}`,
|
||||
})
|
||||
.returning()
|
||||
.then((rows) => rows[0]!);
|
||||
}
|
||||
|
||||
async function grantKeys(companyId: string, principalId: string) {
|
||||
const rows = await db
|
||||
.select({ permissionKey: principalPermissionGrants.permissionKey })
|
||||
.from(principalPermissionGrants)
|
||||
.where(
|
||||
and(
|
||||
eq(principalPermissionGrants.companyId, companyId),
|
||||
eq(principalPermissionGrants.principalType, "user"),
|
||||
eq(principalPermissionGrants.principalId, principalId),
|
||||
),
|
||||
);
|
||||
return rows.map((row) => row.permissionKey).sort();
|
||||
}
|
||||
|
||||
it("inserts only the grants that are missing", async () => {
|
||||
const company = await createCompany();
|
||||
const userId = `user-${randomUUID()}`;
|
||||
const ownerGrants = grantsForHumanRole("owner");
|
||||
await insertMissingPrincipalGrants(db, {
|
||||
companyId: company.id,
|
||||
principalType: "user",
|
||||
principalId: userId,
|
||||
grants: ownerGrants.slice(0, 2),
|
||||
grantedByUserId: null,
|
||||
});
|
||||
|
||||
const inserted = await ensureHumanRoleDefaultGrants(db, {
|
||||
companyId: company.id,
|
||||
principalId: userId,
|
||||
membershipRole: "owner",
|
||||
grantedByUserId: null,
|
||||
});
|
||||
|
||||
expect(inserted).toBe(ownerGrants.length - 2);
|
||||
expect(await grantKeys(company.id, userId)).toEqual(
|
||||
ownerGrants.map((grant) => grant.permissionKey).sort(),
|
||||
);
|
||||
await expect(
|
||||
ensureHumanRoleDefaultGrants(db, {
|
||||
companyId: company.id,
|
||||
principalId: userId,
|
||||
membershipRole: "owner",
|
||||
grantedByUserId: null,
|
||||
}),
|
||||
).resolves.toBe(0);
|
||||
});
|
||||
|
||||
it("does not wait on a transaction that is deleting grants that already exist", async () => {
|
||||
const company = await createCompany();
|
||||
const userId = `user-${randomUUID()}`;
|
||||
await ensureHumanRoleDefaultGrants(db, {
|
||||
companyId: company.id,
|
||||
principalId: userId,
|
||||
membershipRole: "owner",
|
||||
grantedByUserId: null,
|
||||
});
|
||||
|
||||
let releaseLock!: () => void;
|
||||
const lockReleased = new Promise<void>((resolve) => {
|
||||
releaseLock = resolve;
|
||||
});
|
||||
let signalLockHeld!: () => void;
|
||||
const lockHeld = new Promise<void>((resolve) => {
|
||||
signalLockHeld = resolve;
|
||||
});
|
||||
const rollback = new Error("rollback");
|
||||
// A permission edit deletes and re-inserts a member's grants inside one
|
||||
// transaction. Until it commits, an INSERT ... ON CONFLICT on the same keys
|
||||
// must wait for it.
|
||||
const holder = db
|
||||
.transaction(async (tx) => {
|
||||
await tx
|
||||
.delete(principalPermissionGrants)
|
||||
.where(
|
||||
and(
|
||||
eq(principalPermissionGrants.companyId, company.id),
|
||||
eq(principalPermissionGrants.principalId, userId),
|
||||
),
|
||||
);
|
||||
signalLockHeld();
|
||||
await lockReleased;
|
||||
throw rollback;
|
||||
})
|
||||
.catch((error) => {
|
||||
if (error !== rollback) throw error;
|
||||
});
|
||||
|
||||
try {
|
||||
await lockHeld;
|
||||
const seeding = ensureHumanRoleDefaultGrants(db, {
|
||||
companyId: company.id,
|
||||
principalId: userId,
|
||||
membershipRole: "owner",
|
||||
grantedByUserId: null,
|
||||
});
|
||||
const outcome = await Promise.race([
|
||||
seeding,
|
||||
new Promise<"blocked">((resolve) => setTimeout(() => resolve("blocked"), 2_000)),
|
||||
]);
|
||||
expect(outcome).toBe(0);
|
||||
} finally {
|
||||
releaseLock();
|
||||
await holder;
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -3,7 +3,14 @@ import express, { type Request } from "express";
|
||||
import request from "supertest";
|
||||
import { and, eq } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { authUsers, companies, companyMemberships, instanceSettings, instanceUserRoles } from "@paperclipai/db";
|
||||
import {
|
||||
authUsers,
|
||||
companies,
|
||||
companyMemberships,
|
||||
instanceSettings,
|
||||
instanceUserRoles,
|
||||
principalPermissionGrants,
|
||||
} from "@paperclipai/db";
|
||||
import { actorMiddleware, cloudActorHeaderSourceFromHeaders, resolveCloudTenantActor } from "./auth.js";
|
||||
import { idleAdmissionMiddleware, trackIdleRequestHandlers } from "./idle-admission.js";
|
||||
import { idleWorkSnapshot, startTaskDrain, stopTaskDrain } from "../services/task-admission.js";
|
||||
@@ -59,10 +66,14 @@ function createFakeDb(options: {
|
||||
return chain;
|
||||
},
|
||||
select: () => {
|
||||
if (options.selectThrows) throw new Error("select unavailable");
|
||||
return {
|
||||
from: (table: unknown) => ({
|
||||
where: (condition: unknown) => {
|
||||
// Role-default grant seeding reads existing grants first; keep that
|
||||
// read working so selectThrows exercises only the degradable reads.
|
||||
if (options.selectThrows && table !== principalPermissionGrants) {
|
||||
throw new Error("select unavailable");
|
||||
}
|
||||
selectWheres.push({ table, condition });
|
||||
const rows =
|
||||
table === instanceSettings && settingsRow
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { and, eq, notInArray } from "drizzle-orm";
|
||||
import { and, eq, inArray, notInArray } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { agents, companyMemberships, principalPermissionGrants } from "@paperclipai/db";
|
||||
import type { PermissionKey, PrincipalType } from "@paperclipai/shared";
|
||||
@@ -26,11 +26,34 @@ export async function insertMissingPrincipalGrants(
|
||||
): Promise<number> {
|
||||
if (input.grants.length === 0) return 0;
|
||||
|
||||
// Read before writing. Cloud tenant authentication re-seeds role defaults
|
||||
// on every sync, and the grants almost always exist already. An INSERT ...
|
||||
// ON CONFLICT still waits on another transaction that is deleting or
|
||||
// re-inserting a conflicting row, which stalled authentication until the
|
||||
// statement timeout. A plain read never waits on row locks.
|
||||
const existing = await db
|
||||
.select({ permissionKey: principalPermissionGrants.permissionKey })
|
||||
.from(principalPermissionGrants)
|
||||
.where(
|
||||
and(
|
||||
eq(principalPermissionGrants.companyId, input.companyId),
|
||||
eq(principalPermissionGrants.principalType, input.principalType),
|
||||
eq(principalPermissionGrants.principalId, input.principalId),
|
||||
inArray(
|
||||
principalPermissionGrants.permissionKey,
|
||||
input.grants.map((grant) => grant.permissionKey),
|
||||
),
|
||||
),
|
||||
);
|
||||
const existingKeys = new Set(existing.map((row) => row.permissionKey));
|
||||
const missing = input.grants.filter((grant) => !existingKeys.has(grant.permissionKey));
|
||||
if (missing.length === 0) return 0;
|
||||
|
||||
const now = new Date();
|
||||
const inserted = await db
|
||||
.insert(principalPermissionGrants)
|
||||
.values(
|
||||
input.grants.map((grant) => ({
|
||||
missing.map((grant) => ({
|
||||
companyId: input.companyId,
|
||||
principalType: input.principalType,
|
||||
principalId: input.principalId,
|
||||
|
||||
Reference in new issue
Block a user