Corrects validation of secrets in extension installation (firebase#3901)

* Corrects validation of secrets in extension installation * Simplifies askForParam and promptCreateSecret. With this change promptCreateSecret and promptReconfigureSecret both now handle their validation within their own functions. Once they return a string from within askForParam, that string will be valid. I do have questions over how reconfiguring a secret works when you choose to leave the value as is. * Return paramSpec.default when not reconfiguring an extension secret * GCS fixes (firebase#3854) * adding service account api calls * fixing edge case for function regions/buckets/triggers, added code to set the iam role on storage service agent * spacing * adding reducer to clean up code * fix additional edge cases * addressing comments * cleaning up names * exposing more location apis and clean up * linter * refactor into services file * fixing comments * clean up sinon stubs * changing dispatch function to undefined for noop & pubsub services * linter * Change conditional back to a ternary * Adds entry to Changelog Co-authored-by: joehan <joehanley@google.com> Co-authored-by: Cole Rogers <colerogers@users.noreply.github.com>
devpeerapong · Dec 14, 2021 · cddcddd · cddcddd
1 parent e9d1e98
commit cddcddd
Show file tree

Hide file tree

Showing 3 changed files with 90 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1 +1,2 @@
+- Fixes issue when installing a Firebase Extension where secrets would be created before validation.
 - Fixes issue with filtering on a specific storage bucket using functions in the emulator (#3893)
diff --git a/src/extensions/askUserForParam.ts b/src/extensions/askUserForParam.ts
@@ -10,7 +10,6 @@ import { convertExtensionOptionToLabeledList, getRandomString, onceWithJoin } fr
 import { logger } from "../logger";
 import { promptOnce } from "../prompt";
 import * as utils from "../utils";
-import { instance } from "firebase-functions/v1/database";
 
 enum SecretUpdateAction {
   LEAVE,
@@ -94,6 +93,7 @@ export async function askForParam(
             "You may only select one option.",
           choices: convertExtensionOptionToLabeledList(paramSpec.options as ParamOption[]),
         });
+        valid = checkResponse(response, paramSpec);
         break;
       case ParamType.MULTISELECT:
         response = await onceWithJoin({
@@ -113,11 +113,13 @@ export async function askForParam(
             "You may select multiple options.",
           choices: convertExtensionOptionToLabeledList(paramSpec.options as ParamOption[]),
         });
+        valid = checkResponse(response, paramSpec);
         break;
       case ParamType.SECRET:
         response = reconfiguring
           ? await promptReconfigureSecret(projectId, instanceId, paramSpec)
           : await promptCreateSecret(projectId, instanceId, paramSpec);
+        valid = true;
         break;
       default:
         // Default to ParamType.STRING
@@ -127,9 +129,8 @@ export async function askForParam(
           default: paramSpec.default,
           message: `Enter a value for ${label}:`,
         });
+        valid = checkResponse(response, paramSpec);
     }
-
-    valid = checkResponse(response, paramSpec);
   }
   return response;
 }
@@ -157,20 +158,30 @@ async function promptReconfigureSecret(
       } else {
         secretName = await generateSecretName(projectId, instanceId, paramSpec.param);
       }
-
       const secretValue = await promptOnce({
         name: paramSpec.param,
         type: "password",
         message: `This secret will be stored in Cloud Secret Manager as ${secretName}.\nEnter new value for ${paramSpec.label.trim()}:`,
       });
