From 8733cf3ddb13499a86ad4181c498a3794d4bf540 Mon Sep 17 00:00:00 2001
From: Johan Kromhout <jkromhout@ibuildings.nl>
Date: Mon, 20 Jan 2025 09:52:18 +0100
Subject: [PATCH] Improve handling of PDP timeout error

Prior to this change, mulitple stacktraces would be printed in the logs
when this error occured without making it clear which error was triggered.
This change ensures a specific exception gets thrown so it won't trigger
the exception stack dump.

Fixes https://github.com/OpenConext/OpenConext-engineblock/issues/1285
---
 .../Corto/Filter/Command/EnforcePolicy.php    | 18 ++++-
 .../EngineBlock/Exception/PdpCheckFailed.php  | 21 +++++
 .../Filter/Command/EnforcePolicyTest.php      | 78 +++++++++++++++++++
 3 files changed, 115 insertions(+), 2 deletions(-)
 create mode 100644 library/EngineBlock/Exception/PdpCheckFailed.php
 create mode 100644 tests/library/EngineBlock/Test/Corto/Filter/Command/EnforcePolicyTest.php

diff --git a/library/EngineBlock/Corto/Filter/Command/EnforcePolicy.php b/library/EngineBlock/Corto/Filter/Command/EnforcePolicy.php
index 014e17c6bf..fe0f7bb3d0 100644
--- a/library/EngineBlock/Corto/Filter/Command/EnforcePolicy.php
+++ b/library/EngineBlock/Corto/Filter/Command/EnforcePolicy.php
@@ -20,6 +20,13 @@
 
 class EngineBlock_Corto_Filter_Command_EnforcePolicy extends EngineBlock_Corto_Filter_Command_Abstract
 {
+    /**
+     * @throws EngineBlock_Exception_UnknownRequesterIdInAuthnRequest
+     * @throws EngineBlock_Exception_UnknownServiceProvider
+     * @throws EngineBlock_Exception
+     * @throws EngineBlock_Corto_Exception_PEPNoAccess
+     * @throws EngineBlock_Exception_PdpCheckFailed
+     */
     public function execute()
     {
         $log = EngineBlock_ApplicationSingleton::getLog();
@@ -54,8 +61,15 @@ public function execute()
 
         $log->debug("Policy Enforcement Point: Requesting decision from PDP");
 
-        $pdp = $this->getPdpClient();
-        $policyDecision = $pdp->requestDecisionFor($pdpRequest);
+        try {
+            $pdp = $this->getPdpClient();
+            $policyDecision = $pdp->requestDecisionFor($pdpRequest);
+        } catch (\OpenConext\EngineBlock\Http\Exception\HttpException $e) {
+            throw new EngineBlock_Exception_PdpCheckFailed(
+                'Policy Enforcement Point: Could not perform PDP check: ' . $e->getMessage()
+            );
+        }
+
         // The IdP logo is set after getting the PolicyDecision as it would be inappropriate to inject this into the
         // decision request.
         if ($this->_identityProvider->getMdui()->hasLogo()){
diff --git a/library/EngineBlock/Exception/PdpCheckFailed.php b/library/EngineBlock/Exception/PdpCheckFailed.php
new file mode 100644
index 0000000000..43ddb63059
--- /dev/null
+++ b/library/EngineBlock/Exception/PdpCheckFailed.php
@@ -0,0 +1,21 @@
+<?php
+
+/**
+ * Copyright 2025 SURFnet B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class EngineBlock_Exception_PdpCheckFailed extends EngineBlock_Exception
+{
+}
diff --git a/tests/library/EngineBlock/Test/Corto/Filter/Command/EnforcePolicyTest.php b/tests/library/EngineBlock/Test/Corto/Filter/Command/EnforcePolicyTest.php
new file mode 100644
index 0000000000..72c5b699e9
--- /dev/null
+++ b/tests/library/EngineBlock/Test/Corto/Filter/Command/EnforcePolicyTest.php
@@ -0,0 +1,78 @@
+<?php
+
+/**
+ * Copyright 2025 SURFnet B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
+use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider;
+use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
+use OpenConext\EngineBlock\Metadata\MetadataRepository\MetadataRepositoryInterface;
+use PHPUnit\Framework\TestCase;
+use SAML2\Assertion;
+use SAML2\AuthnRequest;
+
+class EngineBlock_Test_Corto_Filter_Command_EnforcePolicyTest extends TestCase
+{
+    use MockeryPHPUnitIntegration;
+
+    public function testThrowsEngineBlockExceptionIfPolicyCannotBeChecked()
+    {
+        $this->expectException('EngineBlock_Exception_PdpCheckFailed');
+        $this->expectExceptionMessage('Policy Enforcement Point: Could not perform PDP check: Resource could not be read (status code "503")');
+
+        $policy = new EngineBlock_Corto_Filter_Command_EnforcePolicy();
+
+        $request = $this->mockRequest();
+        $policy->setRequest($request);
+
+        $repo = Mockery::mock(MetadataRepositoryInterface::class);
+        $server = Mockery::mock(EngineBlock_Corto_ProxyServer::class);
+        $server->expects('getRepository')->andReturn($repo);
+
+        $sp = $this->mockServiceProvider();
+
+        $policy->setServiceProvider($sp);
+        $policy->setProxyServer($server);
+        $policy->setResponseAttributes([]);
+
+        $policy->setCollabPersonId('foo');
+
+        $idp = Mockery::mock(IdentityProvider::class);
+        $idp->entityId = 'bar';
+        $policy->setIdentityProvider($idp);
+
+        $policy->execute();
+    }
+
+    private function mockServiceProvider(): ServiceProvider
+    {
+        $sp = Mockery::mock(ServiceProvider::class);
+        $sp->entityId = 'bar';
+        $sp->shouldReceive('getCoins->isTrustedProxy')->andReturn(false);
+        $sp->shouldReceive('getCoins->policyEnforcementDecisionRequired')->andReturn(true);
+        return $sp;
+    }
+
+    private function mockRequest(): EngineBlock_Saml2_AuthnRequestAnnotationDecorator
+    {
+        $assertion = new Assertion();
+        $request = new AuthnRequest();
+        $response = new SAML2\Response();
+        $response->setAssertions(array($assertion));
+        return new EngineBlock_Saml2_AuthnRequestAnnotationDecorator($request);
+    }
+
+}