diff --git a/common/djangoapps/third_party_auth/middleware.py b/common/djangoapps/third_party_auth/middleware.py index 677534d0fc..fe843f6a7b 100644 --- a/common/djangoapps/third_party_auth/middleware.py +++ b/common/djangoapps/third_party_auth/middleware.py @@ -9,10 +9,17 @@ class ExceptionMiddleware(SocialAuthExceptionMiddleware): """Custom middleware that handles conditional redirection.""" def get_redirect_uri(self, request, exception): + # Fall back to django settings's SOCIAL_AUTH_LOGIN_ERROR_URL. + redirect_uri = super(ExceptionMiddleware, self).get_redirect_uri(request, exception) + # Safe because it's already been validated by # pipeline.parse_query_params. If that pipeline step ever moves later # in the pipeline stack, we'd need to validate this value because it # would be an injection point for attacker data. auth_entry = request.session.get(pipeline.AUTH_ENTRY_KEY) - # Fall back to django settings's SOCIAL_AUTH_LOGIN_ERROR_URL. - return '/' + auth_entry if auth_entry else super(ExceptionMiddleware, self).get_redirect_uri(request, exception) + + # Check if we have an auth entry key we can use instead + if auth_entry and auth_entry in pipeline.AUTH_DISPATCH_URLS: + redirect_uri = pipeline.AUTH_DISPATCH_URLS[auth_entry] + + return redirect_uri diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 966fae9497..4114b5ee36 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -119,6 +119,30 @@ AUTH_ENTRY_REGISTER_2 = 'account_register' AUTH_ENTRY_API = 'api' +# URLs associated with auth entry points +# These are used to request additional user information +# (for example, account credentials when logging in), +# and when the user cancels the auth process +# (e.g., refusing to grant permission on the provider's login page). +# We don't use "reverse" here because doing so may cause modules +# to load that depend on this module. +AUTH_DISPATCH_URLS = { + AUTH_ENTRY_DASHBOARD: '/dashboard', + AUTH_ENTRY_LOGIN: '/login', + AUTH_ENTRY_REGISTER: '/register', + + # TODO (ECOM-369): Replace the dispatch URLs + # for `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` + # with these values, but DO NOT DELETE THESE KEYS. + AUTH_ENTRY_LOGIN_2: '/account/login/', + AUTH_ENTRY_REGISTER_2: '/account/register/', + + # If linking/unlinking an account from the new student profile + # page, redirect to the profile page. Only used if + # `FEATURES['ENABLE_NEW_DASHBOARD']` is true. + AUTH_ENTRY_PROFILE: '/profile/', +} + _AUTH_ENTRY_CHOICES = frozenset([ AUTH_ENTRY_DASHBOARD, AUTH_ENTRY_LOGIN, @@ -483,20 +507,20 @@ def ensure_user_information( return if dispatch_to_login: - return redirect('/login', name='signin_user') + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], name='signin_user') # TODO (ECOM-369): Consolidate this with `dispatch_to_login` # once the A/B test completes. if dispatch_to_login_2: - return redirect(reverse(AUTH_ENTRY_LOGIN_2)) + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN_2]) if is_register and user_unset: - return redirect('/register', name='register_user') + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER], name='register_user') # TODO (ECOM-369): Consolidate this with `is_register` # once the A/B test completes. if is_register_2 and user_unset: - return redirect(reverse(AUTH_ENTRY_REGISTER_2)) + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER_2]) @partial.partial diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index a019fa83d2..da29b9b9a3 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -142,7 +142,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertEqual('link' if linked else 'unlink', icon_state) self.assertEqual(self.PROVIDER_CLASS.NAME, provider_name) - def assert_exception_redirect_looks_correct(self, auth_entry=None): + def assert_exception_redirect_looks_correct(self, expected_uri, auth_entry=None): """Tests middleware conditional redirection. middleware.ExceptionMiddleware makes sure the user ends up in the right @@ -157,13 +157,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertEqual(302, response.status_code) self.assertIn('canceled', location) self.assertIn(self.backend_name, location) - - if auth_entry: - # Custom redirection to form. - self.assertTrue(location.startswith('/' + auth_entry)) - else: - # Stock framework redirection to root. - self.assertTrue(location.startswith('/?')) + self.assertTrue(location.startswith(expected_uri + '?')) def assert_first_party_auth_trumps_third_party_auth(self, email=None, password=None, success=None): """Asserts first party auth was used in place of third party auth. @@ -410,13 +404,19 @@ class IntegrationTest(testutil.TestCase, test.TestCase): # Actual tests, executed once per child. def test_canceling_authentication_redirects_to_login_when_auth_entry_login(self): - self.assert_exception_redirect_looks_correct(auth_entry=pipeline.AUTH_ENTRY_LOGIN) + self.assert_exception_redirect_looks_correct('/login', auth_entry=pipeline.AUTH_ENTRY_LOGIN) def test_canceling_authentication_redirects_to_register_when_auth_entry_register(self): - self.assert_exception_redirect_looks_correct(auth_entry=pipeline.AUTH_ENTRY_REGISTER) + self.assert_exception_redirect_looks_correct('/register', auth_entry=pipeline.AUTH_ENTRY_REGISTER) + + def test_canceling_authentication_redirects_to_login_when_auth_login_2(self): + self.assert_exception_redirect_looks_correct('/account/login/', auth_entry=pipeline.AUTH_ENTRY_LOGIN_2) + + def test_canceling_authentication_redirects_to_login_when_auth_register_2(self): + self.assert_exception_redirect_looks_correct('/account/register/', auth_entry=pipeline.AUTH_ENTRY_REGISTER_2) def test_canceling_authentication_redirects_to_root_when_auth_entry_not_set(self): - self.assert_exception_redirect_looks_correct() + self.assert_exception_redirect_looks_correct('/') def test_full_pipeline_succeeds_for_linking_account(self): # First, create, the request and strategy that store pipeline state,