From 2ceaeea272655f07d8523328ce00e124dbd8b971 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 14 Aug 2023 20:18:36 +0200 Subject: [PATCH] Move RelayState; it's a characteristic of the binding rather than the message --- src/SAML2/Binding.php | 29 ++++++++++++++++ src/SAML2/HTTPPost.php | 4 +-- src/SAML2/HTTPRedirect.php | 4 +-- src/SAML2/XML/samlp/AbstractMessage.php | 34 ------------------- .../XML/samlp/AbstractStatusResponse.php | 4 +-- src/SAML2/XML/samlp/LogoutResponse.php | 3 -- tests/SAML2/HTTPPostTest.php | 4 +-- tests/SAML2/HTTPRedirectTest.php | 8 +++-- 8 files changed, 41 insertions(+), 49 deletions(-) diff --git a/src/SAML2/Binding.php b/src/SAML2/Binding.php index e0cdc3a5f..65548464b 100644 --- a/src/SAML2/Binding.php +++ b/src/SAML2/Binding.php @@ -25,6 +25,13 @@ */ abstract class Binding { + /** + * The RelayState associated with the message. + * + * @var string|null + */ + protected ?string $relayState = null; + /** * The destination of messages. * @@ -149,6 +156,28 @@ public function getDestination(): ?string } + /** + * Set the RelayState associated with he message. + * + * @param string|null $relayState The RelayState. + */ + public function setRelayState(string $relayState = null): void + { + $this->relayState = $relayState; + } + + + /** + * Get the RelayState associated with the message. + * + * @return string|null The RelayState. + */ + public function getRelayState(): ?string + { + return $this->relayState; + } + + /** * Override the destination of a message. * diff --git a/src/SAML2/HTTPPost.php b/src/SAML2/HTTPPost.php index ca7552a8a..0bdf8e330 100644 --- a/src/SAML2/HTTPPost.php +++ b/src/SAML2/HTTPPost.php @@ -43,7 +43,7 @@ public function send(AbstractMessage $message): ResponseInterface } else { $destination = $this->destination; } - $relayState = $message->getRelayState(); + $relayState = $this->getRelayState(); $msgStr = $message->toXML(); @@ -110,7 +110,7 @@ public function receive(ServerRequestInterface $request): AbstractMessage } if (array_key_exists('RelayState', $query)) { - $msg->setRelayState($query['RelayState']); + $this->setRelayState($query['RelayState']); } return $msg; diff --git a/src/SAML2/HTTPRedirect.php b/src/SAML2/HTTPRedirect.php index 2c0cb8327..6ed1b0690 100644 --- a/src/SAML2/HTTPRedirect.php +++ b/src/SAML2/HTTPRedirect.php @@ -56,7 +56,7 @@ public function getRedirectURL(AbstractMessage $message): string $destination = $this->destination; } - $relayState = $message->getRelayState(); + $relayState = $this->getRelayState(); $msgStr = $message->toXML(); Utils::getContainer()->debugMessage($msgStr, 'out'); @@ -152,7 +152,7 @@ public function receive(ServerRequestInterface $request): AbstractMessage $msg = MessageFactory::fromXML($document->documentElement); if (array_key_exists('RelayState', $query)) { - $msg->setRelayState($query['RelayState']); + $this->setRelayState($query['RelayState']); } return $msg; } diff --git a/src/SAML2/XML/samlp/AbstractMessage.php b/src/SAML2/XML/samlp/AbstractMessage.php index 58cea12c9..0c3536d2d 100644 --- a/src/SAML2/XML/samlp/AbstractMessage.php +++ b/src/SAML2/XML/samlp/AbstractMessage.php @@ -41,13 +41,6 @@ abstract class AbstractMessage extends AbstractSamlpElement implements SignableE use SignedElementTrait; - /** - * The RelayState associated with this message. - * - * @var string|null - */ - protected ?string $relayState = null; - /** * The \DOMDocument we are currently building. * @@ -80,7 +73,6 @@ abstract class AbstractMessage extends AbstractSamlpElement implements SignableE * @param string|null $destination * @param string|null $consent * @param \SimpleSAML\SAML2\XML\samlp\Extensions $extensions - * @param string|null $relayState * * @throws \Exception */ @@ -92,7 +84,6 @@ protected function __construct( protected ?string $destination = null, protected ?string $consent = null, ?Extensions $extensions = null, - ?string $relayState = null, ) { Assert::nullOrSame($issueInstant?->getTimeZone()->getName(), 'Z', ProtocolViolationException::class); Assert::nullOrValidNCName($id); // Covers the empty string @@ -100,7 +91,6 @@ protected function __construct( Assert::nullOrValidURI($consent); // Covers the empty string $this->setExtensions($extensions); - $this->setRelayState($relayState); } @@ -191,30 +181,6 @@ public function isMessageConstructedWithSignature(): bool } - /** - * Retrieve the RelayState associated with this message. - * - * @return string|null The RelayState, or NULL if no RelayState is given - */ - public function getRelayState(): ?string - { - return $this->relayState; - } - - - /** - * Set the RelayState associated with this message. - * - * @param string|null $relayState The new RelayState - */ - public function setRelayState(string $relayState = null): void - { - Assert::nullOrNotWhitespaceOnly($relayState); - - $this->relayState = $relayState; - } - - /** * Get the XML element. * diff --git a/src/SAML2/XML/samlp/AbstractStatusResponse.php b/src/SAML2/XML/samlp/AbstractStatusResponse.php index 3304cf46a..da8913124 100644 --- a/src/SAML2/XML/samlp/AbstractStatusResponse.php +++ b/src/SAML2/XML/samlp/AbstractStatusResponse.php @@ -34,7 +34,6 @@ abstract class AbstractStatusResponse extends AbstractMessage * @param string|null $destination * @param string|null $consent * @param \SimpleSAML\SAML2\XML\samlp\Extensions|null $extensions - * @param string|null $relayState * * @throws \Exception */ @@ -48,11 +47,10 @@ protected function __construct( ?string $destination = null, ?string $consent = null, ?Extensions $extensions = null, - ?string $relayState = null, ) { Assert::nullOrValidNCName($inResponseTo); // Covers the empty string - parent::__construct($issuer, $id, $version, $issueInstant, $destination, $consent, $extensions, $relayState); + parent::__construct($issuer, $id, $version, $issueInstant, $destination, $consent, $extensions); } diff --git a/src/SAML2/XML/samlp/LogoutResponse.php b/src/SAML2/XML/samlp/LogoutResponse.php index 3fdb70af7..45b9ae24b 100644 --- a/src/SAML2/XML/samlp/LogoutResponse.php +++ b/src/SAML2/XML/samlp/LogoutResponse.php @@ -37,7 +37,6 @@ final class LogoutResponse extends AbstractStatusResponse * @param string|null $destination * @param string|null $consent * @param \SimpleSAML\SAML2\XML\samlp\Extensions|null $extensions - * @param string|null $relayState * * @throws \Exception */ @@ -51,7 +50,6 @@ public function __construct( ?string $destination = null, ?string $consent = null, ?Extensions $extensions = null, - ?string $relayState = null, ) { parent::__construct( $status, @@ -63,7 +61,6 @@ public function __construct( $destination, $consent, $extensions, - $relayState, ); } diff --git a/tests/SAML2/HTTPPostTest.php b/tests/SAML2/HTTPPostTest.php index 74af423c2..2316f3150 100644 --- a/tests/SAML2/HTTPPostTest.php +++ b/tests/SAML2/HTTPPostTest.php @@ -78,7 +78,7 @@ public function testResponseParsing(): void $this->assertInstanceOf(Response::class, $response); $issuer = $response->getIssuer(); $this->assertEquals('https://engine.test.surfconext.nl/authentication/idp/metadata', $issuer->getContent()); - $relay = $response->getRelayState(); + $relay = $hp->getRelayState(); $this->assertEquals('relaystate001', $relay); } @@ -161,7 +161,6 @@ public function testSendAuthnResponse(): void issuer: $issuer, destination: 'http://example.org/login?success=yes', ); - $response->setRelayState('http://example.org'); $signer = (new SignatureAlgorithmFactory())->getAlgorithm( C::SIG_RSA_SHA256, PEMCertificatesMock::getPrivateKey(PEMCertificatesMock::SELFSIGNED_PRIVATE_KEY), @@ -169,6 +168,7 @@ public function testSendAuthnResponse(): void $response->sign($signer); $hr = new HTTPPost(); + $hr->setRelayState('http://example.org'); $hr->send($response); } } diff --git a/tests/SAML2/HTTPRedirectTest.php b/tests/SAML2/HTTPRedirectTest.php index 53a7eb88d..38b94c70d 100644 --- a/tests/SAML2/HTTPRedirectTest.php +++ b/tests/SAML2/HTTPRedirectTest.php @@ -117,7 +117,7 @@ public function testRequestParsingMoreParams(): void $hr = new HTTPRedirect(); $samlrequest = $hr->receive($request); $this->assertInstanceOf(AbstractRequest::class, $samlrequest); - $relaystate = $samlrequest->getRelayState(); + $relaystate = $hr->getRelayState(); $this->assertEquals('https://profile.surfconext.nl/', $relaystate); } @@ -140,7 +140,7 @@ public function testSignedRequestParsing(): void $hr = new HTTPRedirect(); $samlrequest = $hr->receive($request); $this->assertInstanceOf(AbstractRequest::class, $samlrequest); - $relaystate = $samlrequest->getRelayState(); + $relaystate = $hr->getRelayState(); $this->assertEquals( 'https://demo.moo-archive.nl/module.php/admin/test/default-sp', $relaystate, @@ -163,6 +163,7 @@ public function testSignedRequestValidation(): void $request = $request->withQueryParams($q); $hr = new HTTPRedirect(); + $hr->setRelayState(urlencode($q['RelayState'])); $samlrequest = $hr->receive($request); // validate with the correct certificate, should verify @@ -308,8 +309,9 @@ public function testSendAuthnResponse(): void issuer: $issuer, destination: 'http://example.org/login?success=yes', ); - $response->setRelayState('http://example.org'); + $hr = new HTTPRedirect(); + $hr->setRelayState('http://example.org'); $hr->send($response); }