From 28cb402a049ab7ab09afb655e4adabeb91814fe6 Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Fri, 5 Feb 2016 13:46:12 +0700 Subject: [PATCH 1/6] Return 404 response from third party auth login when SAML disabled --- common/djangoapps/third_party_auth/saml.py | 12 +++++++----- .../djangoapps/third_party_auth/tests/test_views.py | 6 ++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 61f7e0d8f5..68898434e4 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -2,6 +2,7 @@ Slightly customized python-social-auth backend for SAML 2.0 support """ import logging +from django.http import Http404 from social.backends.saml import SAMLAuth, OID_EDU_PERSON_ENTITLEMENT from social.exceptions import AuthForbidden, AuthMissingParameter @@ -25,9 +26,6 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method if not hasattr(self, '_config'): from .models import SAMLConfiguration self._config = SAMLConfiguration.current() # pylint: disable=attribute-defined-outside-init - if not self._config.enabled: - from django.core.exceptions import ImproperlyConfigured - raise ImproperlyConfigured("SAML Authentication is not enabled.") try: return self._config.get_setting(name) except KeyError: @@ -35,14 +33,18 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method def auth_url(self): """ - Check that the request includes an 'idp' parameter before getting the - URL to which we must redirect in order to authenticate the user. + Check that SAML is enabled and that the request includes an 'idp' + parameter before getting the URL to which we must redirect in order to + authenticate the user. + raise Http404 if SAML is disabled raise AuthMissingParameter if the 'idp' parameter is missing. TODO: remove this method once the fix is merged upstream: https://github.com/omab/python-social-auth/pull/821 """ + if not self._config.enabled: + raise Http404 if 'idp' not in self.strategy.request_data(): raise AuthMissingParameter(self, 'idp') return super(SAMLAuthBackend, self).auth_url() diff --git a/common/djangoapps/third_party_auth/tests/test_views.py b/common/djangoapps/third_party_auth/tests/test_views.py index 538c82eb46..29d14b065b 100644 --- a/common/djangoapps/third_party_auth/tests/test_views.py +++ b/common/djangoapps/third_party_auth/tests/test_views.py @@ -143,3 +143,9 @@ class SAMLAuthTest(SAMLTestCase): self.enable_saml() response = self.client.get(self.LOGIN_URL) self.assertEqual(response.status_code, 302) + + def test_login_disabled(self): + """ When SAML is not enabled, the login view should return 404 """ + self.enable_saml(enabled=False) + response = self.client.get(self.LOGIN_URL) + self.assertEqual(response.status_code, 404) From a020464a41c8e833d31474f8976a4740dd1eb747 Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Mon, 8 Feb 2016 14:32:12 +0700 Subject: [PATCH 2/6] Keep SAML configuration check --- common/djangoapps/third_party_auth/saml.py | 12 +++++------- common/djangoapps/third_party_auth/urls.py | 3 ++- common/djangoapps/third_party_auth/views.py | 12 +++++++++++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 68898434e4..61f7e0d8f5 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -2,7 +2,6 @@ Slightly customized python-social-auth backend for SAML 2.0 support """ import logging -from django.http import Http404 from social.backends.saml import SAMLAuth, OID_EDU_PERSON_ENTITLEMENT from social.exceptions import AuthForbidden, AuthMissingParameter @@ -26,6 +25,9 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method if not hasattr(self, '_config'): from .models import SAMLConfiguration self._config = SAMLConfiguration.current() # pylint: disable=attribute-defined-outside-init + if not self._config.enabled: + from django.core.exceptions import ImproperlyConfigured + raise ImproperlyConfigured("SAML Authentication is not enabled.") try: return self._config.get_setting(name) except KeyError: @@ -33,18 +35,14 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method def auth_url(self): """ - Check that SAML is enabled and that the request includes an 'idp' - parameter before getting the URL to which we must redirect in order to - authenticate the user. + Check that the request includes an 'idp' parameter before getting the + URL to which we must redirect in order to authenticate the user. - raise Http404 if SAML is disabled raise AuthMissingParameter if the 'idp' parameter is missing. TODO: remove this method once the fix is merged upstream: https://github.com/omab/python-social-auth/pull/821 """ - if not self._config.enabled: - raise Http404 if 'idp' not in self.strategy.request_data(): raise AuthMissingParameter(self, 'idp') return super(SAMLAuthBackend, self).auth_url() diff --git a/common/djangoapps/third_party_auth/urls.py b/common/djangoapps/third_party_auth/urls.py index a85226f52b..de0fee8ec2 100644 --- a/common/djangoapps/third_party_auth/urls.py +++ b/common/djangoapps/third_party_auth/urls.py @@ -2,7 +2,7 @@ from django.conf.urls import include, patterns, url -from .views import inactive_user_view, saml_metadata_view, lti_login_and_complete_view, post_to_custom_auth_form +from .views import inactive_user_view, saml_metadata_view, lti_login_and_complete_view, post_to_custom_auth_form, login urlpatterns = patterns( '', @@ -10,5 +10,6 @@ urlpatterns = patterns( url(r'^auth/custom_auth_entry', post_to_custom_auth_form, name='tpa_post_to_custom_auth_form'), url(r'^auth/saml/metadata.xml', saml_metadata_view), url(r'^auth/login/(?Plti)/$', lti_login_and_complete_view), + url(r'^auth/login/(?P[^/]+)/$', login), url(r'^auth/', include('social.apps.django_app.urls', namespace='social')), ) diff --git a/common/djangoapps/third_party_auth/views.py b/common/djangoapps/third_party_auth/views.py index 56d34dd178..43600432f4 100644 --- a/common/djangoapps/third_party_auth/views.py +++ b/common/djangoapps/third_party_auth/views.py @@ -7,7 +7,7 @@ from django.http import HttpResponse, HttpResponseServerError, Http404, HttpResp from django.shortcuts import redirect, render from django.views.decorators.csrf import csrf_exempt import social -from social.apps.django_app.views import complete +from social.apps.django_app.views import auth, complete from social.apps.django_app.utils import load_strategy, load_backend from social.utils import setting_name from .models import SAMLConfiguration @@ -61,6 +61,16 @@ def lti_login_and_complete_view(request, backend, *args, **kwargs): return complete(request, backend, *args, **kwargs) +def login(*args, **kwargs): + """ + Wraps the python social auth login view to return a 404 if third party + auth is disabled. + """ + if not SAMLConfiguration.is_enabled(): + raise Http404 + return auth(*args, **kwargs) + + def post_to_custom_auth_form(request): """ Redirect to a custom login/register page. From 78d4ed31a10fe73c7517eb245c1c087faf758ee6 Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Tue, 9 Feb 2016 15:40:27 +0700 Subject: [PATCH 3/6] Revert "Keep SAML configuration check" This reverts commit a020464a41c8e833d31474f8976a4740dd1eb747. --- common/djangoapps/third_party_auth/saml.py | 12 +++++++----- common/djangoapps/third_party_auth/urls.py | 3 +-- common/djangoapps/third_party_auth/views.py | 12 +----------- 3 files changed, 9 insertions(+), 18 deletions(-) diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 61f7e0d8f5..68898434e4 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -2,6 +2,7 @@ Slightly customized python-social-auth backend for SAML 2.0 support """ import logging +from django.http import Http404 from social.backends.saml import SAMLAuth, OID_EDU_PERSON_ENTITLEMENT from social.exceptions import AuthForbidden, AuthMissingParameter @@ -25,9 +26,6 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method if not hasattr(self, '_config'): from .models import SAMLConfiguration self._config = SAMLConfiguration.current() # pylint: disable=attribute-defined-outside-init - if not self._config.enabled: - from django.core.exceptions import ImproperlyConfigured - raise ImproperlyConfigured("SAML Authentication is not enabled.") try: return self._config.get_setting(name) except KeyError: @@ -35,14 +33,18 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method def auth_url(self): """ - Check that the request includes an 'idp' parameter before getting the - URL to which we must redirect in order to authenticate the user. + Check that SAML is enabled and that the request includes an 'idp' + parameter before getting the URL to which we must redirect in order to + authenticate the user. + raise Http404 if SAML is disabled raise AuthMissingParameter if the 'idp' parameter is missing. TODO: remove this method once the fix is merged upstream: https://github.com/omab/python-social-auth/pull/821 """ + if not self._config.enabled: + raise Http404 if 'idp' not in self.strategy.request_data(): raise AuthMissingParameter(self, 'idp') return super(SAMLAuthBackend, self).auth_url() diff --git a/common/djangoapps/third_party_auth/urls.py b/common/djangoapps/third_party_auth/urls.py index de0fee8ec2..a85226f52b 100644 --- a/common/djangoapps/third_party_auth/urls.py +++ b/common/djangoapps/third_party_auth/urls.py @@ -2,7 +2,7 @@ from django.conf.urls import include, patterns, url -from .views import inactive_user_view, saml_metadata_view, lti_login_and_complete_view, post_to_custom_auth_form, login +from .views import inactive_user_view, saml_metadata_view, lti_login_and_complete_view, post_to_custom_auth_form urlpatterns = patterns( '', @@ -10,6 +10,5 @@ urlpatterns = patterns( url(r'^auth/custom_auth_entry', post_to_custom_auth_form, name='tpa_post_to_custom_auth_form'), url(r'^auth/saml/metadata.xml', saml_metadata_view), url(r'^auth/login/(?Plti)/$', lti_login_and_complete_view), - url(r'^auth/login/(?P[^/]+)/$', login), url(r'^auth/', include('social.apps.django_app.urls', namespace='social')), ) diff --git a/common/djangoapps/third_party_auth/views.py b/common/djangoapps/third_party_auth/views.py index 43600432f4..56d34dd178 100644 --- a/common/djangoapps/third_party_auth/views.py +++ b/common/djangoapps/third_party_auth/views.py @@ -7,7 +7,7 @@ from django.http import HttpResponse, HttpResponseServerError, Http404, HttpResp from django.shortcuts import redirect, render from django.views.decorators.csrf import csrf_exempt import social -from social.apps.django_app.views import auth, complete +from social.apps.django_app.views import complete from social.apps.django_app.utils import load_strategy, load_backend from social.utils import setting_name from .models import SAMLConfiguration @@ -61,16 +61,6 @@ def lti_login_and_complete_view(request, backend, *args, **kwargs): return complete(request, backend, *args, **kwargs) -def login(*args, **kwargs): - """ - Wraps the python social auth login view to return a 404 if third party - auth is disabled. - """ - if not SAMLConfiguration.is_enabled(): - raise Http404 - return auth(*args, **kwargs) - - def post_to_custom_auth_form(request): """ Redirect to a custom login/register page. From 2c4e97ebfab54bd37eb326d24c98ac3bbfaaa8c9 Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Tue, 9 Feb 2016 15:45:21 +0700 Subject: [PATCH 4/6] Don't remove auth_url method --- common/djangoapps/third_party_auth/saml.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 68898434e4..1ea831253e 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -39,12 +39,11 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method raise Http404 if SAML is disabled raise AuthMissingParameter if the 'idp' parameter is missing. - - TODO: remove this method once the fix is merged upstream: - https://github.com/omab/python-social-auth/pull/821 """ if not self._config.enabled: raise Http404 + # TODO: remove this check once the fix is merged upstream: + # https://github.com/omab/python-social-auth/pull/821 if 'idp' not in self.strategy.request_data(): raise AuthMissingParameter(self, 'idp') return super(SAMLAuthBackend, self).auth_url() From 9b7bb0dc20f58a5332db17bacd04501f0c934f33 Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Tue, 9 Feb 2016 15:45:49 +0700 Subject: [PATCH 5/6] Make SAMLAuthBackend._config a cached_property Cleaner, and keeps pylint happy --- common/djangoapps/third_party_auth/saml.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 1ea831253e..618ca7a800 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -3,6 +3,7 @@ Slightly customized python-social-auth backend for SAML 2.0 support """ import logging from django.http import Http404 +from django.utils.functional import cached_property from social.backends.saml import SAMLAuth, OID_EDU_PERSON_ENTITLEMENT from social.exceptions import AuthForbidden, AuthMissingParameter @@ -23,9 +24,6 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method def setting(self, name, default=None): """ Get a setting, from SAMLConfiguration """ - if not hasattr(self, '_config'): - from .models import SAMLConfiguration - self._config = SAMLConfiguration.current() # pylint: disable=attribute-defined-outside-init try: return self._config.get_setting(name) except KeyError: @@ -62,3 +60,8 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method log.warning( "SAML user from IdP %s rejected due to missing eduPersonEntitlement %s", idp.name, expected) raise AuthForbidden(self) + + @cached_property + def _config(self): + from .models import SAMLConfiguration + return SAMLConfiguration.current() From ed2cba2e2dad94af2257c7a68b8b06def1cf802d Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Tue, 9 Feb 2016 15:53:42 +0700 Subject: [PATCH 6/6] Keep SAML configuration check --- common/djangoapps/third_party_auth/saml.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 618ca7a800..8b23c21d6f 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -35,10 +35,11 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method parameter before getting the URL to which we must redirect in order to authenticate the user. - raise Http404 if SAML is disabled + raise Http404 if SAML authentication is disabled. raise AuthMissingParameter if the 'idp' parameter is missing. """ if not self._config.enabled: + log.error('SAML authentication is not enabled') raise Http404 # TODO: remove this check once the fix is merged upstream: # https://github.com/omab/python-social-auth/pull/821