diff --git a/sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py b/sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py index 478df7055193..4eb7979d847d 100644 --- a/sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py +++ b/sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py @@ -225,7 +225,7 @@ class BearerTokenChallengePolicy(BearerTokenCredentialPolicy): :param str scopes: Lets you specify the type of access needed. :keyword bool discover_tenant: Determines if tenant discovery should be enabled. Defaults to True. :keyword bool discover_scopes: Determines if scopes from authentication challenges should be provided to token - requests, instead of the scopes given to the policy's constructor. Defaults to True. + requests, instead of the scopes given to the policy's constructor, if any are present. Defaults to True. :raises: :class:`~azure.core.exceptions.ServiceRequestError` """ @@ -257,7 +257,10 @@ def on_challenge(self, request: "PipelineRequest", response: "PipelineResponse") try: challenge = _HttpChallenge(response.http_response.headers.get("WWW-Authenticate")) # azure-identity credentials require an AADv2 scope but the challenge may specify an AADv1 resource + # if no scopes are included in the challenge, challenge.scope and challenge.resource will both be '' scope = challenge.scope or challenge.resource + "/.default" if self._discover_scopes else self._scopes + if scope == "/.default": + scope = self._scopes except ValueError: return False diff --git a/sdk/core/azure-core/azure/core/pipeline/policies/_authentication_async.py b/sdk/core/azure-core/azure/core/pipeline/policies/_authentication_async.py index e57b156d8ed4..bbbdb61d8385 100644 --- a/sdk/core/azure-core/azure/core/pipeline/policies/_authentication_async.py +++ b/sdk/core/azure-core/azure/core/pipeline/policies/_authentication_async.py @@ -141,7 +141,7 @@ class AsyncBearerTokenChallengePolicy(AsyncBearerTokenCredentialPolicy): :param str scopes: Lets you specify the type of access needed. :keyword bool discover_tenant: Determines if tenant discovery should be enabled. Defaults to True. :keyword bool discover_scopes: Determines if scopes from authentication challenges should be provided to token - requests, instead of the scopes given to the policy's constructor. Defaults to True. + requests, instead of the scopes given to the policy's constructor, if any are present. Defaults to True. :raises: :class:`~azure.core.exceptions.ServiceRequestError` """ @@ -173,7 +173,10 @@ async def on_challenge(self, request: "PipelineRequest", response: "PipelineResp try: challenge = _HttpChallenge(response.http_response.headers.get("WWW-Authenticate")) # azure-identity credentials require an AADv2 scope but the challenge may specify an AADv1 resource + # if no scopes are included in the challenge, challenge.scope and challenge.resource will both be '' scope = challenge.scope or challenge.resource + "/.default" if self._discover_scopes else self._scopes + if scope == "/.default": + scope = self._scopes except ValueError: return False diff --git a/sdk/core/azure-core/tests/async_tests/test_authentication_async.py b/sdk/core/azure-core/tests/async_tests/test_authentication_async.py index 893f7c921e40..91098956f13a 100644 --- a/sdk/core/azure-core/tests/async_tests/test_authentication_async.py +++ b/sdk/core/azure-core/tests/async_tests/test_authentication_async.py @@ -227,7 +227,7 @@ async def fake_send(*args, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -async def test_multitenant_policy_uses_scopes_and_tenant(http_request): +async def test_challenge_policy_uses_scopes_and_tenant(http_request): """The policy's token requests should pass the parsed scope and tenant ID from the challenge""" async def test_with_challenge(challenge, expected_scope, expected_tenant): @@ -298,7 +298,7 @@ async def get_token(*scopes, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -async def test_multitenant_policy_disable_tenant_discovery(http_request): +async def test_challenge_policy_disable_tenant_discovery(http_request): """The policy's token requests should exclude the challenge's tenant if requested""" async def test_with_challenge(challenge, challenge_scope): @@ -358,7 +358,7 @@ async def get_token(*scopes, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -async def test_multitenant_policy_disable_scopes_discovery(http_request): +async def test_challenge_policy_disable_scopes_discovery(http_request): """The policy's token requests should exclude the challenge's scope if requested""" async def test_with_challenge(challenge, challenge_scope, challenge_tenant): @@ -418,7 +418,7 @@ async def get_token(*scopes, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -async def test_multitenant_policy_disable_any_discovery(http_request): +async def test_challenge_policy_disable_any_discovery(http_request): """The policy shouldn't respond to the challenge if it can't use the provided scope or tenant""" async def test_with_challenge(challenge, challenge_scope, challenge_tenant): @@ -469,6 +469,66 @@ async def get_token(*scopes, **kwargs): await test_with_challenge(challenge_with_commas, scope, tenant) +@pytest.mark.parametrize("http_request", HTTP_REQUESTS) +async def test_challenge_policy_no_scope_in_challenge(http_request): + """The policy's token requests should use constructor scopes if none are in the challenge""" + + async def test_with_challenge(challenge, challenge_scope, challenge_tenant): + bad_token = "bad_token" + + class Requests: + count = 0 + + class TokenRequests: + count = 0 + + async def send(request): + Requests.count += 1 + if Requests.count == 1: + # first request triggers a 401 response w/ auth challenge + assert bad_token in request.headers["Authorization"] + return challenge + elif Requests.count == 2: + # second request should still be unauthorized because we didn't authenticate with the correct scope + assert bad_token in request.headers["Authorization"] + return challenge + raise ValueError("unexpected request") + + async def get_token(*scopes, **kwargs): + assert len(scopes) == 1 + TokenRequests.count += 1 + if TokenRequests.count == 1: + # first request uses the scope given to the policy, and no tenant ID + assert scopes[0] != challenge_scope + assert kwargs.get("tenant_id") is None + return AccessToken(bad_token, 0) + elif TokenRequests.count == 2: + # second request should use the tenant specified in the auth challenge, and same scope as before + assert scopes[0] != challenge_scope + assert kwargs.get("tenant_id") == challenge_tenant + return AccessToken(bad_token, 0) + raise ValueError("unexpected token request") + + credential = Mock(get_token=Mock(wraps=get_token)) + policy = AsyncBearerTokenChallengePolicy(credential, "scope") + pipeline = AsyncPipeline(policies=[policy], transport=Mock(send=send)) + await pipeline.run(http_request("GET", "https://localhost")) + + tenant = "tenant-id" + endpoint = f"https://authority.net/{tenant}/oauth2/authorize" + resource = "https://challenge.resource" + scope = f"{resource}/.default" + + # this challenge has no `resource` or `scope` field + challenge_with_commas = Mock( + status_code=401, + headers={"WWW-Authenticate": f'Bearer authorization="{endpoint}"'}, + ) + # the request should fail after the challenge because we don't use the correct scope + # after the second 4xx response, the policy should raise the authentication error + await test_with_challenge(challenge_with_commas, scope, tenant) + + def get_completed_future(result=None): fut = asyncio.Future() fut.set_result(result) diff --git a/sdk/core/azure-core/tests/test_authentication.py b/sdk/core/azure-core/tests/test_authentication.py index 66e12af16763..0ae83911d056 100644 --- a/sdk/core/azure-core/tests/test_authentication.py +++ b/sdk/core/azure-core/tests/test_authentication.py @@ -264,7 +264,7 @@ def raise_the_second_time(*args, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -def test_multitenant_policy_uses_scopes_and_tenant(http_request): +def test_challenge_policy_uses_scopes_and_tenant(http_request): """The policy's token requests should pass the parsed scope and tenant ID from the challenge, by default""" def test_with_challenge(challenge, expected_scope, challenge_tenant): @@ -337,7 +337,7 @@ def get_token(*scopes, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -def test_multitenant_policy_disable_tenant_discovery(http_request): +def test_challenge_policy_disable_tenant_discovery(http_request): """The policy's token requests should exclude the challenge's tenant if requested""" def test_with_challenge(challenge, expected_scope): @@ -397,7 +397,7 @@ def get_token(*scopes, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -def test_multitenant_policy_disable_scopes_discovery(http_request): +def test_challenge_policy_disable_scopes_discovery(http_request): """The policy's token requests should exclude the challenge's scope if requested""" def test_with_challenge(challenge, expected_scope, challenge_tenant): @@ -457,7 +457,7 @@ def get_token(*scopes, **kwargs): @pytest.mark.parametrize("http_request", HTTP_REQUESTS) -def test_multitenant_policy_disable_any_discovery(http_request): +def test_challenge_policy_disable_any_discovery(http_request): """The policy shouldn't respond to the challenge if it can't use the provided scope or tenant""" def test_with_challenge(challenge, expected_scope, challenge_tenant): @@ -508,6 +508,66 @@ def get_token(*scopes, **kwargs): test_with_challenge(challenge_with_commas, scope, tenant) +@pytest.mark.parametrize("http_request", HTTP_REQUESTS) +def test_challenge_policy_no_scope_in_challenge(http_request): + """The policy's token requests should use constructor scopes if none are in the challenge""" + + def test_with_challenge(challenge, expected_scope, challenge_tenant): + bad_token = "bad_token" + + class Requests: + count = 0 + + class TokenRequests: + count = 0 + + def send(request): + Requests.count += 1 + if Requests.count == 1: + # first request triggers a 401 response w/ auth challenge + assert bad_token in request.headers["Authorization"] + return challenge + elif Requests.count == 2: + # second request should still be unauthorized because we didn't authenticate with the correct scope + assert bad_token in request.headers["Authorization"] + return challenge + raise ValueError("unexpected request") + + def get_token(*scopes, **kwargs): + assert len(scopes) == 1 + TokenRequests.count += 1 + if TokenRequests.count == 1: + # first request uses the scope given to the policy, and no tenant ID + assert scopes[0] != expected_scope + assert kwargs.get("tenant_id") is None + return AccessToken(bad_token, 0) + elif TokenRequests.count == 2: + # second request should use the tenant specified in the auth challenge, and same scope as before + assert scopes[0] != expected_scope + assert kwargs.get("tenant_id") == challenge_tenant + return AccessToken(bad_token, 0) + raise ValueError("unexpected token request") + + credential = Mock(get_token=Mock(wraps=get_token)) + policy = BearerTokenChallengePolicy(credential, "scope") + pipeline = Pipeline(policies=[policy], transport=Mock(send=send)) + pipeline.run(http_request("GET", "https://localhost")) + + tenant = "tenant-id" + endpoint = f"https://authority.net/{tenant}/oauth2/authorize" + resource = "https://challenge.resource" + scope = f"{resource}/.default" + + # this challenge has no `resource` or `scope` field + challenge_with_commas = Mock( + status_code=401, + headers={"WWW-Authenticate": f'Bearer authorization="{endpoint}"'}, + ) + # the request should fail after the challenge because we don't use the correct scope + # after the second 4xx response, the policy should raise the authentication error + test_with_challenge(challenge_with_commas, scope, tenant) + + @pytest.mark.skipif(azure.core.__version__ >= "2", reason="this test applies only to azure-core 1.x") @pytest.mark.parametrize("http_request", HTTP_REQUESTS) def test_key_vault_regression(http_request):