Skip to content
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

Jump to bottom

{Core} Only pass data to MSAL as kwargs #30062

Merged
merged 1 commit into from
Oct 24, 2024
Merged

{Core} Only pass data to MSAL as kwargs #30062

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
9 changes: 5 additions & 4 deletions src/azure-cli-core/azure/cli/core/auth/credential_adaptor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,13 @@ def signed_session(self, session=None):
def get_token(self, *scopes, **kwargs):
logger.debug("CredentialAdaptor.get_token: scopes=%r, kwargs=%r", scopes, kwargs)

# SDK azure-keyvault-keys 4.5.0b5 passes tenant_id as kwargs, but we don't support tenant_id for now,
# so discard it.
kwargs.pop('tenant_id', None)
# Discard unsupported kwargs: tenant_id, enable_cae
filtered_kwargs = {}
if 'data' in kwargs:
filtered_kwargs['data'] = kwargs['data']

scopes = _normalize_scopes(scopes)
token, _ = self._get_token(scopes, **kwargs)
token, _ = self._get_token(scopes, **filtered_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

As of this PR, it looks like we don't need that filtered_kwargs at all.

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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).

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bebound bebound Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

We only support scopes and data, but not tenant_id, enable_cae. That's why anything else in kwargs is discarded.

return token

def get_auxiliary_tokens(self, *scopes, **kwargs):
Expand Down