diff --git a/src/apphosting/secrets/dialogs.ts b/src/apphosting/secrets/dialogs.ts index 742e6a4c1ad..2a7ab928cef 100644 --- a/src/apphosting/secrets/dialogs.ts +++ b/src/apphosting/secrets/dialogs.ts @@ -1,7 +1,7 @@ import * as clc from "colorette"; const Table = require("cli-table"); -import { MultiServiceAccounts, ServiceAccounts, serviceAccountsForBackend, toMulti } from "."; +import { serviceAccountsForBackend } from "."; import * as apphosting from "../../gcp/apphosting"; import * as prompt from "../../prompt"; import * as utils from "../../utils"; @@ -13,8 +13,7 @@ import * as env from "../../functions/env"; interface BackendMetadata { location: string; id: string; - buildServiceAccount: string; - runServiceAccount: string; + serviceAccounts: string[]; } /** @@ -28,7 +27,11 @@ export function toMetadata( for (const backend of backends) { // Splits format projects//locations//backends/ const [, , , location, , id] = backend.name.split("/"); - metadata.push({ location, id, ...serviceAccountsForBackend(projectNumber, backend) }); + metadata.push({ + location, + id, + serviceAccounts: serviceAccountsForBackend(projectNumber, backend), + }); } return metadata.sort((left, right) => { const cmplocation = left.location.localeCompare(right.location); @@ -39,22 +42,10 @@ export function toMetadata( }); } -/** Displays a single service account or a comma separated list of service accounts. */ -export function serviceAccountDisplay(metadata: ServiceAccounts): string { - if (sameServiceAccount(metadata)) { - return metadata.runServiceAccount; - } - return `${metadata.buildServiceAccount}, ${metadata.runServiceAccount}`; -} - -function sameServiceAccount(metadata: ServiceAccounts): boolean { - return metadata.buildServiceAccount === metadata.runServiceAccount; -} - -const matchesServiceAccounts = (target: ServiceAccounts) => (test: ServiceAccounts) => { +const matchesServiceAccounts = (target: BackendMetadata) => (test: BackendMetadata) => { return ( - target.buildServiceAccount === test.buildServiceAccount && - target.runServiceAccount === test.runServiceAccount + target.serviceAccounts.length === test.serviceAccounts.length && + target.serviceAccounts.every((sa) => test.serviceAccounts.indexOf(sa) !== -1) ); }; @@ -68,38 +59,12 @@ export function tableForBackends( const headers = [ "location", "backend", - metadata.every(sameServiceAccount) ? "service account" : "service accounts", + metadata.every((m) => m.serviceAccounts.length === 1) ? "service account" : "service accounts", ]; - const rows = metadata.map((m) => [m.location, m.id, serviceAccountDisplay(m)]); + const rows = metadata.map((m) => [m.location, m.id, m.serviceAccounts.join(", ")]); return [headers, rows]; } -/** - * Returns a MultiServiceAccounts for all selected service accounts in a ServiceAccount[]. - * If a service account is ever a "build" account in input, it will be a "build" account in the - * output. Otherwise, it will be a "run" account. - */ -export function selectFromMetadata( - input: ServiceAccounts[], - selected: string[], -): MultiServiceAccounts { - const buildAccounts = new Set(); - const runAccounts = new Set(); - - for (const sa of selected) { - if (input.find((m) => m.buildServiceAccount === sa)) { - buildAccounts.add(sa); - } else { - runAccounts.add(sa); - } - } - - return { - buildServiceAccounts: [...buildAccounts], - runServiceAccounts: [...runAccounts], - }; -} - /** Common warning log that there are no backends. Exported to make tests easier. */ export const WARN_NO_BACKENDS = "To use this secret, your backend's service account must have secret accessor permission. " + @@ -117,7 +82,7 @@ export async function selectBackendServiceAccounts( projectNumber: string, projectId: string, options: any, -): Promise { +): Promise { const listBackends = await apphosting.listBackends(projectId, "-"); if (listBackends.unreachable.length) { @@ -130,7 +95,7 @@ export async function selectBackendServiceAccounts( if (!listBackends.backends.length) { utils.logLabeledWarning("apphosting", WARN_NO_BACKENDS); - return { buildServiceAccounts: [], runServiceAccounts: [] }; + return []; } if (listBackends.backends.length === 1) { @@ -141,10 +106,10 @@ export async function selectBackendServiceAccounts( "To use this secret, your backend's service account must have secret accessor permission. Would you like to grant it now?", }); if (grant) { - return toMulti(serviceAccountsForBackend(projectNumber, listBackends.backends[0])); + return serviceAccountsForBackend(projectNumber, listBackends.backends[0]); } utils.logLabeledBullet("apphosting", GRANT_ACCESS_IN_FUTURE); - return { buildServiceAccounts: [], runServiceAccounts: [] }; + return []; } const metadata: BackendMetadata[] = toMetadata(projectNumber, listBackends.backends); @@ -153,8 +118,8 @@ export async function selectBackendServiceAccounts( utils.logLabeledBullet( "apphosting", "To use this secret, your backend's service account must have secret accessor permission. All of your backends use " + - (sameServiceAccount(metadata[0]) ? "service account " : "service accounts ") + - serviceAccountDisplay(metadata[0]) + + (metadata[0].serviceAccounts.length === 1 ? "service account " : "service accounts ") + + metadata[0].serviceAccounts.join(", ") + ". Granting access to one backend will grant access to all backends.", ); const grant = await prompt.confirm({ @@ -163,13 +128,10 @@ export async function selectBackendServiceAccounts( message: "Would you like to grant it now?", }); if (grant) { - return selectFromMetadata(metadata, [ - metadata[0].buildServiceAccount, - metadata[0].runServiceAccount, - ]); + return metadata[0].serviceAccounts; } utils.logLabeledBullet("apphosting", GRANT_ACCESS_IN_FUTURE); - return { buildServiceAccounts: [], runServiceAccounts: [] }; + return []; } utils.logLabeledBullet( @@ -185,8 +147,9 @@ export async function selectBackendServiceAccounts( logger.info(table.toString()); const allAccounts = metadata.reduce((accum: Set, row) => { - accum.add(row.buildServiceAccount); - accum.add(row.runServiceAccount); + for (const sa of row.serviceAccounts) { + accum.add(sa); + } return accum; }, new Set()); const chosen = await prompt.promptOnce({ @@ -199,7 +162,7 @@ export async function selectBackendServiceAccounts( if (!chosen.length) { utils.logLabeledBullet("apphosting", GRANT_ACCESS_IN_FUTURE); } - return selectFromMetadata(metadata, chosen); + return chosen; } function toUpperSnakeCase(key: string): string { diff --git a/src/apphosting/secrets/index.ts b/src/apphosting/secrets/index.ts index 6ce7d2a8a98..eb5f44e2a75 100644 --- a/src/apphosting/secrets/index.ts +++ b/src/apphosting/secrets/index.ts @@ -9,34 +9,6 @@ import { isFunctionsManaged } from "../../gcp/secretManager"; import * as utils from "../../utils"; import * as prompt from "../../prompt"; -/** Interface for holding the service account pair for a given Backend. */ -export interface ServiceAccounts { - buildServiceAccount: string; - runServiceAccount: string; -} - -/** - * Interface for holding a collection of service accounts we need to grant access to. - * Build accounts are special because they also need secret viewer permissions to view versions - * and pin to the latest. Run accounts only need version accessor. - */ -export interface MultiServiceAccounts { - buildServiceAccounts: string[]; - runServiceAccounts: string[]; -} - -/** Utility function to turn a single ServiceAccounts into a MultiServiceAccounts. */ -export function toMulti(accounts: ServiceAccounts): MultiServiceAccounts { - const m: MultiServiceAccounts = { - buildServiceAccounts: [accounts.buildServiceAccount], - runServiceAccounts: [], - }; - if (accounts.buildServiceAccount !== accounts.runServiceAccount) { - m.runServiceAccounts.push(accounts.runServiceAccount); - } - return m; -} - /** * Finds the explicit service account used for a backend or, for legacy cases, * the defaults for GCB and compute. @@ -44,17 +16,11 @@ export function toMulti(accounts: ServiceAccounts): MultiServiceAccounts { export function serviceAccountsForBackend( projectNumber: string, backend: apphosting.Backend, -): ServiceAccounts { +): string[] { if (backend.serviceAccount) { - return { - buildServiceAccount: backend.serviceAccount, - runServiceAccount: backend.serviceAccount, - }; + return [backend.serviceAccount]; } - return { - buildServiceAccount: gcb.getDefaultServiceAccount(projectNumber), - runServiceAccount: gce.getDefaultServiceAccount(projectNumber), - }; + return [gcb.getDefaultServiceAccount(projectNumber), gce.getDefaultServiceAccount(projectNumber)]; } /** @@ -63,20 +29,19 @@ export function serviceAccountsForBackend( export async function grantSecretAccess( projectId: string, secretName: string, - accounts: MultiServiceAccounts, + accounts: string[], ): Promise { + const members = accounts.map((a) => `serviceAccount:${a}`); const newBindings: iam.Binding[] = [ { role: "roles/secretmanager.secretAccessor", - members: [...accounts.buildServiceAccounts, ...accounts.runServiceAccounts].map( - (sa) => `serviceAccount:${sa}`, - ), + members, }, // Cloud Build needs the viewer role so that it can list secret versions and pin the Build to the // latest version. { role: "roles/secretmanager.viewer", - members: accounts.buildServiceAccounts.map((sa) => `serviceAccount:${sa}`), + members, }, ]; diff --git a/src/test/apphosting/secrets/dialogs.spec.ts b/src/test/apphosting/secrets/dialogs.spec.ts index e5a80b7a3d8..e6d4eeec568 100644 --- a/src/test/apphosting/secrets/dialogs.spec.ts +++ b/src/test/apphosting/secrets/dialogs.spec.ts @@ -28,19 +28,14 @@ describe("dialogs", () => { name: "projects/p/locations/l/backends/legacy2", } as any as apphosting.Backend; - const emptyMulti: secrets.MultiServiceAccounts = { - buildServiceAccounts: [], - runServiceAccounts: [], - }; - describe("toMetadata", () => { it("handles explicit account", () => { // Note: passing in out of order to verify the results are sorted. const metadata = dialogs.toMetadata("number", [modernA2, modernA]); expect(metadata).to.deep.equal([ - { location: "l", id: "modernA", buildServiceAccount: "a", runServiceAccount: "a" }, - { location: "l2", id: "modernA2", buildServiceAccount: "a", runServiceAccount: "a" }, + { location: "l", id: "modernA", serviceAccounts: ["a"] }, + { location: "l2", id: "modernA2", serviceAccounts: ["a"] }, ]); }); @@ -51,9 +46,9 @@ describe("dialogs", () => { { location: "l", id: "legacy", - ...secrets.serviceAccountsForBackend("number", legacy), + serviceAccounts: secrets.serviceAccountsForBackend("number", legacy), }, - { location: "l", id: "modernA", buildServiceAccount: "a", runServiceAccount: "a" }, + { location: "l", id: "modernA", serviceAccounts: ["a"] }, ]); }); @@ -63,23 +58,14 @@ describe("dialogs", () => { { location: "l", id: "legacy", - ...secrets.serviceAccountsForBackend("number", legacy), + serviceAccounts: secrets.serviceAccountsForBackend("number", legacy), }, - { location: "l", id: "modernA", buildServiceAccount: "a", runServiceAccount: "a" }, - { location: "l2", id: "modernA2", buildServiceAccount: "a", runServiceAccount: "a" }, + { location: "l", id: "modernA", serviceAccounts: ["a"] }, + { location: "l2", id: "modernA2", serviceAccounts: ["a"] }, ]); }); }); - it("serviceAccountDisplay", () => { - expect( - dialogs.serviceAccountDisplay({ buildServiceAccount: "build", runServiceAccount: "run" }), - ).to.equal("build, run"); - expect( - dialogs.serviceAccountDisplay({ buildServiceAccount: "common", runServiceAccount: "common" }), - ).to.equal("common"); - }); - describe("tableForBackends", () => { it("uses 'service account' header if all backends use one service account", () => { const table = dialogs.tableForBackends(dialogs.toMetadata("number", [modernA, modernB])); @@ -95,37 +81,12 @@ describe("dialogs", () => { const legacyAccounts = secrets.serviceAccountsForBackend("number", legacy); expect(table[0]).to.deep.equal(["location", "backend", "service accounts"]); expect(table[1]).to.deep.equal([ - [ - "l", - "legacy", - `${legacyAccounts.buildServiceAccount}, ${legacyAccounts.runServiceAccount}`, - ], + ["l", "legacy", legacyAccounts.join(", ")], ["l", "modernA", "a"], ]); }); }); - it("selectFromMetadata", () => { - const metadata: secrets.ServiceAccounts[] = [ - { - buildServiceAccount: "build", - runServiceAccount: "run", - }, - { - buildServiceAccount: "common", - runServiceAccount: "common", - }, - { - buildServiceAccount: "omittedBuild", - runServiceAccount: "omittedRun", - }, - ]; - expect(dialogs.selectFromMetadata(metadata, ["build", "run", "common"])).to.deep.equal({ - buildServiceAccounts: ["build", "common"], - runServiceAccounts: ["run"], - }); - }); - describe("selectBackendServiceAccounts", () => { let listBackends: sinon.SinonStub; let utils: sinon.SinonStubbedInstance; @@ -149,7 +110,7 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(emptyMulti); + ).to.eventually.deep.equal([]); expect(utils.logLabeledWarning).to.have.been.calledWith( "apphosting", dialogs.WARN_NO_BACKENDS, @@ -164,7 +125,7 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(emptyMulti); + ).to.eventually.deep.equal([]); expect(utils.logLabeledWarning).to.have.been.calledWith( "apphosting", @@ -186,10 +147,7 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal({ - buildServiceAccounts: [modernA.serviceAccount], - runServiceAccounts: [], - }); + ).to.eventually.deep.equal([modernA.serviceAccount]); expect(prompt.confirm).to.have.been.calledWith({ nonInteractive: undefined, @@ -209,7 +167,7 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(emptyMulti); + ).to.eventually.deep.equal([]); expect(prompt.confirm).to.have.been.calledWith({ nonInteractive: undefined, @@ -233,12 +191,12 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(secrets.toMulti(accounts)); + ).to.eventually.deep.equal(accounts); expect(utils.logLabeledBullet).to.have.been.calledWith( "apphosting", "To use this secret, your backend's service account must have secret accessor permission. " + - `All of your backends use service accounts ${dialogs.serviceAccountDisplay(accounts)}. ` + + `All of your backends use service accounts ${accounts.join(", ")}. ` + "Granting access to one backend will grant access to all backends.", ); expect(prompt.confirm).to.have.been.calledWith({ @@ -259,12 +217,12 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(emptyMulti); + ).to.eventually.deep.equal([]); expect(utils.logLabeledBullet).to.have.been.calledWith( "apphosting", "To use this secret, your backend's service account must have secret accessor permission. " + - `All of your backends use service accounts ${dialogs.serviceAccountDisplay(legacyAccounts)}. ` + + `All of your backends use service accounts ${legacyAccounts.join(", ")}. ` + "Granting access to one backend will grant access to all backends.", ); expect(prompt.confirm).to.have.been.calledWith({ @@ -287,10 +245,7 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal({ - buildServiceAccounts: [modernA.serviceAccount], - runServiceAccounts: [], - }); + ).to.eventually.deep.equal([modernA.serviceAccount]); expect(utils.logLabeledBullet).to.have.been.calledWith( "apphosting", @@ -315,7 +270,7 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(emptyMulti); + ).to.eventually.deep.equal([]); expect(utils.logLabeledBullet).to.have.been.calledWith( "apphosting", @@ -344,18 +299,13 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal({ buildServiceAccounts: ["a", "b"], runServiceAccounts: [] }); + ).to.eventually.deep.equal(["a", "b"]); expect(prompt.promptOnce).to.have.been.calledWith({ type: "checkbox", message: "Which service accounts would you like to grant access? Press Space to select accounts, then Enter to confirm your choices.", - choices: [ - "a", - "b", - legacyAccounts.buildServiceAccount, - legacyAccounts.runServiceAccount, - ].sort(), + choices: ["a", "b", ...legacyAccounts].sort(), }); expect(utils.logLabeledBullet).to.have.been.calledWith( "apphosting", @@ -377,18 +327,13 @@ describe("dialogs", () => { await expect( dialogs.selectBackendServiceAccounts("number", "id", {}), - ).to.eventually.deep.equal(emptyMulti); + ).to.eventually.deep.equal([]); expect(prompt.promptOnce).to.have.been.calledWith({ type: "checkbox", message: "Which service accounts would you like to grant access? Press Space to select accounts, then Enter to confirm your choices.", - choices: [ - "a", - "b", - legacyAccounts.buildServiceAccount, - legacyAccounts.runServiceAccount, - ].sort(), + choices: ["a", "b", ...legacyAccounts].sort(), }); expect(utils.logLabeledBullet).to.have.been.calledWith( "apphosting", diff --git a/src/test/apphosting/secrets/index.spec.ts b/src/test/apphosting/secrets/index.spec.ts index 174a3bdd413..5821515bec8 100644 --- a/src/test/apphosting/secrets/index.spec.ts +++ b/src/test/apphosting/secrets/index.spec.ts @@ -34,18 +34,15 @@ describe("secrets", () => { const backend = { serviceAccount: "sa", } as any as apphosting.Backend; - expect(secrets.serviceAccountsForBackend("number", backend)).to.deep.equal({ - buildServiceAccount: "sa", - runServiceAccount: "sa", - }); + expect(secrets.serviceAccountsForBackend("number", backend)).to.deep.equal(["sa"]); }); it("has a fallback for legacy SAs", () => { const backend = {} as any as apphosting.Backend; - expect(secrets.serviceAccountsForBackend("number", backend)).to.deep.equal({ - buildServiceAccount: gcb.getDefaultServiceAccount("number"), - runServiceAccount: gce.getDefaultServiceAccount("number"), - }); + expect(secrets.serviceAccountsForBackend("number", backend)).to.deep.equal([ + gcb.getDefaultServiceAccount("number"), + gce.getDefaultServiceAccount("number"), + ]); }); }); @@ -186,26 +183,6 @@ describe("secrets", () => { }); }); - describe("toMulti", () => { - it("handles different service accounts", () => { - expect( - secrets.toMulti({ buildServiceAccount: "buildSA", runServiceAccount: "computeSA" }), - ).to.deep.equal({ - buildServiceAccounts: ["buildSA"], - runServiceAccounts: ["computeSA"], - }); - }); - - it("handles the same service account", () => { - expect( - secrets.toMulti({ buildServiceAccount: "explicitSA", runServiceAccount: "explicitSA" }), - ).to.deep.equal({ - buildServiceAccounts: ["explicitSA"], - runServiceAccounts: [], - }); - }); - }); - describe("grantSecretAccess", () => { const secret = { name: "secret", @@ -226,10 +203,7 @@ describe("secrets", () => { gcsm.getIamPolicy.resolves(existingPolicy); gcsm.setIamPolicy.resolves(); - await secrets.grantSecretAccess(secret.projectId, secret.name, { - buildServiceAccounts: ["buildSA"], - runServiceAccounts: ["computeSA"], - }); + await secrets.grantSecretAccess(secret.projectId, secret.name, ["buildSA", "computeSA"]); const newBindings: iam.Binding[] = [ { @@ -242,7 +216,7 @@ describe("secrets", () => { }, { role: "roles/secretmanager.viewer", - members: ["serviceAccount:buildSA"], + members: ["serviceAccount:buildSA", "serviceAccount:computeSA"], }, ];