From a94dc3252b1c12a6973363c095857cba693ff6c4 Mon Sep 17 00:00:00 2001 From: Johan Kromhout 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 --- .../Application/TestDiContainer.php | 16 ++++ .../Corto/Filter/Command/EnforcePolicy.php | 18 +++- .../EngineBlock/Exception/PdpCheckFailed.php | 21 +++++ .../Filter/Command/EnforcePolicyTest.php | 92 +++++++++++++++++++ 4 files changed, 145 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/Application/TestDiContainer.php b/library/EngineBlock/Application/TestDiContainer.php index 74b425fab7..39246a54b9 100644 --- a/library/EngineBlock/Application/TestDiContainer.php +++ b/library/EngineBlock/Application/TestDiContainer.php @@ -17,12 +17,18 @@ */ use OpenConext\EngineBlock\Stepup\StepupEndpoint; +use OpenConext\EngineBlockBundle\Pdp\PdpClientInterface; /** * Creates mocked versions of dependencies for unit testing */ class EngineBlock_Application_TestDiContainer extends EngineBlock_Application_DiContainer { + /** + * @var PdpClientInterface|null + */ + private $pdpClient; + public function getXmlConverter() { return Phake::mock('EngineBlock_Corto_XmlToArray'); @@ -38,6 +44,16 @@ public function getDatabaseConnectionFactory() return Phake::mock('EngineBlock_Database_ConnectionFactory'); } + public function getPdpClient() + { + return $this->pdpClient ?? parent::getPdpClient(); + } + + public function setPdpClient(PdpClientInterface $pdpClient) + { + $this->pdpClient = $pdpClient; + } + public function getConsentFactory() { $consentFactoryMock = Phake::mock('EngineBlock_Corto_Model_Consent_Factory'); 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 @@ +expectException('EngineBlock_Exception_PdpCheckFailed'); + $this->expectExceptionMessage('Policy Enforcement Point: Could not perform PDP check: Resource could not be read (status code "503")'); + + $this->mockPdpClientWithException(new UnreadableResourceException('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); + } + + private function mockPdpClientWithException(Throwable $exception): void + { + $pdpClient = Mockery::mock(PdpClientInterface::class); + $pdpClient->expects('requestDecisionFor')->andThrow($exception); + + /** @var EngineBlock_Application_TestDiContainer $container */ + $container = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer(); + $container->setPdpClient($pdpClient); + } + +}