From f840774169386d688aea874708f3fd531d7aa180 Mon Sep 17 00:00:00 2001
From: Nimisha Asthagiri <nasthagiri@edx.org>
Date: Sat, 22 Dec 2018 22:08:28 -0500
Subject: [PATCH] Fix Studio SSO ARCH-329
---
.../djangoapps/third_party_auth/settings.py | 21 +++++++------
.../third_party_auth/tests/specs/base.py | 30 ++++++++++++++-----
.../tests/specs/test_testshib.py | 2 +-
.../third_party_auth/tests/test_settings.py | 9 ++++--
lms/envs/devstack_docker.py | 1 +
5 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py
index 632780842f4..4b0410d002d 100644
--- a/common/djangoapps/third_party_auth/settings.py
+++ b/common/djangoapps/third_party_auth/settings.py
@@ -12,23 +12,18 @@ If true, it:
from openedx.features.enterprise_support.api import insert_enterprise_pipeline_elements
-_FIELDS_STORED_IN_SESSION = ['auth_entry', 'next']
-_MIDDLEWARE_CLASSES = ['third_party_auth.middleware.ExceptionMiddleware']
-_SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard'
-_SOCIAL_AUTH_AZUREAD_OAUTH2_AUTH_EXTRA_ARGUMENTS = {
- 'msafed': 0
-}
-
def apply_settings(django_settings):
"""Set provider-independent settings."""
# Whitelisted URL query parameters retrained in the pipeline session.
# Params not in this whitelist will be silently dropped.
- django_settings.FIELDS_STORED_IN_SESSION = _FIELDS_STORED_IN_SESSION
+ django_settings.FIELDS_STORED_IN_SESSION = ['auth_entry', 'next']
# Inject exception middleware to make redirects fire.
- django_settings.MIDDLEWARE_CLASSES.extend(_MIDDLEWARE_CLASSES)
+ django_settings.MIDDLEWARE_CLASSES.extend(
+ ['third_party_auth.middleware.ExceptionMiddleware']
+ )
# Where to send the user if there's an error during social authentication
# and we cannot send them to a more specific URL
@@ -36,10 +31,14 @@ def apply_settings(django_settings):
django_settings.SOCIAL_AUTH_LOGIN_ERROR_URL = '/'
# Where to send the user once social authentication is successful.
- django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL = _SOCIAL_AUTH_LOGIN_REDIRECT_URL
+ django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard'
+
+ # Disable sanitizing of redirect urls in social-auth since the platform
+ # already does its own sanitization via the LOGIN_REDIRECT_WHITELIST setting.
+ django_settings.SOCIAL_AUTH_SANITIZE_REDIRECTS = False
# Adding extra key value pair in the url query string for microsoft as per request
- django_settings.SOCIAL_AUTH_AZUREAD_OAUTH2_AUTH_EXTRA_ARGUMENTS = _SOCIAL_AUTH_AZUREAD_OAUTH2_AUTH_EXTRA_ARGUMENTS
+ django_settings.SOCIAL_AUTH_AZUREAD_OAUTH2_AUTH_EXTRA_ARGUMENTS = {'msafed': 0}
# Inject our customized auth pipeline. All auth backends must work with
# this pipeline.
diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py
index 0a01fddc174..190e8edfe5a 100644
--- a/common/djangoapps/third_party_auth/tests/specs/base.py
+++ b/common/djangoapps/third_party_auth/tests/specs/base.py
@@ -161,12 +161,14 @@ class HelperMixin(object):
"""Makes sure the given request is running an auth pipeline."""
self.assertTrue(pipeline.running(request))
- def assert_redirect_to_dashboard_looks_correct(self, response):
- """Asserts a response would redirect to /dashboard."""
+ def assert_redirect_after_pipeline_completes(self, response, expected_redirect_url=None):
+ """Asserts a response would redirect to the expected_redirect_url or SOCIAL_AUTH_LOGIN_REDIRECT_URL."""
self.assertEqual(302, response.status_code)
# NOTE: Ideally we should use assertRedirects(), however it errors out due to the hostname, testserver,
# not being properly set. This may be an issue with the call made by PSA, but we are not certain.
- self.assertTrue(response.get('Location').endswith(django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL))
+ self.assertTrue(response.get('Location').endswith(
+ expected_redirect_url or django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL,
+ ))
def assert_redirect_to_login_looks_correct(self, response):
"""Asserts a response would redirect to /login."""
@@ -560,7 +562,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase, HelperMixin):
self.set_logged_in_cookies(request)
# Fire off the auth pipeline to link.
- self.assert_redirect_to_dashboard_looks_correct(
+ self.assert_redirect_after_pipeline_completes(
actions.do_complete(
request.backend,
social_views._do_login, # pylint: disable=protected-access
@@ -606,7 +608,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase, HelperMixin):
self.assert_social_auth_exists_for_user(request.user, strategy)
# Fire off the disconnect pipeline to unlink.
- self.assert_redirect_to_dashboard_looks_correct(
+ self.assert_redirect_after_pipeline_completes(
actions.do_disconnect(
request.backend,
request.user,
@@ -725,7 +727,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase, HelperMixin):
# Set the cookie and try again
self.set_logged_in_cookies(request)
- self.assert_redirect_to_dashboard_looks_correct(
+ self.assert_redirect_after_pipeline_completes(
actions.do_complete(request.backend, social_views._do_login, user=user, request=request))
self.assert_account_settings_context_looks_correct(account_settings_context(request))
@@ -765,6 +767,20 @@ class IntegrationTest(testutil.TestCase, test.TestCase, HelperMixin):
self.assert_first_party_auth_trumps_third_party_auth(
email='user@example.com', password=u'password', success=True)
+ def test_pipeline_redirects_to_requested_url(self):
+ requested_redirect_url = 'foo' # something different from '/dashboard'
+ request, strategy = self.get_request_and_strategy(redirect_uri='social:complete')
+ strategy.request.backend.auth_complete = mock.MagicMock(return_value=self.fake_auth_complete(strategy))
+ request.session[pipeline.AUTH_REDIRECT_KEY] = requested_redirect_url
+
+ user = self.create_user_models_for_existing_account(strategy, 'user@foo.com', 'password', self.get_username())
+ self.set_logged_in_cookies(request)
+
+ self.assert_redirect_after_pipeline_completes(
+ actions.do_complete(request.backend, social_views._do_login, user=user, request=request), # pylint: disable=protected-access
+ requested_redirect_url,
+ )
+
def test_full_pipeline_succeeds_registering_new_account(self):
# First, create, the request and strategy that store pipeline state.
# Mock out wire traffic.
@@ -831,7 +847,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase, HelperMixin):
# Set the cookie and try again
self.set_logged_in_cookies(request)
- self.assert_redirect_to_dashboard_looks_correct(
+ self.assert_redirect_after_pipeline_completes(
actions.do_complete(strategy.request.backend, social_views._do_login, user=created_user, request=request))
# Now the user has been redirected to the dashboard. Their third party account should now be linked.
self.assert_social_auth_exists_for_user(created_user, strategy)
diff --git a/common/djangoapps/third_party_auth/tests/specs/test_testshib.py b/common/djangoapps/third_party_auth/tests/specs/test_testshib.py
index c81823d3d96..e195e3efd21 100644
--- a/common/djangoapps/third_party_auth/tests/specs/test_testshib.py
+++ b/common/djangoapps/third_party_auth/tests/specs/test_testshib.py
@@ -212,7 +212,7 @@ class TestShibIntegrationTest(SamlIntegrationTestUtilities, IntegrationTestMixin
)
# Fire off the disconnect pipeline to unlink.
- self.assert_redirect_to_dashboard_looks_correct(
+ self.assert_redirect_after_pipeline_completes(
actions.do_disconnect(
request.backend,
user,
diff --git a/common/djangoapps/third_party_auth/tests/test_settings.py b/common/djangoapps/third_party_auth/tests/test_settings.py
index b08472bff0f..e36c3c8b9dd 100644
--- a/common/djangoapps/third_party_auth/tests/test_settings.py
+++ b/common/djangoapps/third_party_auth/tests/test_settings.py
@@ -38,12 +38,11 @@ class SettingsUnitTest(testutil.TestCase):
def test_apply_settings_adds_exception_middleware(self):
settings.apply_settings(self.settings)
- for middleware_name in settings._MIDDLEWARE_CLASSES:
- self.assertIn(middleware_name, self.settings.MIDDLEWARE_CLASSES)
+ self.assertIn('third_party_auth.middleware.ExceptionMiddleware', self.settings.MIDDLEWARE_CLASSES)
def test_apply_settings_adds_fields_stored_in_session(self):
settings.apply_settings(self.settings)
- self.assertEqual(settings._FIELDS_STORED_IN_SESSION, self.settings.FIELDS_STORED_IN_SESSION)
+ self.assertEqual(['auth_entry', 'next'], self.settings.FIELDS_STORED_IN_SESSION)
@unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, testutil.AUTH_FEATURES_KEY + ' not enabled')
def test_apply_settings_enables_no_providers_by_default(self):
@@ -56,3 +55,7 @@ class SettingsUnitTest(testutil.TestCase):
# bad in prod.
settings.apply_settings(self.settings)
self.assertFalse(self.settings.SOCIAL_AUTH_RAISE_EXCEPTIONS)
+
+ def test_apply_settings_turns_off_redirect_sanitization(self):
+ settings.apply_settings(self.settings)
+ self.assertFalse(self.settings.SOCIAL_AUTH_SANITIZE_REDIRECTS)
diff --git a/lms/envs/devstack_docker.py b/lms/envs/devstack_docker.py
index 98650770f04..28b97fe049c 100644
--- a/lms/envs/devstack_docker.py
+++ b/lms/envs/devstack_docker.py
@@ -14,6 +14,7 @@ CMS_BASE = 'localhost:18010'
SITE_NAME = LMS_BASE
LMS_ROOT_URL = 'http://{}'.format(LMS_BASE)
LMS_INTERNAL_ROOT_URL = LMS_ROOT_URL
+LOGIN_REDIRECT_WHITELIST = [CMS_BASE]
ECOMMERCE_PUBLIC_URL_ROOT = 'http://localhost:18130'
ECOMMERCE_API_URL = 'http://edx.devstack.ecommerce:18130/api/v2'
--
GitLab