Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add manual check for turtle service accounts #7041

Merged
merged 4 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 39 additions & 17 deletions src/apphosting/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
cloudRunApiOrigin,
cloudbuildOrigin,
developerConnectOrigin,
iamOrigin,
secretManagerOrigin,
} from "../api";
import { Backend, BackendOutputOnlyFields, API_VERSION, Build, Rollout } from "../gcp/apphosting";
Expand Down Expand Up @@ -90,6 +91,8 @@
message: "Specify your app's root directory relative to your repository",
});

await ensureAppHostingComputeServiceAccount(projectId, serviceAccount);

const backend = await createBackend(
projectId,
location,
Expand Down Expand Up @@ -137,25 +140,60 @@
logSuccess(`Your backend is now deployed at:\n\thttps://${backend.uri}`);
}

/**
* Ensures the service account is present the user has permissions to use it by
* checking the `iam.serviceAccounts.actAs` permission. If the permissions
* check fails, this returns an error. If the permission check fails with a
* "not found" error, this attempts to provision the service account.
*/
export async function ensureAppHostingComputeServiceAccount(
projectId: string,
serviceAccount: string | null,
): Promise<void> {
const sa = serviceAccount || defaultComputeServiceAccountEmail(projectId);
const name = `projects/${projectId}/serviceAccounts/${sa}`;
try {
await iam.testResourceIamPermissions(
iamOrigin(),
"v1",
name,
["iam.serviceAccounts.actAs"],
`projects/${projectId}`,
);
} catch (err: unknown) {
if (!(err instanceof FirebaseError)) {
throw err;
}
if (err.status === 404) {
await provisionDefaultComputeServiceAccount(projectId);
} else if (err.status === 403) {
throw new FirebaseError(
`Failed to create backend due to missing delegation permissions for ${sa}. Make sure you have the iam.serviceAccounts.actAs permission.`,
{ original: err },
);
}
}
}

/**
* Prompts the user for a backend id and verifies that it doesn't match a pre-existing backend.
*/
async function promptNewBackendId(
projectId: string,
location: string,
prompt: any,

Check warning on line 184 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
): Promise<string> {
while (true) {

Check warning on line 186 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected constant condition
const backendId = await promptOnce(prompt);

Check warning on line 187 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `QuestionsThatReturnAString<Answers>`
try {
await apphosting.getBackend(projectId, location, backendId);
} catch (err: any) {

Check warning on line 190 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
if (err.status === 404) {

Check warning on line 191 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .status on an `any` value
return backendId;
}
throw new FirebaseError(
`Failed to check if backend with id ${backendId} already exists in ${location}`,
{ original: err },

Check warning on line 196 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
);
}
logWarning(`Backend with id ${backendId} already exists in ${location}`);
Expand Down Expand Up @@ -200,23 +238,7 @@
});
}

try {
return await createBackendAndPoll();
} catch (err: any) {
if (err.status === 403) {
if (err.message.includes(defaultServiceAccount)) {
// Create the default service account if it doesn't exist and try again.
await provisionDefaultComputeServiceAccount(projectId);
return await createBackendAndPoll();
} else if (serviceAccount && err.message.includes(serviceAccount)) {
throw new FirebaseError(
`Failed to create backend due to missing delegation permissions for ${serviceAccount}. Make sure you have the iam.serviceAccounts.actAs permission.`,
{ children: [err] },
);
}
}
throw err;
}
return await createBackendAndPoll();
}

async function provisionDefaultComputeServiceAccount(projectId: string): Promise<void> {
Expand All @@ -227,9 +249,9 @@
"Firebase App Hosting compute service account",
"Default service account used to run builds and deploys for Firebase App Hosting",
);
} catch (err: any) {

Check warning on line 252 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
// 409 Already Exists errors can safely be ignored.
if (err.status !== 409) {

Check warning on line 254 in src/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .status on an `any` value
throw err;
}
}
Expand Down
139 changes: 61 additions & 78 deletions src/test/apphosting/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import {
deleteBackendAndPoll,
promptLocation,
setDefaultTrafficPolicy,
ensureAppHostingComputeServiceAccount,
} from "../../apphosting/index";
import * as deploymentTool from "../../deploymentTool";
import { FirebaseError } from "../../error";

