Repository: serverless/serverless-azure-functions
Pull request #295 (2019-08-30 21:01:18)

fix: Don't throw error if subscription ID already specified
Checks to see if subscription ID is already specified before throwing error message for empty subscription array from an authentication result.

Apparently when a service principal authenticates, there are no subscriptions in the returned array.


Patch:
diff --git a/package-lock.json b/package-lock.json
index 81b4503..6b9e0de 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -5164,8 +5164,7 @@
         },
         "console-control-strings": {
           "version": "1.1.0",
-          "bundled": true,
-          "optional": true
+          "bundled": true
         },
         "core-util-is": {
           "version": "1.0.2",
@@ -5268,8 +5267,7 @@
         },
         "inherits": {
           "version": "2.0.3",
-          "bundled": true,
-          "optional": true
+          "bundled": true
         },
         "ini": {
           "version": "1.3.5",
@@ -5303,7 +5301,6 @@
         "minipass": {
           "version": "2.3.5",
           "bundled": true,
-          "optional": true,
           "requires": {
             "safe-buffer": "^5.1.2",
             "yallist": "^3.0.0"
@@ -5320,7 +5317,6 @@
         "mkdirp": {
           "version": "0.5.1",
           "bundled": true,
-          "optional": true,
           "requires": {
             "minimist": "0.0.8"
           }
@@ -5393,7 +5389,8 @@
         },
         "number-is-nan": {
           "version": "1.0.1",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "object-assign": {
           "version": "4.1.1",
@@ -5403,7 +5400,6 @@
         "once": {
           "version": "1.4.0",
           "bundled": true,
-          "optional": true,
           "requires": {
             "wrappy": "1"
           }
@@ -5479,8 +5475,7 @@
         },
         "safe-buffer": {
           "version": "5.1.2",
-          "bundled": true,
-          "optional": true
+          "bundled": true
         },
         "safer-buffer": {
           "version": "2.1.2",
@@ -5510,7 +5505,6 @@
         "string-width": {
           "version": "1.0.2",
           "bundled": true,
-          "optional": true,
           "requires": {
             "code-point-at": "^1.0.0",
             "is-fullwidth-code-point": "^1.0.0",
@@ -5567,13 +5561,11 @@
         },
         "wrappy": {
           "version": "1.0.2",
-          "bundled": true,
-          "optional": true
+          "bundled": true
         },
         "yallist": {
           "version": "3.0.3",
-          "bundled": true,
-          "optional": true
+          "bundled": true
         }
       }
     },
diff --git a/src/plugins/login/azureLoginPlugin.ts b/src/plugins/login/azureLoginPlugin.ts
index feafe67..137b387 100644
--- a/src/plugins/login/azureLoginPlugin.ts
+++ b/src/plugins/login/azureLoginPlugin.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}`);
     }
diff --git a/src/services/azureBlobStorageService.ts b/src/services/azureBlobStorageService.ts
index ba04917..144e7bf 100644
--- a/src/services/azureBlobStorageService.ts
+++ b/src/services/azureBlobStorageService.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");
   }
 }
diff --git a/src/services/loginService.ts b/src/services/loginService.ts
index 6bf433f..8ed84dc 100644
--- a/src/services/loginService.ts
+++ b/src/services/loginService.ts
@@ -6,34 +6,40 @@ import {
   AuthResponse,
   AzureTokenCredentialsOptions,
   InteractiveLoginOptions,
-  DeviceTokenCredentials,  
+  DeviceTokenCredentials,
 } from "@azure/ms-rest-nodeauth";
 import { SimpleFileTokenCache } from "../plugins/login/utils/simpleFileTokenCache";
+import { BaseService } from "./baseService";
 
 export interface AzureLoginOptions extends Serverless.Options {
   subscriptionId?: string;
 }
 
-export class AzureLoginService {
+export class AzureLoginService extends BaseService {
+
+  public constructor(serverless: Serverless, options: Serverless.Options) {
+    super(serverless, options, false);
+  }
+
   /**
    * Logs in via service principal login if environment variables are
    * set or via interactive login if environment variables are not set
    * @param options Options for different authentication methods
    */
-  public static async login(options?: AzureTokenCredentialsOptions|InteractiveLoginOptions): Promise<AuthResponse> {
+  public async login(options?: AzureTokenCredentialsOptions|InteractiveLoginOptions): Promise<AuthResponse> {
     const subscriptionId = process.env.azureSubId;
     const clientId = process.env.azureServicePrincipalClientId;
     const secret = process.env.azureServicePrincipalPassword;
     const tenantId = process.env.azureServicePrincipalTenantId;
 
     if (subscriptionId && clientId && secret && tenantId) {
-      return await AzureLoginService.servicePrincipalLogin(clientId, secret, tenantId, options);
+      return await this.servicePrincipalLogin(clientId, secret, tenantId, options);
     } else {
-      return await AzureLoginService.interactiveLogin(options);
+      return await this.interactiveLogin(options);
     }
   }
 
-  public static async interactiveLogin(options?: InteractiveLoginOptions): Promise<AuthResponse> {
+  public async interactiveLogin(options?: InteractiveLoginOptions): Promise<AuthResponse> {
     let authResp: AuthResponse = {credentials: undefined, subscriptions: []};
     const fileTokenCache = new SimpleFileTokenCache();
     if(fileTokenCache.isEmpty()){
@@ -49,7 +55,7 @@ export class AzureLoginService {
 
   }
 
-  public static async servicePrincipalLogin(clientId: string, secret: string, tenantId: string, options: AzureTokenCredentialsOptions): Promise<AuthResponse> {
+  public async servicePrincipalLogin(clientId: string, secret: string, tenantId: string, options: AzureTokenCredentialsOptions): Promise<AuthResponse> {
     return loginWithServicePrincipalSecretWithAuthResponse(clientId, secret, tenantId, options);
   }
 }


Test patch:
diff --git a/src/plugins/login/azureLoginPlugin.test.ts b/src/plugins/login/azureLoginPlugin.test.ts
index 80f7461..292b1c4 100644
--- a/src/plugins/login/azureLoginPlugin.test.ts
+++ b/src/plugins/login/azureLoginPlugin.test.ts
@@ -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,8 +77,8 @@ 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");
   });
@@ -85,13 +86,13 @@ describe("Login Plugin", () => {
   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");
   });
 
@@ -99,18 +100,34 @@ describe("Login Plugin", () => {
     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();
   });
 });
diff --git a/src/services/azureBlobStorageService.test.ts b/src/services/azureBlobStorageService.test.ts
index 6eb6e62..1d84f65 100644
--- a/src/services/azureBlobStorageService.test.ts
+++ b/src/services/azureBlobStorageService.test.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));
diff --git a/src/services/loginService.test.ts b/src/services/loginService.test.ts
index cfbc7bd..032d904 100644
--- a/src/services/loginService.test.ts
+++ b/src/services/loginService.test.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",