From 72f303d1466feaf7c9270216c473310dd13f418e Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Mon, 30 Aug 2021 15:25:19 +0200 Subject: [PATCH 1/2] views: use a view to show all error messages (#56354) --- .../mellon/authentication_failed.html | 20 +- mellon/views.py | 227 +++++++++--------- tests/test_sso_slo.py | 22 +- tests/test_views.py | 16 +- 4 files changed, 155 insertions(+), 130 deletions(-) diff --git a/mellon/templates/mellon/authentication_failed.html b/mellon/templates/mellon/authentication_failed.html index 39a0b25..fc9166e 100644 --- a/mellon/templates/mellon/authentication_failed.html +++ b/mellon/templates/mellon/authentication_failed.html @@ -12,18 +12,24 @@

{% trans "Authentication failed" %}

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

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

{% endif %} + {% for reason in reasons %} +

{% if loop.first %}{% trans "Reason" %} : {% endif %}{{ reason }}

+ {% endfor %}

{% trans "Continue" %}

{% if debug %} - +

{% trans "Information shown because DEBUG is True" %}

+ 
+Issuer: {{ issuer }}
+Message: {{ status_message }}
+Status codes: {{ status_codes|join:", " }}
+Relaystate: {{ relaystate }}
+{% for key, value in debug_details.items %}
+{{key}}: {{value|pprint}}
+{% endfor %}
+
{% endif %} {% endblock %} diff --git a/mellon/views.py b/mellon/views.py index ce83a75..c862383 100644 --- a/mellon/views.py +++ b/mellon/views.py @@ -22,7 +22,6 @@ from importlib import import_module from io import StringIO from xml.sax.saxutils import escape -import django.http import lasso import requests from django.conf import settings @@ -57,13 +56,6 @@ LOGIN_HINT = '{%s}login-hint' % EO_NS User = get_user_model() -class HttpResponseBadRequest(django.http.HttpResponseBadRequest): - def __init__(self, *args, **kwargs): - kwargs['content_type'] = kwargs.get('content_type', 'text/plain') - super().__init__(*args, **kwargs) - self['X-Content-Type-Options'] = 'nosniff' - - class LogMixin: """Initialize a module logger in new objects""" @@ -93,6 +85,20 @@ def check_next_url(request, next_url): class ProfileMixin: profile = None + def dispatch(self, request, *args, **kwargs): + try: + return super().dispatch(request, *args, **kwargs) + # handle common errors + except utils.CreateServerError: + return self.failure( + request, + reason=_( + 'Unable to initialize a SAML server object, the private key ' + 'is maybe invalid or unreadable, please check its access ' + 'rights and content.' + ), + ) + def set_next_url(self, next_url): if not check_next_url(self.request, next_url): return @@ -124,34 +130,69 @@ class ProfileMixin: def get_next_url(self, default=None): return self.get_state('next_url', default=default) - def show_message_status_is_not_success(self, profile, prefix): + @property + def template_base(self): + return self.kwargs.get('template_base', 'base.html') + + def render(self, request, template_names, context, status=None): + context['template_base'] = self.template_base + if 'context_hook' in self.kwargs: + self.kwargs['context_hook'](context) + return render(request, template_names, context, status=status) + + def failure_status_is_not_success(self, request, profile, prefix): status_codes, idp_message = utils.get_status_codes_and_message(profile) args = ['%s: status is not success codes: %r', prefix, status_codes] if idp_message: args[0] += ' message: %s' args.append(idp_message) self.log.warning(*args) + return self.failure( + request, + reasons=[ + _('%s refused by identity provider') % prefix, + idp_message, + _('Status codes: %s') % ', '.join([str(code) for code in status_codes]), + ], + status_codes=status_codes, + ) - def dispatch(self, request, *args, **kwargs): - try: - return super().dispatch(request, *args, **kwargs) - except utils.CreateServerError: - return self.failure( - request, - reason=_( - 'Unable to initialize a SAML server object, the private key ' - 'is maybe invalid or unreadable, please check its access ' - 'rights and content.' - ), - ) - - def failure(self, request, reason='', status_codes=()): + def failure(self, request, reason='', reasons=None, status_codes=(), exception=None, debug_details=None): '''show error message to user after a login failure''' - login = self.profile - idp = utils.get_idp(login and login.remoteProviderId) - if not idp and login: - self.log.warning('entity id %r is unknown', login.remoteProviderId) - return HttpResponseBadRequest('entity id %r is unknown' % login.remoteProviderId) + reasons = reasons or [] + reasons.append(reason) + profile = self.profile + idp = utils.get_idp(profile.remoteProviderId) if profile else {} + if not idp and profile and not isinstance(exception, lasso.ServerProviderNotFoundError): + self.log.warning('entity id %r is unknown', profile.remoteProviderId) + reasons.insert( + 0, + _('Other error: %s') + % (_('the received message EntityID (%r) is unknown.') % profile.remoteProviderId), + ) + if exception and isinstance( + exception, + ( + lasso.DsError, + lasso.ProfileCannotVerifySignatureError, + lasso.LoginInvalidSignatureError, + lasso.LoginInvalidAssertionSignatureError, + ), + ): + reasons.append( + _( + 'There was a problem with a signature. It usually means ' + 'that signature key has changed or is unknown. ' + 'The metadata of the identity provider may be out of date.' + ) + ) + if 'METADATA_URL' not in idp: + reasons.append( + _( + 'Possible solution: you currently do not use METADATA_URL ' + 'to reference the metadata, use it to always keep the metadata up to date.' + ) + ) error_url = utils.get_setting(idp, 'ERROR_URL') error_redirect_after_timeout = utils.get_setting(idp, 'ERROR_REDIRECT_AFTER_TIMEOUT') if error_url: @@ -162,13 +203,15 @@ class ProfileMixin: 'mellon/authentication_failed.html', { 'debug': settings.DEBUG, - 'reason': reason, + 'reasons': reasons, 'status_codes': status_codes, - 'issuer': login and login.remoteProviderId, + 'issuer': profile.remoteProviderId if profile else None, 'next_url': next_url, - 'relaystate': login and login.msgRelayState, + 'relaystate': profile.msgRelayState if profile else None, 'error_redirect_after_timeout': error_redirect_after_timeout, + 'debug_details': debug_details or {}, }, + status=400, ) @@ -178,18 +221,7 @@ class LoginView(ProfileMixin, LogMixin, View): with self.capture_logs() if self.debug_login else nullcontext(): return super().dispatch(request, *args, **kwargs) - @property - def template_base(self): - return self.kwargs.get('template_base', 'base.html') - - def render(self, request, template_names, context): - context['template_base'] = self.template_base - if 'context_hook' in self.kwargs: - self.kwargs['context_hook'](context) - return render(request, template_names, context) - - def get_idp(self, request): - entity_id = request.POST.get('entityID') or request.GET.get('entityID') + def get_idp(self, entity_id): if not entity_id: for idp in utils.get_idps(): return idp @@ -202,41 +234,31 @@ class LoginView(ProfileMixin, LogMixin, View): '''Assertion consumer''' if 'SAMLart' in request.POST: if 'artifact' not in app_settings.ASSERTION_CONSUMER_BINDINGS: - raise Http404('artifact binding is not supported') + raise self.failure(request, _('Artifact binding is not supported')) return self.continue_sso_artifact(request, lasso.HTTP_METHOD_ARTIFACT_POST) if 'SAMLResponse' not in request.POST: if 'post' not in app_settings.ASSERTION_CONSUMER_BINDINGS: - raise Http404('post binding is not supported') + raise self.failure(request, _('Post binding is not supported')) return self.get(request, *args, **kwargs) + # prevent null characters in SAMLResponse if not utils.is_nonnull(request.POST['SAMLResponse']): - return HttpResponseBadRequest('SAMLResponse contains a null character') + return self.failure(_('SAMLResponse contains a null character')) self.log.info('Got SAML Response', extra={'saml_response': request.POST['SAMLResponse']}) self.profile = login = utils.create_login(request) - idp_message = None - status_codes = [] - # prevent null characters in SAMLResponse try: login.processAuthnResponseMsg(request.POST['SAMLResponse']) login.acceptSso() - except lasso.ProfileCannotVerifySignatureError: - self.log.warning( - 'SAML authentication failed: signature validation failed for %r', login.remoteProviderId - ) - except lasso.ParamError: - self.log.exception('lasso param error') except ( lasso.LoginStatusNotSuccessError, lasso.ProfileStatusNotSuccessError, lasso.ProfileRequestDeniedError, ): - self.show_message_status_is_not_success(login, 'SAML authentication failed') - except lasso.Error as e: - return HttpResponseBadRequest('error processing the authentication response: %r' % e) - else: - 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.failure(request, reason=idp_message, status_codes=status_codes) + return self.failure_status_is_not_success(request, login, _('Login')) + except Exception as e: + return self.failure(request, _('Technical error'), exception=e) + if 'RelayState' in request.POST and utils.is_nonnull(request.POST['RelayState']): + login.msgRelayState = request.POST['RelayState'] + return self.sso_success(request, login) def get_attribute_value(self, attribute, attribute_value): # check attribute_value contains only text @@ -369,9 +391,6 @@ class LoginView(ProfileMixin, LogMixin, View): return response def continue_sso_artifact(self, request, method): - idp_message = None - status_codes = [] - if method == lasso.HTTP_METHOD_ARTIFACT_GET: message = request.META['QUERY_STRING'] artifact = request.GET['SAMLart'] @@ -386,15 +405,22 @@ class LoginView(ProfileMixin, LogMixin, View): login.msgRelayState = relay_state try: login.initRequest(message, method) - except lasso.ProfileInvalidArtifactError: + except lasso.ProfileInvalidArtifactError as e: self.log.warning('artifact is malformed %r', artifact) - return HttpResponseBadRequest('artifact is malformed %r' % artifact) - except lasso.ServerProviderNotFoundError: + return self.failure(request, _('Artifact is invalid.'), exception=e) + except lasso.ServerProviderNotFoundError as e: self.log.warning('no entity id found for artifact %s', artifact) - return HttpResponseBadRequest('no entity id found for this artifact %r' % artifact) + return self.failure( + request, + _('the received message EntityID (%r) is unknown.') % login.remoteProviderId, + exception=e, + ) idp = utils.get_idp(login.remoteProviderId) if not idp: - return HttpResponseBadRequest('entity id %r is unknown' % login.remoteProviderId) + return self.failure( + request, + _('the received message EntityID (%r) is unknown.') % login.remoteProviderId, + ) verify_ssl_certificate = utils.get_setting(idp, 'VERIFY_SSL_CERTIFICATE') login.buildRequestMsg() try: @@ -407,18 +433,14 @@ class LoginView(ProfileMixin, LogMixin, View): ) except RequestException as e: self.log.warning('unable to reach %r: %s', login.msgUrl, e) - return self.failure( - request, - reason=_('IdP is temporarily down, please try again ' 'later.'), - status_codes=status_codes, - ) + return self.failure(request, reason=_('IdP is temporarily down, please try again later.')) 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.failure(request, reason=idp_message, status_codes=status_codes) + return self.failure(request, reason=_('IdP is temporarily down, please try again later.')) self.log.info('Got SAML Artifact Response', extra={'saml_response': result.content}) result.encoding = utils.get_xml_encoding(result.content) @@ -429,40 +451,28 @@ class LoginView(ProfileMixin, LogMixin, View): # artifact is invalid, idp returned no response self.log.warning('ArtifactResolveResponse is empty: dead artifact %r', artifact) return self.retry_login() - except lasso.ProfileInvalidMsgError: + except lasso.ProfileInvalidMsgError as e: self.log.warning('ArtifactResolveResponse is malformed %r', result.content[:200]) - if settings.DEBUG: - return HttpResponseBadRequest('ArtififactResolveResponse is malformed\n%r' % result.content) - else: - return HttpResponseBadRequest('ArtififactResolveResponse is malformed') - except lasso.ProfileCannotVerifySignatureError: - self.log.warning( - 'SAML authentication failed: signature validation failed for %r', login.remoteProviderId + return self.failure( + request, + _('Could not process the ArtifactResolveResponse'), + exception=e, ) - except lasso.ParamError: - self.log.exception('lasso param error') except ( lasso.LoginStatusNotSuccessError, lasso.ProfileStatusNotSuccessError, lasso.ProfileRequestDeniedError, ): - status = login.response.status - a = status - while a.statusCode: - status_codes.append(a.statusCode.value) - a = a.statusCode - args = ['SAML authentication failed: status is not success codes: %r', status_codes] - if status.statusMessage: - idp_message = lasso_decode(status.statusMessage) - args[0] += ' message: %r' - args.append(status.statusMessage) - self.log.warning(*args) - except lasso.Error as e: - self.log.exception('unexpected lasso error') - return HttpResponseBadRequest('error processing the authentication response: %r' % e) + return self.failure_status_is_not_success(request, login, _('Login')) + except Exception as e: + return self.failure( + request, + _('Technical error'), + exception=e, + debug_details={'ArtifactResolveResponse_content': result.content}, + ) else: return self.sso_success(request, login) - return self.failure(request, reason=idp_message, status_codes=status_codes) def request_discovery_service(self, request, is_passive=False): return_url = request.build_absolute_uri() @@ -495,9 +505,10 @@ class LoginView(ProfileMixin, LogMixin, View): return self.request_discovery_service(request, is_passive=request.GET.get('passive') == '1') next_url = check_next_url(self.request, request.GET.get(REDIRECT_FIELD_NAME)) - idp = self.get_idp(request) + entity_id = request.POST.get('entityID') or request.GET.get('entityID') + idp = self.get_idp(entity_id) if not idp: - return HttpResponseBadRequest('no idp found') + return self.failure(request, _('No idp found for "%s"') % (entity_id or '')) self.profile = login = utils.create_login(request) self.log.debug('authenticating to %r', idp['ENTITY_ID']) try: @@ -550,7 +561,7 @@ class LoginView(ProfileMixin, LogMixin, View): self.add_login_hints(idp, authn_request, request=request, next_url=next_url or '/') login.buildAuthnRequestMsg() except lasso.Error as e: - return HttpResponseBadRequest('error initializing the authentication request: %r' % e) + return self.failure(request, _('Could not initialize the authentication request'), exception=e) self.log.debug('sending authn request %r', authn_request.dump()) self.log.debug('to url %r', login.msgUrl) return HttpResponseRedirect(login.msgUrl) @@ -661,7 +672,7 @@ class LogoutView(ProfileMixin, LogMixin, View): try: logout.processRequestMsg(msg) except lasso.Error as e: - return HttpResponseBadRequest('error processing logout request: %r' % e) + return self.failure(request, _('Could not process the logout request'), exception=e) entity_id = force_text(logout.remoteProviderId) session_indexes = {force_text(sessionIndex) for sessionIndex in logout.request.sessionIndexes} @@ -706,7 +717,7 @@ class LogoutView(ProfileMixin, LogMixin, View): try: logout.buildResponseMsg() except lasso.Error as e: - return HttpResponseBadRequest('error processing logout request: %r' % e) + return self.failure(request, _('Could not process the logout response'), exception=e) if logout.msgBody: return HttpResponse(force_text(logout.msgBody), content_type='text/xml') else: @@ -759,7 +770,7 @@ class LogoutView(ProfileMixin, LogMixin, View): try: logout.processResponseMsg(request.META['QUERY_STRING']) except lasso.ProfileStatusNotSuccessError: - self.show_message_status_is_not_success(logout, 'SAML logout failed') + return self.failure_status_is_not_success(request, logout, _('Logout')) except lasso.LogoutPartialLogoutError: self.log.warning('partial logout') except lasso.Error as e: diff --git a/tests/test_sso_slo.py b/tests/test_sso_slo.py index 201cca8..ff13a43 100644 --- a/tests/test_sso_slo.py +++ b/tests/test_sso_slo.py @@ -429,12 +429,16 @@ def test_sso_request_denied(db, app, idp, caplog, sp_settings): ) assert not relay_state assert url.endswith(reverse('mellon_login')) - response = app.post(reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}) + response = app.post( + reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}, status=400 + ) + assert 'User is not allowed to login' in response assert ( "status is not success codes: ['urn:oasis:names:tc:SAML:2.0:status:Responder',\ 'urn:oasis:names:tc:SAML:2.0:status:RequestDenied']" in caplog.text ) + assert 'User is not allowed to login' in caplog.text @pytest.mark.urls('urls_tests_template_base') @@ -444,7 +448,9 @@ def test_template_base(db, app, idp, caplog, sp_settings): url, body, relay_state = idp.process_authn_request_redirect( response['Location'], auth_result=False, msg='User is not allowed to login' ) - response = app.post(reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}) + response = app.post( + reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}, status=400 + ) assert 'Theme is ok' in response.text response = app.get(reverse('mellon_login')) @@ -461,7 +467,9 @@ def test_template_hook(db, app, idp, caplog, sp_settings): url, body, relay_state = idp.process_authn_request_redirect( response['Location'], auth_result=False, msg='User is not allowed to login' ) - response = app.post(reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}) + response = app.post( + reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}, status=400 + ) assert 'Theme is ok' in response.text assert 'HOOK' in response.text @@ -477,7 +485,9 @@ def test_no_template_base(db, app, idp, caplog, sp_settings): url, body, relay_state = idp.process_authn_request_redirect( response['Location'], auth_result=False, msg='User is not allowed to login' ) - response = app.post(reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}) + response = app.post( + reverse('mellon_login'), params={'SAMLResponse': body, 'RelayState': relay_state}, status=400 + ) assert 'Theme is ok' not in response.text @@ -496,7 +506,7 @@ def test_sso_request_denied_artifact(db, app, caplog, sp_settings, idp_metadata, assert 'SAMLart' in url acs_artifact_url = url.split('testserver', 1)[1] with HTTMock(idp.mock_artifact_resolver()): - response = app.get(acs_artifact_url, params={'RelayState': relay_state}) + response = app.get(acs_artifact_url, params={'RelayState': relay_state}, status=400) assert ( "status is not success codes: ['urn:oasis:names:tc:SAML:2.0:status:Responder',\ 'urn:oasis:names:tc:SAML:2.0:status:RequestDenied']" @@ -601,7 +611,7 @@ def test_sso_artifact_no_loop(db, app, caplog, sp_settings, idp_metadata, idp_pr # forget the artifact idp.artifact = '' with HTTMock(idp.mock_artifact_resolver()): - response = app.get(acs_artifact_url) + response = app.get(acs_artifact_url, status=400) # check cookie is deleted after failed retry # Py3-Dj111 variation diff --git a/tests/test_views.py b/tests/test_views.py index d2b13dc..2b15a94 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -158,7 +158,7 @@ def test_sp_initiated_login_improperly_configured2(private_settings, client): private_settings.MELLON_IDENTITY_PROVIDERS = [] response = client.get('/login/') assert response.status_code == 400 - assert b'no idp found' in response.content + assert 'No idp found for' in response.content.decode() def test_sp_initiated_login_discovery_service(private_settings, client): @@ -190,8 +190,7 @@ def test_sp_initiated_login_discovery_service_nodisco(private_settings, client): private_settings.MELLON_IDENTITY_PROVIDERS = [] private_settings.MELLON_DISCOVERY_SERVICE_URL = 'https://disco' response = client.get('/login/?nodisco=1') - assert response.status_code == 400 - assert b'no idp found' in response.content + assert 'No idp found for' in response.content.decode() def test_sp_initiated_login(private_settings, client): @@ -254,16 +253,15 @@ def test_sp_initiated_login_requested_authn_context(private_settings, client): ) -def test_malfortmed_artifact(private_settings, client, caplog): +def test_malfortmed_artifact(private_settings, app, caplog): private_settings.MELLON_IDENTITY_PROVIDERS = [ { 'METADATA': open('tests/metadata.xml').read(), } ] - response = client.get('/login/?SAMLart=xxx', status=400) - assert response['Content-Type'] == 'text/plain' - assert response['X-Content-Type-Options'] == 'nosniff' - assert b'artifact is malformed' in response.content + response = app.get('/login/?SAMLart=xxx', status=400) + assert response['Content-Type'].split(';')[0] == 'text/html' + assert 'Artifact is invalid' in response assert 'artifact is malformed' in caplog.text @@ -308,5 +306,5 @@ def test_private_key_unreadable(private_settings, app, tmpdir): fd.write('1') private_key.chmod(0o000) private_settings.MELLON_PRIVATE_KEY = str(private_key) - response = app.get('/login/?next=%2Fwhatever') + response = app.get('/login/?next=%2Fwhatever', status=400) assert 'Unable to initialize a SAML server object' in response -- 2.34.1