Stop logging donor object in it's entirety and just log the selected …

…information. (#2969) * Stop logging donor object in it's entirety and just log the uId. * Refactor exceptions so that we can differentiate between loggable and non-loggable data. * Fix up test coverage * Broken Cleansing exception test * Ensure logging handled in base ApiException * Silly PHPCS grumbling about something I literally can't change without breaking code. * Handle default exception code differently to stop PHPCS from grumbling.
ministryofjustice · Dec 3, 2024 · 393af04 · 393af04
1 parent e6844fc
commit 393af04
Show file tree

Hide file tree

Showing 33 changed files with 624 additions and 206 deletions.
diff --git a/service-api/app/features/context/Integration/LpaContext.php b/service-api/app/features/context/Integration/LpaContext.php
@@ -6,9 +6,12 @@
 
 use App\DataAccess\Repository\Response\InstructionsAndPreferencesImages;
 use App\DataAccess\Repository\Response\InstructionsAndPreferencesImagesResult;
-use App\Entity\Sirius\SiriusLpa;
 use App\Exception\ApiException;
 use App\Exception\BadRequestException;
+use App\Exception\LpaActivationKeyAlreadyRequestedException;
+use App\Exception\LpaAlreadyAddedException;
+use App\Exception\LpaAlreadyHasActivationKeyException;
+use App\Exception\LpaNotRegisteredException;
 use App\Exception\NotFoundException;
 use App\Service\ActorCodes\ActorCodeService;
 use App\Service\Log\RequestTracing;
@@ -450,7 +453,7 @@ public function iRequestToAddAnLPAWhichHasAStatusOtherThanRegistered(): void
                 ],
                 $this->userId
             );
-        } catch (BadRequestException $ex) {
+        } catch (LpaNotRegisteredException $ex) {
             Assert::assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $ex->getCode());
             Assert::assertEquals('LPA status is not registered', $ex->getMessage());
             return;
