address review feedback (Azure#21109)

* address review feedback * update * update * update * crypto 35.0.0 is not compatible with our tests, pin in the dev_requirements * filter packages pinned in dev_reqs OUT of the results when installing during dependency testing * update * adjust check in install_depend_packages.py that was accidentally succeeding because of auto-trimming provided by py3. the check is actually doing the intended thing now. * update * Update sdk/identity/azure-identity/azure/identity/_credentials/on_behalf_of.py Co-authored-by: McCoy Patiño <[email protected]> * update * update * update Co-authored-by: scbedd <[email protected]> Co-authored-by: McCoy Patiño <[email protected]>
AzureSDKAutomation · Oct 7, 2021 · bd9a0ce · bd9a0ce
1 parent cb90cde
commit bd9a0ce
Show file tree

Hide file tree

Showing 17 changed files with 112 additions and 77 deletions.
diff --git a/eng/tox/install_depend_packages.py b/eng/tox/install_depend_packages.py
@@ -140,7 +140,7 @@ def filter_dev_requirements(setup_py_path, released_packages, temp_dir):
     filtered_req = [
         req
         for req in requirements
-        if os.path.basename(req.replace('\n', '')) not in req_to_exclude
+        if os.path.basename(req.replace('\n', '')) not in req_to_exclude and not any([req.startswith(i) for i in req_to_exclude])
     ]
 
     logging.info("Filtered dev requirements: %s", filtered_req)

diff --git a/sdk/identity/azure-identity/CHANGELOG.md b/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,10 @@
   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 removed. Please use `client_secret` or `client_certificate` instead.
+- Make `user_assertion` in the ctor of `OnBehalfOfCredential` a keyword only argument.
 
 ## 1.7.0b4 (2021-09-09)
 

diff --git a/sdk/identity/azure-identity/azure/identity/__init__.py b/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",

diff --git a/sdk/identity/azure-identity/azure/identity/_credentials/__init__.py b/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",

diff --git a/sdk/identity/azure-identity/azure/identity/_credentials/on_behalf_of.py b/sdk/identity/azure-identity/azure/identity/_credentials/on_behalf_of.py
@@ -3,7 +3,7 @@
 # Licensed under the MIT License.
 # ------------------------------------
 import time
-from typing import cast, TYPE_CHECKING
+from typing import TYPE_CHECKING
 
 import six
 
@@ -33,42 +33,50 @@ 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. A client secret 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 TypeError('"user_assertion" is required.')
+        client_certificate = kwargs.pop("client_certificate", None)
+        client_secret = kwargs.pop("client_secret", None)
+
+        if client_certificate:
+            if client_secret:
+                raise ValueError('Specifying both "client_certificate" and "client_secret" is not valid.')
             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"
-                    )
-                    six.raise_from(ValueError(message), ex)
+                # client_certificate isn't a valid cert.
+                message = (
+                    '"client_certificate" is not a valid certificate in PEM or PKCS12 format'
+                )
+                six.raise_from(ValueError(message), ex)
+        elif client_secret:
+            credential = client_secret
+        else:
+            raise TypeError('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

diff --git a/sdk/identity/azure-identity/azure/identity/_version.py b/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"
diff --git a/sdk/identity/azure-identity/azure/identity/aio/__init__.py b/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",

diff --git a/sdk/identity/azure-identity/azure/identity/aio/_credentials/__init__.py b/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",

diff --git a/sdk/identity/azure-identity/azure/identity/aio/_credentials/on_behalf_of.py b/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. A client secret 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 client_certificate:
+            if client_secret:
+                raise ValueError('Specifying both "client_certificate" and "client_secret" is not valid.')
             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" is not a valid 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 TypeError('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__()

diff --git a/sdk/identity/azure-identity/dev_requirements.txt b/sdk/identity/azure-identity/dev_requirements.txt
@@ -2,5 +2,6 @@
 aiohttp>=3.0; python_version >= '3.5'
 mock;python_version<"3.3"
 typing_extensions>=3.7.2
+cryptography<=3.4.8
 -e ../../../tools/azure-sdk-tools
 -e ../../../tools/azure-devtools
diff --git a/sdk/identity/azure-identity/tests/test_application_credential.py b/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

diff --git a/sdk/identity/azure-identity/tests/test_application_credential_async.py b/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

diff --git a/sdk/identity/azure-identity/tests/test_azure_application.py b/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
 
 

diff --git a/sdk/identity/azure-identity/tests/test_azure_application_async.py b/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
 
 

diff --git a/sdk/identity/azure-identity/tests/test_context_manager.py b/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: {})),