From 0a6f9cfdae7365481607dc30389b68f5111b4041 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaime=20Pe=CC=81rez?= <jaime.perez@uninett.no>
Date: Mon, 8 Aug 2016 12:06:58 +0200
Subject: [PATCH] bugfix: Make sure we log the user out before
reauthenticating.
When acting as a proxy, SimpleSAMLphp was re-authenticating the user in case the IdP that authenticated a user in a valid session was not included in the list of IdPs provided by an SP asking for authentication. Since we cannot use Single Sign On there, we should ask the user to logout before authenticating again, avoiding an inconsistent session with SPs associated to different IdPs.
This resolves #84.
---
.../saml/dictionaries/proxy.definition.json | 8 ++
.../saml/dictionaries/proxy.translation.json | 8 ++
modules/saml/lib/Auth/Source/SP.php | 109 ++++++++++++++++--
.../saml/templates/proxy/invalid_session.php | 32 +++++
modules/saml/www/proxy/invalid_session.php | 69 +++++++++++
5 files changed, 214 insertions(+), 12 deletions(-)
create mode 100644 modules/saml/dictionaries/proxy.definition.json
create mode 100644 modules/saml/dictionaries/proxy.translation.json
create mode 100644 modules/saml/templates/proxy/invalid_session.php
create mode 100644 modules/saml/www/proxy/invalid_session.php
diff --git a/modules/saml/dictionaries/proxy.definition.json b/modules/saml/dictionaries/proxy.definition.json
new file mode 100644
index 000000000..938c73fc5
--- /dev/null
+++ b/modules/saml/dictionaries/proxy.definition.json
@@ -0,0 +1,8 @@
+{
+ "invalid_idp": {
+ "en": "Invalid Identity Provider"
+ },
+ "invalid_idp_description": {
+ "en": "You already have a valid session with an identity provider (<em>%IDP%</em>) that is not accepted by <em>%SP%</em>. Would you like to log out from your existing session and log in again with another identity provider?"
+ }
+}
diff --git a/modules/saml/dictionaries/proxy.translation.json b/modules/saml/dictionaries/proxy.translation.json
new file mode 100644
index 000000000..c78ea653f
--- /dev/null
+++ b/modules/saml/dictionaries/proxy.translation.json
@@ -0,0 +1,8 @@
+{
+ "invalid_idp": {
+ "es": "Proveedor de Identidad inválido"
+ },
+ "invalid_idp_description": {
+ "es": "Ya existe una sesión válida con un proveedor de identidad (<em>%IDP%</em>) que <em>%SP%</em> no acepta. ¿Desea cerrar su sesión actual e iniciar una nueva con otro proveedor de identidad?"
+ }
+}
\ No newline at end of file
diff --git a/modules/saml/lib/Auth/Source/SP.php b/modules/saml/lib/Auth/Source/SP.php
index 40076712e..4a091fdb5 100644
--- a/modules/saml/lib/Auth/Source/SP.php
+++ b/modules/saml/lib/Auth/Source/SP.php
@@ -418,24 +418,84 @@ class sspmod_saml_Auth_Source_SP extends SimpleSAML_Auth_Source {
// check if we have an IDPList specified in the request
if (isset($state['saml:IDPList']) && sizeof($state['saml:IDPList']) > 0 &&
- !in_array($state['saml:sp:IdP'], $state['saml:IDPList'], TRUE)) {
+ !in_array($state['saml:sp:IdP'], $state['saml:IDPList'], true))
+ {
/*
- * This is essentially wrong. The IdP used to authenticate the current session is not in the IDPList
- * that we just received, so we are triggering authentication again against an IdP in the IDPList. This
- * is fine if the user wants to, but we SHOULD offer the user to logout before proceeding.
- *
- * After successful authentication in a different IdP, the reauthPostLogin callback will be invoked,
- * overriding the current session with a new one, associated with the new IdP. This will leave us in an
- * inconsistent state, with several service providers with valid sessions they got from different IdPs.
- *
- * TODO: we need to offer the user the possibility to logout before blindly authenticating him again.
+ * The user has an existing, valid session. However, the SP provided a list of IdPs it accepts for
+ * authentication, and the IdP the existing session is related to is not in that list. We need to
+ * inform the user, and ask whether we should logout before starting the authentication process again
+ * with a different IdP, or cancel the current SSO attempt.
*/
- $state['LoginCompletedHandler'] = array('sspmod_saml_Auth_Source_SP', 'reauthPostLogin');
- $this->authenticate($state);
+ SimpleSAML\Logger::warning(
+ "Reauthentication after logout is needed. The IdP '${state['saml:sp:IdP']}' is not in the IDPList ".
+ "provided by the Service Provider '${state['core:SP']}'."
+ );
+
+ $state['saml:sp:IdPMetadata'] = $this->getIdPMetadata($state['saml:sp:IdP']);
+ $state['saml:sp:AuthId'] = $this->authId;
+ self::askForIdPChange($state);
}
}
+ /**
+ * Ask the user to log out before being able to log in again with a different identity provider. Note that this
+ * method is intended for instances of SimpleSAMLphp running as a SAML proxy, and therefore acting both as an SP
+ * and an IdP at the same time.
+ *
+ * This method will never return.
+ *
+ * @param array $state The state array. The following keys must be defined in the array:
+ * - 'saml:sp:IdPMetadata': a SimpleSAML_Configuration object containing the metadata of the IdP that authenticated
+ * the user in the current session.
+ * - 'saml:sp:AuthId': the identifier of the current authentication source.
+ * - 'core:IdP': the identifier of the local IdP.
+ * - 'SPMetadata': an array with the metadata of this local SP.
+ *
+ * @throws SimpleSAML_Error_NoPassive In case the authentication request was passive.
+ */
+ public static function askForIdPChange(array &$state)
+ {
+ assert('array_key_exists("saml:sp:IdPMetadata", $state)');
+ assert('array_key_exists("saml:sp:AuthId", $state)');
+ assert('array_key_exists("core:IdP", $state)');
+ assert('array_key_exists("SPMetadata", $state)');
+
+ if (isset($state['isPassive']) && (bool)$state['isPassive']) {
+ // passive request, we cannot authenticate the user
+ throw new SimpleSAML_Error_NoPassive('Reauthentication required');
+ }
+
+ // save the state WITHOUT a restart URL, so that we don't try an IdP-initiated login if something goes wrong
+ $id = SimpleSAML_Auth_State::saveState($state, 'saml:proxy:invalid_idp', true);
+ $url = SimpleSAML\Module::getModuleURL('saml/proxy/invalid_session.php');
+ SimpleSAML\Utils\HTTP::redirectTrustedURL($url, array('AuthState' => $id));
+ assert('false');
+ }
+
+
+ /**
+ * Log the user out before logging in again.
+ *
+ * This method will never return.
+ *
+ * @param array $state The state array.
+ */
+ public static function reauthLogout(array $state)
+ {
+ SimpleSAML\Logger::debug('Proxy: logging the user out before re-authentication.');
+
+ if (isset($state['Responder'])) {
+ $state['saml:proxy:reauthLogout:PrevResponder'] = $state['Responder'];
+ }
+ $state['Responder'] = array('sspmod_saml_Auth_Source_SP', 'reauthPostLogout');
+
+ $idp = SimpleSAML_IdP::getByState($state);
+ $idp->handleLogoutRequest($state, null);
+ assert('false');
+ }
+
+
/**
* Complete login operation after re-authenticating the user on another IdP.
*
@@ -455,6 +515,31 @@ class sspmod_saml_Auth_Source_SP extends SimpleSAML_Auth_Source {
}
+ /**
+ * Post-logout handler for re-authentication.
+ *
+ * This method will never return.
+ *
+ * @param SimpleSAML_IdP $idp The IdP we are logging out from.
+ * @param array &$state The state array with the state during logout.
+ */
+ public static function reauthPostLogout(SimpleSAML_IdP $idp, array $state) {
+ assert('isset($state["saml:sp:AuthId"])');
+
+ SimpleSAML\Logger::debug('Proxy: logout completed.');
+
+ if (isset($state['saml:proxy:reauthLogout:PrevResponder'])) {
+ $state['Responder'] = $state['saml:proxy:reauthLogout:PrevResponder'];
+ }
+
+ $sp = SimpleSAML_Auth_Source::getById($state['saml:sp:AuthId'], 'sspmod_saml_Auth_Source_SP');
+ /** @var sspmod_saml_Auth_Source_SP $authSource */
+ SimpleSAML\Logger::debug('Proxy: logging in again.');
+ $sp->authenticate($state);
+ assert('false');
+ }
+
+
/**
* Start a SAML 2 logout operation.
*
diff --git a/modules/saml/templates/proxy/invalid_session.php b/modules/saml/templates/proxy/invalid_session.php
new file mode 100644
index 000000000..113137998
--- /dev/null
+++ b/modules/saml/templates/proxy/invalid_session.php
@@ -0,0 +1,32 @@
+<?php
+/**
+ * Template to ask a user whether to logout because of a reauthentication or not.
+ *
+ * @var SimpleSAML_XHTML_Template $this
+ *
+ * @author Jaime PĂ©rez Crespo, UNINETT AS <jaime.perez@uninett.no>
+ *
+ * @package SimpleSAMLphp
+ */
+
+if (!isset($this->data['head'])) {
+ $this->data['head'] = '';
+}
+$this->includeAtTemplateBase('includes/header.php');
+
+$translator = $this->getTranslator();
+
+$params = array(
+ '%IDP%' => $this->data['idp_name'],
+ '%SP%' => $this->data['sp_name'],
+);
+?>
+ <h2><?php echo $translator->t('{saml:proxy:invalid_idp}'); ?></h2>
+ <p><?php echo $translator->t('{saml:proxy:invalid_idp_description}', $params); ?></p>
+ <form method="post" action="?">
+ <input type="hidden" name="AuthState" value="<?php echo htmlspecialchars($this->data['AuthState']); ?>" />
+ <input type="submit" name="continue" value="<?php echo $translator->t('{general:yes_continue}'); ?>" />
+ <input type="submit" name="cancel" value="<?php echo $translator->t('{general:no_cancel}'); ?>" />
+ </form>
+<?php
+$this->includeAtTemplateBase('includes/footer.php');
diff --git a/modules/saml/www/proxy/invalid_session.php b/modules/saml/www/proxy/invalid_session.php
new file mode 100644
index 000000000..ac38a65d7
--- /dev/null
+++ b/modules/saml/www/proxy/invalid_session.php
@@ -0,0 +1,69 @@
+<?php
+
+/**
+ * This file will handle the case of a user with an existing session that's not valid for a specific Service Provider,
+ * since the authenticating IdP is not in the list of IdPs allowed by the SP.
+ *
+ * @author Jaime PĂ©rez Crespo, UNINETT AS <jaime.perez@uninett.no>
+ *
+ * @package SimpleSAMLphp
+ */
+
+// retrieve the authentication state
+if (!array_key_exists('AuthState', $_REQUEST)) {
+ throw new SimpleSAML_Error_BadRequest('Missing mandatory parameter: AuthState');
+}
+
+try {
+ // try to get the state
+ $state = SimpleSAML_Auth_State::loadState($_REQUEST['AuthState'], 'saml:proxy:invalid_idp');
+} catch (Exception $e) {
+ // the user probably hit the back button after starting the logout, try to recover the state with another stage
+ $state = SimpleSAML_Auth_State::loadState($_REQUEST['AuthState'], 'core:Logout:afterbridge');
+
+ // success! Try to continue with reauthentication, since we no longer have a valid session here
+ $idp = SimpleSAML_IdP::getById($state['core:IdP']);
+ sspmod_saml_Auth_Source_SP::reauthPostLogout($idp, $state);
+}
+
+if (isset($_POST['cancel'])) {
+ // the user does not want to logout, cancel login
+ $e = new SimpleSAML_Error_Exception('User refused to reauthenticate with any of the IdPs requested.');
+ sspmod_saml_IdP_SAML2::handleAuthError($e, $state);
+}
+
+if (isset($_POST['continue'])) {
+ // log the user out before being able to login again
+ $as = SimpleSAML_Auth_Source::getById($state['saml:sp:AuthId'], 'sspmod_saml_Auth_Source_SP');
+ /** @var sspmod_saml_Auth_Source_SP $as */
+ $as->reauthLogout($state);
+}
+
+$cfg = SimpleSAML_Configuration::getInstance();
+$template = new SimpleSAML_XHTML_Template($cfg, 'saml:proxy/invalid_session.php');
+$translator = $template->getTranslator();
+$template->data['AuthState'] = (string)$_REQUEST['AuthState'];
+
+// get the name of the IdP
+$idpmdcfg = $state['saml:sp:IdPMetadata'];
+/** @var SimpleSAML_Configuration $idpmdcfg */
+$idpmd = $idpmdcfg->toArray();
+if (array_key_exists('name', $idpmd)) {
+ $template->data['idp_name'] = $translator->getPreferredTranslation($idpmd['name']);
+} elseif (array_key_exists('OrganizationDisplayName', $idpmd)) {
+ $template->data['idp_name'] = $translator->getPreferredTranslation($idpmd['OrganizationDisplayName']);
+} else {
+ $template->data['idp_name'] = $idpmd['entityid'];
+}
+
+// get the name of the SP
+$spmd = $state['SPMetadata'];
+if (array_key_exists('name', $spmd)) {
+ $template->data['sp_name'] = $translator->getPreferredTranslation($spmd['name']);
+} elseif (array_key_exists('OrganizationDisplayName', $spmd)) {
+ $template->data['sp_name'] = $translator->getPreferredTranslation($spmd['OrganizationDisplayName']);
+} else {
+ $template->data['sp_name'] = $spmd['entityid'];
+}
+
+$template->show();
--
GitLab