From c45847b742ea273194d7f2a8ad35a30cb6be275a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McCoy=20Pati=C3=B1o?= Date: Mon, 7 Jun 2021 17:35:50 -0700 Subject: [PATCH 1/6] Make issuer_name optional --- .../azure/keyvault/certificates/_client.py | 20 ++++++++--- .../azure/keyvault/certificates/_models.py | 33 +++++++++++-------- .../keyvault/certificates/aio/_client.py | 20 ++++++++--- .../tests/test_certificates_client.py | 18 +++++++++- .../tests/test_certificates_client_async.py | 20 ++++++++++- .../tests/test_certificates_models.py | 11 ------- 6 files changed, 85 insertions(+), 37 deletions(-) delete mode 100644 sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_models.py diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py index dbb832499e85..c8a0bc38cd66 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py @@ -68,17 +68,20 @@ def begin_create_certificate(self, certificate_name, policy, **kwargs): an :class:`~azure.core.exceptions.HttpResponseError` :param str certificate_name: The name of the certificate. - :param policy: The management policy for the certificate. + :param policy: The management policy for the certificate. Either subject or one of the subject alternative + name properties are required. :type policy: - ~azure.keyvault.certificates.CertificatePolicy + ~azure.keyvault.certificates.CertificatePolicy :keyword bool enabled: Whether the certificate is enabled for use. :keyword tags: Application specific metadata in the form of key-value pairs. :paramtype tags: dict[str, str] :returns: An LROPoller for the create certificate operation. Waiting on the poller - gives you the certificate if creation is successful, the CertificateOperation if not. + gives you the certificate if creation is successful, the CertificateOperation if not. :rtype: ~azure.core.polling.LROPoller[~azure.keyvault.certificates.KeyVaultCertificate or - ~azure.keyvault.certificates.CertificateOperation] - :raises: :class:`~azure.core.exceptions.HttpResponseError` + ~azure.keyvault.certificates.CertificateOperation] + :raises: + :class:`ValueError` if the certificate policy is invalid, + :class:`~azure.core.exceptions.HttpResponseError` for other errors. Keyword arguments - *enabled (bool)* - Determines whether the object is enabled. @@ -92,6 +95,13 @@ def begin_create_certificate(self, certificate_name, policy, **kwargs): :caption: Create a certificate :dedent: 8 """ + if not ( + policy.issuer_name and + (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject) + ): + raise ValueError("You need to set an issuer and either subject or one of the subject alternative names " + + "parameters in the certificate policy") + polling_interval = kwargs.pop("_polling_interval", None) if polling_interval is None: polling_interval = 5 diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py index a8180cb3a6e3..c1838aa6b0fc 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py @@ -618,17 +618,28 @@ def request_id(self): class CertificatePolicy(object): """Management policy for a certificate. - :param str issuer_name: Name of the referenced issuer object or reserved names; for example, - 'Self' or 'Unknown" + :param Optional[str] issuer_name: Optional, but required for + :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and + :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. Name of the referenced issuer + object or reserved names; for example, :attr:`~azure.keyvault.certificates.WellKnownIssuerNames.self` or + :attr:`~azure.keyvault.certificates.WellKnownIssuerNames.unknown` :keyword str subject: The subject name of the certificate. Should be a valid X509 - distinguished name. Either subject or one of the subject alternative name parameters - are required. + distinguished name. Either subject or one of the subject alternative name parameters are required for + :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and + :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. This will be parsed from the + certificate provided to :func:`~azure.keyvault.certificates.CertificateClient.import_certificate`. :keyword Iterable[str] san_emails: Subject alternative emails of the X509 object. Either - subject or one of the subject alternative name parameters are required. + subject or one of the subject alternative name parameters are required for + :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and + :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. :keyword Iterable[str] san_dns_names: Subject alternative DNS names of the X509 object. Either - subject or one of the subject alternative name parameters are required. + subject or one of the subject alternative name parameters are required for + :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and + :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. :keyword Iterable[str] san_user_principal_names: Subject alternative user principal names of the X509 object. - Either subject or one of the subject alternative name parameters are required. + Either subject or one of the subject alternative name parameters are required for + :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and + :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. :keyword bool exportable: Indicates if the private key can be exported. For valid values, see KeyType. :keyword key_type: The type of key pair to be used for the certificate. @@ -659,7 +670,7 @@ class CertificatePolicy(object): # pylint:disable=too-many-instance-attributes def __init__( self, - issuer_name, # type: str + issuer_name=None, # type: Optional[str] **kwargs # type: Any ): # type: (...) -> None @@ -682,12 +693,6 @@ def __init__( self._san_dns_names = kwargs.pop("san_dns_names", None) or None self._san_user_principal_names = kwargs.pop("san_user_principal_names", None) or None - if not ( - self._san_emails or self._san_user_principal_names or self._san_dns_names or self._subject - ): - raise ValueError("You need to set either subject or one of the subject alternative names " + - "parameters") - @classmethod def get_default(cls): return cls(issuer_name=WellKnownIssuerNames.self, subject="CN=DefaultPolicy") diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py index d2553ea5ac19..389425c03106 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py @@ -62,17 +62,20 @@ async def create_certificate( an :class:`~azure.core.exceptions.HttpResponseError` :param str certificate_name: The name of the certificate. - :param policy: The management policy for the certificate. + :param policy: The management policy for the certificate. Either subject or one of the subject alternative + name properties are required. :type policy: - ~azure.keyvault.certificates.CertificatePolicy + ~azure.keyvault.certificates.CertificatePolicy :keyword bool enabled: Whether the certificate is enabled for use. :keyword tags: Application specific metadata in the form of key-value pairs. :paramtype tags: dict[str, str] :returns: A coroutine for the creation of the certificate. Awaiting the coroutine - returns the created KeyVaultCertificate if creation is successful, the CertificateOperation if not. + returns the created KeyVaultCertificate if creation is successful, the CertificateOperation if not. :rtype: ~azure.keyvault.certificates.KeyVaultCertificate or - ~azure.keyvault.certificates.CertificateOperation - :raises: :class:`~azure.core.exceptions.HttpResponseError` + ~azure.keyvault.certificates.CertificateOperation + :raises: + :class:`ValueError` if the certificate policy is invalid, + :class:`~azure.core.exceptions.HttpResponseError` for other errors. Example: .. literalinclude:: ../tests/test_examples_certificates_async.py @@ -82,6 +85,13 @@ async def create_certificate( :caption: Create a certificate :dedent: 8 """ + if not ( + policy.issuer_name and + (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject) + ): + raise ValueError("You need to set an issuer and either subject or one of the subject alternative names " + + "parameters in the certificate policy") + polling_interval = kwargs.pop("_polling_interval", None) if polling_interval is None: polling_interval = 5 diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py index 9c8181f8e49b..8500cd09d3d9 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py @@ -24,7 +24,8 @@ CertificateContentType, LifetimeAction, CertificateIssuer, - IssuerProperties + IssuerProperties, + WellKnownIssuerNames ) import pytest @@ -680,6 +681,21 @@ def test_list_deleted_certificates(self, client, **kwargs): assert "The 'include_pending' parameter to `list_deleted_certificates` is only available for API versions v7.0 and up" in str(excinfo.value) +def test_policy_expected_errors_for_create_cert(): + """An issuer name, and either a subject or subject alternative name property, are required for creation""" + client = CertificateClient("...", object()) + + with pytest.raises(ValueError) as ex: + policy = CertificatePolicy() + client.begin_create_certificate("...", policy=policy) + assert "issuer" in str(ex.value) + assert "subject" in str(ex.value) + + with pytest.raises(ValueError) as ex: + policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self) + client.begin_create_certificate("...", policy=policy) + assert "subject" in str(ex.value) + def test_service_headers_allowed_in_logs(): service_headers = {"x-ms-keyvault-network-info", "x-ms-keyvault-region", "x-ms-keyvault-service-version"} client = CertificateClient("...", object()) diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py index 2c55c69c36c6..b46fdbaf6775 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py @@ -23,7 +23,8 @@ CertificateContentType, LifetimeAction, CertificateIssuer, - IssuerProperties + IssuerProperties, + WellKnownIssuerNames ) from azure.keyvault.certificates.aio import CertificateClient import pytest @@ -692,6 +693,23 @@ async def test_list_deleted_certificates_2016_10_01(self, client, **kwargs): assert "The 'include_pending' parameter to `list_deleted_certificates` is only available for API versions v7.0 and up" in str(excinfo.value) +@pytest.mark.asyncio +async def test_policy_expected_errors_for_create_cert(): + """An issuer name, and either a subject or subject alternative name property, are required for creation""" + client = CertificateClient("...", object()) + + with pytest.raises(ValueError) as ex: + policy = CertificatePolicy() + await client.create_certificate("...", policy=policy) + assert "issuer" in str(ex.value) + assert "subject" in str(ex.value) + + with pytest.raises(ValueError) as ex: + policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self) + await client.create_certificate("...", policy=policy) + assert "subject" in str(ex.value) + + def test_service_headers_allowed_in_logs(): service_headers = {"x-ms-keyvault-network-info", "x-ms-keyvault-region", "x-ms-keyvault-service-version"} client = CertificateClient("...", object()) diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_models.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_models.py deleted file mode 100644 index 0f8f49a7ff8c..000000000000 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_models.py +++ /dev/null @@ -1,11 +0,0 @@ -# ------------------------------------ -# Copyright (c) Microsoft Corporation. -# Licensed under the MIT License. -# ------------------------------------ -from azure.keyvault.certificates import CertificatePolicy -from pytest import raises - -def test_policy_expected_errors(): - with raises(ValueError) as ex: - cert_policy = CertificatePolicy("issuer-name") - assert "subject" in str(ex.value), "Error should be thrown since we haven't set a subject or sans" \ No newline at end of file From a973e4b46305aaad1beba1df79f746bd001e0541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McCoy=20Pati=C3=B1o?= Date: Mon, 7 Jun 2021 17:41:01 -0700 Subject: [PATCH 2/6] Update changelog --- sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md b/sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md index 5677c26918c0..e9c69b8ea2f9 100644 --- a/sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md +++ b/sdk/keyvault/azure-keyvault-certificates/CHANGELOG.md @@ -5,6 +5,7 @@ This is the last version to support Python 3.5. The next version will require Py ### Changed - Key Vault API version 7.2 is now the default - Updated minimum `msrest` version to 0.6.21 +- The `issuer_name` parameter for `CertificatePolicy` is now optional ### Added - Added class `KeyVaultCertificateIdentifier` that parses out a full ID returned by Key Vault, From c525149cbb779211135e6dddf8e837af36f1e870 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McCoy=20Pati=C3=B1o?= Date: Mon, 7 Jun 2021 17:47:22 -0700 Subject: [PATCH 3/6] Improve tests --- .../tests/test_certificates_client.py | 5 +++++ .../tests/test_certificates_client_async.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py index 8500cd09d3d9..53767bbd245c 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py @@ -696,6 +696,11 @@ def test_policy_expected_errors_for_create_cert(): client.begin_create_certificate("...", policy=policy) assert "subject" in str(ex.value) + with pytest.raises(ValueError) as ex: + policy = CertificatePolicy(subject="...") + client.begin_create_certificate("...", policy=policy) + assert "issuer" in str(ex.value) + def test_service_headers_allowed_in_logs(): service_headers = {"x-ms-keyvault-network-info", "x-ms-keyvault-region", "x-ms-keyvault-service-version"} client = CertificateClient("...", object()) diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py index b46fdbaf6775..cb52312e0199 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py @@ -709,6 +709,11 @@ async def test_policy_expected_errors_for_create_cert(): await client.create_certificate("...", policy=policy) assert "subject" in str(ex.value) + with pytest.raises(ValueError) as ex: + policy = CertificatePolicy(subject="...") + await client.create_certificate("...", policy=policy) + assert "issuer" in str(ex.value) + def test_service_headers_allowed_in_logs(): service_headers = {"x-ms-keyvault-network-info", "x-ms-keyvault-region", "x-ms-keyvault-service-version"} From e737579307511ec01ff3ff6517e6ce8481a8fe59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McCoy=20Pati=C3=B1o?= Date: Tue, 8 Jun 2021 12:12:22 -0700 Subject: [PATCH 4/6] Optional issuer; run black --- .../azure/keyvault/certificates/_client.py | 48 +++++++------------ .../keyvault/certificates/aio/_client.py | 40 +++++----------- 2 files changed, 27 insertions(+), 61 deletions(-) diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py index c8a0bc38cd66..6aa19d7a05de 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py @@ -35,6 +35,10 @@ from azure.core.paging import ItemPaged +SAN_SUBJECT_ERROR_MESSAGE = "You need to set either subject or one of the subject alternative names parameters in the " ++"certificate policy" + + class CertificateClient(KeyVaultClientBase): """A high-level interface for managing a vault's certificates. @@ -95,19 +99,14 @@ def begin_create_certificate(self, certificate_name, policy, **kwargs): :caption: Create a certificate :dedent: 8 """ - if not ( - policy.issuer_name and - (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject) - ): - raise ValueError("You need to set an issuer and either subject or one of the subject alternative names " + - "parameters in the certificate policy") + if not (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject): + raise ValueError(SAN_SUBJECT_ERROR_MESSAGE) polling_interval = kwargs.pop("_polling_interval", None) if polling_interval is None: polling_interval = 5 enabled = kwargs.pop("enabled", None) - if enabled is not None: attributes = self._models.CertificateAttributes(enabled=enabled) else: @@ -116,7 +115,7 @@ def begin_create_certificate(self, certificate_name, policy, **kwargs): parameters = self._models.CertificateCreateParameters( certificate_policy=policy._to_certificate_policy_bundle(), certificate_attributes=attributes, - tags=kwargs.pop("tags", None) + tags=kwargs.pop("tags", None), ) cert_bundle = self._client.create_certificate( @@ -342,7 +341,6 @@ def begin_recover_deleted_certificate(self, certificate_name, **kwargs): return KeyVaultOperationPoller(polling_method) - @distributed_trace def import_certificate(self, certificate_name, certificate_bytes, **kwargs): # type: (str, bytes, **Any) -> KeyVaultCertificate @@ -469,8 +467,7 @@ def update_certificate_properties(self, certificate_name, version=None, **kwargs attributes = None parameters = self._models.CertificateUpdateParameters( - certificate_attributes=attributes, - tags=kwargs.pop("tags", None) + certificate_attributes=attributes, tags=kwargs.pop("tags", None) ) bundle = self._client.update_certificate( @@ -538,7 +535,8 @@ def restore_certificate_backup(self, backup, **kwargs): bundle = self._client.restore_certificate( vault_base_url=self.vault_url, parameters=self._models.CertificateRestoreParameters(certificate_bundle_backup=backup), - error_map=_error_map, **kwargs + error_map=_error_map, + **kwargs ) return KeyVaultCertificate._from_certificate_bundle(certificate_bundle=bundle) @@ -805,9 +803,7 @@ def merge_certificate(self, certificate_name, x509_certificates, **kwargs): attributes = None parameters = self._models.CertificateMergeParameters( - x509_certificates=x509_certificates, - certificate_attributes=attributes, - tags=kwargs.pop("tags", None) + x509_certificates=x509_certificates, certificate_attributes=attributes, tags=kwargs.pop("tags", None) ) bundle = self._client.merge_certificate( @@ -894,9 +890,7 @@ def create_issuer(self, issuer_name, provider, **kwargs): else: admin_details = None if organization_id or admin_details: - organization_details = self._models.OrganizationDetails( - id=organization_id, admin_details=admin_details - ) + organization_details = self._models.OrganizationDetails(id=organization_id, admin_details=admin_details) else: organization_details = None if enabled is not None: @@ -912,11 +906,7 @@ def create_issuer(self, issuer_name, provider, **kwargs): ) issuer_bundle = self._client.set_certificate_issuer( - vault_base_url=self.vault_url, - issuer_name=issuer_name, - parameter=parameters, - error_map=_error_map, - **kwargs + vault_base_url=self.vault_url, issuer_name=issuer_name, parameter=parameters, error_map=_error_map, **kwargs ) return CertificateIssuer._from_issuer_bundle(issuer_bundle=issuer_bundle) @@ -961,9 +951,7 @@ def update_issuer(self, issuer_name, **kwargs): else: admin_details = None if organization_id or admin_details: - organization_details = self._models.OrganizationDetails( - id=organization_id, admin_details=admin_details - ) + organization_details = self._models.OrganizationDetails(id=organization_id, admin_details=admin_details) else: organization_details = None if enabled is not None: @@ -975,15 +963,11 @@ def update_issuer(self, issuer_name, **kwargs): provider=kwargs.pop("provider", None), credentials=issuer_credentials, organization_details=organization_details, - attributes=issuer_attributes + attributes=issuer_attributes, ) issuer_bundle = self._client.update_certificate_issuer( - vault_base_url=self.vault_url, - issuer_name=issuer_name, - parameter=parameters, - error_map=_error_map, - **kwargs + vault_base_url=self.vault_url, issuer_name=issuer_name, parameter=parameters, error_map=_error_map, **kwargs ) return CertificateIssuer._from_issuer_bundle(issuer_bundle=issuer_bundle) diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py index 389425c03106..383738442786 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py @@ -23,6 +23,7 @@ IssuerProperties, ) from ._polling_async import CreateCertificatePollerAsync +from .._client import SAN_SUBJECT_ERROR_MESSAGE from .._shared import AsyncKeyVaultClientBase from .._shared._polling_async import AsyncDeleteRecoverPollingMethod from .._shared.exceptions import error_map as _error_map @@ -85,12 +86,8 @@ async def create_certificate( :caption: Create a certificate :dedent: 8 """ - if not ( - policy.issuer_name and - (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject) - ): - raise ValueError("You need to set an issuer and either subject or one of the subject alternative names " + - "parameters in the certificate policy") + if not (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject): + raise ValueError(SAN_SUBJECT_ERROR_MESSAGE) polling_interval = kwargs.pop("_polling_interval", None) if polling_interval is None: @@ -105,7 +102,7 @@ async def create_certificate( parameters = self._models.CertificateCreateParameters( certificate_policy=policy._to_certificate_policy_bundle(), certificate_attributes=attributes, - tags=kwargs.pop("tags", None) + tags=kwargs.pop("tags", None), ) cert_bundle = await self._client.create_certificate( @@ -445,8 +442,7 @@ async def update_certificate_properties( attributes = None parameters = self._models.CertificateUpdateParameters( - certificate_attributes=attributes, - tags=kwargs.pop("tags", None) + certificate_attributes=attributes, tags=kwargs.pop("tags", None) ) bundle = await self._client.update_certificate( @@ -784,9 +780,7 @@ async def merge_certificate( attributes = None parameters = self._models.CertificateMergeParameters( - x509_certificates=x509_certificates, - certificate_attributes=attributes, - tags=kwargs.pop("tags", None) + x509_certificates=x509_certificates, certificate_attributes=attributes, tags=kwargs.pop("tags", None) ) bundle = await self._client.merge_certificate( @@ -871,9 +865,7 @@ async def create_issuer(self, issuer_name: str, provider: str, **kwargs: "Any") else: admin_details = None if organization_id or admin_details: - organization_details = self._models.OrganizationDetails( - id=organization_id, admin_details=admin_details - ) + organization_details = self._models.OrganizationDetails(id=organization_id, admin_details=admin_details) else: organization_details = None if enabled is not None: @@ -889,11 +881,7 @@ async def create_issuer(self, issuer_name: str, provider: str, **kwargs: "Any") ) issuer_bundle = await self._client.set_certificate_issuer( - vault_base_url=self.vault_url, - issuer_name=issuer_name, - parameter=parameters, - error_map=_error_map, - **kwargs + vault_base_url=self.vault_url, issuer_name=issuer_name, parameter=parameters, error_map=_error_map, **kwargs ) return CertificateIssuer._from_issuer_bundle(issuer_bundle=issuer_bundle) @@ -938,9 +926,7 @@ async def update_issuer(self, issuer_name: str, **kwargs: "Any") -> CertificateI else: admin_details = None if organization_id or admin_details: - organization_details = self._models.OrganizationDetails( - id=organization_id, admin_details=admin_details - ) + organization_details = self._models.OrganizationDetails(id=organization_id, admin_details=admin_details) else: organization_details = None if enabled is not None: @@ -952,15 +938,11 @@ async def update_issuer(self, issuer_name: str, **kwargs: "Any") -> CertificateI provider=kwargs.pop("provider", None), credentials=issuer_credentials, organization_details=organization_details, - attributes=issuer_attributes + attributes=issuer_attributes, ) issuer_bundle = await self._client.update_certificate_issuer( - vault_base_url=self.vault_url, - issuer_name=issuer_name, - parameter=parameters, - error_map=_error_map, - **kwargs + vault_base_url=self.vault_url, issuer_name=issuer_name, parameter=parameters, error_map=_error_map, **kwargs ) return CertificateIssuer._from_issuer_bundle(issuer_bundle=issuer_bundle) From b2242083c93f2e032dff7c69907fda8e74ae1bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McCoy=20Pati=C3=B1o?= Date: Tue, 8 Jun 2021 12:36:46 -0700 Subject: [PATCH 5/6] Constant message; address feedback --- .../azure/keyvault/certificates/_client.py | 5 ++-- .../azure/keyvault/certificates/_models.py | 25 ++++++------------- .../keyvault/certificates/aio/_client.py | 4 +-- .../tests/test_certificates_client.py | 10 +++----- .../tests/test_certificates_client_async.py | 11 +++----- 5 files changed, 18 insertions(+), 37 deletions(-) diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py index 6aa19d7a05de..0ba57e214bbb 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_client.py @@ -35,8 +35,7 @@ from azure.core.paging import ItemPaged -SAN_SUBJECT_ERROR_MESSAGE = "You need to set either subject or one of the subject alternative names parameters in the " -+"certificate policy" +NO_SAN_OR_SUBJECT = "You need to set either subject or one of the subject alternative names parameters in the policy" class CertificateClient(KeyVaultClientBase): @@ -100,7 +99,7 @@ def begin_create_certificate(self, certificate_name, policy, **kwargs): :dedent: 8 """ if not (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject): - raise ValueError(SAN_SUBJECT_ERROR_MESSAGE) + raise ValueError(NO_SAN_OR_SUBJECT) polling_interval = kwargs.pop("_polling_interval", None) if polling_interval is None: diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py index c1838aa6b0fc..3747fe89be14 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/_models.py @@ -618,28 +618,19 @@ def request_id(self): class CertificatePolicy(object): """Management policy for a certificate. - :param Optional[str] issuer_name: Optional, but required for - :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and - :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. Name of the referenced issuer - object or reserved names; for example, :attr:`~azure.keyvault.certificates.WellKnownIssuerNames.self` or + :param Optional[str] issuer_name: Optional. Name of the referenced issuer object or reserved names; for example, + :attr:`~azure.keyvault.certificates.WellKnownIssuerNames.self` or :attr:`~azure.keyvault.certificates.WellKnownIssuerNames.unknown` :keyword str subject: The subject name of the certificate. Should be a valid X509 distinguished name. Either subject or one of the subject alternative name parameters are required for - :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and - :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. This will be parsed from the - certificate provided to :func:`~azure.keyvault.certificates.CertificateClient.import_certificate`. + creating a certificate. This will be ignored when importing a certificate; the subject will be parsed from + the imported certificate. :keyword Iterable[str] san_emails: Subject alternative emails of the X509 object. Either - subject or one of the subject alternative name parameters are required for - :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and - :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. + subject or one of the subject alternative name parameters are required for creating a certificate. :keyword Iterable[str] san_dns_names: Subject alternative DNS names of the X509 object. Either - subject or one of the subject alternative name parameters are required for - :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and - :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. + subject or one of the subject alternative name parameters are required for creating a certificate. :keyword Iterable[str] san_user_principal_names: Subject alternative user principal names of the X509 object. - Either subject or one of the subject alternative name parameters are required for - :func:`~azure.keyvault.certificates.CertificateClient.begin_create_certificate` and - :func:`~azure.keyvault.certificates.aio.CertificateClient.create_certificate`. + Either subject or one of the subject alternative name parameters are required for creating a certificate. :keyword bool exportable: Indicates if the private key can be exported. For valid values, see KeyType. :keyword key_type: The type of key pair to be used for the certificate. @@ -992,7 +983,7 @@ def lifetime_actions(self): @property def issuer_name(self): - # type: () -> str + # type: () -> Optional[str] """Name of the referenced issuer object or reserved names for the issuer of the certificate. diff --git a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py index 383738442786..ebe294092d39 100644 --- a/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/azure/keyvault/certificates/aio/_client.py @@ -23,7 +23,7 @@ IssuerProperties, ) from ._polling_async import CreateCertificatePollerAsync -from .._client import SAN_SUBJECT_ERROR_MESSAGE +from .._client import NO_SAN_OR_SUBJECT from .._shared import AsyncKeyVaultClientBase from .._shared._polling_async import AsyncDeleteRecoverPollingMethod from .._shared.exceptions import error_map as _error_map @@ -87,7 +87,7 @@ async def create_certificate( :dedent: 8 """ if not (policy.san_emails or policy.san_user_principal_names or policy.san_dns_names or policy.subject): - raise ValueError(SAN_SUBJECT_ERROR_MESSAGE) + raise ValueError(NO_SAN_OR_SUBJECT) polling_interval = kwargs.pop("_polling_interval", None) if polling_interval is None: diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py index 53767bbd245c..3f12e6696acf 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py @@ -27,6 +27,7 @@ IssuerProperties, WellKnownIssuerNames ) +from azure.keyvault.certificates._client import NO_SAN_OR_SUBJECT import pytest from _shared.test_case import KeyVaultTestCase @@ -688,18 +689,13 @@ def test_policy_expected_errors_for_create_cert(): with pytest.raises(ValueError) as ex: policy = CertificatePolicy() client.begin_create_certificate("...", policy=policy) - assert "issuer" in str(ex.value) - assert "subject" in str(ex.value) + assert str(ex.value) == NO_SAN_OR_SUBJECT with pytest.raises(ValueError) as ex: policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self) client.begin_create_certificate("...", policy=policy) - assert "subject" in str(ex.value) + assert str(ex.value) == NO_SAN_OR_SUBJECT - with pytest.raises(ValueError) as ex: - policy = CertificatePolicy(subject="...") - client.begin_create_certificate("...", policy=policy) - assert "issuer" in str(ex.value) def test_service_headers_allowed_in_logs(): service_headers = {"x-ms-keyvault-network-info", "x-ms-keyvault-region", "x-ms-keyvault-service-version"} diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py index cb52312e0199..15e68b8e1de4 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py @@ -27,6 +27,7 @@ WellKnownIssuerNames ) from azure.keyvault.certificates.aio import CertificateClient +from azure.keyvault.certificates._client import NO_SAN_OR_SUBJECT import pytest from _shared.test_case_async import KeyVaultTestCase @@ -701,18 +702,12 @@ async def test_policy_expected_errors_for_create_cert(): with pytest.raises(ValueError) as ex: policy = CertificatePolicy() await client.create_certificate("...", policy=policy) - assert "issuer" in str(ex.value) - assert "subject" in str(ex.value) + assert str(ex.value) == NO_SAN_OR_SUBJECT with pytest.raises(ValueError) as ex: policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self) await client.create_certificate("...", policy=policy) - assert "subject" in str(ex.value) - - with pytest.raises(ValueError) as ex: - policy = CertificatePolicy(subject="...") - await client.create_certificate("...", policy=policy) - assert "issuer" in str(ex.value) + assert str(ex.value) == NO_SAN_OR_SUBJECT def test_service_headers_allowed_in_logs(): From f813da25377a2611082424ee916aa1e4e0922bc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McCoy=20Pati=C3=B1o?= Date: Tue, 8 Jun 2021 14:12:08 -0700 Subject: [PATCH 6/6] Cleaner tests --- .../tests/test_certificates_client.py | 8 +++----- .../tests/test_certificates_client_async.py | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py index 3f12e6696acf..b2a92458d739 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py @@ -683,18 +683,16 @@ def test_list_deleted_certificates(self, client, **kwargs): def test_policy_expected_errors_for_create_cert(): - """An issuer name, and either a subject or subject alternative name property, are required for creation""" + """Either a subject or subject alternative name property are required for creating a certificate""" client = CertificateClient("...", object()) - with pytest.raises(ValueError) as ex: + with pytest.raises(ValueError, match=NO_SAN_OR_SUBJECT): policy = CertificatePolicy() client.begin_create_certificate("...", policy=policy) - assert str(ex.value) == NO_SAN_OR_SUBJECT - with pytest.raises(ValueError) as ex: + with pytest.raises(ValueError, match=NO_SAN_OR_SUBJECT): policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self) client.begin_create_certificate("...", policy=policy) - assert str(ex.value) == NO_SAN_OR_SUBJECT def test_service_headers_allowed_in_logs(): diff --git a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py index 15e68b8e1de4..665d5f17ccb4 100644 --- a/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py +++ b/sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client_async.py @@ -696,18 +696,16 @@ async def test_list_deleted_certificates_2016_10_01(self, client, **kwargs): @pytest.mark.asyncio async def test_policy_expected_errors_for_create_cert(): - """An issuer name, and either a subject or subject alternative name property, are required for creation""" + """Either a subject or subject alternative name property are required for creating a certificate""" client = CertificateClient("...", object()) - with pytest.raises(ValueError) as ex: + with pytest.raises(ValueError, match=NO_SAN_OR_SUBJECT): policy = CertificatePolicy() await client.create_certificate("...", policy=policy) - assert str(ex.value) == NO_SAN_OR_SUBJECT - with pytest.raises(ValueError) as ex: + with pytest.raises(ValueError, match=NO_SAN_OR_SUBJECT): policy = CertificatePolicy(issuer_name=WellKnownIssuerNames.self) await client.create_certificate("...", policy=policy) - assert str(ex.value) == NO_SAN_OR_SUBJECT def test_service_headers_allowed_in_logs():