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

Fix #371: Allow optional subscriptionId in creds #373

merged 5 commits into from
Nov 27, 2023

Conversation

MoChilia
Copy link
Member

@MoChilia MoChilia commented Nov 21, 2023

Description

This pr will remove the mandatory requirement of subscriptionId in creds to solve #371.

subscription-id allow-no-subscriptions Log in successfully Test
✔️ ✔️ Creds:
- name: Login with creds
uses: ./
with:
creds: ${{secrets.SP1}}
enable-AzPSSession: true
OIDC:
- name: Login with individual parameters
uses: ./
with:
client-id: ${{ secrets.SP1_CLIENT_ID }}
tenant-id: ${{ secrets.SP1_TENANT_ID }}
subscription-id: ${{ secrets.SP1_SUBSCRIPTION_ID }}
enable-AzPSSession: true
✔️ ✔️ Creds:
- 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
OIDC:
- name: Login with individual parameters, no subscription, allow no subscription
uses: ./
with:
client-id: ${{ secrets.OIDC_SP2_CLIENT_ID }}
tenant-id: ${{ secrets.OIDC_SP2_TENANT_ID }}
allow-no-subscriptions: true
enable-AzPSSession: true
✔️ ✔️ ✔️ Creds:
- name: Login with creds, allow no subscription
uses: ./
with:
creds: ${{secrets.SP1}}
allow-no-subscriptions: true
enable-AzPSSession: true
OIDC:
- 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
Creds:
- 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
OIDC:
- 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

Validation for both subscription-id and allow-no-subscriptions are missed:

if (!this.subscriptionId && !this.allowNoSubscriptionsLogin) {
throw new Error("Ensure subscriptionId is supplied.");
}

If subscription-id is given, then set the subscription:

if (this.loginConfig.subscriptionId) {
await this.setSubscription();
}

if(subscriptionId){
loginCmdlet += `-Subscription '${subscriptionId}' `;
}

Negative test for login tenant-level account with subscription-id (no permission to subscriptions):

# 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
uses: ./
with:
creds: ${{secrets.SP1}}
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
enable-AzPSSession: true
- name: Check Last step failed
if: steps.login_12.outcome == 'success'
uses: actions/github-script@v3
with:
script: |
core.setFailed('Last action should fail but not. Please check it.')

@MoChilia MoChilia marked this pull request as draft November 24, 2023 03:24
@MoChilia MoChilia merged commit 34b958d into master Nov 27, 2023
24 checks passed
@MoChilia MoChilia deleted the fix-371 branch November 27, 2023 07:17
@jiasli
Copy link
Member

jiasli commented Nov 28, 2023

If both allow-no-subscriptions and subscription-id are provided, will subscription-id take effect? If so, it is worth explaining that in the document.

@MoChilia
Copy link
Member Author

If both allow-no-subscriptions and subscription-id are provided, will subscription-id take effect? If so, it is worth explaining that in the document.

subscription-id will always be set once it is provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants