Fix "could not assert Secret Manager permissions" Cloud Build error (#…

…6904)

* fix secret manager admin permissions required bug for cloudbuild connections

* only ensureSecretManagerAdminGrant when creating an  oauth token

---------

Co-authored-by: Mathusan Selvarajah <mathusan@google.com>

src/init/features/apphosting/repo.ts

@@ -96,7 +96,6 @@ export async function linkGitHubRepository(
   const existingConns = await listAppHostingConnections(projectId);
 
   if (existingConns.length === 0) {
-    await ensureSecretManagerAdminGrant(projectId);
     existingConns.push(
       await createFullyInstalledConnection(projectId, location, generateConnectionId(), oauthConn),
     );
@@ -170,11 +169,24 @@ async function createFullyInstalledConnection(
  * Gets or creates the sentinel GitHub connection resource that contains our Firebase-wide GitHub Oauth token.
  * This Oauth token can be used to create other connections without reprompting the user to grant access.
  */
-async function getOrCreateOauthConnection(
+export async function getOrCreateOauthConnection(
   projectId: string,
   location: string,
 ): Promise<gcb.Connection> {
-  let conn = await getOrCreateConnection(projectId, location, APPHOSTING_OAUTH_CONN_NAME);
+  let conn: gcb.Connection;
+  try {
+    conn = await gcb.getConnection(projectId, location, APPHOSTING_OAUTH_CONN_NAME);
+  } catch (err: unknown) {
+    if ((err as any).status === 404) {
+      // Cloud build P4SA requires the secret manager admin role.
+      // This is required when creating an initial connection which is the Oauth connection in our case.
+      await ensureSecretManagerAdminGrant(projectId);
+      conn = await createConnection(projectId, location, APPHOSTING_OAUTH_CONN_NAME);
+    } else {
+      throw err;
+    }
+  }
+
   while (conn.installationState.stage === "PENDING_USER_OAUTH") {
     utils.logBullet("You must authorize the Cloud Build GitHub app.");
     utils.logBullet("Sign in to GitHub and authorize Cloud Build GitHub app:");

src/test/init/apphosting/repo.spec.ts

@@ -2,10 +2,12 @@ import * as sinon from "sinon";
 import { expect } from "chai";
 
 import * as gcb from "../../../gcp/cloudbuild";
+import * as rm from "../../../gcp/resourceManager";
 import * as prompt from "../../../prompt";
 import * as poller from "../../../operation-poller";
 import * as repo from "../../../init/features/apphosting/repo";
 import * as utils from "../../../utils";
+import * as srcUtils from "../../../../src/getProjectNumber";
 import { FirebaseError } from "../../../error";
 
 const projectId = "projectId";
@@ -53,8 +55,11 @@ describe("composer", () => {
     let getConnectionStub: sinon.SinonStub;
     let getRepositoryStub: sinon.SinonStub;
     let createConnectionStub: sinon.SinonStub;
+    let serviceAccountHasRolesStub: sinon.SinonStub;
     let createRepositoryStub: sinon.SinonStub;
     let fetchLinkableRepositoriesStub: sinon.SinonStub;
+    let getProjectNumberStub: sinon.SinonStub;
+    let openInBrowserPopupStub: sinon.SinonStub;
 
     beforeEach(() => {
       promptOnceStub = sandbox.stub(prompt, "promptOnce").throws("Unexpected promptOnce call");
@@ -70,13 +75,20 @@ describe("composer", () => {
       createConnectionStub = sandbox
         .stub(gcb, "createConnection")
         .throws("Unexpected createConnection call");
+      serviceAccountHasRolesStub = sandbox.stub(rm, "serviceAccountHasRoles").resolves(true);
       createRepositoryStub = sandbox
         .stub(gcb, "createRepository")
         .throws("Unexpected createRepository call");
       fetchLinkableRepositoriesStub = sandbox
         .stub(gcb, "fetchLinkableRepositories")
         .throws("Unexpected fetchLinkableRepositories call");
       sandbox.stub(utils, "openInBrowser").resolves();
+      openInBrowserPopupStub = sandbox
+        .stub(utils, "openInBrowserPopup")
+        .throws("Unexpected openInBrowserPopup call");
+      getProjectNumberStub = sandbox
+        .stub(srcUtils, "getProjectNumber")
+        .throws("Unexpected getProjectNumber call");
     });
 
     afterEach(() => {
@@ -139,6 +151,24 @@ describe("composer", () => {
       expect(createConnectionStub).to.be.calledWith(projectId, location, connectionId);
     });
 
+    it("checks if secret manager admin role is granted for cloud build P4SA when creating an oauth connection", async () => {
+      getConnectionStub.onFirstCall().rejects(new FirebaseError("error", { status: 404 }));
+      getConnectionStub.onSecondCall().resolves(completeConn);
+      createConnectionStub.resolves(op);
+      pollOperationStub.resolves(pendingConn);
+      promptOnceStub.resolves("any key");
+      getProjectNumberStub.onFirstCall().resolves(projectId);
+      openInBrowserPopupStub.resolves({ url: "", cleanup: sandbox.stub() });
+
+      await repo.getOrCreateOauthConnection(projectId, location);
+      expect(serviceAccountHasRolesStub).to.be.calledWith(
+        projectId,
+        `service-${projectId}@gcp-sa-cloudbuild.iam.gserviceaccount.com`,
+        ["roles/secretmanager.admin"],
+        true,
+      );
+    });
+
     it("creates repository if it doesn't exist", async () => {
       getConnectionStub.resolves(completeConn);
       fetchLinkableRepositoriesStub.resolves(repos);