From 5eb6adecd2aad1bc2a9415c84a81f4e5c4bacf7d Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Thu, 12 Oct 2017 12:12:31 +0200 Subject: [PATCH] prevent redirection loop on artifact resolution errors (fixes #14810) Signature of method sso_failure() is changed as name of a context variable for template mellon/authentication_failed.html (idp_message => reason). --- mellon/locale/fr/LC_MESSAGES/django.po | 9 +--- mellon/templates/mellon/authentication_failed.html | 2 +- mellon/views.py | 39 +++++++++----- tests/test_sso_slo.py | 62 +++++++++++++++++++--- 4 files changed, 86 insertions(+), 26 deletions(-) diff --git a/mellon/locale/fr/LC_MESSAGES/django.po b/mellon/locale/fr/LC_MESSAGES/django.po index 0f37e69..9d7c279 100644 --- a/mellon/locale/fr/LC_MESSAGES/django.po +++ b/mellon/locale/fr/LC_MESSAGES/django.po @@ -86,10 +86,5 @@ msgstr "Aucun utilisateur trouvé pour l'identifiant SAML %(name_id)s" msgid "IdP is temporarily down, please try again later." msgstr "Le fournisseur d'identités est temporairement inaccessible, veuillez réessayer plus tard." -#~ msgid "" -#~ "The authentication has failed, you can return to\n" -#~ " the last page you where.\n" -#~ " " -#~ msgstr "" -#~ "L'authentification a échoué, vous pouvez retourner à la dernière page atteinte." +msgid "There were too many redirections with the identity provider." +msgstr "Il y a eu trop de redirections avec le fournisseur d'identité." diff --git a/mellon/templates/mellon/authentication_failed.html b/mellon/templates/mellon/authentication_failed.html index a32853b..19200a7 100644 --- a/mellon/templates/mellon/authentication_failed.html +++ b/mellon/templates/mellon/authentication_failed.html @@ -12,7 +12,7 @@

{% trans "Authentication failed" %}

{% blocktrans %}The authentication has failed.{% endblocktrans %} - {% if idp_message %}

{% trans "Reason" %} : {{ idp_message }}

{% endif %} + {% if reason %}

{% trans "Reason" %} : {{ reason }}

{% endif %}

{% trans "Continue" %} diff --git a/mellon/views.py b/mellon/views.py index e01dc13..dc5eeca 100644 --- a/mellon/views.py +++ b/mellon/views.py @@ -19,6 +19,7 @@ from django.utils.translation import ugettext as _ from . import app_settings, utils +RETRY_LOGIN_COOKIE = 'MELLON_RETRY_LOGIN' lasso.setFlag('thin-sessions') @@ -126,24 +127,24 @@ class LoginView(ProfileMixin, LogMixin, View): if 'RelayState' in request.POST and utils.is_nonnull(request.POST['RelayState']): login.msgRelayState = request.POST['RelayState'] return self.sso_success(request, login) - return self.sso_failure(request, login, idp_message, status_codes) + return self.sso_failure(request, reason=idp_message, status_codes=status_codes) - def sso_failure(self, request, login, idp_message, status_codes): + def sso_failure(self, request, reason='', status_codes=()): '''show error message to user after a login failure''' + login = self.profile idp = utils.get_idp(login.remoteProviderId) error_url = utils.get_setting(idp, 'ERROR_URL') error_redirect_after_timeout = utils.get_setting(idp, 'ERROR_REDIRECT_AFTER_TIMEOUT') if error_url: error_url = resolve_url(error_url) - next_url = error_url or login.msgRelayState or resolve_url(settings.LOGIN_REDIRECT_URL) + next_url = error_url or self.get_next_url(default=resolve_url(settings.LOGIN_REDIRECT_URL)) return render(request, 'mellon/authentication_failed.html', { 'debug': settings.DEBUG, - 'idp_message': idp_message, + 'reason': reason, 'status_codes': status_codes, 'issuer': login.remoteProviderId, 'next_url': next_url, - 'error_url': error_url, 'relaystate': login.msgRelayState, 'error_redirect_after_timeout': error_redirect_after_timeout, }) @@ -186,7 +187,9 @@ class LoginView(ProfileMixin, LogMixin, View): attributes['authn_context_class_ref'] = \ authn_context.authnContextClassRef self.log.debug('trying to authenticate with attributes %r', attributes) - return self.authenticate(request, login, attributes) + response = self.authenticate(request, login, attributes) + response.delete_cookie(RETRY_LOGIN_COOKIE) + return response def authenticate(self, request, login, attributes): user = auth.authenticate(saml_attributes=attributes) @@ -217,12 +220,23 @@ class LoginView(ProfileMixin, LogMixin, View): return HttpResponseRedirect(next_url) def retry_login(self): - '''Retry login if it failed for a temporary error''' + '''Retry login if it failed for a temporary error. + + Use a cookie to prevent looping forever. + ''' + if RETRY_LOGIN_COOKIE in self.request.COOKIES: + response = self.sso_failure( + self.request, + reason=_('There were too many redirections with the identity provider.')) + response.delete_cookie(RETRY_LOGIN_COOKIE) + return response url = reverse('mellon_login') next_url = self.get_next_url() if next_url: url = '%s?%s' % (url, urlencode({REDIRECT_FIELD_NAME: next_url})) - return HttpResponseRedirect(url) + response = HttpResponseRedirect(url) + response.set_cookie(RETRY_LOGIN_COOKIE, value='1', max_age=None) + return response def continue_sso_artifact(self, request, method): idp_message = None @@ -264,12 +278,13 @@ class LoginView(ProfileMixin, LogMixin, View): verify=verify_ssl_certificate) except RequestException, e: self.log.warning('unable to reach %r: %s', login.msgUrl, e) - return self.sso_failure(request, login, _('IdP is temporarily down, please try again ' - 'later.'), status_codes) + return self.sso_failure(request, + reason=_('IdP is temporarily down, please try again ' 'later.'), + status_codes=status_codes) if result.status_code != 200: self.log.warning('SAML authentication failed: IdP returned %s when given artifact: %r', result.status_code, result.content) - return self.sso_failure(request, login, idp_message, status_codes) + return self.sso_failure(request, reason=idp_message, status_codes=status_codes) self.log.info('Got SAML Artifact Response', extra={'saml_response': result.content}) try: @@ -310,7 +325,7 @@ class LoginView(ProfileMixin, LogMixin, View): return HttpResponseBadRequest('error processing the authentication response: %r' % e) else: return self.sso_success(request, login) - return self.sso_failure(request, login, idp_message, status_codes) + return self.sso_failure(request, login, reason=idp_message, status_codes=status_codes) def request_discovery_service(self, request, is_passive=False): self_url = request.build_absolute_uri(request.path) diff --git a/tests/test_sso_slo.py b/tests/test_sso_slo.py index 795e53d..9c70889 100644 --- a/tests/test_sso_slo.py +++ b/tests/test_sso_slo.py @@ -1,4 +1,5 @@ import lasso +from urlparse import urlparse from pytest import fixture @@ -110,7 +111,7 @@ def test_sso_slo(db, app, idp, caplog, sp_settings): response = app.post(reverse('mellon_login'), params={'SAMLResponse': body}) assert 'created new user' in caplog.text assert 'logged in using SAML' in caplog.text - assert response['Location'].endswith(sp_settings.LOGIN_REDIRECT_URL) + assert urlparse(response['Location']).path == sp_settings.LOGIN_REDIRECT_URL def test_sso(db, app, idp, caplog, sp_settings): @@ -120,7 +121,7 @@ def test_sso(db, app, idp, caplog, sp_settings): response = app.post(reverse('mellon_login'), params={'SAMLResponse': body}) assert 'created new user' in caplog.text assert 'logged in using SAML' in caplog.text - assert response['Location'].endswith(sp_settings.LOGIN_REDIRECT_URL) + assert urlparse(response['Location']).path == sp_settings.LOGIN_REDIRECT_URL def test_sso_request_denied(db, app, idp, caplog, sp_settings): @@ -147,16 +148,16 @@ def test_sso_artifact(db, app, caplog, sp_settings, idp_metadata, idp_private_ke response = app.get(acs_artifact_url) assert 'created new user' in caplog.text assert 'logged in using SAML' in caplog.text - assert response['Location'].endswith(sp_settings.LOGIN_REDIRECT_URL) + assert urlparse(response['Location']).path == sp_settings.LOGIN_REDIRECT_URL # force delog + assert app.session app.session.flush() assert 'dead artifact' not in caplog.text with HTTMock(idp.mock_artifact_resolver()): response = app.get(acs_artifact_url) # verify retry login was asked assert 'dead artifact' in caplog.text - assert response.status_code == 302 - assert reverse('mellon_login') in url + assert urlparse(response['Location']).path == reverse('mellon_login') response = response.follow() url, body = idp.process_authn_request_redirect(response['Location']) reset_caplog(caplog) @@ -170,4 +171,53 @@ def test_sso_artifact(db, app, caplog, sp_settings, idp_metadata, idp_private_ke response = app.get(acs_artifact_url) assert 'created new user' in caplog.text assert 'logged in using SAML' in caplog.text - assert response['Location'].endswith(sp_settings.LOGIN_REDIRECT_URL) + assert urlparse(response['Location']).path == sp_settings.LOGIN_REDIRECT_URL + + +def test_sso_artifact_no_loop(db, app, caplog, sp_settings, idp_metadata, idp_private_key, rf): + sp_settings.MELLON_DEFAULT_ASSERTION_CONSUMER_BINDING = 'artifact' + request = rf.get('/') + sp_metadata = create_metadata(request) + idp = MockIdp(idp_metadata, idp_private_key, sp_metadata) + response = app.get(reverse('mellon_login')) + url, body = idp.process_authn_request_redirect(response['Location']) + assert body is None + assert reverse('mellon_login') in url + assert 'SAMLart' in url + acs_artifact_url = url.split('testserver', 1)[1] + + # forget the artifact + idp.artifact = '' + + with HTTMock(idp.mock_artifact_resolver()): + response = app.get(acs_artifact_url) + assert 'MELLON_RETRY_LOGIN=1;' in response['Set-Cookie'] + + # first error, we retry + assert urlparse(response['Location']).path == reverse('mellon_login') + + # check we are not logged + assert not app.session + + # redo + response = app.get(reverse('mellon_login')) + url, body = idp.process_authn_request_redirect(response['Location']) + assert body is None + assert reverse('mellon_login') in url + assert 'SAMLart' in url + acs_artifact_url = url.split('testserver', 1)[1] + + # forget the artifact + idp.artifact = '' + with HTTMock(idp.mock_artifact_resolver()): + response = app.get(acs_artifact_url) + + # check cookie is deleted after failed retry + assert 'MELLON_RETRY_LOGIN=;' in response['Set-Cookie'] + assert 'Location' not in response + + # check we are still not logged + assert not app.session + + # check return url is in page + assert '"%s"' % sp_settings.LOGIN_REDIRECT_URL in response.content -- 2.1.4