From 8ea62add84c03ebae0e0965cabf2909601cb724f Mon Sep 17 00:00:00 2001 From: Pieter van der Meulen Date: Wed, 30 Apr 2025 18:44:23 +0200 Subject: [PATCH 1/3] Fix phpstan complaint: MetadataFactory::GetCertificateData() does not handle an invalid certificate --- src/Metadata/MetadataFactory.php | 4 ++ .../Unit/Metadata/MetadataFactoryTest.php | 38 +++++++++++++++++++ src/Tests/Unit/Metadata/certificate.pem | 23 +++++++++++ .../Unit/Metadata/invalid_certificate.pem | 23 +++++++++++ 4 files changed, 88 insertions(+) create mode 100644 src/Tests/Unit/Metadata/MetadataFactoryTest.php create mode 100644 src/Tests/Unit/Metadata/certificate.pem create mode 100644 src/Tests/Unit/Metadata/invalid_certificate.pem diff --git a/src/Metadata/MetadataFactory.php b/src/Metadata/MetadataFactory.php index ab18242..d4787fe 100644 --- a/src/Metadata/MetadataFactory.php +++ b/src/Metadata/MetadataFactory.php @@ -114,6 +114,10 @@ private function getCertificateData(string $publicKeyFile): string $matches = []; preg_match(Certificate::CERTIFICATE_PATTERN, $certificate, $matches); + if (! isset($matches[1])) { + throw new \RuntimeException(sprintf('Could not parse PEM certificate in %s', $publicKeyFile)); + } + return str_replace([' ', "\n"], '', $matches[1]); } diff --git a/src/Tests/Unit/Metadata/MetadataFactoryTest.php b/src/Tests/Unit/Metadata/MetadataFactoryTest.php new file mode 100644 index 0000000..2330526 --- /dev/null +++ b/src/Tests/Unit/Metadata/MetadataFactoryTest.php @@ -0,0 +1,38 @@ +getMockBuilder(MetadataFactory::class) + ->disableOriginalConstructor() + ->onlyMethods(['getCertificateData']) + ->getMock(); + + // Setup a reflection to call the private method + $reflectionMethod = new ReflectionMethod($metadataFactoryMock::class, 'getCertificateData'); + + // Test getCertificateData method with a valid certificate + $result = $reflectionMethod->invoke($metadataFactoryMock, $publicKeyFile); + $this->assertEquals($expectedCertificate, $result); + + // Test with an invalid certificate + $invalidPublicKeyFile = __DIR__ . '/invalid_certificate.pem'; // File with invalid certificate + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Could not parse PEM certificate in ' . $invalidPublicKeyFile); + $reflectionMethod->invoke($metadataFactoryMock, $invalidPublicKeyFile); + } +} \ No newline at end of file diff --git a/src/Tests/Unit/Metadata/certificate.pem b/src/Tests/Unit/Metadata/certificate.pem new file mode 100644 index 0000000..e8aa2a1 --- /dev/null +++ b/src/Tests/Unit/Metadata/certificate.pem @@ -0,0 +1,23 @@ +-----BEGIN CERTIFICATE----- +MIIDwTCCAqmgAwIBAgIUYuSUugwc4J4NyW9WGqYJ/liwM4owDQYJKoZIhvcNAQEL +BQAwcDELMAkGA1UEBhMCTkwxEDAOBgNVBAgMB1V0cmVjaHQxEDAOBgNVBAcMB1V0 +cmVjaHQxJzAlBgNVBAoMHkRldmVsb3BtZW50IERvY2tlciBlbnZpcm9ubWVudDEU +MBIGA1UEAwwLR2F0ZXdheSBJRFAwHhcNMjMwNTE3MTIxNTEyWhcNMzMwNTE0MTIx +NTEyWjBwMQswCQYDVQQGEwJOTDEQMA4GA1UECAwHVXRyZWNodDEQMA4GA1UEBwwH +VXRyZWNodDEnMCUGA1UECgweRGV2ZWxvcG1lbnQgRG9ja2VyIGVudmlyb25tZW50 +MRQwEgYDVQQDDAtHYXRld2F5IElEUDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC +AQoCggEBAM2ulQVs5WpbJOAf7Cv/VPDTJqbWHVdUxAmdwZJlcNTRKNFVp4aJzQ3d +piyiGghI5odnzU0/BWBoHZFNYPU/OFr/gzn6iJGxL63L9+mFgE8PR9HpkV5TaRnr +21+nZ0EXWjDZk9Px0enERicCItTeQzAUJeA0A9miIcK5IKIz/zSBSR3c802SGD/V +elUqY7Z2/UJM97cT92L+4Fz+4zhxxoThbPbrR0CweiROIt82grdwg7zf0+b62MOu +VtqFh0yPLRAFfLc4LjHuxFUdUvOHVta7x74dwdmHikqfujM10XN+sNns3LDJde2y +PWchU6ktq7cjgbYfIW/vzVzafP1Jk40CAwEAAaNTMFEwHQYDVR0OBBYEFGYn6LWR +DZa7+YryUncIlwJB2VorMB8GA1UdIwQYMBaAFGYn6LWRDZa7+YryUncIlwJB2Vor +MA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBAJ57lcOF6PWWW56m +S2s5gKFImtfRFzlfiyHsF14L7+nQ5NjfOhpU0wRpnTjK91KP0wCwlxzGFXR8yfqf +BFJryIV7aDdYPH/RIkwVaNBI0fsD/ozlYb18seieDEGLvQtTlrmc0UNHtWz6FW3L +2geM3ENaqpOATl1Ywp4EPML7Dh0CbhhyM8PnPCEsdclouIeP5/B9Swfk3omXehof +6bkFbntqA03msFBiW50twkfKeKULcJGXo667hto27KNxZUauqtPbnAGpUQmge8nx +SQlN8RPwlvygVM4LVMF9qP9YxloTH0xVNwN4noZUhfMNsKoJ7Hg5Xulaok8oCqmz +EiSroEg= +-----END CERTIFICATE----- diff --git a/src/Tests/Unit/Metadata/invalid_certificate.pem b/src/Tests/Unit/Metadata/invalid_certificate.pem new file mode 100644 index 0000000..bb9c3b3 --- /dev/null +++ b/src/Tests/Unit/Metadata/invalid_certificate.pem @@ -0,0 +1,23 @@ +-- +MIIDwTCCAqmgAwIBAgIUYuSUugwc4J4NyW9WGqYJ/liwM4owDQYJKoZIhvcNAQEL +BQAwcDELMAkGA1UEBhMCTkwxEDAOBgNVBAgMB1V0cmVjaHQxEDAOBgNVBAcMB1V0 +cmVjaHQxJzAlBgNVBAoMHkRldmVsb3BtZW50IERvY2tlciBlbnZpcm9ubWVudDEU +MBIGA1UEAwwLR2F0ZXdheSBJRFAwHhcNMjMwNTE3MTIxNTEyWhcNMzMwNTE0MTIx +NTEyWjBwMQswCQYDVQQGEwJOTDEQMA4GA1UECAwHVXRyZWNodDEQMA4GA1UEBwwH +VXRyZWNodDEnMCUGA1UECgweRGV2ZWxvcG1lbnQgRG9ja2VyIGVudmlyb25tZW50 +MRQwEgYDVQQDDAtHYXRld2F5IElEUDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC +AQoCggEBAM2ulQVs5WpbJOAf7Cv/VPDTJqbWHVdUxAmdwZJlcNTRKNFVp4aJzQ3d +piyiGghI5odnzU0/BWBoHZFNYPU/OFr/gzn6iJGxL63L9+mFgE8PR9HpkV5TaRnr +21+nZ0EXWjDZk9Px0enERicCItTeQzAUJeA0A9miIcK5IKIz/zSBSR3c802SGD/V +elUqY7Z2/UJM97cT92L+4Fz+4zhxxoThbPbrR0CweiROIt82grdwg7zf0+b62MOu +VtqFh0yPLRAFfLc4LjHuxFUdUvOHVta7x74dwdmHikqfujM10XN+sNns3LDJde2y +PWchU6ktq7cjgbYfIW/vzVzafP1Jk40CAwEAAaNTMFEwHQYDVR0OBBYEFGYn6LWR +DZa7+YryUncIlwJB2VorMB8GA1UdIwQYMBaAFGYn6LWRDZa7+YryUncIlwJB2Vor +MA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBAJ57lcOF6PWWW56m +S2s5gKFImtfRFzlfiyHsF14L7+nQ5NjfOhpU0wRpnTjK91KP0wCwlxzGFXR8yfqf +BFJryIV7aDdYPH/RIkwVaNBI0fsD/ozlYb18seieDEGLvQtTlrmc0UNHtWz6FW3L +2geM3ENaqpOATl1Ywp4EPML7Dh0CbhhyM8PnPCEsdclouIeP5/B9Swfk3omXehof +6bkFbntqA03msFBiW50twkfKeKULcJGXo667hto27KNxZUauqtPbnAGpUQmge8nx +SQlN8RPwlvygVM4LVMF9qP9YxloTH0xVNwN4noZUhfMNsKoJ7Hg5Xulaok8oCqmz +EiSroEg= +-----END CERTIFICATE----- From ae757423c5eb2ff6fc4a2201d4756301c346180b Mon Sep 17 00:00:00 2001 From: Pieter van der Meulen Date: Wed, 30 Apr 2025 18:49:17 +0200 Subject: [PATCH 2/3] Fix import --- src/Metadata/MetadataFactory.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Metadata/MetadataFactory.php b/src/Metadata/MetadataFactory.php index d4787fe..52e508a 100644 --- a/src/Metadata/MetadataFactory.php +++ b/src/Metadata/MetadataFactory.php @@ -25,6 +25,7 @@ use Surfnet\SamlBundle\Signing\KeyPair; use Symfony\Component\Routing\RouterInterface; use Twig\Environment; +use Surfnet\SamlBundle\Exception\RuntimeException; class MetadataFactory { @@ -115,7 +116,7 @@ private function getCertificateData(string $publicKeyFile): string preg_match(Certificate::CERTIFICATE_PATTERN, $certificate, $matches); if (! isset($matches[1])) { - throw new \RuntimeException(sprintf('Could not parse PEM certificate in %s', $publicKeyFile)); + throw new RuntimeException(sprintf('Could not parse PEM certificate in %s', $publicKeyFile)); } return str_replace([' ', "\n"], '', $matches[1]); From 6671293ff40f6570601d30cbafa612e8a96a270b Mon Sep 17 00:00:00 2001 From: Pieter van der Meulen Date: Wed, 30 Apr 2025 17:20:17 +0200 Subject: [PATCH 3/3] Do not throw an exception when the samlAuthenticationLogger is used before forAuthentication() is called on it This obscures the original log message --- src/Monolog/SamlAuthenticationLogger.php | 6 ++---- src/Tests/Unit/Monolog/SamlAuthenticationLoggerTest.php | 8 +++++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Monolog/SamlAuthenticationLogger.php b/src/Monolog/SamlAuthenticationLogger.php index 3b6d18d..3e13edd 100644 --- a/src/Monolog/SamlAuthenticationLogger.php +++ b/src/Monolog/SamlAuthenticationLogger.php @@ -96,12 +96,10 @@ public function log($level, $message, array $context = []): void */ private function modifyContext(array $context): array { - if (!$this->sari) { - throw new RuntimeException('Authentication logging context is unknown'); + if ($this->sari) { + $context['sari'] = $this->sari; } - $context['sari'] = $this->sari; - return $context; } } diff --git a/src/Tests/Unit/Monolog/SamlAuthenticationLoggerTest.php b/src/Tests/Unit/Monolog/SamlAuthenticationLoggerTest.php index 82ed6bd..fb881c4 100644 --- a/src/Tests/Unit/Monolog/SamlAuthenticationLoggerTest.php +++ b/src/Tests/Unit/Monolog/SamlAuthenticationLoggerTest.php @@ -47,10 +47,12 @@ public function it_returns_a_logger_for_an_authentication(): void /** * @test */ - public function it_errors_when_no_authentication(): void + public function it_does_not_throw_when_no_authentication(): void { - self::expectException(RuntimeException::class); - $logger = new SamlAuthenticationLogger(m::mock(LoggerInterface::class)); + $innerLogger = m::mock(LoggerInterface::class); + $innerLogger->shouldReceive('emergency')->with('message2', [])->once(); + + $logger = new SamlAuthenticationLogger($innerLogger); $logger->emergency('message2'); } }