Conversation
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
requested review from
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
the
ARM
| if tenant and tenant != account[_TENANT_ID]: | ||
| raise CLIError(non_current_tenant_template.format('managed identity')) |
There was a problem hiding this comment.
Instead of raising an error, another possible solution is to ignore the specified tenant and just return the access token for the current tenant, but it will result in wrong access token being returned, making it difficult to troubleshoot. I don't agree with this solution.
jiasli
requested review from
bebound,
calvinhzy,
evelyn-ys,
jsntcy,
kairu-ms and
necusjz
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR allows specifying --tenant with the current tenant for Cloud Shell and managed identity accounts in the az account get-access-token command. Previously, any tenant specification was forbidden for these account types, but this caused issues when Azure SDK components always pass the tenant ID during authentication challenges.
Key changes:
- Modified error handling to allow tenant specification when it matches the current account's tenant
- Updated error messages to be more descriptive about the specific restriction
- Enhanced test coverage to verify both allowed and disallowed tenant specifications
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/azure-cli-core/azure/cli/core/_profile.py | Updated logic to allow current tenant specification and improved error messages |
| src/azure-cli-core/azure/cli/core/tests/test_profile.py | Added test cases for current tenant specification and updated error message assertions |
| non_current_tenant_template = ("For {} account, getting access token for non-current tenants is not " | ||
| "supported. The specified tenant must be the current tenant " |
There was a problem hiding this comment.
[nitpick] The template string construction is unnecessarily complex and hard to read. Consider using a single f-string or format method call instead of concatenating strings and mixing f-string syntax.
| non_current_tenant_template = ("For {} account, getting access token for non-current tenants is not " | |
| "supported. The specified tenant must be the current tenant " | |
| non_current_tenant_template = (f"For {{}} account, getting access token for non-current tenants is not " | |
| f"supported. The specified tenant must be the current tenant " |
There was a problem hiding this comment.
This comment is incorrect. Prefixing the substring with f is unnecessary.
|
|
||
| non_current_tenant_template = ("For {} account, getting access token for non-current tenants is not " | ||
| "supported. The specified tenant must be the current tenant " | ||
| f"{account[_TENANT_ID]}") |
There was a problem hiding this comment.
[nitpick] The f-string formatting here is redundant since you're just converting a variable to string. You can directly use {account[_TENANT_ID]} in the format method call.
| f"{account[_TENANT_ID]}") | |
| "{}").format(account[_TENANT_ID]) |
There was a problem hiding this comment.
This copilot comment is incorrect. f-string is preferred in this case.
5 participants
Related command
az account get-access-tokenDescription
Resolve Azure/azure-sdk-for-python#41875
#11798 forbids passing
--tenanttoaz account get-access-token.However, when Key Vault data-plane SDK receives a challenge, it will always pass the
tenant_idtoAzureCliCredentialandaz account get-access-token. If the current account is a Cloud Shell or managed identity account,az account get-access-tokenwill fail:azure-cli/src/azure-cli-core/azure/cli/core/_profile.py
Lines 370 to 373 in bc6c4d9
As it is impossible for SDK to know which account type is used by Azure CLI and pass
--tenantconditionally, it is better for CLI to allow--tenantwith the current tenant.This PR allows specifying
--tenantwith the current tenant for Cloud Shell or managed identity account, but still raises error when getting access token for a non-current tenant.Testing Guide
History Notes
[Profile]
az account get-access-token: Specifying--tenantwith the current tenant is now allowed for Cloud Shell and managed identity account