fix: Don't throw error if subscription ID already specified (#295)

Author: tbarlow12

package-lock.json

@@ -4,6 +4,7 @@ import { MockFactory } from "../../test/mockFactory";
 import { invokeHook, setEnvVariables, unsetEnvVariables } from "../../test/utils";
 import { AzureLoginPlugin } from "./azureLoginPlugin";
 import { loginHooks } from "./loginHooks";
+import { ServerlessAzureConfig } from "../../models/serverless";
 
 describe("Login Plugin", () => {
 
@@ -36,8 +37,8 @@ describe("Login Plugin", () => {
   }
 
   beforeEach(() => {
-    AzureLoginService.interactiveLogin = createMockLoginFunction();
-    AzureLoginService.servicePrincipalLogin = createMockLoginFunction();
+    AzureLoginService.prototype.interactiveLogin = createMockLoginFunction();
+    AzureLoginService.prototype.servicePrincipalLogin = createMockLoginFunction();
   });
 
   it("contains the hooks as contained in loginHooks", () => {
@@ -46,28 +47,28 @@ describe("Login Plugin", () => {
 
   it("returns if azure credentials are set", async () => {
     await invokeLoginHook(true);
-    expect(AzureLoginService.interactiveLogin).not.toBeCalled();
-    expect(AzureLoginService.servicePrincipalLogin).not.toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).not.toBeCalled();
+    expect(AzureLoginService.prototype.servicePrincipalLogin).not.toBeCalled();
   });
 
   it("calls login if azure credentials are not set", async () => {
     unsetServicePrincipalEnvVariables();
     await invokeLoginHook();
-    expect(AzureLoginService.interactiveLogin).toBeCalled();
-    expect(AzureLoginService.servicePrincipalLogin).not.toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).toBeCalled();
+    expect(AzureLoginService.prototype.servicePrincipalLogin).not.toBeCalled();
   });
 
   it("calls service principal login if environment variables are set", async () => {
     setServicePrincipalEnvVariables();
     const sls = MockFactory.createTestServerless();
     await invokeLoginHook(false, sls);
-    expect(AzureLoginService.servicePrincipalLogin).toBeCalledWith(
+    expect(AzureLoginService.prototype.servicePrincipalLogin).toBeCalledWith(
       "azureServicePrincipalClientId",
       "azureServicePrincipalPassword",
       "azureServicePrincipalTenantId",
       undefined // would be options
     )
-    expect(AzureLoginService.interactiveLogin).not.toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).not.toBeCalled();
     expect(JSON.stringify(sls.variables["azureCredentials"])).toEqual(JSON.stringify(credentials));
     expect(sls.variables["subscriptionId"]).toEqual("azureSubId");
   });
@@ -76,41 +77,57 @@ describe("Login Plugin", () => {
     unsetServicePrincipalEnvVariables();
     const sls = MockFactory.createTestServerless();
     await invokeLoginHook(false, sls);
-    expect(AzureLoginService.servicePrincipalLogin).not.toBeCalled();
-    expect(AzureLoginService.interactiveLogin).toBeCalled();
+    expect(AzureLoginService.prototype.servicePrincipalLogin).not.toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).toBeCalled();
     expect(JSON.stringify(sls.variables["azureCredentials"])).toEqual(JSON.stringify(credentials));
     expect(sls.variables["subscriptionId"]).toEqual("azureSubId");
   });
 
   it("logs an error from authentication and crashes with it", async () => {
     unsetServicePrincipalEnvVariables();
     const error = new Error("This is my error message")
-    AzureLoginService.interactiveLogin = jest.fn(() => {
+    AzureLoginService.prototype.interactiveLogin = jest.fn(() => {
       throw error;
     });
     const sls = MockFactory.createTestServerless();
     await expect(invokeLoginHook(false, sls)).rejects.toThrow(error);
-    expect(AzureLoginService.interactiveLogin).toBeCalled();
-    expect(AzureLoginService.servicePrincipalLogin).not.toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).toBeCalled();
+    expect(AzureLoginService.prototype.servicePrincipalLogin).not.toBeCalled();
     expect(sls.cli.log).lastCalledWith("Error logging into azure");
   });
 
   it("Uses the default subscription ID" , async () => {
     const sls = MockFactory.createTestServerless();
     const opt = MockFactory.createTestServerlessOptions();
     await invokeLoginHook(false, sls, opt);
-    expect(AzureLoginService.interactiveLogin).toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).toBeCalled();
     expect(sls.variables["subscriptionId"]).toEqual("azureSubId");
     expect(sls.cli.log).toBeCalledWith("Using subscription ID: azureSubId");
   });
 
   it("Throws an error with empty subscription list", async () => {
+    unsetServicePrincipalEnvVariables();
     const authResponse = MockFactory.createTestAuthResponse();
     authResponse.subscriptions = [];
-    AzureLoginService.interactiveLogin = createMockLoginFunction(authResponse);
+    AzureLoginService.prototype.interactiveLogin = createMockLoginFunction(authResponse);
     const sls = MockFactory.createTestServerless();
+    delete sls.variables["subscriptionId"];
     const opt = MockFactory.createTestServerlessOptions();
     await expect(invokeLoginHook(false, sls, opt)).rejects.toThrow();
-    expect(AzureLoginService.interactiveLogin).toBeCalled();
+    expect(AzureLoginService.prototype.interactiveLogin).toBeCalled();
+  });
+
+  it("Does not throw an error with empty subscription list if subscription previously specified", async () => {
+    unsetServicePrincipalEnvVariables();
+    const authResponse = MockFactory.createTestAuthResponse();
+    authResponse.subscriptions = [];
+    AzureLoginService.prototype.interactiveLogin = createMockLoginFunction(authResponse);
+    const sls = MockFactory.createTestServerless();
+    delete sls.variables["subscriptionId"];
+    const subId = "my subscription id";
+    (sls.service as any as ServerlessAzureConfig).provider.subscriptionId = subId;
+    const opt = MockFactory.createTestServerlessOptions();
+    await invokeLoginHook(false, sls, opt)
+    expect(AzureLoginService.prototype.interactiveLogin).toBeCalled();
   });
 });

src/plugins/login/azureLoginPlugin.test.ts

@@ -23,15 +23,18 @@ export class AzureLoginPlugin extends AzureBasePlugin<AzureLoginOptions> {
     this.log("Logging into Azure");
 
     try {
-      const authResult = await AzureLoginService.login();
+      const loginService = new AzureLoginService(this.serverless, this.options);
+      const authResult = await loginService.login();
+
       this.serverless.variables["azureCredentials"] = authResult.credentials;
       // Use environment variable for sub ID or use the first subscription in the list (service principal can
       // have access to more than one subscription)
-      if (!authResult.subscriptions.length) {
+      let subId = loginService.getSubscriptionId();
+      if (!subId && !authResult.subscriptions.length) {
         throw new Error("Authentication returned an empty list of subscriptions. " +
           "Try another form of authentication. See the serverless-azure-functions README for more help");
       }
-      const subId = authResult.subscriptions[0].id;
+      subId = subId || authResult.subscriptions[0].id;
       this.serverless.variables["subscriptionId"] = subId;
       this.serverless.cli.log(`Using subscription ID: ${subId}`);
     }

src/plugins/login/azureLoginPlugin.ts

@@ -49,7 +49,7 @@ describe("Azure Blob Storage Service", () => {
     }) as any;
 
     BlockBlobURL.fromContainerURL = jest.fn(() => blockBlobUrl) as any;
-    AzureLoginService.login = jest.fn(() => Promise.resolve({
+    AzureLoginService.prototype.login = jest.fn(() => Promise.resolve({
       credentials: {
         getToken: jest.fn(() => {
           return {
@@ -142,7 +142,7 @@ describe("Azure Blob Storage Service", () => {
     expect(ContainerURL.fromServiceURL).not.toBeCalled();
     expect(ContainerURL.prototype.create).not.toBeCalled
   })
-  
+
   it("should delete a container", async () => {
     const containerToDelete = "delete container";
     ContainerURL.fromServiceURL = jest.fn(() => new ContainerURL(null, null));

src/services/azureBlobStorageService.test.ts

@@ -45,7 +45,7 @@ export class AzureBlobStorageService extends BaseService {
     if (this.storageCredential) {
       return;
     }
-    this.storageCredential = (this.authType === AzureStorageAuthType.SharedKey) 
+    this.storageCredential = (this.authType === AzureStorageAuthType.SharedKey)
       ?
       new SharedKeyCredential(this.storageAccountName, await this.getKey())
       :
@@ -91,10 +91,10 @@ export class AzureBlobStorageService extends BaseService {
         blockSize: 4 * 1024 * 1024, // 4MB block size
         parallelism: 20, // 20 concurrency
       }
-    );    
+    );
     fs.writeFileSync(targetPath, buffer, "binary");
   }
-  
+
   /**
    * Delete a blob from Azure Blob Storage
    * @param containerName Name of container containing blob
@@ -195,7 +195,7 @@ export class AzureBlobStorageService extends BaseService {
   public async generateBlobSasTokenUrl(containerName: string, blobName: string, days: number = 365): Promise<string> {
     this.checkInitialization();
     if (this.authType !== AzureStorageAuthType.SharedKey) {
-      throw new Error("Need to authenticate with shared key in order to generate SAS tokens. " + 
+      throw new Error("Need to authenticate with shared key in order to generate SAS tokens. " +
       "Initialize Blob Service with SharedKey auth type");
     }
 
@@ -219,7 +219,7 @@ export class AzureBlobStorageService extends BaseService {
       version: "2016-05-31"
     },
     this.storageCredential as SharedKeyCredential);
-    
+
     const blobUrl = this.getBlockBlobURL(containerName, blobName);
     return `${blobUrl.url}?${blobSas}`
   }
@@ -274,7 +274,8 @@ export class AzureBlobStorageService extends BaseService {
    * Get access token by logging in (again) with a storage-specific context
    */
   private async getToken(): Promise<string> {
-    const authResponse = await AzureLoginService.login({
+    const loginService = new AzureLoginService(this.serverless, this.options);
+    const authResponse = await loginService.login({
       tokenAudience: "https://storage.azure.com/"
     });
     const token = await authResponse.credentials.getToken();
@@ -296,8 +297,8 @@ export class AzureBlobStorageService extends BaseService {
    * the credentials will not be available for any operation
    */
   private checkInitialization() {
-    Guard.null(this.storageCredential, "storageCredential", 
-      "Azure Blob Storage Service has not been initialized. Make sure .initialize() has been called " + 
+    Guard.null(this.storageCredential, "storageCredential",
+      "Azure Blob Storage Service has not been initialized. Make sure .initialize() has been called " +
       "before performing any operation");
   }
 }

src/services/azureBlobStorageService.ts

@@ -7,9 +7,19 @@ import * as nodeauth from "@azure/ms-rest-nodeauth";
 
 jest.mock("../plugins/login/utils/simpleFileTokenCache");
 import { SimpleFileTokenCache } from "../plugins/login/utils/simpleFileTokenCache";
+import { MockFactory } from "../test/mockFactory";
 
 describe("Login Service", () => {
 
+  let loginService: AzureLoginService;
+
+  beforeEach(() => {
+    loginService = new AzureLoginService(
+      MockFactory.createTestServerless(),
+      MockFactory.createTestServerlessOptions()
+    )
+  })
+
   it("logs in interactively with no cached login", async () => {
     // Ensure env variables are not set
     delete process.env.azureSubId;
@@ -25,7 +35,7 @@ describe("Login Service", () => {
       { value: jest.fn(() => emptyObj) }
     );
 
-    await AzureLoginService.login();
+    await loginService.login();
     expect(SimpleFileTokenCache).toBeCalled();
     expect(open).toBeCalledWith("https://microsoft.com/devicelogin");
     expect(nodeauth.interactiveLoginWithAuthResponse).toBeCalled();
@@ -42,7 +52,7 @@ describe("Login Service", () => {
     SimpleFileTokenCache.prototype.isEmpty = jest.fn(() => false);
     SimpleFileTokenCache.prototype.first = jest.fn(() => ({ userId: "" }));
 
-    await AzureLoginService.login();
+    await loginService.login();
     expect(SimpleFileTokenCache).toBeCalled();
     expect(nodeauth.DeviceTokenCredentials).toBeCalled();
     expect(SimpleFileTokenCache.prototype.listSubscriptions).toBeCalled();
@@ -55,7 +65,7 @@ describe("Login Service", () => {
     process.env.azureServicePrincipalPassword = "azureServicePrincipalPassword";
     process.env.azureServicePrincipalTenantId = "azureServicePrincipalTenantId";
 
-    await AzureLoginService.login();
+    await loginService.login();
     expect(nodeauth.loginWithServicePrincipalSecretWithAuthResponse).toBeCalledWith(
       "azureServicePrincipalClientId",
       "azureServicePrincipalPassword",