address review feedback

sdk/identity/azure-identity/CHANGELOG.md

@@ -1,6 +1,6 @@
 # Release History
 
-## 1.7.0b5 (Unreleased)
+## 1.7.0 (2021-10-12)
 
 ### Breaking Changes
 > These changes do not impact the API of stable versions such as 1.6.0.
@@ -10,7 +10,9 @@
   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`
+- `regional_authority` argument is removed for `CertificateCredential` and `ClientSecretCredential`.
+- `AzureApplicationCredential` is removed.
+- `client_credential` in the ctor of `OnBehalfOfCredential` is deprecated. Please use `client_secret` or `client_certificate` instead.
 
 ## 1.7.0b4 (2021-09-09)
 

sdk/identity/azure-identity/azure/identity/__init__.py

@@ -9,7 +9,6 @@
 from ._constants import AzureAuthorityHosts, KnownAuthorities
 from ._credentials import (
     AuthorizationCodeCredential,
-    AzureApplicationCredential,
     AzureCliCredential,
     AzurePowerShellCredential,
     CertificateCredential,
@@ -32,7 +31,6 @@
     "AuthenticationRecord",
     "AuthenticationRequiredError",
     "AuthorizationCodeCredential",
-    "AzureApplicationCredential",
     "AzureAuthorityHosts",
     "AzureCliCredential",
     "AzurePowerShellCredential",

sdk/identity/azure-identity/azure/identity/_credentials/__init__.py

@@ -2,7 +2,6 @@
 # Copyright (c) Microsoft Corporation.
 # Licensed under the MIT License.
 # ------------------------------------
-from .application import AzureApplicationCredential
 from .authorization_code import AuthorizationCodeCredential
 from .azure_powershell import AzurePowerShellCredential
 from .browser import InteractiveBrowserCredential
@@ -22,7 +21,6 @@
 
 __all__ = [
     "AuthorizationCodeCredential",
-    "AzureApplicationCredential",
     "AzureCliCredential",
     "AzurePowerShellCredential",
     "CertificateCredential",

sdk/identity/azure-identity/azure/identity/_credentials/on_behalf_of.py

@@ -33,42 +33,51 @@ class OnBehalfOfCredential(MsalCredential, GetTokenMixin):
 
     :param str tenant_id: ID of the service principal's tenant. Also called its "directory" ID.
     :param str client_id: the service principal's client ID
-    :param client_credential: a credential to authenticate the service principal, either one of its client secrets (a
-        string) or the bytes of a certificate in PEM or PKCS12 format including the private key
-    :type client_credential: str or bytes
-    :param str user_assertion: the access token the credential will use as the user assertion when requesting
-        on-behalf-of tokens
+    :keyword str client_secret: Optional. client secrets to authenticate the service principal.
+        Either client_secret or client_certificate must be provided.
+    :keyword bytes client_certificate: Optional. the bytes of a certificate in PEM or PKCS12 format including
+        the private key to authenticate the service principal. Either client_secret or client_certificate must
+        be provided.
+    :keyword str user_assertion: Required. the access token the credential will use as the user assertion when
+        requesting on-behalf-of tokens
 
     :keyword str authority: Authority of an Azure Active Directory endpoint, for example "login.microsoftonline.com",
         the authority for Azure Public Cloud (which is the default). :class:`~azure.identity.AzureAuthorityHosts`
         defines authorities for other clouds.
-    :keyword password: a certificate password. Used only when **client_credential** is certificate bytes. If this value
+    :keyword password: a certificate password. Used only when **client_certificate** is provided. If this value
         is a unicode string, it will be encoded as UTF-8. If the certificate requires a different encoding, pass
         appropriately encoded bytes instead.
     :paramtype password: str or bytes
     """
 
-    def __init__(self, tenant_id, client_id, client_credential, user_assertion, **kwargs):
-        # type: (str, str, Union[bytes, str], str, **Any) -> None
-        credential = cast("Union[Dict, str]", client_credential)
-        if isinstance(client_credential, six.binary_type):
+    def __init__(self, tenant_id, client_id, **kwargs):
+        # type: (str, str, **Any) -> None
+        self._assertion = kwargs.pop("user_assertion", None)
+        if not self._assertion:
+            raise ValueError("'user_assertion' is required.")
+        client_certificate = kwargs.pop("client_certificate", None)
+        client_secret = kwargs.pop("client_secret", None)
+
+        if client_certificate:
             try:
                 credential = get_client_credential(
-                    certificate_path=None, password=kwargs.pop("password", None), certificate_data=client_credential
+                    certificate_path=None, password=kwargs.pop("password", None), certificate_data=client_certificate
                 )
             except ValueError as ex:
                 # client_credential isn't a valid cert. On 2.7 str == bytes and we ignore this exception because we
                 # can't tell whether the caller intended to provide a cert. On Python 3 we can say the caller provided
                 # either an invalid cert, or a client secret as bytes; both are errors.
                 if six.PY3:
                     message = (
-                        '"client_credential" should be either a client secret (a string)'
-                        + " or the bytes of a certificate in PEM or PKCS12 format"
+                        '"client_certificate" should be the bytes of a certificate in PEM or PKCS12 format'
                     )
                     six.raise_from(ValueError(message), ex)
+        elif client_secret:
+            credential = client_secret
+        else:
+            raise ValueError("Either `client_certificate` or `client_secret` must be provided")
 
         super(OnBehalfOfCredential, self).__init__(client_id, credential, tenant_id=tenant_id, **kwargs)
-        self._assertion = user_assertion
         self._auth_record = None  # type: Optional[AuthenticationRecord]
 
     @wrap_exceptions

sdk/identity/azure-identity/azure/identity/_version.py

@@ -2,4 +2,4 @@
 # Copyright (c) Microsoft Corporation.
 # Licensed under the MIT License.
 # ------------------------------------
-VERSION = "1.7.0b5"
+VERSION = "1.7.0"

sdk/identity/azure-identity/azure/identity/aio/__init__.py

@@ -6,7 +6,6 @@
 
 from ._credentials import (
     AuthorizationCodeCredential,
-    AzureApplicationCredential,
     AzureCliCredential,
     AzurePowerShellCredential,
     CertificateCredential,
@@ -23,7 +22,6 @@
 
 __all__ = [
     "AuthorizationCodeCredential",
-    "AzureApplicationCredential",
     "AzureCliCredential",
     "AzurePowerShellCredential",
     "CertificateCredential",

sdk/identity/azure-identity/azure/identity/aio/_credentials/__init__.py

@@ -2,7 +2,6 @@
 # Copyright (c) Microsoft Corporation.
 # Licensed under the MIT License.
 # ------------------------------------
-from .application import AzureApplicationCredential
 from .authorization_code import AuthorizationCodeCredential
 from .azure_powershell import AzurePowerShellCredential
 from .chained import ChainedTokenCredential
@@ -19,7 +18,6 @@
 
 __all__ = [
     "AuthorizationCodeCredential",
-    "AzureApplicationCredential",
     "AzureCliCredential",
     "AzurePowerShellCredential",
     "CertificateCredential",

sdk/identity/azure-identity/azure/identity/aio/_credentials/on_behalf_of.py

@@ -30,16 +30,18 @@ class OnBehalfOfCredential(AsyncContextManager, GetTokenMixin):
 
     :param str tenant_id: ID of the service principal's tenant. Also called its "directory" ID.
     :param str client_id: the service principal's client ID
-    :param client_credential: a credential to authenticate the service principal, either one of its client secrets (a
-        string) or the bytes of a certificate in PEM or PKCS12 format including the private key
-    :paramtype client_credential: str or bytes
-    :param str user_assertion: the access token the credential will use as the user assertion when requesting
-        on-behalf-of tokens
+    :keyword str client_secret: Optional. client secrets to authenticate the service principal.
+        Either client_secret or client_certificate must be provided.
+    :keyword bytes client_certificate: Optional. the bytes of a certificate in PEM or PKCS12 format including
+        the private key to authenticate the service principal. Either client_secret or client_certificate must
+        be provided.
+    :keyword str user_assertion: Required. the access token the credential will use as the user assertion when
+        requesting on-behalf-of tokens
 
     :keyword str authority: Authority of an Azure Active Directory endpoint, for example "login.microsoftonline.com",
         the authority for Azure Public Cloud (which is the default). :class:`~azure.identity.AzureAuthorityHosts`
         defines authorities for other clouds.
-    :keyword password: a certificate password. Used only when **client_credential** is certificate bytes. If this value
+    :keyword password: a certificate password. Used only when **client_certificate** is provided. If this value
         is a unicode string, it will be encoded as UTF-8. If the certificate requires a different encoding, pass
         appropriately encoded bytes instead.
     :paramtype password: str or bytes
@@ -49,31 +51,37 @@ def __init__(
         self,
         tenant_id: str,
         client_id: str,
-        client_credential: "Union[bytes, str]",
+        *,
+        client_certificate: bytes = None,
+        client_secret: str = None,
         user_assertion: str,
         **kwargs: "Any"
     ) -> None:
         super().__init__()
         validate_tenant_id(tenant_id)
 
-        if isinstance(client_credential, bytes):
+        self._assertion = user_assertion
+        if not self._assertion:
+            raise ValueError("'user_assertion' is required.")
+
+        if client_certificate:
             try:
-                cert = get_client_credential(None, kwargs.pop("password", None), client_credential)
+                cert = get_client_credential(None, kwargs.pop("password", None), client_certificate)
             except ValueError as ex:
                 message = (
-                    '"client_credential" should be either a client secret (a string)'
-                    + " or the bytes of a certificate in PEM or PKCS12 format"
+                    '"client_certificate" should be the bytes of a certificate in PEM or PKCS12 format'
                 )
                 raise ValueError(message) from ex
             self._client_credential = AadClientCertificate(
                 cert["private_key"], password=cert.get("passphrase")
             )  # type: Union[str, AadClientCertificate]
+        elif client_secret:
+            self._client_credential = client_secret
         else:
-            self._client_credential = client_credential
+            raise ValueError("Either `client_certificate` or `client_secret` must be provided")
 
         # note AadClient handles "authority" and any pipeline kwargs
         self._client = AadClient(tenant_id, client_id, **kwargs)
-        self._assertion = user_assertion
 
     async def __aenter__(self):
         await self._client.__aenter__()

sdk/identity/azure-identity/tests/test_application_credential.py

@@ -5,7 +5,8 @@
 import os
 
 from azure.core.credentials import AccessToken
-from azure.identity import AzureApplicationCredential, CredentialUnavailableError
+from azure.identity import CredentialUnavailableError
+from azure.identity._credentials.application import AzureApplicationCredential
 from azure.identity._constants import EnvironmentVariables
 import pytest
 from six.moves.urllib_parse import urlparse

sdk/identity/azure-identity/tests/test_application_credential_async.py

@@ -7,7 +7,7 @@
 
 from azure.core.credentials import AccessToken
 from azure.identity import CredentialUnavailableError
-from azure.identity.aio import AzureApplicationCredential
+from azure.identity.aio._credentials.application import AzureApplicationCredential
 from azure.identity._constants import EnvironmentVariables
 import pytest
 from six.moves.urllib_parse import urlparse

sdk/identity/azure-identity/tests/test_azure_application.py

@@ -7,7 +7,7 @@
 except ImportError:
     from mock import patch  # type: ignore
 
-from azure.identity import AzureApplicationCredential
+from azure.identity._credentials.application import AzureApplicationCredential
 from azure.identity._constants import EnvironmentVariables
 
 

sdk/identity/azure-identity/tests/test_azure_application_async.py

@@ -4,7 +4,7 @@
 # ------------------------------------
 from unittest.mock import patch
 
-from azure.identity.aio import AzureApplicationCredential
+from azure.identity.aio._credentials.application import AzureApplicationCredential
 from azure.identity._constants import EnvironmentVariables
 
 

sdk/identity/azure-identity/tests/test_context_manager.py

@@ -7,8 +7,8 @@
 except ImportError:
     from mock import MagicMock, patch  # type: ignore
 
+from azure.identity._credentials.application import AzureApplicationCredential
 from azure.identity import (
-    AzureApplicationCredential,
     AzureCliCredential,
     AzurePowerShellCredential,
     AuthorizationCodeCredential,
@@ -62,7 +62,7 @@ def get_credential(self, **kwargs):
     CredentialFixture(InteractiveBrowserCredential),
     CredentialFixture(
         OnBehalfOfCredential,
-        {kwarg: "..." for kwarg in ("tenant_id", "client_id", "client_credential", "user_assertion")},
+        {kwarg: "..." for kwarg in ("tenant_id", "client_id", "client_secret", "user_assertion")},
     ),
     CredentialFixture(UsernamePasswordCredential, {"client_id": "...", "username": "...", "password": "..."}),
     CredentialFixture(VisualStudioCodeCredential, ctor_patch_factory=lambda: patch(GET_USER_SETTINGS, lambda: {})),

sdk/identity/azure-identity/tests/test_obo.py

@@ -77,7 +77,7 @@ def test_obo(self):
             client_id, self.obo_settings["username"], self.obo_settings["password"], tenant_id=tenant_id
         )
         assertion = user_credential.get_token(self.obo_settings["scope"]).token
-        credential = OnBehalfOfCredential(tenant_id, client_id, self.obo_settings["client_secret"], assertion)
+        credential = OnBehalfOfCredential(tenant_id, client_id, client_secret=self.obo_settings["client_secret"], user_assertion=assertion)
         credential.get_token(self.obo_settings["scope"])
 
     def test_obo_cert(self):
@@ -88,7 +88,7 @@ def test_obo_cert(self):
             client_id, self.obo_settings["username"], self.obo_settings["password"], tenant_id=tenant_id
         )
         assertion = user_credential.get_token(self.obo_settings["scope"]).token
-        credential = OnBehalfOfCredential(tenant_id, client_id, self.obo_settings["cert_bytes"], assertion)
+        credential = OnBehalfOfCredential(tenant_id, client_id, client_certificate=self.obo_settings["cert_bytes"], user_assertion=assertion)
         credential.get_token(self.obo_settings["scope"])
 
 
@@ -111,7 +111,7 @@ def send(request, **_):
 
     transport = Mock(send=Mock(wraps=send))
     credential = OnBehalfOfCredential(
-        first_tenant, "client-id", "secret", "assertion", transport=transport
+        first_tenant, "client-id", client_secret="secret", user_assertion="assertion", transport=transport
     )
     token = credential.get_token("scope")
     assert token.token == first_token
@@ -140,7 +140,7 @@ def test_authority(authority):
         return_value=Mock(acquire_token_on_behalf_of=lambda *_, **__: {"access_token": "**", "expires_in": 42})
     )
 
-    credential = OnBehalfOfCredential(tenant_id, "client-id", "secret", "assertion", authority=authority)
+    credential = OnBehalfOfCredential(tenant_id, "client-id", client_secret="secret", user_assertion="assertion", authority=authority)
     with patch("msal.ConfidentialClientApplication", mock_ctor):
         # must call get_token because the credential constructs the MSAL application lazily
         credential.get_token("scope")
@@ -152,7 +152,7 @@ def test_authority(authority):
 
     # authority can be configured via environment variable
     with patch.dict("os.environ", {EnvironmentVariables.AZURE_AUTHORITY_HOST: authority}, clear=True):
-        credential = OnBehalfOfCredential(tenant_id, "client-id", "secret", "assertion")
+        credential = OnBehalfOfCredential(tenant_id, "client-id", client_secret="secret", user_assertion="assertion")
     with patch("msal.ConfidentialClientApplication", mock_ctor):
         credential.get_token("scope")
 
@@ -165,16 +165,16 @@ def test_tenant_id_validation():
     """The credential should raise ValueError when given an invalid tenant_id"""
     valid_ids = {"c878a2ab-8ef4-413b-83a0-199afb84d7fb", "contoso.onmicrosoft.com", "organizations", "common"}
     for tenant in valid_ids:
-        OnBehalfOfCredential(tenant, "client-id", "secret", "assertion")
+        OnBehalfOfCredential(tenant, "client-id", client_secret="secret", user_assertion="assertion")
     invalid_ids = {"my tenant", "my_tenant", "/", "\\", '"my-tenant"', "'my-tenant'"}
     for tenant in invalid_ids:
         with pytest.raises(ValueError):
-            OnBehalfOfCredential(tenant, "client-id", "secret", "assertion")
+            OnBehalfOfCredential(tenant, "client-id", client_secret="secret", user_assertion="assertion")
 
 
 def test_no_scopes():
     """The credential should raise ValueError when get_token is called with no scopes"""
-    credential = OnBehalfOfCredential("tenant-id", "client-id", "client-secret", "assertion")
+    credential = OnBehalfOfCredential("tenant-id", "client-id", client_secret="client-secret", user_assertion="assertion")
     with pytest.raises(ValueError):
         credential.get_token()
 
@@ -192,8 +192,8 @@ def send(request, **_):
     credential = OnBehalfOfCredential(
         "tenant-id",
         "client-id",
-        "client-secret",
-        "assertion",
+        client_secret="client-secret",
+        user_assertion="assertion",
         policies=[ContentDecodePolicy(), policy],
         transport=Mock(send=send),
     )