Skip to content

{Core} Refactor code for passing arguments #30300

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.

Sign up for GitHub

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

Merged
jiasli merged 1 commit into from
Nov 14, 2024
Merged

{Core} Refactor code for passing arguments #30300

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 13 additions & 13 deletions src/azure-cli-core/azure/cli/core/_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,12 +362,10 @@ def logout_all(self):
identity.logout_all_users()
identity.logout_all_service_principal()

def get_login_credentials(self, resource=None, client_id=None, subscription_id=None, aux_subscriptions=None,
aux_tenants=None):
def get_login_credentials(self, resource=None, subscription_id=None, aux_subscriptions=None, aux_tenants=None):
Copy link
Member Author

@jiasli jiasli Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client_id should not be supported by get_login_credentials().

"""Get a CredentialAdaptor instance to be used with both Track 1 and Track 2 SDKs.

:param resource: The resource ID to acquire an access token. Only provide it for Track 1 SDKs.
:param client_id:
:param subscription_id:
:param aux_subscriptions:
:param aux_tenants:
Expand Down Expand Up @@ -410,10 +408,10 @@ def get_login_credentials(self, resource=None, client_id=None, subscription_id=N
if sub[_TENANT_ID] != account[_TENANT_ID]:
external_tenants.append(sub[_TENANT_ID])

credential = self._create_credential(account, client_id=client_id)
credential = self._create_credential(account)
external_credentials = []
for external_tenant in external_tenants:
external_credentials.append(self._create_credential(account, external_tenant, client_id=client_id))
external_credentials.append(self._create_credential(account, tenant_id=external_tenant))
Copy link
Member Author

@jiasli jiasli Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_create_credential() defines tenant_id as a keyword argument:

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Line 659 in 0f28626

def _create_credential(self, account, tenant_id=None, client_id=None):

Calling _create_credential() should follow the same pattern.

from azure.cli.core.auth.credential_adaptor import CredentialAdaptor
cred = CredentialAdaptor(credential,
auxiliary_credentials=external_credentials,
Expand Down Expand Up @@ -460,7 +458,7 @@ def get_raw_token(self, resource=None, scopes=None, subscription=None, tenant=No
scopes_to_resource(scopes))

else:
cred = self._create_credential(account, tenant)
cred = self._create_credential(account, tenant_id=tenant)

sdk_token = cred.get_token(*scopes)
# Convert epoch int 'expires_on' to datetime string 'expiresOn' for backward compatibility
Expand Down Expand Up @@ -658,14 +656,14 @@ def _try_parse_msi_account_name(account):
def _create_credential(self, account, tenant_id=None, client_id=None):
"""Create a credential object driven by MSAL

:param account:
:param account: The CLI account to create credential for
:param tenant_id: If not None, override tenantId from 'account'
:param client_id:
:param client_id: Client ID of another public client application
:return:
"""
user_type = account[_USER_ENTITY][_USER_TYPE]
username_or_sp_id = account[_USER_ENTITY][_USER_NAME]
tenant_id = tenant_id if tenant_id else account[_TENANT_ID]
tenant_id = tenant_id or account[_TENANT_ID]
Copy link
Member Author

@jiasli jiasli Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.python.org/3/library/stdtypes.html#boolean-operations-and-or-not

x or y: if x is true, then x, else y

identity = _create_identity_instance(self.cli_ctx, self._authority, tenant_id=tenant_id, client_id=client_id)

# User
Expand Down Expand Up @@ -694,7 +692,7 @@ def refresh_accounts(self):
tenant = s[_TENANT_ID]
subscriptions = []
try:
identity_credential = self._create_credential(s, tenant)
identity_credential = self._create_credential(s, tenant_id=tenant)
if is_service_principal:
subscriptions = subscription_finder.find_using_specific_tenant(tenant, identity_credential)
else:
Expand Down Expand Up @@ -938,7 +936,7 @@ def _transform_subscription_for_multiapi(s, s_dict):
s_dict[_MANAGED_BY_TENANTS] = [{_TENANT_ID: t.tenant_id} for t in s.managed_by_tenants]


def _create_identity_instance(cli_ctx, *args, **kwargs):
def _create_identity_instance(cli_ctx, authority, tenant_id=None, client_id=None):
"""Lazily import and create Identity instance to avoid unnecessary imports."""
from .auth.identity import Identity
from .util import should_encrypt_token_cache
Expand All @@ -955,9 +953,11 @@ def _create_identity_instance(cli_ctx, *args, **kwargs):
# PREVIEW: In Azure Stack environment, use core.instance_discovery=false to disable MSAL's instance discovery.
instance_discovery = cli_ctx.config.getboolean('core', 'instance_discovery', True)

return Identity(*args, encrypt=encrypt, use_msal_http_cache=use_msal_http_cache,
return Identity(authority, tenant_id=tenant_id, client_id=client_id,
encrypt=encrypt,
use_msal_http_cache=use_msal_http_cache,
enable_broker_on_windows=enable_broker_on_windows,
instance_discovery=instance_discovery, **kwargs)
instance_discovery=instance_discovery)


def _on_azure_arc_windows():
Expand Down