Merge pull request #130 from OpenConext/bugfix/exception-from-logger

Do not throw an exception when the samlAuthenticationLogger is used before forAuthentication() is called on it.

- Pro: this does not hide the error being logged at runtime
- Con: It is harder for a developer to notice that they forgot to call forAuthentication(). In this case it will still log, but will not log a sari, defeating the purpose of this logger.

Author: pmeulen

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
 {
@@ -114,6 +115,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]);
     }
 

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;
     }
 }

src/Tests/Unit/Metadata/MetadataFactoryTest.php

@@ -0,0 +1,38 @@
+<?php
+
+
+namespace Surfnet\SamlBundle\Tests\Unit\Metadata;
+
+use PHPUnit\Framework\TestCase;
+use ReflectionMethod;
+use Surfnet\SamlBundle\Metadata\MetadataFactory;
+
+class MetadataFactoryTest extends TestCase
+{
+    public function testGetCertificateData(): void
+    {
+        $publicKeyFile = __DIR__ . '/certificate.pem';  // File with test certificate in PEM format
+        // Read the public key file and remove the first and last lines and all newlines
+        $expectedCertificate = str_replace("\n", '', implode("", array_slice(file($publicKeyFile), 1, -1)));
+
+        // Setup a mock for the MetadataFactory with the real getCertificateData method
+        // and add the mocked File class to it
+        $metadataFactoryMock = $this->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);
+    }
+}

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-----

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-----

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');
     }
 }