Skip to content

Commit fa91e4e

Browse files
chlowellchlowell
authored
Fix error creating EnvironmentCredential with username/password (Azure#7260)
1 parent 2a0c6c3 commit fa91e4e

File tree

5 files changed

+84
-15
lines changed

5 files changed

+84
-15
lines changed

‎sdk/identity/azure-identity/HISTORY.md

+7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
# Release History
22

3+
## 1.0.0b4
4+
### Fixes and improvements:
5+
- `UsernamePasswordCredential` correctly handles environment configuration with
6+
no tenant information (#7260)
7+
- MSAL's user realm discovery requests are sent through credential
8+
pipelines (#7260)
9+
310
## 1.0.0b3 (2019-09-10)
411
### New features:
512
- `SharedTokenCacheCredential` authenticates with tokens stored in a local

‎sdk/identity/azure-identity/azure/identity/_internal/msal_credentials.py

+4-7
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,6 @@
2020
except AttributeError: # Python 2.7, abc exists, but not ABC
2121
ABC = abc.ABCMeta("ABC", (object,), {"__slots__": ()}) # type: ignore
2222

23-
try:
24-
from unittest import mock
25-
except ImportError: # python < 3.3
26-
import mock # type: ignore
27-
2823
try:
2924
from typing import TYPE_CHECKING
3025
except ImportError:
@@ -64,10 +59,11 @@ def _create_app(self, cls):
6459
"""Creates an MSAL application, patching msal.authority to use an azure-core pipeline during tenant discovery"""
6560

6661
# MSAL application initializers use msal.authority to send AAD tenant discovery requests
67-
with mock.patch("msal.authority.requests", self._adapter):
62+
with self._adapter:
6863
app = cls(client_id=self._client_id, client_credential=self._client_credential, authority=self._authority)
6964

7065
# monkeypatch the app to replace requests.Session with MsalTransportAdapter
66+
app.client.session.close()
7167
app.client.session = self._adapter
7268

7369
return app
@@ -106,8 +102,9 @@ class PublicClientCredential(MsalCredential):
106102

107103
def __init__(self, **kwargs):
108104
# type: (Any) -> None
105+
tenant = kwargs.pop("tenant", None) or "organizations"
109106
super(PublicClientCredential, self).__init__(
110-
authority="https://login.microsoftonline.com/" + kwargs.pop("tenant", "organizations"), **kwargs
107+
authority="https://login.microsoftonline.com/" + tenant, **kwargs
111108
)
112109

113110
@abc.abstractmethod

‎sdk/identity/azure-identity/azure/identity/_internal/msal_transport_adapter.py

+21-3
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,18 @@
1212
from azure.core.pipeline.policies import ContentDecodePolicy, NetworkTraceLoggingPolicy, ProxyPolicy, RetryPolicy
1313
from azure.core.pipeline.transport import HttpRequest, RequestsTransport
1414

15+
try:
16+
from unittest import mock
17+
except ImportError: # python < 3.3
18+
import mock # type: ignore
19+
1520
try:
1621
from typing import TYPE_CHECKING
1722
except ImportError:
1823
TYPE_CHECKING = False
1924

2025
if TYPE_CHECKING:
21-
# pylint:disable=unused-import
26+
# pylint:disable=unused-import,ungrouped-imports
2227
from typing import Any, Dict, Mapping, Optional
2328
from azure.core.pipeline import PipelineResponse
2429

@@ -38,17 +43,30 @@ def json(self, **kwargs):
3843

3944
def raise_for_status(self):
4045
# type: () -> None
41-
raise ClientAuthenticationError("authentication failed", self._response)
46+
if self.status_code >= 400:
47+
raise ClientAuthenticationError("authentication failed", self._response)
4248

4349

4450
class MsalTransportAdapter(object):
45-
"""Wraps an azure-core pipeline with the shape of requests.Session"""
51+
"""
52+
Wraps an azure-core pipeline with the shape of requests.Session.
53+
54+
Used as a context manager, patches msal.authority to intercept calls to requests.
55+
"""
4656

4757
def __init__(self, **kwargs):
4858
# type: (Any) -> None
4959
super(MsalTransportAdapter, self).__init__()
60+
self._patch = mock.patch("msal.authority.requests", self)
5061
self._pipeline = self._build_pipeline(**kwargs)
5162

63+
def __enter__(self):
64+
self._patch.__enter__()
65+
return self
66+
67+
def __exit__(self, *args):
68+
self._patch.__exit__(*args)
69+
5270
@staticmethod
5371
def _create_config(**kwargs):
5472
# type: (Any) -> Configuration

‎sdk/identity/azure-identity/azure/identity/credentials.py

+4-3
Original file line numberDiff line numberDiff line change
@@ -433,9 +433,10 @@ def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
433433

434434
if not result:
435435
# cache miss -> request a new token
436-
result = app.acquire_token_by_username_password(
437-
username=self._username, password=self._password, scopes=scopes
438-
)
436+
with self._adapter:
437+
result = app.acquire_token_by_username_password(
438+
username=self._username, password=self._password, scopes=scopes
439+
)
439440

440441
if "access_token" not in result:
441442
raise ClientAuthenticationError(message="authentication failed: {}".format(result.get("error_description")))

‎sdk/identity/azure-identity/tests/test_identity.py

+48-2
Original file line numberDiff line numberDiff line change
@@ -411,10 +411,13 @@ def test_interactive_credential_timeout():
411411
def test_username_password_credential():
412412
expected_token = "access-token"
413413
transport = validating_transport(
414-
requests=[Request()] * 2, # not validating requests because they're formed by MSAL
414+
requests=[Request()] * 3, # not validating requests because they're formed by MSAL
415415
responses=[
416-
# expecting tenant discovery then a token request
416+
# tenant discovery
417417
mock_response(json_payload={"authorization_endpoint": "https://a/b", "token_endpoint": "https://a/b"}),
418+
# user realm discovery, interests MSAL only when the response body contains account_type == "Federated"
419+
mock_response(json_payload={}),
420+
# token request
418421
mock_response(
419422
json_payload={
420423
"access_token": expected_token,
@@ -436,3 +439,46 @@ def test_username_password_credential():
436439

437440
token = credential.get_token("scope")
438441
assert token.token == expected_token
442+
443+
444+
def test_username_password_environment_credential(monkeypatch):
445+
client_id = "fake-client-id"
446+
username = "foo@bar.com"
447+
password = "password"
448+
expected_token = "***"
449+
450+
create_transport = functools.partial(
451+
validating_transport,
452+
requests=[Request()] * 3, # not validating requests because they're formed by MSAL
453+
responses=[
454+
# tenant discovery
455+
mock_response(json_payload={"authorization_endpoint": "https://a/b", "token_endpoint": "https://a/b"}),
456+
# user realm discovery, interests MSAL only when the response body contains account_type == "Federated"
457+
mock_response(json_payload={}),
458+
# token request
459+
mock_response(
460+
json_payload={
461+
"access_token": expected_token,
462+
"expires_in": 42,
463+
"token_type": "Bearer",
464+
"ext_expires_in": 42,
465+
}
466+
),
467+
],
468+
)
469+
470+
monkeypatch.setenv(EnvironmentVariables.AZURE_CLIENT_ID, client_id)
471+
monkeypatch.setenv(EnvironmentVariables.AZURE_USERNAME, username)
472+
monkeypatch.setenv(EnvironmentVariables.AZURE_PASSWORD, password)
473+
474+
token = EnvironmentCredential(transport=create_transport()).get_token("scope")
475+
476+
# not validating expires_on because doing so requires monkeypatching time, and this is tested elsewhere
477+
assert token.token == expected_token
478+
479+
# now with a tenant id
480+
monkeypatch.setenv(EnvironmentVariables.AZURE_TENANT_ID, "tenant_id")
481+
482+
token = EnvironmentCredential(transport=create_transport()).get_token("scope")
483+
484+
assert token.token == expected_token

0 commit comments

Comments
 (0)