Revize 9e6f791a

Re #8571 - Fixed problems during walk-through code review

src/controllers/certificates_controller.py

@@ -12 +12 @@
 from src.exceptions.database_exception import DatabaseException
 from src.model.subject import Subject
 from src.services.certificate_service import CertificateService, RevocationReasonInvalidException, \
-    CertificateStatusInvalidException, CertificateNotFoundException
+    CertificateStatusInvalidException, CertificateNotFoundException, CertificateAlreadyRevokedException
 #  responsibility.
 from src.services.key_service import KeyService
 
@@ -35 +35 @@
 
 E_NO_ISSUER_FOUND = {"success": False, "data": "No certificate authority with such unique ID exists."}
 E_NO_CERTIFICATES_FOUND = {"success": False, "data": "No such certificate found."}
+E_NO_CERTIFICATE_ALREADY_REVOKED = {"success": False, "data": "Certificate is already revoked."}
 E_NO_CERT_PRIVATE_KEY_FOUND = {"success": False,
                                "data": "Internal server error (certificate's private key cannot be found)."}
 E_NOT_JSON_FORMAT = {"success": False, "data": "The request must be JSON-formatted."}
@@ -340 +341 @@
             try:
                 # set certificate status using certificate_service
                 self.certificate_service.set_certificate_revocation_status(identifier, status, reason)
-            except (RevocationReasonInvalidException, CertificateStatusInvalidException, CertificateNotFoundException):
+            except (RevocationReasonInvalidException, CertificateStatusInvalidException):
                 # these exceptions are thrown in case invalid status or revocation reason is passed to the controller
                 return E_WRONG_PARAMETERS, C_BAD_REQUEST
+            except CertificateAlreadyRevokedException:
+                return E_NO_CERTIFICATE_ALREADY_REVOKED, C_BAD_REQUEST
+            except CertificateNotFoundException:
+                return E_NO_CERTIFICATES_FOUND, C_NOT_FOUND
             return {"success": True,
                     "data": "Certificate status updated successfully."}, C_SUCCESS
         # throw an error in case the request does not contain a json body

src/exceptions/unknown_exception.py

@@ +1 @@
+class UnknownException(Exception):
+    def __init__(self, message):
+        self.message = message
+
+    def __str__(self):
+        return self.message

src/services/certificate_service.py

@@ -6 +6 @@
 from src.constants import ROOT_CA_ID, INTERMEDIATE_CA_ID, CA_ID, CERTIFICATE_ID, CERTIFICATE_STATES, \
     CERTIFICATE_REVOCATION_REASONS
 from src.dao.certificate_repository import CertificateRepository
+from src.exceptions.unknown_exception import UnknownException
 from src.model.certificate import Certificate
 from src.model.private_key import PrivateKey
 from src.model.subject import Subject
@@ -279 +280 @@
         if reason not in CERTIFICATE_REVOCATION_REASONS:
             raise RevocationReasonInvalidException(reason)
 
+        # check whether the certificate exists
+        certificate = self.certificate_repository.read(id)
+        if certificate is None:
+            raise CertificateNotFoundException(id)
+
         updated = False
         if status == STATUS_VALID:
             updated = self.certificate_repository.clear_certificate_revocation(id)
         elif status == STATUS_REVOKED:
+            # check if the certificate is not revoked already
+            revoked = self.certificate_repository.get_all_revoked_by(certificate.parent_id)
+            if certificate.certificate_id in [x.certificate_id for x in revoked]:
+                raise CertificateAlreadyRevokedException(id)
+
             revocation_timestamp = int(time.time())
             updated = self.certificate_repository.set_certificate_revoked(id, str(revocation_timestamp), reason)
 
         if not updated:
-            raise CertificateNotFoundException(id)
+            # TODO log this
+            raise UnknownException("Repository returned 'false' from clear_certificate_revocation() "
+                                   "or set_certificate_revoked().")
 
     def get_subject_from_certificate(self, certificate: Certificate) -> Subject:
         """
@@ -357 +370 @@
 class CertificateNotFoundException(Exception):
     """
     Exception that denotes that the caller was trying to set
-    a certificate to an invalid state
+    work with non-existing certificate
     """
 
     def __init__(self, id):
@@ -365 +378 @@
 
     def __str__(self):
         return f"Certificate id '{self.id}' does not exist."
+
+
+class CertificateAlreadyRevokedException(Exception):
+    """
+    Exception that denotes that the caller was trying to revoke
+    a certificate that is already revoked
+    """
+
+    def __init__(self, id):
+        self.id = id
+
+    def __str__(self):
+        return f"Certificate id '{self.id}' is already revoked."

src/services/cryptography.py

@@ -126 +126 @@
         # reference in openssl req command using -extensions option.
         extensions += "\n"+CA_EXTENSIONS
         if len(config) == 0:
-            config += MINIMAL_CONFIG_FILE+"[ " + SSCRT_SECTION + " ]"+"\n"+extensions
+            config += MINIMAL_CONFIG_FILE
         config += "\n[ " + SSCRT_SECTION + " ]" + "\n" + extensions
 
         with TemporaryFile("openssl.conf", config) as conf_path:

tests/integration_tests/rest_api/certificates_test.py

@@ -634 +634 @@
     assert "success" in revoke_ret.json
     assert revoke_ret.json["success"]
 
+    revoke_ret = server.patch(f"/api/certificates/{cert_id}", content_type="application/json", json=revocation_body)
+
+    assert revoke_ret.status_code == 400
+    assert "data" in revoke_ret.json
+    assert "success" in revoke_ret.json
+    assert not revoke_ret.json["success"]
+
     # set to valid again
     valid_body = {
         "status":  "valid"
@@ -687 +694 @@
     }
     revoke_ret = server.patch(f"/api/certificates/54791", content_type="application/json", json=revocation_body)
 
-    assert revoke_ret.status_code == 400
+    assert revoke_ret.status_code == 404
     assert "data" in revoke_ret.json
     assert "success" in revoke_ret.json
     assert not revoke_ret.json["success"]

tests/integration_tests/services/certificate_service_test.py

@@ -5 +5 @@
 from src.constants import SSL_ID, CA_ID, AUTHENTICATION_ID, INTERMEDIATE_CA_ID, ROOT_CA_ID, CERTIFICATE_ID, SIGNATURE_ID
 from src.model.subject import Subject
 from src.services.certificate_service import RevocationReasonInvalidException, CertificateStatusInvalidException, \
-    CertificateNotFoundException
+    CertificateNotFoundException, CertificateAlreadyRevokedException
 
 
 def export_crt(crt):
@@ -321 +321 @@
 
     with pytest.raises(CertificateNotFoundException) as e:
         certificate_service_unique.set_certificate_revocation_status(5974, "revoked", "unspecified")
+
+    certificate_service_unique.set_certificate_revocation_status(root_ca_cert.certificate_id, "revoked", "unspecified")
+
+    with pytest.raises(CertificateAlreadyRevokedException) as e:
+        certificate_service_unique.set_certificate_revocation_status(root_ca_cert.certificate_id, "revoked",
+                                                                     "unspecified")
+