From e73e73464e0bcf0e73bc8bdb54667d6af95c5dd9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaime=20Pe=CC=81rez=20Crespo?= <jaime.perez@uninett.no>
Date: Thu, 20 Dec 2018 17:32:34 +0100
Subject: [PATCH] Do not store the username and password obtained via ECP in
the state array.
---
modules/core/lib/Auth/UserPassBase.php | 19 +++++--
modules/saml/lib/IdP/SAML2.php | 17 ------
.../core/lib/Auth/UserPassBaseTest.php | 55 +++++++++++++++++--
tests/modules/saml/lib/IdP/SAML2Test.php | 37 -------------
4 files changed, 63 insertions(+), 65 deletions(-)
diff --git a/modules/core/lib/Auth/UserPassBase.php b/modules/core/lib/Auth/UserPassBase.php
index 5f0ee4ffa..847334466 100644
--- a/modules/core/lib/Auth/UserPassBase.php
+++ b/modules/core/lib/Auth/UserPassBase.php
@@ -192,11 +192,20 @@ abstract class UserPassBase extends \SimpleSAML\Auth\Source
$state['forcedUsername'] = $this->forcedUsername;
}
- // ECP requests supply authentication credentials with the AUthnRequest
- // so we validate them now rather than redirecting
- if (isset($state['core:auth:username']) && isset($state['core:auth:password'])) {
- $username = $state['core:auth:username'];
- $password = $state['core:auth:password'];
+ // ECP requests supply authentication credentials with the AuthnRequest
+ // so we validate them now rather than redirecting. The SAML spec
+ // doesn't define how the credentials are transferred, but Office 365
+ // uses the Authorization header, so we will just use that in lieu of
+ // other use cases.
+ if (isset($state['saml:Binding']) && $state['saml:Binding'] === \SAML2\Constants::BINDING_PAOS) {
+ if (!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])) {
+ \SimpleSAML\Logger::error("ECP AuthnRequest did not contain Basic Authentication header");
+ // TODO Return a SOAP fault instead of using the current binding?
+ throw new \SimpleSAML\Error\Error("WRONGUSERPASS");
+ }
+
+ $username = $_SERVER['PHP_AUTH_USER'];
+ $password = $_SERVER['PHP_AUTH_PW'];
if (isset($state['forcedUsername'])) {
$username = $state['forcedUsername'];
diff --git a/modules/saml/lib/IdP/SAML2.php b/modules/saml/lib/IdP/SAML2.php
index f530451e0..1c787412d 100644
--- a/modules/saml/lib/IdP/SAML2.php
+++ b/modules/saml/lib/IdP/SAML2.php
@@ -455,26 +455,9 @@ class SAML2
'saml:RequestedAuthnContext' => $authnContext,
];
- // ECP AuthnRequests need to supply credentials
- if ($binding instanceof SOAP) {
- self::processSOAPAuthnRequest($state);
- }
-
$idp->handleAuthenticationRequest($state);
}
- public static function processSOAPAuthnRequest(array &$state)
- {
- if (!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])) {
- Logger::error("ECP AuthnRequest did not contain Basic Authentication header");
- // TODO Throw some sort of ECP-specific exception / convert this to SOAP fault
- throw new \SimpleSAML\Error\Error("WRONGUSERPASS");
- }
-
- $state['core:auth:username'] = $_SERVER['PHP_AUTH_USER'];
- $state['core:auth:password'] = $_SERVER['PHP_AUTH_PW'];
- }
-
/**
* Send a logout request to a given association.
*
diff --git a/tests/modules/core/lib/Auth/UserPassBaseTest.php b/tests/modules/core/lib/Auth/UserPassBaseTest.php
index cd5a328ac..f477ad2ab 100644
--- a/tests/modules/core/lib/Auth/UserPassBaseTest.php
+++ b/tests/modules/core/lib/Auth/UserPassBaseTest.php
@@ -6,11 +6,13 @@ class UserPassBaseTest extends \PHPUnit_Framework_TestCase
{
public function testAuthenticateECPCallsLoginAndSetsAttributes()
{
- $state = [];
+ $state = [
+ 'saml:Binding' => \SAML2\Constants::BINDING_PAOS,
+ ];
$attributes = ['attrib' => 'val'];
- $username = $state['core:auth:username'] = 'username';
- $password = $state['core:auth:password'] = 'password';
+ $username = $_SERVER['PHP_AUTH_USER'] = 'username';
+ $password = $_SERVER['PHP_AUTH_PW'] = 'password';
$stub = $this->getMockBuilder('\SimpleSAML\Module\core\Auth\UserPassBase')
->disableOriginalConstructor()
@@ -27,15 +29,56 @@ class UserPassBaseTest extends \PHPUnit_Framework_TestCase
$this->assertSame($attributes, $state['Attributes']);
}
+
+ public function testAuthenticateECPMissingUsername()
+ {
+ $this->setExpectedException('\SimpleSAML\Error\Error', 'WRONGUSERPASS');
+
+ $state = [
+ 'saml:Binding' => \SAML2\Constants::BINDING_PAOS,
+ ];
+
+ unset($_SERVER['PHP_AUTH_USER']);
+ $_SERVER['PHP_AUTH_PW'] = 'password';
+
+ $stub = $this->getMockBuilder('\SimpleSAML\Module\core\Auth\UserPassBase')
+ ->disableOriginalConstructor()
+ ->getMockForAbstractClass();
+
+ $stub->authenticate($state);
+ }
+
+
+ public function testAuthenticateECPMissingPassword()
+ {
+ $this->setExpectedException('\SimpleSAML\Error\Error', 'WRONGUSERPASS');
+
+ $state = [
+ 'saml:Binding' => \SAML2\Constants::BINDING_PAOS,
+ ];
+
+ $_SERVER['PHP_AUTH_USER'] = 'username';
+ unset($_SERVER['PHP_AUTH_PW']);
+
+ $stub = $this->getMockBuilder('\SimpleSAML\Module\core\Auth\UserPassBase')
+ ->disableOriginalConstructor()
+ ->getMockForAbstractClass();
+
+ $stub->authenticate($state);
+ }
+
+
public function testAuthenticateECPCallsLoginWithForcedUsername()
{
- $state = [];
+ $state = [
+ 'saml:Binding' => \SAML2\Constants::BINDING_PAOS,
+ ];
$attributes = [];
$forcedUsername = 'forcedUsername';
- $state['core:auth:username'] = 'username';
- $password = $state['core:auth:password'] = 'password';
+ $_SERVER['PHP_AUTH_USER'] = 'username';
+ $password = $_SERVER['PHP_AUTH_PW'] = 'password';
$stub = $this->getMockBuilder('\SimpleSAML\Module\core\Auth\UserPassBase')
->disableOriginalConstructor()
diff --git a/tests/modules/saml/lib/IdP/SAML2Test.php b/tests/modules/saml/lib/IdP/SAML2Test.php
index dbd026097..607234a17 100644
--- a/tests/modules/saml/lib/IdP/SAML2Test.php
+++ b/tests/modules/saml/lib/IdP/SAML2Test.php
@@ -9,43 +9,6 @@ use SimpleSAML\Test\Utils\ClearStateTestCase;
class SAML2Test extends ClearStateTestCase
{
- public function testProcessSOAPAuthnRequest()
- {
- $username = $_SERVER['PHP_AUTH_USER'] = 'username';
- $password = $_SERVER['PHP_AUTH_PW'] = 'password';
- $state = [];
-
- \SimpleSAML\Module\saml\IdP\SAML2::processSOAPAuthnRequest($state);
-
- $this->assertEquals($username, $state['core:auth:username']);
- $this->assertEquals($password, $state['core:auth:password']);
- }
-
- public function testProcessSOAPAuthnRequestMissingUsername()
- {
- $this->setExpectedException('\SimpleSAML\Error\Error', 'WRONGUSERPASS');
-
- $_SERVER['PHP_AUTH_PW'] = 'password';
- unset($_SERVER['PHP_AUTH_USER']);
- $state = [];
- Configuration::loadFromArray([
- 'baseurlpath' => 'https://idp.example.com/',
- ], '', 'simplesaml');
-
- \SimpleSAML\Module\saml\IdP\SAML2::processSOAPAuthnRequest($state);
- }
-
- public function testProcessSOAPAuthnRequestMissingPassword()
- {
- $this->setExpectedException('\SimpleSAML\Error\Error', 'WRONGUSERPASS');
-
- $_SERVER['PHP_AUTH_USER'] = 'username';
- unset($_SERVER['PHP_AUTH_PW']);
- $state = [];
-
- \SimpleSAML\Module\saml\IdP\SAML2::processSOAPAuthnRequest($state);
- }
-
/**
* Default values for the state array expected to be generated at the start of logins
* @var array
--
GitLab