diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f9d3f16f..f25054236 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A group reached in two ways can be approved for a shared app + +Approving a Bot for a shared app now succeeds when the same group appears in its published audience +and its assigned groups. Repeated people or groups are stored once; a person and a group with the +same name remain separate, and no additional audience is approved. + **Before upgrading.** Sixteen things change for an existing deployment: - Bots now share one computer per person instead of each having its own. Migration `0069_isolated_bots` adds `agent_profiles.isolated`, off for every existing Bot. With the diff --git a/server/src/plugins/shared-use-store.ts b/server/src/plugins/shared-use-store.ts index 59ef1f14c..87a248b61 100644 --- a/server/src/plugins/shared-use-store.ts +++ b/server/src/plugins/shared-use-store.ts @@ -284,13 +284,25 @@ export function createSharedUseStore( input.approval.audience === "people" && input.approval.members.length > 0 ) { - await tx.insert(sharedUseApprovalMembers).values( - input.approval.members.map((member) => ({ - agentId: input.botId, - serverId, - ...member, - })), - ); + // Publication and assignment can name the same group. Membership is a set of exact + // (kind, value) pairs; a user and a group with the same value are still distinct. + await tx + .insert(sharedUseApprovalMembers) + .values( + input.approval.members.map((member) => ({ + agentId: input.botId, + serverId, + ...member, + })), + ) + .onConflictDoNothing({ + target: [ + sharedUseApprovalMembers.agentId, + sharedUseApprovalMembers.serverId, + sharedUseApprovalMembers.kind, + sharedUseApprovalMembers.value, + ], + }); } /* A pending request this approval now answers is closed as approved, so the inbox never asks twice. */ const pending = await tx diff --git a/server/tests/shared-use-store.integration.test.ts b/server/tests/shared-use-store.integration.test.ts index 8c226408b..9d4b8257a 100644 --- a/server/tests/shared-use-store.integration.test.ts +++ b/server/tests/shared-use-store.integration.test.ts @@ -12,6 +12,7 @@ import { responsibilities, sharedUseRequests, teamBotAudience, + teamBotAssignments, teamBotPublications, users, } from "../src/db/schema"; @@ -273,6 +274,64 @@ describe("the shared-use gate", () => { }); }); +describe("approval members", () => { + test("repeated people and groups store one exact membership without widening access", async () => { + await store.setApproval({ + botId: bot, + serverId: app, + by: id("owner"), + approval: { + audience: "people", + outsideInput: false, + members: [ + { kind: "user", value: id("named") }, + { kind: "user", value: id("named") }, + { kind: "group", value: "platform" }, + { kind: "group", value: "platform" }, + { kind: "group", value: id("named") }, + ], + }, + }); + const approval = await store.approvalFor(bot, app); + expect(approval?.members).toHaveLength(3); + expect(approval?.members).toEqual( + expect.arrayContaining([ + { kind: "user", value: id("named") }, + { kind: "group", value: "platform" }, + { kind: "group", value: id("named") }, + ]), + ); + expect((await call("named")).allowed).toBe(true); + expect((await call("newhire")).allowed).toBe(true); + expect((await call("stranger")).allowed).toBe(false); + }); + + test("an audience also assigned to the Bot can be approved", async () => { + await database + .insert(teamBotPublications) + .values({ agentId: bot, publishedBy: id("owner"), audience: "people" }); + await database + .insert(teamBotAudience) + .values({ agentId: bot, kind: "group", value: "platform" }); + await database + .insert(teamBotAssignments) + .values({ agentId: bot, groupName: "platform", assignedBy: id("owner") }); + await call("newhire"); + const [pending] = await store.pendingFor(bot); + expect(pending?.proposed.members).toHaveLength(2); + await store.decide({ + id: pending!.id, + by: id("admin"), + decision: "approve", + }); + expect((await store.approvalFor(bot, app))?.members).toEqual([ + { kind: "group", value: "platform" }, + ]); + expect((await call("newhire")).allowed).toBe(true); + expect((await call("stranger")).allowed).toBe(false); + }); +}); + describe("decide", () => { test("two admins deciding one request record exactly one decision", async () => { await call("stranger");