From c80b596890cbe18ee247df11192219f15faf3469 Mon Sep 17 00:00:00 2001 From: John Cox Date: Thu, 24 Apr 2014 16:23:47 -0700 Subject: [PATCH] Fix two error message corner cases --- .../djangoapps/third_party_auth/pipeline.py | 7 ++-- .../third_party_auth/tests/specs/base.py | 32 +++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 49da12c660..c2c8888721 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -244,11 +244,14 @@ def get_duplicate_provider(messages): message there is a request to associate a social account S with an edX account E if S is already associated with an edX account E'. + This messaging approach is stringly-typed and the particular string is + unfortunately not in a reusable constant. + Returns: provider.BaseProvider child instance. The provider of the duplicate account, or None if there is no duplicate (and hence no error). """ - social_auth_messages = [m for m in messages if m.extra_tags.startswith('social-auth')] + social_auth_messages = [m for m in messages if m.message.endswith('is already in use.')] if not social_auth_messages: return @@ -348,7 +351,7 @@ def redirect_to_supplementary_form(strategy, details, response, uid, is_dashboar if is_dashboard: return - if is_login and user is None: + if is_login and user is None or user and not user.is_active: return redirect('/login', name='signin_user') if is_register and user is None: diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index 98ce0d6a34..d5e2e93ded 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -210,6 +210,13 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertIn(argument_string, ['true', 'false']) self.assertEqual(boolean, True if argument_string == 'true' else False) + def assert_json_failure_response_is_inactive_account(self, response): + """Asserts failure on /login for inactive account looks right.""" + self.assertEqual(200, response.status_code) # Yes, it's a 200 even though it's a failure. + payload = json.loads(response.content) + self.assertFalse(payload.get('success')) + self.assertIn('This account has not been activated', payload.get('value')) + def assert_json_failure_response_is_missing_social_auth(self, response): """Asserts failure on /login for missing social auth looks right.""" self.assertEqual(200, response.status_code) # Yes, it's a 200 even though it's a failure. @@ -508,12 +515,13 @@ class IntegrationTest(testutil.TestCase, test.TestCase): # Monkey-patch storage for messaging; pylint: disable-msg=protected-access request._messages = fallback.FallbackStorage(request) middleware.ExceptionMiddleware().process_exception( - request, exceptions.AuthAlreadyAssociated(self.PROVIDER_CLASS.BACKEND_CLASS.name, 'duplicate')) + request, + exceptions.AuthAlreadyAssociated(self.PROVIDER_CLASS.BACKEND_CLASS.name, 'account is already in use.')) self.assert_dashboard_response_looks_correct( student_views.dashboard(request), user, duplicate=True, linked=True) - def test_full_pipeline_succeeds_for_signing_in_to_existing_account(self): + def test_full_pipeline_succeeds_for_signing_in_to_existing_active_account(self): # First, create, the request and strategy that store pipeline state, # configure the backend, and mock out wire traffic. request, strategy = self.get_request_and_strategy( @@ -522,6 +530,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase): user = self.create_user_models_for_existing_account( strategy, 'user@example.com', 'password', self.get_username()) self.assert_social_auth_exists_for_user(user, strategy) + self.assertTrue(user.is_active) # Begin! Ensure that the login form contains expected controls before # the user starts the pipeline. @@ -550,6 +559,17 @@ class IntegrationTest(testutil.TestCase, test.TestCase): actions.do_complete(strategy, social_views._do_login, user=user)) self.assert_dashboard_response_looks_correct(student_views.dashboard(request), user) + def test_signin_fails_if_account_not_active(self): + _, strategy = self.get_request_and_strategy( + auth_entry=pipeline.AUTH_ENTRY_LOGIN, redirect_uri='social:complete') + strategy.backend.auth_complete = mock.MagicMock(return_value=self.fake_auth_complete(strategy)) + user = self.create_user_models_for_existing_account(strategy, 'user@example.com', 'password', self.get_username()) + + user.is_active = False + user.save() + + self.assert_json_failure_response_is_inactive_account(student_views.login_user(strategy.request)) + def test_signin_fails_if_no_account_associated(self): _, strategy = self.get_request_and_strategy( auth_entry=pipeline.AUTH_ENTRY_LOGIN, redirect_uri='social:complete') @@ -620,6 +640,14 @@ class IntegrationTest(testutil.TestCase, test.TestCase): created_user = self.get_user_by_email(strategy, email) self.assert_password_overridden_by_pipeline(overridden_password, created_user.username) + # The user's account isn't created yet, so an attempt to complete the + # pipeline will error out on /login: + self.assert_redirect_to_login_looks_correct( + actions.do_complete(strategy, social_views._do_login, user=created_user)) + # So we activate the account in order to verify the redirect to /dashboard: + created_user.is_active = True + created_user.save() + # Last step in the pipeline: we re-invoke the pipeline and expect to # end up on /dashboard, with the correct social auth object now in the # backend and the correct user's data on display.