describe("operationsConverter", () => {
const sandbox: sinon.SinonSandbox = sinon.createSandbox();
describe("apphosting setup functions", () => {
const projectId = "projectId";
const location = "us-central1";
const backendId = "backendId";
Expand All @@ -29,37 +29,39 @@ describe("operationsConverter", () => {
let listLocationsStub: sinon.SinonStub;
let createServiceAccountStub: sinon.SinonStub;
let addServiceAccountToRolesStub: sinon.SinonStub;
let testResourceIamPermissionsStub: sinon.SinonStub;

beforeEach(() => {
promptOnceStub = sandbox.stub(prompt, "promptOnce").throws("Unexpected promptOnce call");
pollOperationStub = sandbox
.stub(poller, "pollOperation")
.throws("Unexpected pollOperation call");
createBackendStub = sandbox
promptOnceStub = sinon.stub(prompt, "promptOnce").throws("Unexpected promptOnce call");
pollOperationStub = sinon.stub(poller, "pollOperation").throws("Unexpected pollOperation call");
createBackendStub = sinon
.stub(apphosting, "createBackend")
.throws("Unexpected createBackend call");
deleteBackendStub = sandbox
deleteBackendStub = sinon
.stub(apphosting, "deleteBackend")
.throws("Unexpected deleteBackend call");
updateTrafficStub = sandbox
updateTrafficStub = sinon
.stub(apphosting, "updateTraffic")
.throws("Unexpected updateTraffic call");
listLocationsStub = sandbox
listLocationsStub = sinon
.stub(apphosting, "listLocations")
.throws("Unexpected listLocations call");
createServiceAccountStub = sandbox
createServiceAccountStub = sinon
.stub(iam, "createServiceAccount")
.throws("Unexpected createServiceAccount call");
addServiceAccountToRolesStub = sandbox
addServiceAccountToRolesStub = sinon
.stub(resourceManager, "addServiceAccountToRoles")
.throws("Unexpected addServiceAccountToRoles call");
testResourceIamPermissionsStub = sinon
.stub(iam, "testResourceIamPermissions")
.throws("Unexpected testResourceIamPermissions call");
});

afterEach(() => {
sandbox.verifyAndRestore();
sinon.verifyAndRestore();
});

describe("onboardBackend", () => {
describe("createBackend", () => {
const webAppId = "webAppId";

const op = {
Expand Down Expand Up @@ -108,68 +110,6 @@ describe("operationsConverter", () => {
expect(createBackendStub).to.be.calledWith(projectId, location, backendInput);
});

it("should provision the default compute service account", async () => {
createBackendStub.resolves(op);
pollOperationStub
// Initial CreateBackend operation should throw a permission denied to trigger service account creation.
.onFirstCall()
.throws(
new FirebaseError(
`missing actAs permission on firebase-app-hosting-compute@${projectId}.iam.gserviceaccount.com`,
{ status: 403 },
),
)
.onSecondCall()
.resolves(completeBackend);
createServiceAccountStub.resolves({});
addServiceAccountToRolesStub.resolves({});

await createBackend(
projectId,
location,
backendId,
cloudBuildConnRepo,
/* serviceAccount= */ null,
webAppId,
);

// CreateBackend should be called twice; once initially and once after the service account was created
expect(createBackendStub).to.be.calledTwice;
expect(createServiceAccountStub).to.be.calledOnce;
bkendall marked this conversation as resolved.
Show resolved Hide resolved
expect(addServiceAccountToRolesStub).to.be.calledOnce;
});

it("does not try to provision a custom service account", () => {
createBackendStub.resolves(op);
pollOperationStub
// Initial CreateBackend operation should throw a permission denied to
// potentially trigger service account creation.
.onFirstCall()
.throws(
new FirebaseError("missing actAs permission on my-service-account", { status: 403 }),
)
.onSecondCall()
.resolves(completeBackend);

expect(
createBackend(
projectId,
location,
backendId,
cloudBuildConnRepo,
/* serviceAccount= */ "my-service-account",
webAppId,
),
).to.be.rejectedWith(
FirebaseError,
"Failed to create backend due to missing delegation permissions for my-service-account. Make sure you have the iam.serviceAccounts.actAs permission.",
);

expect(createBackendStub).to.be.calledOnce;
expect(createServiceAccountStub).to.not.have.been.called;
expect(addServiceAccountToRolesStub).to.not.have.been.called;
});

it("should set default rollout policy to 100% all at once", async () => {
const completeTraffic: apphosting.Traffic = {
name: `projects/${projectId}/locations/${location}/backends/${backendId}/traffic`,
Expand Down Expand Up @@ -199,7 +139,50 @@ describe("operationsConverter", () => {
});
});

describe("delete backend", () => {
describe("ensureAppHostingComputeServiceAccount", () => {
const serviceAccount = "[email protected]";

it("should succeed if the user has permissions for the service account", async () => {
testResourceIamPermissionsStub.resolves();

await expect(ensureAppHostingComputeServiceAccount(projectId, serviceAccount)).to.be
.fulfilled;
});

it("should succeed if the user can create the service account when it does not exist", async () => {
testResourceIamPermissionsStub.rejects(
new FirebaseError("Permission denied", { status: 404 }),
);
createServiceAccountStub.resolves();
addServiceAccountToRolesStub.resolves();

await expect(ensureAppHostingComputeServiceAccount(projectId, serviceAccount)).to.be
.fulfilled;
});

it("should throw an error if the user does not have permissions", async () => {
testResourceIamPermissionsStub.rejects(
new FirebaseError("Permission denied", { status: 403 }),
);

await expect(
ensureAppHostingComputeServiceAccount(projectId, serviceAccount),
).to.be.rejectedWith(/Failed to create backend due to missing delegation permissions/);
});

it("should throw the error if the user cannot create the service account", async () => {
testResourceIamPermissionsStub.rejects(
new FirebaseError("Permission denied", { status: 404 }),
);
createServiceAccountStub.rejects(new FirebaseError("failed to create SA"));

await expect(
ensureAppHostingComputeServiceAccount(projectId, serviceAccount),
).to.be.rejectedWith("failed to create SA");
});
});

describe("deleteBackendAndPoll", () => {
it("should delete a backend", async () => {
const op = {
name: `projects/${projectId}/locations/${location}/backends/${backendId}`,
Expand All @@ -214,7 +197,7 @@ describe("operationsConverter", () => {
});
});

describe("prompt location", () => {
describe("promptLocation", () => {
const supportedLocations = [{ name: "us-central1", locationId: "us-central1" }];

beforeEach(() => {
Expand Down
Loading