diff --git a/src/app/(app)/products/new/page.test.tsx b/src/app/(app)/products/new/page.test.tsx new file mode 100644 index 00000000..120822ca --- /dev/null +++ b/src/app/(app)/products/new/page.test.tsx @@ -0,0 +1,118 @@ +import NewProductPage from "./page"; +import { accountsTable, membershipsTable, getPageSession } from "@/lib"; +import { listUsableDataConnections } from "@/lib/data-connections"; +import { + Account, + Membership, + MembershipRole, + MembershipState, + UserSession, +} from "@/types"; + +jest.mock("@/lib", () => ({ + getPageSession: jest.fn(), + accountsTable: { fetchManyByIds: jest.fn() }, + membershipsTable: { listByUser: jest.fn() }, +})); + +jest.mock("@/lib/data-connections", () => ({ + listUsableDataConnections: jest.fn(), +})); + +jest.mock("@/lib/api/authz", () => ({ + isAuthorized: jest.fn(() => true), +})); + +const mockGetPageSession = getPageSession as jest.MockedFunction< + typeof getPageSession +>; +const mockAccountsTable = accountsTable as jest.Mocked; +const mockMembershipsTable = membershipsTable as jest.Mocked< + typeof membershipsTable +>; +const mockListUsable = listUsableDataConnections as jest.MockedFunction< + typeof listUsableDataConnections +>; + +function membership(overrides: Partial): Membership { + return { + membership_id: "m", + account_id: "user", + membership_account_id: "org", + role: MembershipRole.Owners, + state: MembershipState.Member, + ...overrides, + } as Membership; +} + +/** The account ids the page decides may own the new product. */ +async function ownerIdsPassedToConnectionListing(memberships: Membership[]) { + mockMembershipsTable.listByUser.mockResolvedValue(memberships); + mockAccountsTable.fetchManyByIds.mockImplementation(async (ids: string[]) => + ids.map((account_id) => ({ account_id }) as Account) + ); + await NewProductPage({ params: Promise.resolve({}), searchParams: Promise.resolve({}) } as never); + return mockListUsable.mock.calls.at(-1)?.[1]; +} + +beforeEach(() => { + jest.clearAllMocks(); + mockGetPageSession.mockResolvedValue({ + identity_id: "id-1", + account: { account_id: "user", flags: [] }, + } as unknown as UserSession); + mockListUsable.mockResolvedValue([]); +}); + +// The list drives which owned connections are serialized into the page, so a +// membership that can't own a product must not widen it (see #462). +describe("NewProductPage owner scoping", () => { + test("includes an account the user owns account-wide", async () => { + expect( + await ownerIdsPassedToConnectionListing([ + membership({ membership_account_id: "org-a" }), + ]) + ).toEqual(["user", "org-a"]); + }); + + test("includes an account-wide maintainer's account", async () => { + expect( + await ownerIdsPassedToConnectionListing([ + membership({ + membership_account_id: "org-a", + role: MembershipRole.Maintainers, + }), + ]) + ).toEqual(["user", "org-a"]); + }); + + test("excludes an account where the membership is scoped to one product", async () => { + expect( + await ownerIdsPassedToConnectionListing([ + membership({ membership_account_id: "org-b", repository_id: "prod-1" }), + ]) + ).toEqual(["user"]); + }); + + test("excludes an account where the user only reads data", async () => { + expect( + await ownerIdsPassedToConnectionListing([ + membership({ + membership_account_id: "org-b", + role: MembershipRole.ReadData, + }), + ]) + ).toEqual(["user"]); + }); + + test("excludes an invited-but-not-yet-member account", async () => { + expect( + await ownerIdsPassedToConnectionListing([ + membership({ + membership_account_id: "org-b", + state: MembershipState.Invited, + }), + ]) + ).toEqual(["user"]); + }); +}); diff --git a/src/app/(app)/products/new/page.tsx b/src/app/(app)/products/new/page.tsx index 6f7a5b25..dc858804 100644 --- a/src/app/(app)/products/new/page.tsx +++ b/src/app/(app)/products/new/page.tsx @@ -2,7 +2,12 @@ import { ProductCreationForm } from "@/components/features/products/ProductCreat import { accountsTable, getPageSession, membershipsTable } from "@/lib"; import { isAuthorized } from "@/lib/api/authz"; import { listUsableDataConnections } from "@/lib/data-connections"; -import { Actions, DataConnectionObjectSchema, MembershipState } from "@/types"; +import { + Actions, + DataConnectionObjectSchema, + MembershipRole, + MembershipState, +} from "@/types"; import { Heading, Text } from "@radix-ui/themes"; import { FormTitle } from "@/components/core"; @@ -42,21 +47,37 @@ export default async function NewProductPage({ const memberships = await membershipsTable.listByUser( session.account.account_id ); + // Only accounts this user may actually create products under. A read-data + // member can't (createRepository requires owner/maintainer), and a membership + // scoped to one product doesn't reach the account at all — offering either + // would both present an owner createProduct rejects and, since this list also + // scopes the connections below, expose that account's storage details. const potentialOwnerAccounts = [ session.account, ...(await accountsTable.fetchManyByIds( memberships - .filter((membership) => membership.state === MembershipState.Member) + .filter( + (membership) => + membership.state === MembershipState.Member && + !membership.repository_id && + (membership.role === MembershipRole.Owners || + membership.role === MembershipRole.Maintainers) + ) .map((membership) => membership.membership_account_id) )), ]; - // Strip credentials before handing connections to the client component. - const dataConnections = (await listUsableDataConnections(session)).map( - (connection) => - DataConnectionObjectSchema.omit({ authentication: true }).parse( - connection - ) + // Only connections usable by an account this user can create products under — + // an owned connection exposes its account's bucket names and prefixes, so the + // form's owner filter must not be the only one. Strip credentials before + // handing what remains to the client component. + const dataConnections = ( + await listUsableDataConnections( + session, + potentialOwnerAccounts.map((account) => account.account_id) + ) + ).map((connection) => + DataConnectionObjectSchema.omit({ authentication: true }).parse(connection) ); return ( diff --git a/src/lib/data-connections.test.ts b/src/lib/data-connections.test.ts index 943d520c..c89e5a02 100644 --- a/src/lib/data-connections.test.ts +++ b/src/lib/data-connections.test.ts @@ -137,7 +137,9 @@ describe("listUsableDataConnections (issue #461)", () => { test("offers an org's own connection to an org owner", async () => { listing([orgConnection]); expect( - ids(await listUsableDataConnections(sessions["organization-owner-user"])) + ids(await listUsableDataConnections(sessions["organization-owner-user"], [ + "organization", + ])) ).toEqual(["organization--byob"]); }); @@ -150,14 +152,35 @@ describe("listUsableDataConnections (issue #461)", () => { listing([orgConnection]); expect( - ids(await listUsableDataConnections(sessions["organization-owner-user"])) + ids(await listUsableDataConnections(sessions["organization-owner-user"], [ + "organization", + ])) ).toEqual(["organization--byob"]); }); test("offers a read-only connection", async () => { listing([{ ...orgConnection, read_only: true } as DataConnection]); expect( - ids(await listUsableDataConnections(sessions["organization-owner-user"])) + ids(await listUsableDataConnections(sessions["organization-owner-user"], [ + "organization", + ])) + ).toEqual(["organization--byob"]); + }); + + // The owner filter has to happen here, not in ProductCreationForm: an owned + // connection carries its account's bucket name and prefixes, so a client-side + // filter would still have shipped them to every user's browser. + test("withholds another account's connection from the listing", async () => { + listing([orgConnection]); + expect( + ids(await listUsableDataConnections(sessions["regular-user"], ["regular"])) + ).toEqual([]); + }); + + test("still offers system-level connections to any account", async () => { + listing([{ ...orgConnection, owner: undefined } as DataConnection]); + expect( + ids(await listUsableDataConnections(sessions["regular-user"], ["regular"])) ).toEqual(["organization--byob"]); }); }); @@ -215,3 +238,4 @@ describe("canUseDataConnectionFor", () => { ).toBe(false); }); }); + diff --git a/src/lib/data-connections.ts b/src/lib/data-connections.ts index 6ede1b25..e5df3a50 100644 --- a/src/lib/data-connections.ts +++ b/src/lib/data-connections.ts @@ -63,20 +63,31 @@ export function canUseDataConnectionFor( } /** - * List the data connections a user is permitted to use when creating a product. + * List the data connections a user is permitted to use when creating a product + * under one of `ownerAccountIds` (the accounts they may create products for). + * + * A connection is usable when it is available to one of those accounts — + * system-level (unowned) or owned by it — and the session is authorized both to + * read it (`GetDataConnection`) and to create products against it + * (`UseDataConnection`). + * + * The owner filter belongs here rather than at the call site: an owned + * connection carries its account's bucket names and prefixes, so filtering it + * out in the browser would still have shipped it there. * - * A connection is usable when the session is authorized both to read it - * (`GetDataConnection`) and to create products against it (`UseDataConnection`). * The returned objects are unsanitized (credentials intact); callers that hand * these to the client must strip `authentication` first. */ export async function listUsableDataConnections( - session: UserSession | null + session: UserSession | null, + ownerAccountIds: string[] ): Promise { + const usableBy = new Set(ownerAccountIds); const dataConnections = await dataConnectionsTable.listAll(); return dataConnections.filter( (dataConnection) => + (!dataConnection.owner || usableBy.has(dataConnection.owner)) && isAuthorized(session, dataConnection, Actions.UseDataConnection) && isAuthorized(session, dataConnection, Actions.GetDataConnection) );