-      if (!secret) {
-        secret = await secretManagerApi.createSecret(
-          projectId,
-          secretName,
-          secretsUtils.getSecretLabels(instanceId)
-        );
+      if (secretValue === "" && paramSpec.required) {
+        logger.info(`Secret value cannot be empty for required param ${paramSpec.param}`);
+        return promptReconfigureSecret(projectId, instanceId, paramSpec);
+      } else if (secretValue !== "") {
+        if (checkResponse(secretValue, paramSpec)) {
+          if (!secret) {
+            secret = await secretManagerApi.createSecret(
+              projectId,
+              secretName,
+              secretsUtils.getSecretLabels(instanceId)
+            );
+          }
+          return addNewSecretVersion(projectId, instanceId, secret, paramSpec, secretValue);
+        } else {
+          return promptReconfigureSecret(projectId, instanceId, paramSpec);
+        }
+      } else {
+        return "";
       }
-      return addNewSecretVersion(projectId, instanceId, secret, paramSpec, secretValue);
     case SecretUpdateAction.LEAVE:
     default:
       return paramSpec.default || "";
@@ -192,16 +203,21 @@ export async function promptCreateSecret(
   });
   if (secretValue === "" && paramSpec.required) {
     logger.info(`Secret value cannot be empty for required param ${paramSpec.param}`);
-    return await promptCreateSecret(projectId, instanceId, paramSpec, name);
+    return promptCreateSecret(projectId, instanceId, paramSpec, name);
   } else if (secretValue !== "") {
-    const secret = await secretManagerApi.createSecret(
-      projectId,
-      name,
-      secretsUtils.getSecretLabels(instanceId)
-    );
-    return addNewSecretVersion(projectId, instanceId, secret, paramSpec, secretValue);
+    if (checkResponse(secretValue, paramSpec)) {
+      const secret = await secretManagerApi.createSecret(
+        projectId,
+        name,
+        secretsUtils.getSecretLabels(instanceId)
+      );
+      return addNewSecretVersion(projectId, instanceId, secret, paramSpec, secretValue);
+    } else {
+      return promptCreateSecret(projectId, instanceId, paramSpec, name);
+    }
+  } else {
+    return "";
   }
-  return secretValue;
 }
 
 async function generateSecretName(

diff --git a/src/test/extensions/askUserForParam.spec.ts b/src/test/extensions/askUserForParam.spec.ts
@@ -11,6 +11,8 @@ import * as utils from "../../utils";
 import * as prompt from "../../prompt";
 import { ParamType } from "../../extensions/extensionsApi";
 import * as extensionsHelper from "../../extensions/extensionsHelper";
+import * as secretManagerApi from "../../gcp/secretManager";
+import * as secretsUtils from "../../extensions/secretsUtils";
 
 describe("askUserForParam", () => {
   const testSpec = {
@@ -198,7 +200,7 @@ describe("askUserForParam", () => {
       expect(res).to.equal("");
     });
   });
-  describe("askForParam", () => {
+  describe("askForParam with string param", () => {
     let promptStub: sinon.SinonStub;
 
     beforeEach(() => {
@@ -218,6 +220,57 @@ describe("askUserForParam", () => {
     });
   });
 
+  describe("askForParam with secret param", () => {
+    const stubSecret = {
+      name: "new-secret",
+      projectId: "firebase-project-123",
+    };
+    const stubSecretVersion = {
+      secret: stubSecret,
+      versionId: "1.0.0",
+    };
+    const secretSpec = {
+      param: "API_KEY",
+      type: ParamType.SECRET,
+      label: "API Key",
+      default: "XXX.YYY",
+    };
+
+    let promptStub: sinon.SinonStub;
+    let createSecret: sinon.SinonStub;
+    let secretExists: sinon.SinonStub;
+    let addVersion: sinon.SinonStub;
+    let grantRole: sinon.SinonStub;
+
+    beforeEach(() => {
+      promptStub = sinon.stub(prompt, "promptOnce");
+      promptStub.onCall(0).returns("ABC.123");
+
+      secretExists = sinon.stub(secretManagerApi, "secretExists");
+      secretExists.onCall(0).resolves(false);
+      createSecret = sinon.stub(secretManagerApi, "createSecret");
+      createSecret.onCall(0).resolves(stubSecret);
+      addVersion = sinon.stub(secretManagerApi, "addVersion");
+      addVersion.onCall(0).resolves(stubSecretVersion);
+
+      grantRole = sinon.stub(secretsUtils, "grantFirexServiceAgentSecretAdminRole");
+      grantRole.onCall(0).resolves(undefined);
+    });
+
+    afterEach(() => {
+      promptStub.restore();
+    });
+
+    it("should keep prompting user until valid input is given", async () => {
+      const result = await askForParam("project-id", "instance-id", secretSpec, false);
+      expect(promptStub.calledOnce).to.be.true;
+      expect(grantRole.calledOnce).to.be.true;
+      expect(result).to.be.equal(
+        `projects/${stubSecret.projectId}/secrets/${stubSecret.name}/versions/${stubSecretVersion.versionId}`
+      );
+    });
+  });
+
   describe("ask", () => {
     let subVarSpy: sinon.SinonSpy;
     let promptStub: sinon.SinonStub;