From 5fef06ae42f27e5b1c07ed7b220948df6212e77c Mon Sep 17 00:00:00 2001 From: MoChilia Date: Tue, 21 Nov 2023 14:25:49 +0800 Subject: [PATCH 1/5] fix371 --- .github/workflows/azure-login-positive.yml | 21 +++++++++++++++++++++ __tests__/LoginConfig.test.ts | 11 ----------- src/common/LoginConfig.ts | 4 ++-- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/.github/workflows/azure-login-positive.yml b/.github/workflows/azure-login-positive.yml index 8d5581803..973d01720 100644 --- a/.github/workflows/azure-login-positive.yml +++ b/.github/workflows/azure-login-positive.yml @@ -198,6 +198,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: diff --git a/__tests__/LoginConfig.test.ts b/__tests__/LoginConfig.test.ts index 855af818a..54859d8db 100644 --- a/__tests__/LoginConfig.test.ts +++ b/__tests__/LoginConfig.test.ts @@ -75,17 +75,6 @@ describe("LoginConfig Test", () => { }); - 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', diff --git a/src/common/LoginConfig.ts b/src/common/LoginConfig.ts index be03bb602..c46860593 100644 --- a/src/common/LoginConfig.ts +++ b/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'."); } } From 46c5f5d1bb5c0a0b2e7911459e74bed8c2153582 Mon Sep 17 00:00:00 2001 From: MoChilia Date: Thu, 23 Nov 2023 14:47:07 +0800 Subject: [PATCH 2/5] set subId once it is given --- .github/workflows/azure-login-negative.yml | 2 +- __tests__/LoginConfig.test.ts | 26 ++++++++++++++++++++++ src/Cli/AzureCliLogin.ts | 2 +- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/.github/workflows/azure-login-negative.yml b/.github/workflows/azure-login-negative.yml index 3cb37d5e6..cd67693c3 100644 --- a/.github/workflows/azure-login-negative.yml +++ b/.github/workflows/azure-login-negative.yml @@ -347,7 +347,7 @@ 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 + # SP1 is ignored and SP2 will be used for login, but it will failed since SP2 has no access to the given subscription - name: Login with both creds and individual parameters id: login_12 continue-on-error: true diff --git a/__tests__/LoginConfig.test.ts b/__tests__/LoginConfig.test.ts index 54859d8db..49a6c1492 100644 --- a/__tests__/LoginConfig.test.ts +++ b/__tests__/LoginConfig.test.ts @@ -75,6 +75,32 @@ describe("LoginConfig Test", () => { }); + + 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' + } + 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 () => { let creds = { 'clientId': 'client-id', diff --git a/src/Cli/AzureCliLogin.ts b/src/Cli/AzureCliLogin.ts index 03a86f201..5719bd51b 100644 --- a/src/Cli/AzureCliLogin.ts +++ b/src/Cli/AzureCliLogin.ts @@ -131,7 +131,7 @@ export class AzureCliLogin { } async setSubscription() { - if (this.loginConfig.allowNoSubscriptionsLogin) { + if (!this.loginConfig.subscriptionId) { return; } let args = ["account", "set", "--subscription", this.loginConfig.subscriptionId]; From bdbe7157ce9529ace878cee60b204129151f7bd8 Mon Sep 17 00:00:00 2001 From: MoChilia Date: Fri, 24 Nov 2023 13:01:00 +0800 Subject: [PATCH 3/5] modify test to logout first --- .github/workflows/azure-login-negative.yml | 5 +++++ src/Cli/AzureCliLogin.ts | 9 ++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/azure-login-negative.yml b/.github/workflows/azure-login-negative.yml index cd67693c3..dead8273d 100644 --- a/.github/workflows/azure-login-negative.yml +++ b/.github/workflows/azure-login-negative.yml @@ -347,6 +347,11 @@ jobs: script: | core.setFailed('Last action should fail but not. Please check it.') + # 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 failed since SP2 has no access to the given subscription - name: Login with both creds and individual parameters id: login_12 diff --git a/src/Cli/AzureCliLogin.ts b/src/Cli/AzureCliLogin.ts index 5719bd51b..fc67fbffc 100644 --- a/src/Cli/AzureCliLogin.ts +++ b/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.subscriptionId) { - 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); } From 31435a1ad28627a6fd370066fca1073bfdba2bd6 Mon Sep 17 00:00:00 2001 From: MoChilia Date: Mon, 27 Nov 2023 11:24:42 +0800 Subject: [PATCH 4/5] update tests --- .github/workflows/azure-login-negative.yml | 33 +++++++++++++++++++++- .github/workflows/azure-login-positive.yml | 30 +++++++++++++++++--- __tests__/LoginConfig.test.ts | 6 ++-- 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/.github/workflows/azure-login-negative.yml b/.github/workflows/azure-login-negative.yml index dead8273d..21bed1f0e 100644 --- a/.github/workflows/azure-login-negative.yml +++ b/.github/workflows/azure-login-negative.yml @@ -352,7 +352,7 @@ jobs: run: | az logout - # SP1 is ignored and SP2 will be used for login, but it will failed since SP2 has no access to the given subscription + # 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 @@ -387,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: diff --git a/.github/workflows/azure-login-positive.yml b/.github/workflows/azure-login-positive.yml index 973d01720..7bc5d3822 100644 --- a/.github/workflows/azure-login-positive.yml +++ b/.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.OIDC_SP2_CLIENT_ID }} + tenant-id: ${{ secrets.OIDC_SP2_TENANT_ID}} + subscription-id: ${{ secrets.OIDC_SP2_TENANT_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: diff --git a/__tests__/LoginConfig.test.ts b/__tests__/LoginConfig.test.ts index 49a6c1492..0f75f42d3 100644 --- a/__tests__/LoginConfig.test.ts +++ b/__tests__/LoginConfig.test.ts @@ -74,9 +74,8 @@ describe("LoginConfig Test", () => { await testCreds(creds1); }); - - - test('initialize with creds, lack of subscriptionId, but allowNoSubscriptionsLogin=true', async () => { + + test('initialize with creds, lack of subscriptionId, but allowNoSubscriptionsLogin=true', async () => { let creds1 = { 'clientId': 'client-id', 'clientSecret': 'client-secret', @@ -100,7 +99,6 @@ describe("LoginConfig Test", () => { expect(loginConfig.subscriptionId).toBe(""); }); - test('initialize with creds', async () => { let creds = { 'clientId': 'client-id', From 7acbecf424173a875bf7db0d5fd041a53769e699 Mon Sep 17 00:00:00 2001 From: MoChilia Date: Mon, 27 Nov 2023 11:47:49 +0800 Subject: [PATCH 5/5] update test --- .github/workflows/azure-login-positive.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/azure-login-positive.yml b/.github/workflows/azure-login-positive.yml index 7bc5d3822..fdcca1517 100644 --- a/.github/workflows/azure-login-positive.yml +++ b/.github/workflows/azure-login-positive.yml @@ -178,9 +178,9 @@ jobs: - name: Login with individual parameters, allow no subscription uses: ./ with: - client-id: ${{ secrets.OIDC_SP2_CLIENT_ID }} - tenant-id: ${{ secrets.OIDC_SP2_TENANT_ID}} - subscription-id: ${{ secrets.OIDC_SP2_TENANT_ID }} + 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