Fix Azure#371: Allow optional subscriptionId in creds (Azure#373)

* fix371

* set subId once it is given

* modify test to logout first

* update tests

* update test

.github/workflows/azure-login-negative.yml

@@ -347,7 +347,12 @@ jobs:
           script: |
             core.setFailed('Last action should fail but not. Please check it.')
 
-    # Secret of SP1 in creds will be used to sign in SP2
+    # logout first to avoid the conflict with SP1
+    - name: Azure CLI logout
+      run: |
+        az logout
+
+    # SP1 is ignored and SP2 will be used for login, but it will fail since SP2 has no access to the given subscription
     - name: Login with both creds and individual parameters
       id: login_12
       continue-on-error: true
@@ -382,6 +387,37 @@ jobs:
       with:
           script: |
             core.setFailed('Last action should fail but not. Please check it.')
+    
+    - name: Login with individual parameters, no subscription-id, no allow-no-subscriptions
+      id: login_14
+      continue-on-error: true
+      uses: ./
+      with:
+        client-id: ${{ secrets.OIDC_SP2_CLIENT_ID }}
+        tenant-id: ${{ secrets.OIDC_SP2_TENANT_ID }}
+        enable-AzPSSession: true
+
+    - name: Check Last step failed
+      if: steps.login_14.outcome == 'success'
+      uses: actions/github-script@v3
+      with:
+          script: |
+            core.setFailed('Last action should fail but not. Please check it.')
+    
+    - name: Login with creds, no subscription-id, no allow-no-subscriptions
+      id: login_15
+      continue-on-error: true
+      uses: ./
+      with:
+        creds: '{"clientId":"${{ secrets.OIDC_SP2_CLIENT_ID }}","clientSecret":"${{ secrets.SP2_CLIENT_SECRET }}","tenantId":"${{ secrets.OIDC_SP2_TENANT_ID }}"}'
+        enable-AzPSSession: true
+
+    - name: Check Last step failed
+      if: steps.login_15.outcome == 'success'
+      uses: actions/github-script@v3
+      with:
+          script: |
+            core.setFailed('Last action should fail but not. Please check it.')
 
   VMTest:
     strategy:

.github/workflows/azure-login-positive.yml

@@ -61,10 +61,9 @@ jobs:
     - name: Login with individual parameters
       uses: ./
       with:
-        client-id: ${{ secrets.OIDC_SP2_CLIENT_ID }}
-        tenant-id: ${{ secrets.OIDC_SP2_TENANT_ID }}
-        # subscription-id: ${{ secrets.OIDC_SP2_SUBSCRIPTION_ID }}
-        allow-no-subscriptions: true
+        client-id: ${{ secrets.SP1_CLIENT_ID }}
+        tenant-id: ${{ secrets.SP1_TENANT_ID }}
+        subscription-id: ${{ secrets.SP1_SUBSCRIPTION_ID }}
         enable-AzPSSession: true
 
     - name: Run Azure Cli again
@@ -176,6 +175,29 @@ jobs:
                 throw "Not all checks passed!"
             }
 
+    - name: Login with individual parameters, allow no subscription
+      uses: ./
+      with:
+        client-id: ${{ secrets.SP1_CLIENT_ID }}
+        tenant-id: ${{ secrets.SP1_TENANT_ID}}
+        subscription-id: ${{ secrets.SP1_SUBSCRIPTION_ID }}
+        allow-no-subscriptions: true
+        enable-AzPSSession: true
+
+    - name: Run Azure Cli again
+      run: |
+        az account show --output none
+
+    - name: Run Azure PowerShell again
+      uses: azure/powershell@v1.2.0
+      with:
+        azPSVersion: "latest"
+        inlineScript: |
+            $checkResult = (Get-AzContext).Environment.Name -eq 'AzureCloud'
+            if(-not $checkResult){
+                throw "Not all checks passed!"
+            }
+
     - name: Login with individual parameters, no subscription, allow no subscription
       uses: ./
       with:
@@ -198,6 +220,27 @@ jobs:
                 throw "Not all checks passed!"
             }
 