@@ -1993,7 +1996,7 @@ public function iProvideTheDetailsFromAValidPaperDocumentThatAlreadyHasAnActivat
 
         try {
             $addOlderLpa->validateRequest($this->userId, $data);
-        } catch (BadRequestException $ex) {
+        } catch (LpaAlreadyHasActivationKeyException $ex) {
             Assert::assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $ex->getCode());
             Assert::assertEquals('LPA has an activation key already', $ex->getMessage());
             Assert::assertEquals(
@@ -2826,7 +2829,7 @@ public function iProvideTheDetailsFromAValidPaperLPAWhichIHaveAlreadyAddedToMyAc
 
         try {
             $addOlderLpa->validateRequest($this->userId, $data);
-        } catch (BadRequestException $ex) {
+        } catch (LpaAlreadyAddedException $ex) {
             Assert::assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $ex->getCode());
             Assert::assertEquals('LPA already added', $ex->getMessage());
             Assert::assertEquals($expectedResponse, $ex->getAdditionalData());
@@ -2934,7 +2937,7 @@ public function iProvideTheDetailsFromAValidPaperLPAWhichIHaveAlreadyRequestedAn
 
         try {
             $addOlderLpa->validateRequest($this->userId, $data);
-        } catch (BadRequestException $ex) {
+        } catch (LpaActivationKeyAlreadyRequestedException $ex) {
             Assert::assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $ex->getCode());
             Assert::assertEquals('Activation key already requested for LPA', $ex->getMessage());
             Assert::assertEquals($expectedResponse, $ex->getAdditionalData());

diff --git a/service-api/app/src/App/src/Exception/AbstractApiException.php b/service-api/app/src/App/src/Exception/AbstractApiException.php
@@ -4,6 +4,7 @@
 
 namespace App\Exception;
 
+use Fig\Http\Message\StatusCodeInterface;
 use RuntimeException;
 use Throwable;
 
@@ -12,47 +13,38 @@
  */
 abstract class AbstractApiException extends RuntimeException
 {
-    /**
-     * @var array
-     */
-    private array $additionalData = [];
-
     /**
      * AbstractApiException constructor
      *
-     * Following the sprint of https://framework.zend.com/blog/2017-03-23-expressive-error-handling.html
+     * Following the spirit of https://framework.zend.com/blog/2017-03-23-expressive-error-handling.html
      *
-     * @param string $title
-     * @param string|null $message
-     * @param array $additionalData
+     * @param string         $title
+     * @param string|null    $message
+     * @param int            $code
+     * @param array          $additionalData
      * @param Throwable|null $previous
      */
-    public function __construct(private string $title, ?string $message = null, array $additionalData = [], ?Throwable $previous = null)
-    {
-        //  Ensure the the required data is set in the extending exception classes
-        if (!is_numeric($this->getCode())) {
-            throw new RuntimeException('A numeric code must be set for API exceptions');
-        }
-
+    public function __construct(
+        private string $title,
+        ?string $message = null,
+        int $code = StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR,
+        protected array $additionalData = [],
+        ?Throwable $previous = null,
+    ) {
         //  If no message has been provided make it equal the title
         if (empty($message)) {
             $message = $title;
         }
 
         //  Set the remaining data for the exception
-        $this->additionalData = $additionalData;
-
-        parent::__construct($message, $this->getCode(), $previous);
+        parent::__construct($message, $code, $previous);
     }
 
     public function getTitle(): string
     {
         return $this->title;
     }
 
-    /**
-     * @return array
-     */
     public function getAdditionalData(): array
     {
         return $this->additionalData;

diff --git a/service-api/app/src/App/src/Exception/ApiException.php b/service-api/app/src/App/src/Exception/ApiException.php
@@ -4,34 +4,25 @@
 
 namespace App\Exception;
 
+use Fig\Http\Message\StatusCodeInterface;
 use Psr\Http\Message\ResponseInterface;
+use RuntimeException;
 use Throwable;
 
-class ApiException extends AbstractApiException
+class ApiException extends AbstractApiException implements LoggableAdditionalDataInterface
 {
-    // A safe bet for an exception is a 500 error response
-    public const DEFAULT_ERROR = 500;
-
     // The title is suitably generic, further details (from previous Throwables) will be
     // encapsulated in the stacktrace.
     public const DEFAULT_TITLE = 'An API exception has occurred';
+    public const DEFAULT_CODE  = StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR;
 
-    protected ?ResponseInterface $response;
-
-    /**
-     * ApiException constructor
-     *
-     * @param string $message
-     * @param int $code
-     * @param ResponseInterface|null $response
-     * @param Throwable|null $previous
-     */
-    public function __construct(string $message, int $code = self::DEFAULT_ERROR, ?ResponseInterface $response = null, ?Throwable $previous = null)
-    {
-        $this->response = $response;
-        $this->code     = $code;
-
-        parent::__construct(self::DEFAULT_TITLE, $message, [], $previous);
+    public function __construct(
+        string $message,
+        int $code = self::DEFAULT_CODE,
+        protected ?ResponseInterface $response = null,
+        ?Throwable $previous = null,
+    ) {
+        parent::__construct(self::DEFAULT_TITLE, $message, $code, [], $previous);
     }
 
     public function getResponse(): ?ResponseInterface
@@ -44,6 +35,7 @@ public function getResponse(): ?ResponseInterface
      * associative array.
      *
      * @return array
+     * @throws RuntimeException
      */
     public function getAdditionalData(): array
     {
@@ -52,9 +44,17 @@ public function getAdditionalData(): array
             : [];
     }
 
-    public static function create(?string $message = null, ?ResponseInterface $response = null, ?Throwable $previous = null): ApiException
+    public function getAdditionalDataForLogging(): array
     {
-        $code = self::DEFAULT_ERROR;
+        return $this->getAdditionalData();
+    }
+
+    public static function create(
+        ?string $message = null,
+        ?ResponseInterface $response = null,
+        ?Throwable $previous = null,
+    ): ApiException {
+        $code = self::DEFAULT_CODE;
 
         if (! is_null($response)) {
             $body = json_decode($response->getBody()->getContents(), true);
@@ -66,15 +66,15 @@ public static function create(?string $message = null, ?ResponseInterface $respo
                 if (is_array($body) && isset($body['details'])) {
                     $message = $body['details'];
                 }
+            }
 
-                //  If there is still no message then compose a standard message
-                if (is_null($message)) {
-                    $message = sprintf(
-                        'HTTP: %d - %s',
-                        $response->getStatusCode(),
-                        is_array($body) ? print_r($body, true) : 'Unexpected API response'
-                    );
-                }
+            //  If there is still no message then compose a standard message
+            if (is_null($message)) {
+                $message = sprintf(
+                    'HTTP: %d - %s',
+                    $response->getStatusCode(),
+                    is_array($body) ? print_r($body, true) : 'Unexpected API response'
+                );
             }
         }
 

diff --git a/service-api/app/src/App/src/Exception/BadRequestException.php b/service-api/app/src/App/src/Exception/BadRequestException.php
@@ -9,20 +9,11 @@
 
 class BadRequestException extends AbstractApiException
 {
-    /**
-     * Exception title
-     */
     public const TITLE = 'Bad Request';
+    public const CODE  = StatusCodeInterface::STATUS_BAD_REQUEST;
 
-    protected $code = StatusCodeInterface::STATUS_BAD_REQUEST;
-
-    /**
-     * @param string $message
-     * @param array $additionalData
-     * @param Throwable|null $previous
-     */
     public function __construct(?string $message = null, array $additionalData = [], ?Throwable $previous = null)
     {
-        parent::__construct(self::TITLE, $message, $additionalData, $previous);
+        parent::__construct(self::TITLE, $message, self::CODE, $additionalData, $previous);
     }
 }
diff --git a/service-api/app/src/App/src/Exception/ConflictException.php b/service-api/app/src/App/src/Exception/ConflictException.php
@@ -9,20 +9,11 @@
 
 class ConflictException extends AbstractApiException
 {
-    /**
-     * Exception title
-     */
     public const TITLE = 'Conflict';
+    public const CODE  = StatusCodeInterface::STATUS_CONFLICT;
 
-    protected $code = StatusCodeInterface::STATUS_CONFLICT;
-
-    /**
-     * @param string $message
-     * @param array $additionalData
-     * @param Throwable|null $previous
-     */
     public function __construct(?string $message = null, array $additionalData = [], ?Throwable $previous = null)
     {
-        parent::__construct(self::TITLE, $message, $additionalData, $previous);
+        parent::__construct(self::TITLE, $message, self::CODE, $additionalData, $previous);
     }
 }
diff --git a/service-api/app/src/App/src/Exception/CreationException.php b/service-api/app/src/App/src/Exception/CreationException.php
@@ -9,20 +9,11 @@
 
 class CreationException extends AbstractApiException
 {
-    /**
-     * Exception title
-     */
     public const TITLE = 'Creation error';
+    public const CODE  = StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR;
 
-    protected $code = StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR;
-
-    /**
-     * @param string $message
-     * @param array $additionalData
-     * @param Throwable|null $previous
-     */
     public function __construct(?string $message = null, array $additionalData = [], ?Throwable $previous = null)
     {
-        parent::__construct(self::TITLE, $message, $additionalData, $previous);
+        parent::__construct(self::TITLE, $message, self::CODE, $additionalData, $previous);
     }
 }
diff --git a/service-api/app/src/App/src/Exception/ForbiddenException.php b/service-api/app/src/App/src/Exception/ForbiddenException.php
@@ -9,20 +9,11 @@
 
 class ForbiddenException extends AbstractApiException
 {
-    /**
-     * Exception title
-     */
     public const TITLE = 'Forbidden';
+    public const CODE  = StatusCodeInterface::STATUS_FORBIDDEN;
 
-    protected $code = StatusCodeInterface::STATUS_FORBIDDEN;
-
-    /**
-     * @param string $message
-     * @param array $additionalData
-     * @param Throwable|null $previous
-     */
     public function __construct(?string $message = null, array $additionalData = [], ?Throwable $previous = null)
     {
-        parent::__construct(self::TITLE, $message, $additionalData, $previous);
+        parent::__construct(self::TITLE, $message, self::CODE, $additionalData, $previous);
     }
 }
diff --git a/service-api/app/src/App/src/Exception/GoneException.php b/service-api/app/src/App/src/Exception/GoneException.php
@@ -9,20 +9,11 @@
 
 class GoneException extends AbstractApiException
 {
-    /**
-     * Exception title
-     */
     public const TITLE = 'Gone';
+    public const CODE  = StatusCodeInterface::STATUS_GONE;
 
-    protected $code = StatusCodeInterface::STATUS_GONE;
-
-    /**
-     * @param string $message
-     * @param array $additionalData
-     * @param Throwable|null $previous
-     */
-    public function __construct(?string $message = null, ?array $additionalData = [], ?Throwable $previous = null)
+    public function __construct(?string $message = null, array $additionalData = [], ?Throwable $previous = null)
     {
-        parent::__construct(self::TITLE, $message, $additionalData, $previous);
+        parent::__construct(self::TITLE, $message, self::CODE, $additionalData, $previous);
     }
 }
diff --git a/service-api/app/src/App/src/Exception/LoggableAdditionalDataInterface.php b/service-api/app/src/App/src/Exception/LoggableAdditionalDataInterface.php
@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Exception;
+
+interface LoggableAdditionalDataInterface
+{
+    public function getAdditionalDataForLogging(): array;
+}
diff --git a/service-api/app/src/App/src/Exception/LpaActivationKeyAlreadyRequestedException.php b/service-api/app/src/App/src/Exception/LpaActivationKeyAlreadyRequestedException.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Exception;
+
+use Fig\Http\Message\StatusCodeInterface;
+
+class LpaActivationKeyAlreadyRequestedException extends AbstractApiException implements LoggableAdditionalDataInterface
+{
+    public const MESSAGE = 'Activation key already requested for LPA';
+    public const TITLE   = 'Bad Request';
+    public const CODE    = StatusCodeInterface::STATUS_BAD_REQUEST;
+
+    public function __construct(array $additionalData = [])
+    {
+        parent::__construct(self::TITLE, self::MESSAGE, self::CODE, $additionalData);
+    }
+
+    public function getAdditionalDataForLogging(): array
+    {
+        $data = $this->getAdditionalData();
+
+        // choose to be explicit about what is being logged to avoid leakage.
+        return [
+            'donor'                => [
+                'uId' => $data['donor']['uId'] ?? '',
+            ],
+            'caseSubtype'          => $data['caseSubtype'] ?? '',
+            'activationKeyDueDate' => $data['activationKeyDueDate'] ?? '',
+        ];
+    }
+}