{Keyvault} Remove useless AAD endpoint and honor user input --subscription to support cross sub operations
#26539
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
ghost
added
the
ghost
requested a review
from 
|
KeyVault refinement |
| _, _, tenant_id = profile.get_login_credentials( | ||
| resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id) |
There was a problem hiding this comment.
This is indeed why a token for AD Graph is retrieved.
class MSIAuthentication(BasicTokenAuthentication):
def __init__(self, port=50342, **kwargs):
....
self.set_token()When msrestazure.azure_active_directory.MSIAuthentication is initialized, it gets an access token immediately, not until signed_session or get_token is called, so get_login_credentials will trigger a web request to get AD Graph token.
This behavior will be changed in #25959 in order to align with other MSAL-based credentials - azure.cli.core.auth.msal_authentication.UserCredential， azure.cli.core.auth.msal_authentication.ServicePrincipalCredential.
There was a problem hiding this comment.
Another question, why does the original code choose active_directory_graph_resource_id instead of active_directory_resource_id (ARM's resource ID)? Is it because the name is confusing?
Update: Well. It looks like #3631 simply copied the code for creating AD Graph client but only kept the tenant_id.
There was a problem hiding this comment.
When
msrestazure.azure_active_directory.MSIAuthenticationis initialized, it gets an access token immediately, not untilsigned_sessionorget_tokenis called, soget_login_credentialswill trigger a web request to get AD Graph token.
This explains why AAD call shows up when we create keyvault in Cloudshell sandbox as it's following MSI workflow.
There was a problem hiding this comment.
Anyway the whole call for get_login_credentials has been removed. Doesn't matter which resource id we need to use anymore😉
| profile = Profile(cli_ctx=cmd.cli_ctx) | ||
| _, _, tenant_id = profile.get_login_credentials( | ||
| resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id) | ||
| tenant_id = profile.get_subscription(subscription=cmd.cli_ctx.data['subscription_id'])[_TENANT_ID] |
There was a problem hiding this comment.
This may result in KeyError as subscription_id may not always be in cmd.cli_ctx.data.
A little bit hacky, but azure.cli.command_modules.role accesses the private attribute _config of the client to retrieve the subscription ID which is from cli_ctx.data['subscription_id']:
azure-cli/src/azure-cli/azure/cli/command_modules/role/custom.py
Lines 65 to 66 in 00fe3e5
Let me think how we can improve this hack from core level.
evelyn-ys
changed the title
--subscription to support cross sub operations
| profile = Profile(cli_ctx=cmd.cli_ctx) | ||
| _, _, tenant_id = profile.get_login_credentials( | ||
| resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id) | ||
| tenant_id = profile.get_subscription(subscription=cmd.cli_ctx.data.get('subscription_id', None))[_TENANT_ID] |
There was a problem hiding this comment.
None is the default of default in dict.get: https://docs.python.org/3/library/stdtypes.html#dict.get
…iption` to support cross sub operations (Azure#26539) * Removed unused aad endpoint * honor user input subscription * avoid keyerror * add data plane cross sub support
3 participants
Description
In #22432, we migrated AAD Graph to MS Graph. Some related code in Keyvault module was also modified from
azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Lines 681 to 689 in 1451bd5
azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Lines 679 to 684 in cac3f06
Before migration, we need to call
profile.get_login_credentialsto getcredandtenant_idinfo.After migration, we don't need the
credanymore but didn't dropprofile.get_login_credentialsjust to gettenant_idinfo.But there's better way to get
tenant_id, it's a waste to callprofile.get_login_credentialsjust for tenant info. Besides, the existingprofile.get_login_credentialsis still passingresourcewith AAD Graph endpoint which is deprecated. This PR aims to clear these useless code.Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.