{Core} Only pass data to MSAL as kwargs#30062
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Auto-Assign
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
|
||
| scopes = _normalize_scopes(scopes) | ||
| token, _ = self._get_token(scopes, **kwargs) | ||
| token, _ = self._get_token(scopes, **filtered_kwargs) |
There was a problem hiding this comment.
As of this PR, it looks like we don't need that filtered_kwargs at all.
| token, _ = self._get_token(scopes, **filtered_kwargs) | |
| token, _ = self._get_token(scopes, data=kwargs.get("data")) |
Open question:
Currently there are multiple get_token(..., **kwargs) function scattering here and there. If/since we say - via this PR - that only data is an expected input parameter, why don't we take one step further and refactor those functions to get_token(..., data=None)?
Even if we somehow still want to accept and then ignore arbitrary parameters, a more readable pattern is def get_token(..., data=None, **ignored) and then we do NOT relay the ignored into underlying functions.
There was a problem hiding this comment.
The reason why I don't use data=kwargs.get("data") is because this will cause data always to be passed, even as None. I want to make sure data is passed only when the caller of get_token passes data.
As for not making data a public argument, this is because Azure CLI Core is also used by other people or projects, I don't want to make data a public argument before MSAL does the same (AzureAD/microsoft-authentication-library-for-python#755).
There was a problem hiding this comment.
The reason why I don't use
data=kwargs.get("data")is because this will causedataalways to be passed, even asNone. I want to make suredatais passed only when the caller ofget_tokenpassesdata.
Fair enough. In that case, I would suggest you put that reasoning as a comment at the line of filtered_kwargs, to prevent a future maintainer refactoring it to the simpler way.
As for not making
dataa public argument, this is because Azure CLI Core is also used by other people or projects, I don't want to makedataa public argument before MSAL does the same (AzureAD/microsoft-authentication-library-for-python#755).
Good point. This PR is ready to go as-is, without AzureAD/microsoft-authentication-library-for-python#755 (If we are really going to expose that in MSAL, we will revisit its naming.)
There was a problem hiding this comment.
I would suggest you put that reasoning as a comment at the line of
filtered_kwargs, to prevent a future maintainer refactoring it to the simpler way.
I am the only one maintaining this part. Let's keep it as is, as I will change it soon.
There was a problem hiding this comment.
One silly question, are tenant_id and data the only known kwargs that might be passed to get_token currently?
def get_token(self, *scopes, **kwargs)
There was a problem hiding this comment.
We only support scopes and data, but not tenant_id, enable_cae. That's why anything else in kwargs is discarded.
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
Approved with suggested changes.
| if 'data' in kwargs: | ||
| filtered_kwargs['data'] = kwargs['data'] |
There was a problem hiding this comment.
claims is a supported keyword argument of UserCredential.get_token:
Not passing claims breaks CAE silent re-authentication. Interestingly, no issue is reported. 🤔
5 participants
Description
Azure CLI currently passes
kwargsreceived byget_token()directly to MSAL. Since SDK is adding more keyword arguments, such asenable_cae(Azure/azure-sdk-for-python#37358), this will cause failure asenable_caeis not recognized by MSAL.This PR changes the behavior so that only
datais passed to MSAL askwargs.Also see AzureAD/microsoft-authentication-library-for-python#755
Testing Guide
az extension add --name sshaz ssh vm -n xxx -g xxx