Skip to content

[Identity] Update get_token method signatures #34953

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

Closed
Paul Van Eck (pvaneck) wants to merge 1 commit into from
Closed

[Identity] Update get_token method signatures #34953

Show file tree
Hide file tree
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
17 changes: 14 additions & 3 deletions sdk/identity/azure-identity/azure/identity/_credentials/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ def __init__(self, **kwargs: Any) -> None:
)

def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes`.

Expand All @@ -76,17 +81,23 @@ def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token if possible. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
:raises ~azure.core.exceptions.ClientAuthenticationError: authentication failed. The exception has a
`message` attribute listing each authentication attempt and its error message.
"""
if self._successful_credential:
token = self._successful_credential.get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
token = self._successful_credential.get_token(
*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs
)
_LOGGER.info(
"%s acquired a token from %s", self.__class__.__name__, self._successful_credential.__class__.__name__
)
return token

return super(AzureApplicationCredential, self).get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
return super(AzureApplicationCredential, self).get_token(
*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs
)
16 changes: 14 additions & 2 deletions sdk/identity/azure-identity/azure/identity/_credentials/authorization_code.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ def close(self) -> None:
self.__exit__()

def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes`.

Expand All @@ -78,6 +83,8 @@ def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand All @@ -87,7 +94,12 @@ def get_token(
"""
# pylint:disable=useless-super-delegation
return super(AuthorizationCodeCredential, self).get_token(
*scopes, claims=claims, tenant_id=tenant_id, client_secret=self._client_secret, **kwargs
*scopes,
claims=claims,
tenant_id=tenant_id,
enable_cae=enable_cae,
client_secret=self._client_secret,
**kwargs
)

def _acquire_token_silently(self, *scopes: str, **kwargs) -> Optional[AccessToken]:
Expand Down
3 changes: 3 additions & 0 deletions sdk/identity/azure-identity/azure/identity/_credentials/azd_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def get_token(
*scopes: str,
claims: Optional[str] = None, # pylint:disable=unused-argument
tenant_id: Optional[str] = None,
enable_cae: bool = False, # pylint:disable=unused-argument
**kwargs: Any,
) -> AccessToken:
"""Request an access token for `scopes`.
Expand All @@ -109,6 +110,8 @@ def get_token(
https://learn.microsoft.com/entra/identity-platform/scopes-oidc.
:keyword str claims: not used by this credential; any value provided will be ignored.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested

@xiangyan99 Xiang Yan (xiangyan99) Apr 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really use it?

It seems not make sense to explicitly list the argument then explicitly say not supported.

@pvaneck Paul Van Eck (pvaneck) Apr 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Based on discussion yesterday, I thought the agreement was to have our get_token implementations use matching signatures as the TokenCredential Protocol get_token.

The current signature mismatch is what is causing the PyCharm IDE to complain:
image

I agree it is weird to list it and not use it, but that's pretty much the nature of trying to match the signature for credentials with differing sets of capabilities. In this same credential, we do the same thing with claims.

We can sit on this PR for now as we solicit more feedback on if we should cater to the PyCharm type-checker. It definitely doesn't need to go into next week's release. MyPy is fine resolving our Credentials as TokenCredentials even without the exact signature match, so there is still a bit of uncertainty here on if we should make this change.

@xiangyan99 Xiang Yan (xiangyan99) Apr 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My concern is if we explicitly say one argument is not supported.

Does that mean for the arguments that we don't explicitly say not supported means they are supported?

token. Not supported by this credential.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand Down
3 changes: 3 additions & 0 deletions sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ def get_token(
*scopes: str,
claims: Optional[str] = None, # pylint:disable=unused-argument
tenant_id: Optional[str] = None,
enable_cae: bool = False, # pylint:disable=unused-argument
**kwargs: Any,
) -> AccessToken:
"""Request an access token for `scopes`.
Expand All @@ -86,6 +87,8 @@ def get_token(
https://learn.microsoft.com/entra/identity-platform/scopes-oidc.
:keyword str claims: not used by this credential; any value provided will be ignored.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Not supported by this credential.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand Down
3 changes: 3 additions & 0 deletions sdk/identity/azure-identity/azure/identity/_credentials/azure_powershell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ def get_token(
*scopes: str,
claims: Optional[str] = None, # pylint:disable=unused-argument
tenant_id: Optional[str] = None,
enable_cae: bool = False, # pylint:disable=unused-argument
**kwargs: Any,
) -> AccessToken:
"""Request an access token for `scopes`.
Expand All @@ -101,6 +102,8 @@ def get_token(
https://learn.microsoft.com/entra/identity-platform/scopes-oidc.
:keyword str claims: not used by this credential; any value provided will be ignored.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Not supported by this credential.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand Down
13 changes: 11 additions & 2 deletions sdk/identity/azure-identity/azure/identity/_credentials/chained.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ def close(self) -> None:
self.__exit__()

def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Request a token from each chained credential, in order, returning the first token received.

Expand All @@ -82,6 +87,8 @@ def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token if the underlying credential supports it. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand All @@ -91,7 +98,9 @@ def get_token(
history = []
for credential in self.credentials:
try:
token = credential.get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
token = credential.get_token(
*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs
)
_LOGGER.info("%s acquired a token from %s", self.__class__.__name__, credential.__class__.__name__)
self._successful_credential = credential
within_credential_chain.set(False)
Expand Down
15 changes: 12 additions & 3 deletions sdk/identity/azure-identity/azure/identity/_credentials/default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,12 @@ def __init__(self, **kwargs: Any) -> None: # pylint: disable=too-many-statement
super(DefaultAzureCredential, self).__init__(*credentials)

def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes`.

Expand All @@ -208,6 +213,8 @@ def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token if the underlying credential supports it. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand All @@ -216,12 +223,14 @@ def get_token(
`message` attribute listing each authentication attempt and its error message.
"""
if self._successful_credential:
token = self._successful_credential.get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
token = self._successful_credential.get_token(
*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs
)
_LOGGER.info(
"%s acquired a token from %s", self.__class__.__name__, self._successful_credential.__class__.__name__
)
return token
within_dac.set(True)
token = super().get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
token = super().get_token(*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs)
within_dac.set(False)
return token
11 changes: 9 additions & 2 deletions sdk/identity/azure-identity/azure/identity/_credentials/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,12 @@ def close(self) -> None:

@log_get_token("EnvironmentCredential")
def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes`.

Expand All @@ -135,6 +140,8 @@ def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand All @@ -148,4 +155,4 @@ def get_token(
"this issue."
)
raise CredentialUnavailableError(message=message)
return self._credential.get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
return self._credential.get_token(*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs)
9 changes: 8 additions & 1 deletion sdk/identity/azure-identity/azure/identity/_credentials/managed_identity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,12 @@ def close(self) -> None:

@log_get_token("ManagedIdentityCredential")
def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False, # pylint:disable=unused-argument
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes`.

Expand All @@ -123,6 +128,8 @@ def get_token(

:keyword str claims: not used by this credential; any value provided will be ignored.
:keyword str tenant_id: not used by this credential; any value provided will be ignored.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Not supported by this credential.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand Down
13 changes: 9 additions & 4 deletions sdk/identity/azure-identity/azure/identity/_credentials/silent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,24 +55,29 @@ def __exit__(self, *args):
self._client.__exit__(*args)

def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
if not scopes:
raise ValueError('"get_token" requires at least one scope')

token_cache = self._cae_cache if kwargs.get("enable_cae") else self._cache
token_cache = self._cae_cache if enable_cae else self._cache

# Try to load the cache if it is None.
if not token_cache:
token_cache = self._initialize_cache(is_cae=bool(kwargs.get("enable_cae")))
token_cache = self._initialize_cache(is_cae=enable_cae)

# If the cache is still None, raise an error.
if not token_cache:
if within_dac.get():
raise CredentialUnavailableError(message="Shared token cache unavailable")
raise ClientAuthenticationError(message="Shared token cache unavailable")

return self._acquire_token_silent(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
return self._acquire_token_silent(*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs)

def _initialize_cache(self, is_cae: bool = False) -> Optional[TokenCache]:

Expand Down
9 changes: 8 additions & 1 deletion sdk/identity/azure-identity/azure/identity/_credentials/vscode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,12 @@ def close(self) -> None:

@log_get_token("VSCodeCredential")
def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False, # pylint:disable=unused-argument
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes` as the user currently signed in to Visual Studio Code.

Expand All @@ -153,6 +158,8 @@ def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Not supported by this credential.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand Down
7 changes: 6 additions & 1 deletion sdk/identity/azure-identity/azure/identity/_internal/managed_identity_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ def close(self) -> None:
self.__exit__()

def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False, # pylint:disable=unused-argument
**kwargs: Any
) -> AccessToken:
if not self._client:
raise CredentialUnavailableError(message=self.get_unavailable_message())
Expand Down
15 changes: 12 additions & 3 deletions sdk/identity/azure-identity/azure/identity/aio/_credentials/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,12 @@ def __init__(
)

async def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Asynchronously request an access token for `scopes`.

Expand All @@ -75,17 +80,21 @@ async def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token if possible. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
:raises ~azure.core.exceptions.ClientAuthenticationError: authentication failed. The exception has a
`message` attribute listing each authentication attempt and its error message.
"""
if self._successful_credential:
token = await self._successful_credential.get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
token = await self._successful_credential.get_token(
*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs
)
_LOGGER.info(
"%s acquired a token from %s", self.__class__.__name__, self._successful_credential.__class__.__name__
)
return token

return await super().get_token(*scopes, claims=claims, tenant_id=tenant_id, **kwargs)
return await super().get_token(*scopes, claims=claims, tenant_id=tenant_id, enable_cae=enable_cae, **kwargs)
16 changes: 14 additions & 2 deletions sdk/identity/azure-identity/azure/identity/aio/_credentials/authorization_code.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ def __init__(
super().__init__()

async def get_token(
self, *scopes: str, claims: Optional[str] = None, tenant_id: Optional[str] = None, **kwargs: Any
self,
*scopes: str,
claims: Optional[str] = None,
tenant_id: Optional[str] = None,
enable_cae: bool = False,
**kwargs: Any
) -> AccessToken:
"""Request an access token for `scopes`.

Expand All @@ -85,6 +90,8 @@ async def get_token(
:keyword str claims: additional claims required in the token, such as those returned in a resource provider's
claims challenge following an authorization failure.
:keyword str tenant_id: optional tenant to include in the token request.
:keyword bool enable_cae: Indicates whether to enable Continuous Access Evaluation (CAE) for the requested
token. Defaults to False.

:return: An access token with the desired scopes.
:rtype: ~azure.core.credentials.AccessToken
Expand All @@ -93,7 +100,12 @@ async def get_token(
``response`` attribute.
"""
return await super(AuthorizationCodeCredential, self).get_token(
*scopes, claims=claims, tenant_id=tenant_id, client_secret=self._client_secret, **kwargs
*scopes,
claims=claims,
tenant_id=tenant_id,
enable_cae=enable_cae,
client_secret=self._client_secret,
**kwargs
)

async def _acquire_token_silently(self, *scopes: str, **kwargs: Any) -> Optional[AccessToken]:
Expand Down
Loading