+    - name: Login with creds, no subscription, allow no subscription
+      uses: ./
+      with:
+        creds: '{"clientId":"${{ secrets.OIDC_SP2_CLIENT_ID }}","clientSecret":"${{ secrets.SP2_CLIENT_SECRET }}","tenantId":"${{ secrets.OIDC_SP2_TENANT_ID }}"}'
+        allow-no-subscriptions: true
+        enable-AzPSSession: true
+
+    - name: Run Azure Cli
+      run: |
+        az account show --output none
+
+    - name: Run Azure PowerShell
+      uses: azure/powershell@v1.2.0
+      with:
+        azPSVersion: "latest"
+        inlineScript: |
+            $checkResult = (Get-AzContext).Environment.Name -eq 'AzureCloud'
+            if(-not $checkResult){
+                throw "Not all checks passed!"
+            }
+
   VMTest:
     strategy:
       matrix:

__tests__/LoginConfig.test.ts

@@ -74,16 +74,29 @@ describe("LoginConfig Test", () => {
         await testCreds(creds1);
 
     });
-
-    test('initialize with creds, lack of subscriptionId', async () => {
+   
+    test('initialize with creds, lack of subscriptionId, but allowNoSubscriptionsLogin=true', async () => {
         let creds1 = {
             'clientId': 'client-id',
             'clientSecret': 'client-secret',
             'tenantId': 'tenant-id',
             // 'subscriptionId': 'subscription-id'
         }
-        await testCreds(creds1);
-
+        setEnv('environment', 'azurecloud');
+        setEnv('enable-AzPSSession', 'true');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+        setEnv('creds', JSON.stringify(creds1));
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        expect(loginConfig.environment).toBe("azurecloud");
+        expect(loginConfig.enableAzPSSession).toBeTruthy();
+        expect(loginConfig.allowNoSubscriptionsLogin).toBeTruthy();
+        expect(loginConfig.authType).toBe("SERVICE_PRINCIPAL");
+        expect(loginConfig.servicePrincipalId).toBe("client-id");
+        expect(loginConfig.servicePrincipalSecret).toBe("client-secret");
+        expect(loginConfig.tenantId).toBe("tenant-id");
+        expect(loginConfig.subscriptionId).toBe("");
     });
 
     test('initialize with creds', async () => {

src/Cli/AzureCliLogin.ts

@@ -126,14 +126,13 @@ export class AzureCliLogin {
             args.push("--allow-no-subscriptions");
         }
         await this.executeAzCliCommand(args, true, this.loginOptions);
-        await this.setSubscription();
+        if (this.loginConfig.subscriptionId) {
+            await this.setSubscription();
+        }
         core.info(`Azure CLI login succeeds by using ${methodName}.`);
     }
 
     async setSubscription() {
-        if (this.loginConfig.allowNoSubscriptionsLogin) {
-            return;
-        }
         let args = ["account", "set", "--subscription", this.loginConfig.subscriptionId];
         await this.executeAzCliCommand(args, true, this.loginOptions);
         core.info("Subscription is set successfully.");
@@ -160,7 +159,7 @@ function defaultExecOptions(): exec.ExecOptions {
                 if (error && error.trim().length !== 0 && !startsWithWarning) {
                     if (startsWithError) {
                         //removing the keyword 'ERROR' to avoid duplicates while throwing error
-                        error = error.slice(5);
+                        error = error.slice(7);
                     }
                     core.error(error);
                 }

src/common/LoginConfig.ts

@@ -69,8 +69,8 @@ export class LoginConfig {
         this.tenantId = this.tenantId ? this.tenantId : secrets.getSecret("$.tenantId", false);
         this.subscriptionId = this.subscriptionId ? this.subscriptionId : secrets.getSecret("$.subscriptionId", false);
         this.resourceManagerEndpointUrl = secrets.getSecret("$.resourceManagerEndpointUrl", false);
-        if (!this.servicePrincipalId || !this.servicePrincipalSecret || !this.tenantId || !this.subscriptionId) {
-            throw new Error("Not all parameters are provided in 'creds'. Double-check if all keys are defined in 'creds': 'clientId', 'clientSecret', 'subscriptionId', 'tenantId'.");
+        if (!this.servicePrincipalId || !this.servicePrincipalSecret || !this.tenantId) {
+            throw new Error("Not all parameters are provided in 'creds'. Double-check if all keys are defined in 'creds': 'clientId', 'clientSecret', 'tenantId'.");
         }
     }