From d5ad6ff4409b44e3b1a55f8eae1271e57c4e5872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20P=C3=A9ters?= Date: Wed, 13 Jan 2016 22:09:08 +0100 Subject: [PATCH] general: don't use session for after_url persistence (#5637) --- tests/test_admin_pages.py | 4 ++-- tests/test_backoffice_pages.py | 3 +-- tests/test_form_pages.py | 6 +++--- tests/test_saml_auth.py | 22 ++++++++++++++++++---- wcs/qommon/errors.py | 7 ++++--- wcs/qommon/ident/idp.py | 15 ++++++++------- wcs/qommon/ident/password.py | 12 ++++++++---- wcs/qommon/saml2.py | 18 +++++++++--------- wcs/qommon/sessions.py | 3 +-- wcs/root.py | 9 +++++++-- 10 files changed, 61 insertions(+), 38 deletions(-) diff --git a/tests/test_admin_pages.py b/tests/test_admin_pages.py index 531a33f..2fb2925 100644 --- a/tests/test_admin_pages.py +++ b/tests/test_admin_pages.py @@ -83,8 +83,7 @@ def test_empty_site(pub): def test_with_user(pub): create_superuser(pub) resp = get_app(pub).get('/backoffice/', status=302) - resp = resp.follow() - assert resp.location == 'http://example.net/login/' + assert resp.location == 'http://example.net/login/?next=http%3A%2F%2Fexample.net%2Fbackoffice%2F' def test_with_superuser(pub): app = login(get_app(pub)) @@ -2318,6 +2317,7 @@ def test_settings_idp(pub): from test_saml_auth import setup_environment setup_environment(pub) + pub.user_class.wipe() # makes sure there are no users resp = resp.click('Identity Providers') assert 'http://sso.example.net/' in resp.body diff --git a/tests/test_backoffice_pages.py b/tests/test_backoffice_pages.py index 200e431..9d874d9 100644 --- a/tests/test_backoffice_pages.py +++ b/tests/test_backoffice_pages.py @@ -139,8 +139,7 @@ def teardown_module(module): def test_backoffice_unlogged(pub): create_superuser(pub) resp = get_app(pub).get('/backoffice/', status=302) - resp = resp.follow() - assert resp.location == 'http://example.net/login/' + assert resp.location == 'http://example.net/login/?next=http%3A%2F%2Fexample.net%2Fbackoffice%2F' def test_backoffice_home(pub): create_superuser(pub) diff --git a/tests/test_form_pages.py b/tests/test_form_pages.py index 3cee080..3ecb1ec 100644 --- a/tests/test_form_pages.py +++ b/tests/test_form_pages.py @@ -183,7 +183,7 @@ def test_home_inaccessible(pub): formdef.store() home = get_app(pub).get('/') assert home.status_int == 302 - assert home.location == 'http://example.net/login' + assert home.location == 'http://example.net/login/?next=http%3A%2F%2Fexample.net%2F' def test_home_always_advertise(pub): formdef = create_formdef() @@ -552,10 +552,10 @@ def test_form_visit_existing(pub): formdata_user.store() resp = get_app(pub).get('/test/%s/' % formdata.id) - assert resp.location == 'http://example.net/login' + assert resp.location.startswith('http://example.net/login/?next=') resp = get_app(pub).get('/test/%s/' % formdata_user.id) - assert resp.location == 'http://example.net/login' + assert resp.location.startswith('http://example.net/login/?next=') resp = login(get_app(pub), username='foo', password='foo').get('/test/%s/' % formdata_user.id) assert 'The form has been recorded on' in resp diff --git a/tests/test_saml_auth.py b/tests/test_saml_auth.py index fda8d5b..e71c3f9 100644 --- a/tests/test_saml_auth.py +++ b/tests/test_saml_auth.py @@ -2,6 +2,7 @@ import datetime import os import sys import shutil +import urlparse try: import lasso @@ -80,6 +81,9 @@ def setup_environment(pub, idp_number=1): pub.write_cfg() + pub.user_class.wipe() + pub.user_class().store() + def teardown_module(module): shutil.rmtree(pub.APP_DIR) @@ -159,7 +163,7 @@ def test_assertion_consumer(): body = saml2.assertionConsumerPost() assert req.response.status_code == 303 - assert req.response.headers['location'] == 'http://example.net/' + assert req.response.headers['location'] == 'http://example.net' assert req.session.user is not None def test_assertion_consumer_existing_federation(): @@ -205,11 +209,11 @@ def test_assertion_consumer_existing_federation(): def test_assertion_consumer_redirect_after_url(): setup_environment(pub) req = get_assertion_consumer_request() - req.session.after_url = '/foobar' + req.form['RelayState'] = '/foobar' saml2 = Saml2Directory() saml_response_body = req.form['SAMLResponse'] body = saml2.assertionConsumerPost() - assert req.response.headers['location'] == 'http://example.net/foobar' + assert req.response.headers['location'] == '/foobar' def test_saml_login_page(): setup_environment(pub) @@ -225,6 +229,16 @@ def test_saml_login_page_several_idp(): assert resp.status_int == 302 assert resp.location.startswith('http://sso.example.net/saml2/sso?SAMLRequest=') +def test_saml_backoffice_redirect(): + setup_environment(pub) + resp = get_app(pub).get('/backoffice/') + assert resp.status_int == 302 + assert resp.location.startswith('http://example.net/login/?next=') + resp = resp.follow() + assert resp.location.startswith('http://sso.example.net/saml2/sso') + assert urlparse.parse_qs(urlparse.urlparse(resp.location).query)['SAMLRequest'] + assert urlparse.parse_qs(urlparse.urlparse(resp.location).query)['RelayState'] == ['http://example.net/backoffice/'] + def test_saml_register(): setup_environment(pub) get_app(pub).get('/register/', status=404) @@ -234,7 +248,7 @@ def test_saml_register(): # if there's no specific registration URL, this initiates a SSO and there # should be a registration link on the identity provider resp = get_app(pub).get('/register/') - assert resp.location == 'http://example.net/login/' + assert resp.location == 'http://example.net/login/?next=http%3A%2F%2Fexample.net%2Fregister%2F' resp = resp.follow() assert resp.location.startswith('http://sso.example.net/saml2/sso?SAMLRequest=') diff --git a/wcs/qommon/errors.py b/wcs/qommon/errors.py index 3601631..65bfc38 100644 --- a/wcs/qommon/errors.py +++ b/wcs/qommon/errors.py @@ -14,8 +14,9 @@ # You should have received a copy of the GNU General Public License # along with this program; if not, see . -from quixote import get_publisher +import urllib +from quixote import get_publisher import quixote from quixote.errors import * from quixote.html import TemplateIO, htmltext @@ -50,10 +51,10 @@ class AccessUnauthorizedError(AccessForbiddenError): if request.user: return AccessForbiddenError.render(self) - session.after_url = request.get_frontoffice_url() if self.public_msg: session.message = ('error', self.public_msg) - login_url = get_publisher().get_root_url() + 'login' + login_url = get_publisher().get_root_url() + 'login/' + login_url += '?' + urllib.urlencode({'next': request.get_frontoffice_url()}) quixote.redirect(login_url) class EmailError(Exception): diff --git a/wcs/qommon/ident/idp.py b/wcs/qommon/ident/idp.py index 11ba1c6..8276ce3 100644 --- a/wcs/qommon/ident/idp.py +++ b/wcs/qommon/ident/idp.py @@ -20,6 +20,7 @@ except ImportError: lasso = None from cStringIO import StringIO +import urllib import urllib2 import urlparse @@ -146,12 +147,13 @@ class MethodDirectory(Directory): get_cfg('saml_identities', {}).get('registration-url'), vars) return redirect(registration_url) - get_session().after_url = get_request().get_frontoffice_url() ident_methods = get_cfg('identification', {}).get('methods', []) if len(ident_methods) > 1: - return redirect(get_publisher().get_root_url() + 'login/idp/') + login_url = get_publisher().get_root_url() + 'login/idp/' else: - return redirect(get_publisher().get_root_url() + 'login/') + login_url = get_publisher().get_root_url() + 'login/' + login_url += '?' + urllib.urlencode({'next': get_request().get_frontoffice_url()}) + return redirect(login_url) if not get_request().user.anonymous: raise errors.AccessForbiddenError() @@ -1058,12 +1060,11 @@ class IdPAuthMethod(AuthMethod): def login(self): idps = get_cfg('idp', {}) - if get_response().iframe_mode and not get_session().after_url: - t = get_request().get_url() - get_session().after_url = str('%s://%s%s' % ( + if get_response().iframe_mode and not get_request().form.get('next'): + get_request().form = {'next': '%s://%s%s' % ( get_request().get_scheme(), get_request().get_server(False), - get_publisher().get_root_url())) + get_publisher().get_root_url())} # there is only one visible IdP, perform login automatically on # this one. diff --git a/wcs/qommon/ident/password.py b/wcs/qommon/ident/password.py index 44497f8..3ee9f45 100644 --- a/wcs/qommon/ident/password.py +++ b/wcs/qommon/ident/password.py @@ -195,8 +195,13 @@ class MethodDirectory(Directory): return Directory._q_traverse(self, path) def login(self): + next_url = get_request().form.get('next') + if get_request().get_method() == 'GET': + get_request().form = {} identities_cfg = get_cfg('identities', {}) - form = Form(enctype = 'multipart/form-data', id = 'login-form', use_tokens = False) + form = Form(enctype='multipart/form-data', id='login-form', + use_tokens=False, action=get_request().get_url()) + form.add_hidden('next', next_url) if identities_cfg.get('email-as-username', False): form.add(StringWidget, 'username', title = _('Email'), size=25, required=True) else: @@ -285,9 +290,8 @@ class MethodDirectory(Directory): account.warned_about_unused_account = False account.store() - if session.after_url: - after_url = session.after_url - session.after_url = None + if form and form.get_widget('next').parse(): + after_url = form.get_widget('next').parse() return redirect(after_url) else: return redirect(get_publisher().get_root_url() + get_publisher().after_login_url) diff --git a/wcs/qommon/saml2.py b/wcs/qommon/saml2.py index 2396b36..4159b36 100644 --- a/wcs/qommon/saml2.py +++ b/wcs/qommon/saml2.py @@ -226,6 +226,8 @@ class Saml2Directory(Directory): login.request.forceAuthn = False login.request.isPassive = get_request().form.get('IsPassive') == 'true' login.request.consent = 'urn:oasis:names:tc:SAML:2.0:consent:current-implicit' + login.msgRelayState = get_request().form.get('next') + print 'relay state:', login.msgRelayState login.buildAuthnRequestMsg() return redirect(login.msgUrl) @@ -385,16 +387,14 @@ class Saml2Directory(Directory): relay_state = request.form.get('RelayState', None) session = get_session() response = get_response() - if session.after_url: - after_url = session.after_url - session.after_url = None - return redirect(after_url) - response.set_status(303) - if relay_state == 'backoffice' and get_publisher().backoffice_directory_class: - response.headers['location'] = urlparse.urljoin( - get_request().get_url(), str('../backoffice/')) + if relay_state == 'backoffice': + after_url = get_publisher().get_backoffice_url() + elif relay_state: + after_url = relay_state else: - response.headers['location'] = urlparse.urljoin(get_request().get_url(), str('..')) + after_url = get_publisher().get_frontoffice_url() + response.set_status(303) + response.headers['location'] = after_url response.content_type = 'text/plain' return "Your browser should redirect you" diff --git a/wcs/qommon/sessions.py b/wcs/qommon/sessions.py index 7556f85..b980aee 100644 --- a/wcs/qommon/sessions.py +++ b/wcs/qommon/sessions.py @@ -69,7 +69,6 @@ class Session(QommonSession, CaptchaSession, StorableObject): _names = 'sessions' name_identifier = None - after_url = None lasso_session_dump = None lasso_session_index = None lasso_anonymous_identity_dump = None @@ -104,7 +103,7 @@ class Session(QommonSession, CaptchaSession, StorableObject): return (time.time() - self.get_access_time()) > duration def has_info(self): - return self.name_identifier or self.after_url or \ + return self.name_identifier or \ self.lasso_session_dump or self.message or \ self.lasso_anonymous_identity_dump or \ self.lasso_manage_name_id_dump or \ diff --git a/wcs/root.py b/wcs/root.py index 9382c9d..b4f164e 100644 --- a/wcs/root.py +++ b/wcs/root.py @@ -17,6 +17,7 @@ import json import os import re +import urllib try: from hobo.scrutiny.wsgi.middleware import VersionMiddleware @@ -75,7 +76,7 @@ class LoginDirectory(Directory): ident_methods = get_cfg('identification', {}).get('methods', []) if get_request().form.get('ReturnUrl'): - get_session().after_url = get_request().form.get('ReturnUrl') + get_request().form['next'] = get_request().form.pop('ReturnUrl') if len(ident_methods) == 0: get_logger().info('no ident method defined') @@ -109,7 +110,11 @@ class LoginDirectory(Directory): if form.is_submitted() and not form.has_errors(): method = form.get_widget('method').parse() if get_publisher().ident_methods.get(method)().is_interactive(): - return redirect('../ident/%s/login' % method) + login_url = '../ident/%s/login' % method + if get_request().form.get('next'): + login_url += '?' + urllib.urlencode( + {'next': get_request().form.get('next')}) + return redirect(login_url) else: try: return qommon.ident.login(method) -- 2.7.0.rc3