From 46455967b31a76a606b0c9430b71a0678d07ecd5 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne
Date: Wed, 7 Mar 2018 17:19:51 +0100
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 @@
{% 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 a7778d7..748ef14 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 as 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.14.2