Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Metadata/MetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use Surfnet\SamlBundle\Signing\KeyPair;
use Symfony\Component\Routing\RouterInterface;
use Twig\Environment;
use Surfnet\SamlBundle\Exception\RuntimeException;

class MetadataFactory
{
Expand Down Expand Up @@ -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]);
}

Expand Down
6 changes: 2 additions & 4 deletions src/Monolog/SamlAuthenticationLogger.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will indeed prevent an exception from being raised, but if this logger is used, it should use forAuthentication first.

If something needs to be logged in situations where we don't have a requestID, another logger should be used.

So I suspect this has something to do with the wrong logger being used after the Symfony upgrade.

It would be safe to merge, but then the sari in the logs might disappear in certain cases were a sari would be helpful.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with everything you said, except:

... but then the sari in the logs might disappear in certain cases were a sari would be helpful.

This patch can never make a sari disappear, all it does is not throw when the sari is not there.

E.g. when expectedInResponseTo is null here, for instance because of session troubles,

$logger = $this->authenticationLogger->forAuthentication($expectedInResponseTo);
the later calls to the logger will throw an exception, hiding the original problem.

Or this could happen because the logger is used before forAuthentication() is called like you said.

These issues in the use of this logger should be addresses to, but the choice to make a log action throw hides valuable info, and does not help at all. I ran into this while debugging such an issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @pablothedude : The point is that without the exception a developer can use this class wrongly (i.e DI'ing it without calling forAuthentication()) and not notice this.

$context['sari'] = $this->sari;
}

$context['sari'] = $this->sari;

return $context;
}
}
38 changes: 38 additions & 0 deletions src/Tests/Unit/Metadata/MetadataFactoryTest.php
Original file line number Diff line number Diff line change
@@ -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);
}
}
23 changes: 23 additions & 0 deletions src/Tests/Unit/Metadata/certificate.pem
Original file line number Diff line number Diff line change
@@ -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-----
23 changes: 23 additions & 0 deletions src/Tests/Unit/Metadata/invalid_certificate.pem
Original file line number Diff line number Diff line change
@@ -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-----
8 changes: 5 additions & 3 deletions src/Tests/Unit/Monolog/SamlAuthenticationLoggerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
}
Loading