From 0366ac29c8916f5ef3cc0a6d04460479572ac23a Mon Sep 17 00:00:00 2001 From: Paul Van Eck Date: Wed, 26 Oct 2022 10:55:52 -0700 Subject: [PATCH] [Identity] Fix issue with cache option usage (#27030) If a user supplies `TokenCachePersistenceOptions` to a `SharedTokenCacheCredential`, these options are not used when the cache is loaded. This can lead to issues when users are trying to use caches with custom names since the default name is used instead. This commit ensures that user-provided cache options are propagated when the cache is loaded. Ref: #26982 Signed-off-by: Paul Van Eck --- sdk/identity/azure-identity/CHANGELOG.md | 6 ++++-- .../azure/identity/_credentials/silent.py | 12 ++++++++---- .../identity/_internal/shared_token_cache.py | 12 ++++++++---- .../tests/test_shared_cache_credential.py | 13 +++++++++++++ .../test_shared_cache_credential_async.py | 19 ++++++++++++++++--- 5 files changed, 49 insertions(+), 13 deletions(-) diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/sdk/identity/azure-identity/CHANGELOG.md index a58a1158f8e4..0e2d24d85365 100644 --- a/sdk/identity/azure-identity/CHANGELOG.md +++ b/sdk/identity/azure-identity/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- Fixed issue where user-supplied `TokenCachePersistenceOptions` weren't propagated when using `SharedTokenCacheCredential` ([#26982](https://github.com/Azure/azure-sdk-for-python/issues/26982)) + ### Other Changes ## 1.12.0b2 (2022-10-11) @@ -133,7 +135,7 @@ Azure-identity is supported on Python 3.7 or later. For more details, please rea ### Bugs Fixed - Handle injected "tenant_id" and "claims" ([#23138](https://github.com/Azure/azure-sdk-for-python/issues/23138)) - + "tenant_id" argument in get_token() method is only supported by: - `AuthorizationCodeCredential` @@ -163,7 +165,7 @@ Azure-identity is supported on Python 3.7 or later. For more details, please rea > Only code written against a beta version such as 1.7.0b1 may be affected. - The `allow_multitenant_authentication` argument has been removed and the default behavior is now as if it were true. - The multitenant authentication feature can be totally disabled by setting the environment variable + The multitenant authentication feature can be totally disabled by setting the environment variable `AZURE_IDENTITY_DISABLE_MULTITENANTAUTH` to `True`. - `azure.identity.RegionalAuthority` is removed. - `regional_authority` argument is removed for `CertificateCredential` and `ClientSecretCredential`. diff --git a/sdk/identity/azure-identity/azure/identity/_credentials/silent.py b/sdk/identity/azure-identity/azure/identity/_credentials/silent.py index a1d36972e8c0..4bb222e94d0c 100644 --- a/sdk/identity/azure-identity/azure/identity/_credentials/silent.py +++ b/sdk/identity/azure-identity/azure/identity/_credentials/silent.py @@ -36,6 +36,7 @@ def __init__(self, authentication_record, **kwargs): self._tenant_id = kwargs.pop("tenant_id", None) or self._auth_record.tenant_id validate_tenant_id(self._tenant_id) self._cache = kwargs.pop("_cache", None) + self._cache_persistence_options = kwargs.pop("cache_persistence_options", None) self._client_applications = {} # type: Dict[str, PublicClientApplication] self._additionally_allowed_tenants = kwargs.pop("additionally_allowed_tenants", []) self._client = MsalClient(**kwargs) @@ -64,10 +65,13 @@ def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument def _initialize(self): if not self._cache and platform.system() in {"Darwin", "Linux", "Windows"}: try: - # This credential accepts the user's default cache regardless of whether it's encrypted. It doesn't - # create a new cache. If the default cache exists, the user must have created it earlier. If it's - # unencrypted, the user must have allowed that. - self._cache = _load_persistent_cache(TokenCachePersistenceOptions(allow_unencrypted_storage=True)) + # If no cache options were provided, the default cache will be used. This credential accepts the + # user's default cache regardless of whether it's encrypted. It doesn't create a new cache. If the + # default cache exists, the user must have created it earlier. If it's unencrypted, the user must + # have allowed that. + options = self._cache_persistence_options or \ + TokenCachePersistenceOptions(allow_unencrypted_storage=True) + self._cache = _load_persistent_cache(options) except Exception: # pylint:disable=broad-except pass diff --git a/sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py b/sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py index 7228e2a01496..cd5561f3e199 100644 --- a/sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py +++ b/sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py @@ -95,6 +95,7 @@ def __init__(self, username=None, **kwargs): # pylint:disable=unused-argument self._username = username self._tenant_id = kwargs.pop("tenant_id", None) self._cache = kwargs.pop("_cache", None) + self._cache_persistence_options = kwargs.pop("cache_persistence_options", None) self._client = None # type: Optional[AadClientBase] self._client_kwargs = kwargs self._client_kwargs["tenant_id"] = "organizations" @@ -116,10 +117,13 @@ def _initialize(self): def _load_cache(self): if not self._cache and self.supported(): try: - # This credential accepts the user's default cache regardless of whether it's encrypted. It doesn't - # create a new cache. If the default cache exists, the user must have created it earlier. If it's - # unencrypted, the user must have allowed that. - self._cache = _load_persistent_cache(TokenCachePersistenceOptions(allow_unencrypted_storage=True)) + # If no cache options were provided, the default cache will be used. This credential accepts the + # user's default cache regardless of whether it's encrypted. It doesn't create a new cache. If the + # default cache exists, the user must have created it earlier. If it's unencrypted, the user must + # have allowed that. + options = self._cache_persistence_options or \ + TokenCachePersistenceOptions(allow_unencrypted_storage=True) + self._cache = _load_persistent_cache(options) except Exception: # pylint:disable=broad-except pass diff --git a/sdk/identity/azure-identity/tests/test_shared_cache_credential.py b/sdk/identity/azure-identity/tests/test_shared_cache_credential.py index 524d47b2531e..21e2b4394346 100644 --- a/sdk/identity/azure-identity/tests/test_shared_cache_credential.py +++ b/sdk/identity/azure-identity/tests/test_shared_cache_credential.py @@ -9,6 +9,7 @@ AzureAuthorityHosts, CredentialUnavailableError, SharedTokenCacheCredential, + TokenCachePersistenceOptions, ) from azure.identity._constants import DEVELOPER_SIGN_ON_CLIENT_ID, EnvironmentVariables from azure.identity._internal.shared_token_cache import ( @@ -764,6 +765,18 @@ def test_initialization(): assert mock_cache_loader.call_count == 1 +def test_initialization_with_cache_options(): + """the credential should use user-supplied persistence options""" + + with patch("azure.identity._internal.shared_token_cache._load_persistent_cache") as mock_cache_loader: + options = TokenCachePersistenceOptions(name="foo.cache") + credential = SharedTokenCacheCredential(cache_persistence_options=options) + + with pytest.raises(CredentialUnavailableError): + credential.get_token("scope") + mock_cache_loader.assert_called_once_with(options) + + def test_authentication_record_authenticating_tenant(): """when given a record and 'tenant_id', the credential should authenticate in the latter""" diff --git a/sdk/identity/azure-identity/tests/test_shared_cache_credential_async.py b/sdk/identity/azure-identity/tests/test_shared_cache_credential_async.py index 3ae1bd466979..6e778e5e8b20 100644 --- a/sdk/identity/azure-identity/tests/test_shared_cache_credential_async.py +++ b/sdk/identity/azure-identity/tests/test_shared_cache_credential_async.py @@ -7,7 +7,7 @@ from azure.core.exceptions import ClientAuthenticationError from azure.core.pipeline.policies import SansIOHTTPPolicy -from azure.identity import CredentialUnavailableError +from azure.identity import CredentialUnavailableError, TokenCachePersistenceOptions from azure.identity.aio import SharedTokenCacheCredential from azure.identity._constants import EnvironmentVariables from azure.identity._internal.shared_token_cache import ( @@ -68,14 +68,14 @@ async def send(*_, **__): _cache=populated_cache(get_account_event("test@user", "uid", "utid")), transport=transport ) - # async with before initialization: credential should call aexit but not aenter + # async with before initialization: credential should call __aexit__ but not __aenter__ async with credential: await credential.get_token("scope") assert transport.__aenter__.call_count == 0 assert transport.__aexit__.call_count == 1 - # async with after initialization: credential should call aenter and aexit + # async with after initialization: credential should call __aenter__ and __aexit__ async with credential: await credential.get_token("scope") assert transport.__aenter__.call_count == 1 @@ -621,6 +621,19 @@ async def test_initialization(): assert mock_cache_loader.call_count == 1 +@pytest.mark.asyncio +async def test_initialization_with_cache_options(): + """the credential should use user-supplied persistence options""" + + with patch("azure.identity._internal.shared_token_cache._load_persistent_cache") as mock_cache_loader: + options = TokenCachePersistenceOptions(name="foo.cache") + credential = SharedTokenCacheCredential(cache_persistence_options=options) + + with pytest.raises(CredentialUnavailableError): + await credential.get_token("scope") + mock_cache_loader.assert_called_once_with(options) + + @pytest.mark.asyncio async def test_multitenant_authentication(): first_token = "***"