{Core} Refactor code for MSAL authentication #29439
Conversation
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Account
| self.authority.tenant, claims) | ||
| result = self.acquire_token_silent_with_error(list(scopes), self._account, claims_challenge=claims, **kwargs) | ||
| self._msal_app.authority.tenant, claims) | ||
| result = self._msal_app.acquire_token_silent_with_error(list(scopes), self._account, claims_challenge=claims, |
There was a problem hiding this comment.
By saving msal.PublicClientApplication instance to self._msal_app, self._account is no longer saved as an attribute of msal.PublicClientApplication. This avoids the possible conflict of msal.PublicClientApplication introducing an _account attribute of its own.
| match = re.search(r'-+BEGIN CERTIFICATE-+(?P<public>[^-]+)-+END CERTIFICATE-+', cert_file_string, re.I) | ||
| public_certificate = match.group().strip() |
There was a problem hiding this comment.
We should not test code with the code itself:
azure-cli/src/azure-cli-core/azure/cli/core/auth/identity.py
Lines 290 to 292 in d142b43
|
|
||
| class TestIdentity(unittest.TestCase): | ||
|
|
||
| @mock.patch("azure.cli.core.auth.identity.ServicePrincipalStore.save_entry") | ||
| @mock.patch("msal.application.ConfidentialClientApplication.acquire_token_for_client") | ||
| @mock.patch("msal.application.ConfidentialClientApplication.__init__") | ||
| @mock.patch("msal.application.ConfidentialClientApplication.__init__", return_value=None) |
There was a problem hiding this comment.
msal.application.ConfidentialClientApplication.__init__ is previously called as super().__init__ in azure.cli.core.auth.msal_authentication.ServicePrincipalCredential.__init__. The returned MagicMock gets discarded.
Now msal.application.ConfidentialClientApplication.__init__ is directly invoked. Without return_value=None, patching msal.application.ConfidentialClientApplication.__init__ makes it return a MagicMock. pytest fails:
self = <azure.cli.core.auth.identity.Identity object at 0x0000021CEACFC150>
client_id = 'sp_id1', credential = {'client_secret': 'test_secret'}
scopes = 'openid'
def login_with_service_principal(self, client_id, credential, scopes):
"""
`credential` is a dict returned by ServicePrincipalAuth.build_credential
"""
sp_auth = ServicePrincipalAuth.build_from_credential(self.tenant_id, client_id, credential)
client_credential = sp_auth.get_msal_client_credential()
> cca = ConfidentialClientApplication(client_id, client_credential, **self._msal_app_kwargs)
E TypeError: __init__() should return None, not 'MagicMock'
..\identity.py:196: TypeError
| client_credential = sp_auth.get_msal_client_credential() | ||
| cca = ConfidentialClientApplication(client_id, client_credential, **self._msal_app_kwargs) | ||
| result = cca.acquire_token_for_client(scopes) |
There was a problem hiding this comment.
Because ServicePrincipalCredential stops inheriting from msal.ConfidentialClientApplication, it is no longer possible to call acquire_token_for_client on the cred. We prepare client_credential and directly create a ConfidentialClientApplication instance.
There was a problem hiding this comment.
This file is renamed to more accurately reflect its content.
| logger = get_logger(__name__) | ||
|
|
||
|
|
||
| class UserCredential(PublicClientApplication): | ||
| class UserCredential: # pylint: disable=too-few-public-methods |
There was a problem hiding this comment.
Interestingly, pylint doesn't think this is worth a class. Haha
Used when class has too few public methods, so be sure it's really worth it.
Ref: https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/too-few-public-methods.html
There was a problem hiding this comment.
We have to define such class so that it can be passed to SDK as a credential.
|
This PR breaks from azure.cli.core.auth.msal_authentication import ServicePrincipalCredential
credential, _, _ = profile.get_login_credentials(subscription_id=profile.get_subscription()["id"],
resource=consts.KAP_1P_Server_App_Scope)
if isinstance(credential._credential, ServicePrincipalCredential):
# This is a workaround to fix the issue where the token is not being refreshed
# https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/692
credential._credential.remove_tokens_for_client()This is considered a bad practice as internal As AzureAD/microsoft-authentication-library-for-python#692 has been resolved, this workaround should no longer be necessary. |
Related command
az loginDescription
Change 1: Refactor
UserCredentialandServicePrincipalCredentialIn the current code,
UserCredentialinherits frommsal.PublicClientApplicationServicePrincipalCredentialinherits frommsal.ConfidentialClientApplicationThis exposes the internal MSAL application instances to the caller SDK which only requires the
get_token()callback interface.This PR stops
UserCredentialandServicePrincipalCredentialfrom inheriting from MSAL, but makes them keep an internal MSAL application instance, in order to greatly reduce MSAL applications' exposure.Change 2: Refactor
ServicePrincipalAuthAll attributes are set at
__init__()to avoid frequently callinggetattr()to check the existence of attributes. The logic for generatingclient_credentialis moved toServicePrincipalAuth.get_msal_client_credential().This PR paves way for