diff --git a/server/src/__tests__/access-routes-permissions-upgrade.test.ts b/server/src/__tests__/access-routes-permissions-upgrade.test.ts index c05b9f2a4b..e7588e8b3b 100644 --- a/server/src/__tests__/access-routes-permissions-upgrade.test.ts +++ b/server/src/__tests__/access-routes-permissions-upgrade.test.ts @@ -261,7 +261,7 @@ describeEmbeddedPostgres("access routes permissions upgrade compatibility", () = )).resolves.toBe(false); }); - it("preserves ambiguous legacy grants while limiting them to the current role", async () => { + it("requires legacy permission review before a role-only demotion", async () => { const { company, owner } = await createCompanyWithOwner(db); const member = await db.insert(companyMemberships).values({ companyId: company.id, @@ -288,7 +288,10 @@ describeEmbeddedPostgres("access routes permissions upgrade compatibility", () = const res = await request(await createApp(db, company.id, owner.principalId)) .patch(`/api/companies/${company.id}/members/${member.id}`) .send({ membershipRole: "operator" }); - expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body).toMatchObject({ + error: "Review this member's legacy permissions before changing their role", + }); const preserved = await db .select() @@ -297,12 +300,28 @@ describeEmbeddedPostgres("access routes permissions upgrade compatibility", () = expect(preserved).toEqual(expect.arrayContaining([ expect.objectContaining({ permissionKey: "tools:admin", grantOrigin: "legacy_unknown" }), ])); + + const reviewed = await request(await createApp(db, company.id, owner.principalId)) + .patch(`/api/companies/${company.id}/members/${member.id}/role-and-grants`) + .send({ + membershipRole: "operator", + grants: [{ permissionKey: "tools:admin", scope: null }], + }); + expect(reviewed.status, JSON.stringify(reviewed.body)).toBe(200); + expect(reviewed.body.membershipRole).toBe("operator"); + const reviewedGrants = await db + .select() + .from(principalPermissionGrants) + .where(eq(principalPermissionGrants.principalId, member.principalId)); + expect(reviewedGrants).toEqual([ + expect.objectContaining({ permissionKey: "tools:admin", grantOrigin: "explicit" }), + ]); await expect(ownerHasRequiredGrant( db, member.principalId, [company.id], "tools:manage", - )).resolves.toBe(false); + )).resolves.toBe(true); }); it("sweeps personal connection access when the member route suspends a user", async () => { diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index fedc2bfcfa..7e35827575 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -227,7 +227,7 @@ describeEmbeddedPostgres("authorization service", () => { expect(decision.explanation).toContain("Allowed by explicit grant tasks:assign"); }); - it("limits ambiguous legacy user grants to the active membership role", async () => { + it("preserves ambiguous historical explicit grants across a direct role change", async () => { const company = await createCompany(db, "LegacyGrantRoleCeiling"); const userId = `user-${randomUUID()}`; await db.insert(companyMemberships).values({ @@ -252,7 +252,7 @@ describeEmbeddedPostgres("authorization service", () => { principalId: userId, action: "tools:admin", permissionKey: "tools:admin", - })).resolves.toMatchObject({ allowed: true, reason: "allow_role_default" }); + })).resolves.toMatchObject({ allowed: true, reason: "allow_explicit_grant" }); await db .update(companyMemberships) @@ -269,7 +269,7 @@ describeEmbeddedPostgres("authorization service", () => { principalId: userId, action: "tools:admin", permissionKey: "tools:admin", - })).resolves.toMatchObject({ allowed: false, reason: "deny_missing_grant" }); + })).resolves.toMatchObject({ allowed: true, reason: "allow_explicit_grant" }); await expect(db.select().from(principalPermissionGrants).where(and( eq(principalPermissionGrants.companyId, company.id), eq(principalPermissionGrants.principalId, userId), diff --git a/server/src/security/board-key-owner-authority.ts b/server/src/security/board-key-owner-authority.ts index fb840778ad..0b06422d55 100644 --- a/server/src/security/board-key-owner-authority.ts +++ b/server/src/security/board-key-owner-authority.ts @@ -10,7 +10,6 @@ import type { BoardPermissionKey, PermissionKey, } from "@paperclipai/shared"; -import { grantsForHumanRole, normalizeHumanRole } from "../services/company-member-roles.js"; export function isBoardKeyWriteAction(action: BoardPermissionKey) { return /:(?:write|manage|control|operate|run|decide|create|import_export)$/.test(action); @@ -164,13 +163,6 @@ export async function ownerHasRequiredGrant( )); const liveKeysByCompany = new Map>(); for (const row of rows) { - if (row.grantOrigin === "legacy_unknown") { - const currentRolePermissionKeys = new Set( - grantsForHumanRole(normalizeHumanRole(membershipRoleByCompany.get(row.companyId))) - .map((grant) => grant.permissionKey), - ); - if (!currentRolePermissionKeys.has(row.permissionKey as PermissionKey)) continue; - } const liveKeys = liveKeysByCompany.get(row.companyId) ?? new Set(); liveKeys.add(row.permissionKey); liveKeysByCompany.set(row.companyId, liveKeys); diff --git a/server/src/services/access.ts b/server/src/services/access.ts index cf109f7059..4c3952eb79 100644 --- a/server/src/services/access.ts +++ b/server/src/services/access.ts @@ -1131,6 +1131,22 @@ export function accessService(db: Db) { .filter((permissionKey) => !nextDefaultKeys.has(permissionKey)); if (retiredDefaultKeys.length > 0) { + const ambiguousLegacyGrants = await tx + .select({ id: principalPermissionGrants.id }) + .from(principalPermissionGrants) + .where(and( + eq(principalPermissionGrants.companyId, companyId), + eq(principalPermissionGrants.principalType, "user"), + eq(principalPermissionGrants.principalId, existing.principalId), + inArray(principalPermissionGrants.permissionKey, retiredDefaultKeys), + eq(principalPermissionGrants.grantOrigin, "legacy_unknown"), + )); + if (ambiguousLegacyGrants.length > 0) { + throw conflict( + "Review this member's legacy permissions before changing their role", + ); + } + await tx .delete(principalPermissionGrants) .where(and( diff --git a/server/src/services/authorization.ts b/server/src/services/authorization.ts index 04d1685cb6..70b1d2032c 100644 --- a/server/src/services/authorization.ts +++ b/server/src/services/authorization.ts @@ -697,18 +697,6 @@ export function authorizationService(db: Db | DbTransaction) { }); } - const legacyGrantMatchesCurrentRole = grant.grantOrigin !== "legacy_unknown" - || input.principalType !== "user" - || grantsForHumanRole(normalizeHumanRole(membership.membershipRole, "operator")) - .some((defaultGrant) => defaultGrant.permissionKey === input.permissionKey); - if (!legacyGrantMatchesCurrentRole) { - return deny({ - action: input.action, - reason: "deny_missing_grant", - explanation: `Missing permission: ${input.permissionKey}.`, - }); - } - if ( !(await scopeAllows(db, input.companyId, grant.scope, input.scope, { requireStructuredScope: input.permissionKey === "tasks:assign_scope", @@ -729,10 +717,8 @@ export function authorizationService(db: Db | DbTransaction) { return allow({ action: input.action, - reason: grant.grantOrigin === "legacy_unknown" ? "allow_role_default" : "allow_explicit_grant", - explanation: grant.grantOrigin === "legacy_unknown" - ? `Allowed by the ${membership.membershipRole ?? "operator"} membership role.` - : `Allowed by explicit grant ${input.permissionKey}.`, + reason: "allow_explicit_grant", + explanation: `Allowed by explicit grant ${input.permissionKey}.`, grant: { principalType: input.principalType, principalId: input.principalId,