adjust logic according to review comments

Azure · Oct 11, 2023 · 49abb07 · 49abb07
1 parent 0514d07
commit 49abb07
Show file tree

Hide file tree

Showing 6 changed files with 404 additions and 86 deletions.
diff --git a/__tests__/LoginConfig.test.ts b/__tests__/LoginConfig.test.ts
@@ -0,0 +1,259 @@
+import { LoginConfig } from "../src/common/LoginConfig";
+
+describe("LoginConfig Test", () => {
+
+    function setEnv(name: string, value: string) {
+        process.env[`INPUT_${name.replace(/ /g, '_').toUpperCase()}`] = value;
+    }
+
+    function cleanEnv() {
+        for (const envKey in process.env) {
+            if (envKey.startsWith('INPUT_')) {
+                delete process.env[envKey]
+            }
+        }
+    }
+
+    async function testCreds(creds:any){
+        setEnv('environment', 'azurecloud');
+        setEnv('enable-AzPSSession', 'true');
+        setEnv('allow-no-subscriptions', 'false');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+        setEnv('creds', JSON.stringify(creds));
+        let loginConfig = new LoginConfig();
+        try{
+            await loginConfig.initialize();
+            throw new Error("The last step should fail.");
+        }catch(error){
+            expect(error.message.includes("Not all parameters are provided in 'creds'.")).toBeTruthy();
+        }
+    }
+
+    function testValidateWithErrorMessage(loginConfig:LoginConfig, errorMessage:string){
+        try{
+            loginConfig.validate();
+            throw new Error("The last step should fail.");
+        }catch(error){
+            expect(error.message.includes(errorMessage)).toBeTruthy();
+        }
+    }
+
+    beforeEach(() => {
+        cleanEnv();
+    });
+
+    test('initialize with creds, lack of clientId', async () => {
+        let creds1 = {
+            // 'clientId': 'client-id',
+            'clientSecret': 'client-secret',
+            'tenantId': 'tenant-id',
+            'subscriptionId': 'subscription-id'
+        }
+        await testCreds(creds1);
+
+    });
+
+    test('initialize with creds, lack of clientSecret', async () => {
+        let creds1 = {
+            'clientId': 'client-id',
+            // 'clientSecret': 'client-secret',
+            'tenantId': 'tenant-id',
+            'subscriptionId': 'subscription-id'
+        }
+        await testCreds(creds1);
+
+    });
+
+    test('initialize with creds, lack of tenantId', async () => {
+        let creds1 = {
+            'clientId': 'client-id',
+            'clientSecret': 'client-secret',
+            // 'tenantId': 'tenant-id',
+            'subscriptionId': 'subscription-id'
+        }
+        await testCreds(creds1);
+
+    });
+
+    test('initialize with creds, lack of subscriptionId', async () => {
+        let creds1 = {
+            'clientId': 'client-id',
+            'clientSecret': 'client-secret',
+            'tenantId': 'tenant-id',
+            // 'subscriptionId': 'subscription-id'
+        }
+        await testCreds(creds1);
+
+    });
+
+    test('initialize with creds', async () => {
+        let creds = {
+            'clientId': 'client-id',
+            'clientSecret': 'client-secret',
+            'tenantId': 'tenant-id',
+            'subscriptionId': 'subscription-id'
+        }
+
+        setEnv('environment', 'azurecloud');
+        setEnv('enable-AzPSSession', 'true');
+        setEnv('allow-no-subscriptions', 'false');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+        setEnv('creds', JSON.stringify(creds));
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        expect(loginConfig.environment).toBe("azurecloud");
+        expect(loginConfig.enableAzPSSession).toBeTruthy();
+        expect(loginConfig.allowNoSubscriptionsLogin).toBeFalsy();
+        expect(loginConfig.authType).toBe("SERVICE_PRINCIPAL");
+        expect(loginConfig.servicePrincipalId).toBe("client-id");
+        expect(loginConfig.servicePrincipalKey).toBe("client-secret");
+        expect(loginConfig.tenantId).toBe("tenant-id");
+        expect(loginConfig.subscriptionId).toBe("subscription-id");
+    });
+
+    test('initialize with individual parameters', async () => {
+        setEnv('environment', 'azureusgovernment');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+        setEnv('tenant-id', 'tenant-id');
+        setEnv('subscription-id', 'subscription-id');
+        setEnv('client-id', 'client-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        expect(loginConfig.environment).toBe("azureusgovernment");
+        expect(loginConfig.enableAzPSSession).toBeFalsy();
+        expect(loginConfig.allowNoSubscriptionsLogin).toBeTruthy();
+        expect(loginConfig.authType).toBe("SERVICE_PRINCIPAL");
+        expect(loginConfig.servicePrincipalId).toBe("client-id");
+        expect(loginConfig.tenantId).toBe("tenant-id");
+        expect(loginConfig.subscriptionId).toBe("subscription-id");
+    });
+
+    test('initialize with both creds and individual parameters', async () => {
+        setEnv('environment', 'azureusgovernment');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+
+        setEnv('tenant-id', 'tenant-id-aa');
+        setEnv('subscription-id', 'subscription-id-aa');
+        setEnv('client-id', 'client-id-aa');
+
+        let creds = {
+            'clientId': 'client-id-bb',
+            'clientSecret': 'client-secret-bb',
+            'tenantId': 'tenant-id-bb',
+            'subscriptionId': 'subscription-id-bb'
+        }
+        setEnv('creds', JSON.stringify(creds));
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        expect(loginConfig.environment).toBe("azureusgovernment");
+        expect(loginConfig.enableAzPSSession).toBeFalsy();
+        expect(loginConfig.allowNoSubscriptionsLogin).toBeTruthy();
+        expect(loginConfig.authType).toBe("SERVICE_PRINCIPAL");
+        expect(loginConfig.servicePrincipalId).toBe("client-id-aa");
+        expect(loginConfig.servicePrincipalKey).toBeNull();
+        expect(loginConfig.tenantId).toBe("tenant-id-aa");
+        expect(loginConfig.subscriptionId).toBe("subscription-id-aa");
+    });
+
+    test('validate with wrong environment', async () => {
+        setEnv('environment', 'aWrongCloud');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+
+        setEnv('tenant-id', 'tenant-id');
+        setEnv('subscription-id', 'subscription-id');
+        setEnv('client-id', 'client-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        testValidateWithErrorMessage(loginConfig, "Unsupported value 'awrongcloud' for environment is passed.");
+    });
+
+    test('validate with wrong authType', async () => {
+        setEnv('environment', 'azurestack');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE-PRINCIPAL');
+
+        setEnv('tenant-id', 'tenant-id');
+        setEnv('subscription-id', 'subscription-id');
+        setEnv('client-id', 'client-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        testValidateWithErrorMessage(loginConfig, "Unsupported value 'SERVICE-PRINCIPAL' for authentication type is passed.");
+    });
+
+    test('validate with SERVICE_PRINCIPAL, lack of tenant id', async () => {
+        setEnv('environment', 'azurestack');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+
+        // setEnv('tenant-id', 'tenant-id');
+        setEnv('subscription-id', 'subscription-id');
+        setEnv('client-id', 'client-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        testValidateWithErrorMessage(loginConfig, "Using auth-type: SERVICE_PRINCIPAL. Not all values are present. Ensure 'client-id' and 'tenant-id' are supplied.");
+    });
+
+    test('validate with SERVICE_PRINCIPAL, lack of client id', async () => {
+        setEnv('environment', 'azurestack');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+
+        setEnv('tenant-id', 'tenant-id');
+        setEnv('subscription-id', 'subscription-id');
+        // setEnv('client-id', 'client-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        testValidateWithErrorMessage(loginConfig, "Using auth-type: SERVICE_PRINCIPAL. Not all values are present. Ensure 'client-id' and 'tenant-id' are supplied.");
+    });
+
+    test('validate without subscriptionId and allowNoSubscriptionsLogin=false', async () => {
+        setEnv('environment', 'azurestack');
+        setEnv('enable-AzPSSession', 'false');
+        setEnv('allow-no-subscriptions', 'false');
+        setEnv('auth-type', 'IDENTITY');
+
+        // setEnv('subscription-id', 'subscription-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        testValidateWithErrorMessage(loginConfig, "Ensure subscriptionId is supplied.");
+    });
+
+    test('validate without subscriptionId and allowNoSubscriptionsLogin=true', async () => {
+        setEnv('environment', 'azurestack');
+        setEnv('enable-AzPSSession', 'true');
+        setEnv('allow-no-subscriptions', 'true');
+        setEnv('auth-type', 'IDENTITY');
+
+        // setEnv('subscription-id', 'subscription-id');
+
+        let loginConfig = new LoginConfig();
+        await loginConfig.initialize();
+        loginConfig.validate();
+        expect(loginConfig.environment).toBe("azurestack");
+        expect(loginConfig.enableAzPSSession).toBeTruthy();
+        expect(loginConfig.allowNoSubscriptionsLogin).toBeTruthy();
+        expect(loginConfig.authType).toBe("IDENTITY");
+        expect(loginConfig.servicePrincipalId).toBe("");
+        expect(loginConfig.servicePrincipalKey).toBeNull();
+        expect(loginConfig.tenantId).toBe("");
+        expect(loginConfig.subscriptionId).toBe("");
+    });
+
+});
diff --git a/__tests__/PowerShell/AzPSScriptBuilder.test.ts b/__tests__/PowerShell/AzPSScriptBuilder.test.ts
@@ -19,11 +19,12 @@ describe("Getting AzLogin PS script", () => {
         cleanEnv();
     });
 
-    test('PS script to get latest module path should work', () => {
-        expect(AzPSSCriptBuilder.getLatestModulePathScript("TestModule")).toContain("(Get-Module -Name 'TestModule' -ListAvailable | Sort-Object Version -Descending | Select-Object -First 1).ModuleBase");
+    test('getImportLatestModuleScript', () => {
+        expect(AzPSSCriptBuilder.getImportLatestModuleScript("TestModule")).toContain("(Get-Module -Name 'TestModule' -ListAvailable | Sort-Object Version -Descending | Select-Object -First 1).Path");
+        expect(AzPSSCriptBuilder.getImportLatestModuleScript("TestModule")).toContain("Import-Module -Name $latestModulePath");
     });
 
-    test('PS script should not set context while passing allowNoSubscriptionsLogin as true', () => {
+    test('getAzPSLoginScript for SP+secret with allowNoSubscriptionsLogin=true', () => {
         setEnv('environment', 'azurecloud');
         setEnv('enable-AzPSSession', 'true');
         setEnv('allow-no-subscriptions', 'true');
@@ -39,15 +40,15 @@ describe("Getting AzLogin PS script", () => {
         let loginConfig = new LoginConfig();
         loginConfig.initialize();
         return AzPSSCriptBuilder.getAzPSLoginScript(loginConfig).then(([loginMethod, loginScript]) => {
-            expect(loginScript.includes("Connect-AzAccount -ServicePrincipal -Environment 'azurecloud' -Tenant 'tenant-id' -Credential")).toBeTruthy();
-            expect(loginScript.includes("Set-AzContext -SubscriptionId")).toBeFalsy;
+            expect(loginScript.includes("Clear-AzContext -Scope Process; Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue; $psLoginSecrets = ConvertTo-SecureString 'client-secret' -AsPlainText -Force; $psLoginCredential = New-Object System.Management.Automation.PSCredential('client-id', $psLoginSecrets); Connect-AzAccount -ServicePrincipal -Environment 'azurecloud' -Tenant 'tenant-id' -Subscription 'subscription-id' -Credential $psLoginCredential | out-null;")).toBeTruthy();
+            expect(loginMethod).toBe('service principal with secret');
         });
     });
 
-    test('PS script should set context while passing allowNoSubscriptionsLogin as false', () => {
+    test('getAzPSLoginScript for SP+secret with allowNoSubscriptionsLogin=false', () => {
         setEnv('environment', 'azurecloud');
         setEnv('enable-AzPSSession', 'true');
-        setEnv('allow-no-subscriptions', 'false');
+        setEnv('allow-no-subscriptions', 'false'); // same as true
         setEnv('auth-type', 'SERVICE_PRINCIPAL');
         let creds = {
             'clientId': 'client-id',
@@ -60,28 +61,60 @@ describe("Getting AzLogin PS script", () => {
         let loginConfig = new LoginConfig();
         loginConfig.initialize();
         return AzPSSCriptBuilder.getAzPSLoginScript(loginConfig).then(([loginMethod, loginScript]) => {
-            expect(loginScript.includes("Connect-AzAccount -ServicePrincipal -Environment 'azurecloud' -Tenant 'tenant-id' -Credential")).toBeTruthy();
-            expect(loginScript.includes("Set-AzContext -SubscriptionId")).toBeTruthy();
+            expect(loginScript.includes("Clear-AzContext -Scope Process; Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue; $psLoginSecrets = ConvertTo-SecureString 'client-secret' -AsPlainText -Force; $psLoginCredential = New-Object System.Management.Automation.PSCredential('client-id', $psLoginSecrets); Connect-AzAccount -ServicePrincipal -Environment 'azurecloud' -Tenant 'tenant-id' -Subscription 'subscription-id' -Credential $psLoginCredential | out-null;")).toBeTruthy();
+            expect(loginMethod).toBe('service principal with secret');
         });
     });
 
-    test('PS script should use system managed identity to login when auth-type is IDENTITY', () => {
+    test('getAzPSLoginScript for OIDC', () => {
         setEnv('environment', 'azurecloud');
         setEnv('enable-AzPSSession', 'true');
         setEnv('allow-no-subscriptions', 'false');
+        setEnv('tenant-id', 'tenant-id');
         setEnv('subscription-id', 'subscription-id');
+        setEnv('client-id', 'client-id');
+        setEnv('auth-type', 'SERVICE_PRINCIPAL');
+
+        let loginConfig = new LoginConfig();
+        loginConfig.initialize();
+        jest.spyOn(loginConfig, 'getFederatedToken').mockImplementation(async () => {loginConfig.federatedToken = "fake-token";});
+        return AzPSSCriptBuilder.getAzPSLoginScript(loginConfig).then(([loginMethod, loginScript]) => {
+            expect(loginScript.includes("Clear-AzContext -Scope Process; Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue; Connect-AzAccount -ServicePrincipal -Environment 'azurecloud' -Tenant 'tenant-id' -Subscription 'subscription-id' -ApplicationId 'client-id' -FederatedToken 'fake-token' | out-null;")).toBeTruthy();
+            expect(loginMethod).toBe('OIDC');
+        });
+    });
+
+    test('getAzPSLoginScript for System MI', () => {
+        setEnv('environment', 'azurecloud');
+        setEnv('enable-AzPSSession', 'true');
+        setEnv('allow-no-subscriptions', 'false');
+        setEnv('subscription-id', 'subscription-id');
+        setEnv('auth-type', 'IDENTITY');
+
+        let loginConfig = new LoginConfig();
+        loginConfig.initialize();
+        return AzPSSCriptBuilder.getAzPSLoginScript(loginConfig).then(([loginMethod, loginScript]) => {
+            expect(loginScript.includes("Clear-AzContext -Scope Process; Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue; Connect-AzAccount -Identity -Environment 'azurecloud' -Subscription 'subscription-id'  | out-null;")).toBeTruthy();
+            expect(loginMethod).toBe('system-assigned managed identity');
+        });
+    });
+
+    test('getAzPSLoginScript for System MI without subscription id', () => {
+        setEnv('environment', 'azurecloud');
+        setEnv('enable-AzPSSession', 'true');
+        setEnv('allow-no-subscriptions', 'false');
+        // setEnv('subscription-id', 'subscription-id');
         setEnv('auth-type', 'IDENTITY');
 
         let loginConfig = new LoginConfig();
         loginConfig.initialize();
         return AzPSSCriptBuilder.getAzPSLoginScript(loginConfig).then(([loginMethod, loginScript]) => {
-            expect(loginScript.includes("Connect-AzAccount -Identity -Environment 'azurecloud'")).toBeTruthy();
-            expect(loginScript.includes("Connect-AzAccount -Identity -Environment 'azurecloud' -AccountId 'client-id'")).toBeFalsy();
-            expect(loginScript.includes("Set-AzContext -SubscriptionId")).toBeTruthy();
+            expect(loginScript.includes("Clear-AzContext -Scope Process; Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue; Connect-AzAccount -Identity -Environment 'azurecloud'  | out-null;")).toBeTruthy();
+            expect(loginMethod).toBe('system-assigned managed identity');
         });
     });
 
-    test('PS script should use user managed identity to login when auth-type is IDENTITY', () => {
+    test('getAzPSLoginScript for user-assigned MI', () => {
         setEnv('environment', 'azurecloud');
         setEnv('enable-AzPSSession', 'true');
         setEnv('allow-no-subscriptions', 'true');
@@ -91,8 +124,8 @@ describe("Getting AzLogin PS script", () => {
         let loginConfig = new LoginConfig();
         loginConfig.initialize();
         return AzPSSCriptBuilder.getAzPSLoginScript(loginConfig).then(([loginMethod, loginScript]) => {
-            expect(loginScript.includes("Connect-AzAccount -Identity -Environment 'azurecloud' -AccountId 'client-id'")).toBeTruthy();
-            expect(loginScript.includes("Set-AzContext -SubscriptionId")).toBeFalsy();
+            expect(loginScript.includes("Clear-AzContext -Scope Process; Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue; Connect-AzAccount -Identity -Environment 'azurecloud' -AccountId 'client-id' | out-null;")).toBeTruthy();
+            expect(loginMethod).toBe('user-assigned managed identity');
         });
     });
 

diff --git a/src/Cli/AzureCliLogin.ts b/src/Cli/AzureCliLogin.ts
@@ -39,7 +39,7 @@ export class AzureCliLogin {
         await this.executeAzCliCommand(["cloud", "set", "-n", this.loginConfig.environment], false);
         core.info(`Done setting cloud: "${this.loginConfig.environment}"`);
 
-        if (this.loginConfig.authType == "service_principal") {
+        if (this.loginConfig.authType === LoginConfig.AUTH_TYPE_SERVICE_PRINCIPAL) {
             let args = ["--service-principal",
                 "--username", this.loginConfig.servicePrincipalId,
                 "--tenant", this.loginConfig.tenantId
@@ -134,10 +134,6 @@ export class AzureCliLogin {
         if (this.loginConfig.allowNoSubscriptionsLogin) {
             return;
         }
-        if (!this.loginConfig.subscriptionId) {
-            core.warning('No subscription-id is given. Skip setting subscription... If there are mutiple subscriptions under the tenant, please input subscription-id to specify which subscription to use.');
-            return;
-        }
         let args = ["account", "set", "--subscription", this.loginConfig.subscriptionId];
         await this.executeAzCliCommand(args, true, this.loginOptions);
         core.info("Subscription is set successfully.");