diff --git a/src/lib/actions/products.test.ts b/src/lib/actions/products.test.ts index 6a1a45f3..56f7723a 100644 --- a/src/lib/actions/products.test.ts +++ b/src/lib/actions/products.test.ts @@ -184,6 +184,10 @@ describe("createProduct", () => { const result = await createProduct(undefined, buildFormData()); expect(result.success).toBe(false); + expect(result.message).toBe( + "You are not permitted to use the selected data connection" + ); + expect(result.fieldErrors).toEqual({}); expect(productsTable.create).not.toHaveBeenCalled(); }); @@ -210,7 +214,10 @@ describe("createProduct", () => { ); expect(result.success).toBe(false); - expect(result.fieldErrors.data_connection_id).toBeDefined(); + expect(result.message).toBe("Invalid data connection for this account"); + expect(result.fieldErrors.data_connection_id).toEqual([ + "Selected data connection is not available for this account", + ]); expect(productsTable.create).not.toHaveBeenCalled(); }); diff --git a/src/lib/actions/products.ts b/src/lib/actions/products.ts index 0530eb65..77266740 100644 --- a/src/lib/actions/products.ts +++ b/src/lib/actions/products.ts @@ -12,6 +12,7 @@ import { import { getPageSession, LOGGER } from "@/lib"; import { FormState } from "@/components/core/DynamicForm"; import { isAuthorized, isAdmin } from "../api/authz"; +import { denyDataConnectionFor } from "@/lib/data-connections"; import { revalidatePath } from "next/cache"; import { productUrl, editProductDetailsUrl, accountUrl } from "@/lib/urls"; import { getProxyCredentials } from "@/lib/actions/proxy-credentials"; @@ -171,33 +172,33 @@ export async function createProduct( }; } - // Enforce that the user may create products against this connection - // (connections gated behind an account flag). - if (!isAuthorized(session, dataConnection, Actions.UseDataConnection)) { - return { - fieldErrors: {}, - data: formData, - message: "You are not permitted to use the selected data connection", - success: false, - }; - } - - // Enforce that the connection is available for the product's account. An - // owned connection may only be used by the account that owns it. - if ( - dataConnection.owner && - dataConnection.owner !== validatedFields.data.account_id + // Enforce that this connection may back a product under this account, and + // report each denial reason as its own field error. + switch ( + denyDataConnectionFor( + session, + dataConnection, + validatedFields.data.account_id + ) ) { - return { - fieldErrors: { - data_connection_id: [ - "Selected data connection is not available for this account", - ], - }, - data: formData, - message: "Invalid data connection for this account", - success: false, - }; + case "not-usable": + return { + fieldErrors: {}, + data: formData, + message: "You are not permitted to use the selected data connection", + success: false, + }; + case "wrong-account": + return { + fieldErrors: { + data_connection_id: [ + "Selected data connection is not available for this account", + ], + }, + data: formData, + message: "Invalid data connection for this account", + success: false, + }; } // Enforce the connection's allowed visibilities. Even though the form only diff --git a/src/lib/data-connections.test.ts b/src/lib/data-connections.test.ts index 943d520c..d55720c8 100644 --- a/src/lib/data-connections.test.ts +++ b/src/lib/data-connections.test.ts @@ -1,6 +1,7 @@ import { canManageDataConnection, canUseDataConnectionFor, + denyDataConnectionFor, listUsableDataConnections, } from "./data-connections"; import { isAdmin, canManageAccountDataConnections } from "@/lib/api/authz"; @@ -162,6 +163,64 @@ describe("listUsableDataConnections (issue #461)", () => { }); }); +// The single definition of which connections may back a product owned by a +// given account, and why not when they may not. `createProduct` switches on the +// reason to pick a field error. Uses the real `isAuthorized`. +describe("denyDataConnectionFor", () => { + const user = sessions["regular-user"]; + + test("allows a system-level (unowned) connection", () => { + expect( + denyDataConnectionFor(user, connection({ owner: undefined }), "acme") + ).toBeNull(); + }); + + test("allows a connection the account owns", () => { + expect( + denyDataConnectionFor(user, connection({ owner: "acme" }), "acme") + ).toBeNull(); + }); + + test("reports wrong-account for a connection another account owns", () => { + expect( + denyDataConnectionFor(user, connection({ owner: "rival" }), "acme") + ).toBe("wrong-account"); + }); + + test("allows a read-only connection", () => { + expect( + denyDataConnectionFor( + user, + connection({ owner: "acme", read_only: true }), + "acme" + ) + ).toBeNull(); + }); + + test("reports not-usable for a flag-gated connection the caller lacks the flag for", () => { + expect( + denyDataConnectionFor( + user, + connection({ required_flag: AccountFlags.CREATE_DATA_CONNECTIONS }), + "acme" + ) + ).toBe("not-usable"); + }); + + test("reports not-usable ahead of wrong-account when both apply", () => { + expect( + denyDataConnectionFor( + user, + connection({ + owner: "rival", + required_flag: AccountFlags.CREATE_DATA_CONNECTIONS, + }), + "acme" + ) + ).toBe("not-usable"); + }); +}); + // Which connections may back a product owned by a given account: system-level // (unowned) plus the account's own. See issue #461. Uses the real `isAuthorized`. describe("canUseDataConnectionFor", () => { diff --git a/src/lib/data-connections.ts b/src/lib/data-connections.ts index 6ede1b25..6f27208b 100644 --- a/src/lib/data-connections.ts +++ b/src/lib/data-connections.ts @@ -39,27 +39,44 @@ export async function canManageDataConnection( return canManageAccountDataConnections(session, ownerAccount); } +export type DataConnectionDenial = "not-usable" | "wrong-account"; + /** - * Whether `connection` may back a product owned by `accountId`: the connection - * itself must permit the caller (`UseDataConnection` covers flag-gated - * connections) and must be available to that account — either - * system-level (unowned) or owned by it. + * Why `connection` may not back a product owned by `accountId`, or null if it + * may. Two rules: the connection itself must permit the caller + * (`not-usable` — `UseDataConnection` covers flag-gated connections), and it + * must be available to that account — either system-level (unowned) or owned + * by it (`wrong-account`). + * + * The reason is returned rather than a bare boolean so `createProduct` can + * surface each rule as its own form field error; callers that only need a + * yes/no use `canUseDataConnectionFor`. * * This is only the connection half of associating the two; the caller side is * each call site's own gate — `canManageAccount` on the owning account for the - * mirror actions, `Actions.CreateRepository` for `createProduct` (which applies - * the same two connection rules inline, to report them as distinct field - * errors). + * mirror actions, `Actions.CreateRepository` for `createProduct`. */ +export function denyDataConnectionFor( + session: UserSession | null, + connection: DataConnection, + accountId: string +): DataConnectionDenial | null { + if (!isAuthorized(session, connection, Actions.UseDataConnection)) { + return "not-usable"; + } + if (connection.owner && connection.owner !== accountId) { + return "wrong-account"; + } + return null; +} + +/** Boolean form of {@link denyDataConnectionFor}. */ export function canUseDataConnectionFor( session: UserSession | null, connection: DataConnection, accountId: string ): boolean { - return ( - isAuthorized(session, connection, Actions.UseDataConnection) && - (!connection.owner || connection.owner === accountId) - ); + return denyDataConnectionFor(session, connection, accountId) === null; } /**