From 1e146c3e5a4f9c2501a6db038bde2e8916986db7 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Wed, 6 Jul 2022 19:06:03 +0200 Subject: [PATCH 2/4] misc: improve passive sso on state change (#67090) - automatic_sso is renamed try_passive_sso to be clearer on the goal of the method, - test for possible passive sso is now done before rendering the current page, - on a succesfull SSO if cookie is present, its value is saved in the Quixote session, - behaviour of try_passive_sso is changed: - if user is logged in or cookie value differs from the value in '*-passive-auth-tried' cookie, the '*-passive-auth-tried' cookie is expired, - if user is logged and cookie cookie value differs from the one saved in the Quixote session, user is logged out, if not treatment stop here. - if the cookie cookie is not valued or if its value is equal to '*-passive-auth-tried' cookie, treatment stops here, - if the cookie is valued and its value differs from the '*-passive-auth-tried' cookie cookie value, then '*-passive-auth-tried' cookie is set to the value of cookie and a passive SSO is tried. --- tests/test_saml_auth.py | 40 ++++++++++++++++++- wcs/qommon/saml2.py | 5 +++ wcs/qommon/sessions.py | 1 + wcs/root.py | 87 +++++++++++++++++++++++++---------------- 4 files changed, 99 insertions(+), 34 deletions(-) diff --git a/tests/test_saml_auth.py b/tests/test_saml_auth.py index ca80813ad..c09f96831 100644 --- a/tests/test_saml_auth.py +++ b/tests/test_saml_auth.py @@ -22,6 +22,7 @@ from wcs.qommon.ident.idp import MethodAdminDirectory from wcs.qommon.misc import get_lasso_server from wcs.qommon.saml2 import Saml2Directory, SOAPException +from .test_fc_auth import get_session from .test_hobo_notify import PROFILE from .utilities import clean_temporary_pub, create_temporary_pub, get_app @@ -604,7 +605,6 @@ def test_opened_session_cookie(pub): cookie_store = http.cookies.SimpleCookie() cookie_store.load(resp.headers['Set-Cookie']) assert list(cookie_store.keys()) == [cookie_name] - assert 'Secure' in resp.headers['Set-Cookie'] assert 'HttpOnly' in resp.headers['Set-Cookie'] assert 'SameSite=None' in resp.headers['Set-Cookie'] assert 'Path=/' in resp.headers['Set-Cookie'] @@ -615,6 +615,44 @@ def test_opened_session_cookie(pub): ) assert cookie_name in app.cookies + # if we try again, no passive authentication occurs + resp = app.get('/?parameter=value') + assert resp.status_int != 302 + + # if IDP_OPENED_SESSION is modified, then passive authentication is tried again + app.set_cookie('IDP_OPENED_SESSION', '2') + resp = app.get('/?parameter=value') + assert resp.status_int == 302 + + # simulate a saml login + user = pub.user_class() + user.store() + request = mock.Mock() + request.get_environ.return_value = '1.1.1.1' + with mock.patch('quixote.session.get_request', return_value=request), mock.patch( + 'wcs.qommon.saml2', return_value=mock.Mock(cookies={'IDP_OPENED_SESSION': '2'}) + ): + session = get_session_manager().session_class(id=None) + session.set_user(user.id) + session.opened_session_value = '2' + session.id = 'abcd' + session.store() + app.set_cookie(pub.config.session_cookie_name, session.id) + assert get_session(app).opened_session_value == '2' + + resp = app.get('/?parameter=value') + assert resp.status_int == 200 + assert get_session(app).opened_session_value == '2' + assert get_session(app).user == user.id + # '*-passive-auth-tried' cookie was removed, since we logged in. + assert cookie_name not in app.cookies + + # if the IDP_OPENED_SESSION cookie change then we are logged out + app.set_cookie('IDP_OPENED_SESSION', '3') + resp = app.get('/?parameter=value') + assert not get_session(app) + assert not get_session_manager().session_class.get(session.id, ignore_errors=True) + def test_no_opened_session_cookie(pub): app = get_app(pub) diff --git a/wcs/qommon/saml2.py b/wcs/qommon/saml2.py index 22766cc45..1a1b543b5 100644 --- a/wcs/qommon/saml2.py +++ b/wcs/qommon/saml2.py @@ -377,6 +377,11 @@ class Saml2Directory(Directory): user = self.lookup_user(session, login) if user: session.set_user(user.id) + # save value of idp_session_cookie_name for wcs.root.RootDirectory.try_passive_sso() + idp_session_cookie_name = get_publisher().get_site_option('idp_session_cookie_name') + if idp_session_cookie_name: + if idp_session_cookie_name in get_request().cookies: + session.opened_session_value = get_request().cookies[idp_session_cookie_name] else: return error_page('Error associating user on SSO') session.lasso_identity_provider_id = login.remoteProviderId diff --git a/wcs/qommon/sessions.py b/wcs/qommon/sessions.py index 544fe090d..2ab044ddd 100644 --- a/wcs/qommon/sessions.py +++ b/wcs/qommon/sessions.py @@ -88,6 +88,7 @@ class Session(QommonSession, CaptchaSession, StorableObject): forced = False # should only be overwritten by authentication methods extra_user_variables = None + opened_session_value = None username = None # only set on password authentication diff --git a/wcs/root.py b/wcs/root.py index 07fc281cd..2722bb24d 100644 --- a/wcs/root.py +++ b/wcs/root.py @@ -287,6 +287,10 @@ class RootDirectory(Directory): self.forced_language = False self.feed_substitution_parts() + output = self.try_passive_sso() + if output: + return output + response = get_response() if not hasattr(response, 'filter'): response.filter = {} @@ -304,43 +308,60 @@ class RootDirectory(Directory): except errors.TraversalError: pass - output = root.RootDirectory()._q_traverse(path) - return self.automatic_sso(output) + return root.RootDirectory()._q_traverse(path) + + def try_passive_sso(self): + publisher = get_publisher() + idp_session_cookie_name = publisher.get_site_option('idp_session_cookie_name') + passive_tried_cookie_name = '%s-passive-auth-tried' % publisher.config.session_cookie_name + + if not idp_session_cookie_name: + return + ident_methods = get_cfg('identification', {}).get('methods', []) + idps = get_cfg('idp', {}) + if len(idps) != 1: + return + if ident_methods and 'idp' not in ident_methods: + return - def automatic_sso(self, output): request = get_request() + cookies = request.cookies response = get_response() - publisher = get_publisher() - OPENED_SESSION_COOKIE = publisher.get_site_option('idp_session_cookie_name') - PASSIVE_TRIED_COOKIE = '%s-passive-auth-tried' % publisher.config.session_cookie_name - if OPENED_SESSION_COOKIE not in request.cookies and PASSIVE_TRIED_COOKIE in request.cookies: - response.expire_cookie(PASSIVE_TRIED_COOKIE) - return output - elif OPENED_SESSION_COOKIE in request.cookies and PASSIVE_TRIED_COOKIE not in request.cookies: - ident_methods = get_cfg('identification', {}).get('methods', []) - idps = get_cfg('idp', {}) - if request.user: - return output - if len(idps) != 1: - return output - if ident_methods and 'idp' not in ident_methods: - return output - response.set_cookie( - PASSIVE_TRIED_COOKIE, - '1', - secure=1, - httponly=1, - path=publisher.config.session_cookie_path, - domain=publisher.config.session_cookie_domain, - ) - url = request.get_url() - query = request.get_query() - if query: - url += '?' + query - return root.tryauth(url) - else: - return output + # expire passive_tried_cookie_name if already logged or if not equal to passive_tried_cookie_name + if passive_tried_cookie_name in cookies and ( + request.user or cookies.get(passive_tried_cookie_name) != cookies.get(idp_session_cookie_name) + ): + response.expire_cookie(passive_tried_cookie_name) + + if request.user: + if request.session.opened_session_value and request.session.opened_session_value != cookies.get( + idp_session_cookie_name + ): + # logout current user if saved value for idp_session_cookie_name differs from the current one + get_session_manager().expire_session() + get_request()._user = () + else: + # already logged, stop here. + return + if idp_session_cookie_name not in cookies or cookies.get(idp_session_cookie_name) == cookies.get( + passive_tried_cookie_name + ): + # no session on the idp or passive sso already tried, stop here. + return + response.set_cookie( + passive_tried_cookie_name, + cookies.get(idp_session_cookie_name), + secure=request.scheme == 'https', + httponly=1, + path=publisher.config.session_cookie_path, + domain=publisher.config.session_cookie_domain, + ) + url = request.get_url() + query = request.get_query() + if query: + url += '?' + query + return root.tryauth(url) def _q_lookup(self, component): if ( -- 2.37.2