Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #371: Allow optional subscriptionId in creds #373

Merged
merged 5 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion .github/workflows/azure-login-negative.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
51 changes: 47 additions & 4 deletions .github/workflows/azure-login-positive.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
21 changes: 17 additions & 4 deletions __tests__/LoginConfig.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
9 changes: 4 additions & 5 deletions src/Cli/AzureCliLogin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions src/common/LoginConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'.");
}
}

Expand Down
Loading