Merge pull request #5929 from edx/will/logistration-fix-cancel-third-party-auth

Logistration: Redirect to a valid URL when cancelling third party auth

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

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

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,