From 9cf4191079a5d08106a6be1344fe9a822742afc4 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 8 May 2021 13:07:25 +0100 Subject: [PATCH] Reviewed and updated SAML2 authncontext option Added tests to cover. Changed default to align with existing default. Added env option parsing. For #1998 --- .env.example.complete | 7 +------ app/Config/saml2.php | 12 ++++++++---- tests/Auth/Saml2Test.php | 35 +++++++++++++++++++++++++++++++++++ tests/Unit/ConfigTest.php | 12 +++++++++++- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index a3b0702d5..71fe66bca 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -222,12 +222,7 @@ SAML2_IDP_x509=null SAML2_ONELOGIN_OVERRIDES=null SAML2_DUMP_USER_DETAILS=false SAML2_AUTOLOAD_METADATA=false - -# SAML Authentication context. -# Set to false and no AuthContext will be sent in the AuthNRequest, -# Set true and you will get an AuthContext 'exact' 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport' -# Set an array with the possible auth context values: array ('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:X509'), -SAML2_IDP_AUTHNCONTEXT=false +SAML2_IDP_AUTHNCONTEXT=true # SAML group sync configuration # Refer to https://www.bookstackapp.com/docs/admin/saml2-auth/ diff --git a/app/Config/saml2.php b/app/Config/saml2.php index 0e186c269..8ba969549 100644 --- a/app/Config/saml2.php +++ b/app/Config/saml2.php @@ -1,5 +1,7 @@ [ - // Specifies Authentication context - // false means that IDP choose authentication method - // null force Form based authentication or is possible set via array supported methods. See to onelogin/php-sampl/advance_settings - 'requestedAuthnContext' => env('SAML2_IDP_AUTHNCONTEXT',false), + // SAML2 Authn context + // When set to false no AuthContext will be sent in the AuthNRequest, + // When set to true (Default) you will get an AuthContext 'exact' 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport'. + // Multiple forced values can be passed via a space separated array, For example: + // SAML2_IDP_AUTHNCONTEXT="urn:federation:authentication:windows urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport" + 'requestedAuthnContext' => is_string($SAML2_IDP_AUTHNCONTEXT) ? explode(' ', $SAML2_IDP_AUTHNCONTEXT) : $SAML2_IDP_AUTHNCONTEXT, ], ], diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php index 58c02b471..b6b02e2f7 100644 --- a/tests/Auth/Saml2Test.php +++ b/tests/Auth/Saml2Test.php @@ -28,6 +28,7 @@ class Saml2Test extends TestCase 'saml2.autoload_from_metadata' => false, 'saml2.onelogin.idp.x509cert' => $this->testCert, 'saml2.onelogin.debug' => false, + 'saml2.onelogin.security.requestedAuthnContext' => true, ]); } @@ -328,6 +329,40 @@ class Saml2Test extends TestCase }); } + public function test_login_request_contains_expected_default_authncontext() + { + $authReq = $this->getAuthnRequest(); + $this->assertStringContainsString('samlp:RequestedAuthnContext Comparison="exact"', $authReq); + $this->assertStringContainsString('urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport', $authReq); + } + + public function test_false_idp_authncontext_option_does_not_pass_authncontext_in_saml_request() + { + config()->set(['saml2.onelogin.security.requestedAuthnContext' => false]); + $authReq = $this->getAuthnRequest(); + $this->assertStringNotContainsString('samlp:RequestedAuthnContext', $authReq); + $this->assertStringNotContainsString('', $authReq); + } + + public function test_array_idp_authncontext_option_passes_value_as_authncontextclassref_in_request() + { + config()->set(['saml2.onelogin.security.requestedAuthnContext' => ['urn:federation:authentication:windows', 'urn:federation:authentication:linux']]); + $authReq = $this->getAuthnRequest(); + $this->assertStringContainsString('samlp:RequestedAuthnContext', $authReq); + $this->assertStringContainsString('urn:federation:authentication:windows', $authReq); + $this->assertStringContainsString('urn:federation:authentication:linux', $authReq); + } + + protected function getAuthnRequest(): string + { + $req = $this->post('/saml2/login'); + $location = $req->headers->get('Location'); + $query = explode('?', $location)[1]; + $params = []; + parse_str($query, $params); + return gzinflate(base64_decode($params['SAMLRequest'])); + } + protected function withGet(array $options, callable $callback) { return $this->withGlobal($_GET, $options, $callback); diff --git a/tests/Unit/ConfigTest.php b/tests/Unit/ConfigTest.php index 1d4decc2b..bb475c354 100644 --- a/tests/Unit/ConfigTest.php +++ b/tests/Unit/ConfigTest.php @@ -67,12 +67,22 @@ class ConfigTest extends TestCase $this->checkEnvConfigResult('APP_URL', '', 'session.path', '/'); } + public function test_saml2_idp_authn_context_string_parsed_as_space_separated_array() + { + $this->checkEnvConfigResult( + 'SAML2_IDP_AUTHNCONTEXT', + 'urn:federation:authentication:windows urn:federation:authentication:linux', + 'saml2.onelogin.security.requestedAuthnContext', + ['urn:federation:authentication:windows', 'urn:federation:authentication:linux'] + ); + } + /** * Set an environment variable of the given name and value * then check the given config key to see if it matches the given result. * Providing a null $envVal clears the variable. */ - protected function checkEnvConfigResult(string $envName, ?string $envVal, string $configKey, string $expectedResult) + protected function checkEnvConfigResult(string $envName, ?string $envVal, string $configKey, mixed $expectedResult) { $this->runWithEnv($envName, $envVal, function() use ($configKey, $expectedResult) { $this->assertEquals($expectedResult, config($configKey));