Merge pull request #10302 from open-craft/sso-login-unactivated

SSO - Allow users to sign in with unactivated accounts (SOL-1324)

common/djangoapps/third_party_auth/pipeline.py

@@ -151,19 +151,6 @@ class AuthEntryError(AuthException):
     """
 
 
-class NotActivatedException(AuthException):
-    """ Raised when a user tries to login to an unverified account """
-    def __init__(self, backend, email):
-        self.email = email
-        super(NotActivatedException, self).__init__(backend, email)
-
-    def __str__(self):
-        return (
-            _('This account has not yet been activated. An activation email has been re-sent to {email_address}.')
-            .format(email_address=self.email)
-        )
-
-
 class ProviderUserState(object):
     """Object representing the provider state (attached or not) for a user.
 
@@ -514,26 +501,27 @@ def ensure_user_information(strategy, auth_entry, backend=None, user=None, socia
             # This parameter is used by the auth_exchange app, which always allows users to
             # login, whether or not their account is validated.
             pass
-        # IF the user has just registered a new account as part of this pipeline, that is fine
-        # and we allow the login to continue this once, because if we pause again to force the
-        # user to activate their account via email, the pipeline may get lost (e.g. email takes
-        # too long to arrive, user opens the activation email on a different device, etc.).
-        # This is consistent with first party auth and ensures that the pipeline completes
-        # fully, which is critical.
-        # But if this is an existing account, we refuse to allow them to login again until they
-        # check their email and activate the account.
-        elif social is not None:
-            # This third party account is already linked to a user account. That means that the
-            # user's account existed before this pipeline originally began (since the creation
-            # of the 'social' link entry occurs in one of the following pipeline steps).
-            # Reject this login attempt and tell the user to validate their account first.
-
-            # Send them another activation email:
-            student.views.reactivation_email_for_user(user)
-
-            raise NotActivatedException(backend, user.email)
-        # else: The user must have just successfully registered their account, so we proceed.
-        # We know they did not just login, because the login process rejects unverified users.
+        elif social is None:
+            # The user has just registered a new account as part of this pipeline. Their account
+            # is inactive but we allow the login to continue, because if we pause again to force
+            # the user to activate their account via email, the pipeline may get lost (e.g.
+            # email takes too long to arrive, user opens the activation email on a different
+            # device, etc.). This is consistent with first party auth and ensures that the
+            # pipeline completes fully, which is critical.
+            pass
+        else:
+            # This is an existing account, linked to a third party provider but not activated.
+            # Double-check these criteria:
+            assert user is not None
+            assert social is not None
+            # We now also allow them to login again, because if they had entered their email
+            # incorrectly then there would be no way for them to recover the account, nor
+            # register anew via SSO. See SOL-1324 in JIRA.
+            # However, we will log a warning for this case:
+            logger.warning(
+                'User "%s" is using third_party_auth to login but has not yet activated their account. ',
+                user.username
+            )
 
 
 @partial.partial

common/djangoapps/third_party_auth/settings.py

@@ -73,11 +73,16 @@ def apply_settings(django_settings):
     django_settings.SOCIAL_AUTH_RAISE_EXCEPTIONS = False
 
     # Allow users to login using social auth even if their account is not verified yet
-    # The 'ensure_user_information' step controls this and only allows brand new users
-    # to login without verification. Repeat logins are not permitted until the account
-    # gets verified.
-    django_settings.INACTIVE_USER_LOGIN = True
-    django_settings.INACTIVE_USER_URL = '/auth/inactive'
+    # This is required since we [ab]use django's 'is_active' flag to indicate verified
+    # accounts; without this set to True, python-social-auth won't allow us to link the
+    # user's account to the third party account during registration (since the user is
+    # not verified at that point).
+    # We also generally allow unverified third party auth users to login (see the logic
+    # in ensure_user_information in pipeline.py) because otherwise users who use social
+    # auth to register with an invalid email address can become "stuck".
+    # TODO: Remove the following if/when email validation is separated from the is_active flag.
+    django_settings.SOCIAL_AUTH_INACTIVE_USER_LOGIN = True
+    django_settings.SOCIAL_AUTH_INACTIVE_USER_URL = '/auth/inactive'
 
     # Context processors required under Django.
     django_settings.SOCIAL_AUTH_UUID_LENGTH = 4

common/djangoapps/third_party_auth/tests/specs/test_generic.py

+"""
+Use the 'Dummy' auth provider for generic integration tests of third_party_auth.
+"""
+import unittest
+from third_party_auth.tests import testutil
+
+from .base import IntegrationTestMixin
+
+
+@unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, 'third_party_auth not enabled')
+class GenericIntegrationTest(IntegrationTestMixin, testutil.TestCase):
+    """
+    Basic integration tests of third_party_auth using Dummy provider
+    """
+    PROVIDER_ID = "oa2-dummy"
+    PROVIDER_NAME = "Dummy"
+    PROVIDER_BACKEND = "dummy"
+
+    USER_EMAIL = "adama@fleet.colonies.gov"
+    USER_NAME = "William Adama"
+    USER_USERNAME = "Galactica1"
+
+    def setUp(self):
+        super(GenericIntegrationTest, self).setUp()
+        self.configure_dummy_provider(enabled=True)
+
+    def do_provider_login(self, provider_redirect_url):
+        """
+        Mock logging in to the Dummy provider
+        """
+        # For the Dummy provider, the provider redirect URL is self.complete_url
+        self.assertEqual(provider_redirect_url, self.url_prefix + self.complete_url)
+        return self.client.get(provider_redirect_url)

common/djangoapps/third_party_auth/tests/specs/test_lti.py

@@ -29,6 +29,8 @@ class IntegrationTestLTI(testutil.TestCase):
 
     def setUp(self):
         super(IntegrationTestLTI, self).setUp()
+        self.client.defaults['SERVER_NAME'] = 'testserver'
+        self.url_prefix = 'http://testserver'
         self.configure_lti_provider(
             name='Other Tool Consumer 1', enabled=True,
             lti_consumer_key='other1',

common/djangoapps/third_party_auth/tests/testutil.py

@@ -10,6 +10,7 @@ from django.contrib.auth.models import User
 from provider.oauth2.models import Client as OAuth2Client
 from provider import constants
 import django.test
+from mako.template import Template
 import mock
 import os.path
 
@@ -27,6 +28,18 @@ AUTH_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH'
 AUTH_FEATURE_ENABLED = AUTH_FEATURES_KEY in settings.FEATURES
 
 
+def patch_mako_templates():
+    """ Patch mako so the django test client can access template context """
+    orig_render = Template.render_unicode
+
+    def wrapped_render(*args, **kwargs):
+        """ Render the template and send the context info to any listeners that want it """
+        django.test.signals.template_rendered.send(sender=None, template=None, context=kwargs)
+        return orig_render(*args, **kwargs)
+
+    return mock.patch.multiple(Template, render_unicode=wrapped_render, render=wrapped_render)
+
+
 class FakeDjangoSettings(object):
     """A fake for Django settings."""
 
@@ -110,6 +123,13 @@ class ThirdPartyAuthTestMixin(object):
         return cls.configure_oauth_provider(**kwargs)
 
     @classmethod
+    def configure_dummy_provider(cls, **kwargs):
+        """ Update the settings for the Twitter third party auth provider/backend """
+        kwargs.setdefault("name", "Dummy")
+        kwargs.setdefault("backend_name", "dummy")
+        return cls.configure_oauth_provider(**kwargs)
+
+    @classmethod
     def verify_user_email(cls, email):
         """ Mark the user with the given email as verified """
         user = User.objects.get(email=email)
@@ -135,19 +155,18 @@ class ThirdPartyAuthTestMixin(object):
 
 class TestCase(ThirdPartyAuthTestMixin, django.test.TestCase):
     """Base class for auth test cases."""
-    pass
+    def setUp(self):
+        super(TestCase, self).setUp()
+        # Explicitly set a server name that is compatible with all our providers:
+        # (The SAML lib we use doesn't like the default 'testserver' as a domain)
+        self.client.defaults['SERVER_NAME'] = 'example.none'
+        self.url_prefix = 'http://example.none'
 
 
 class SAMLTestCase(TestCase):
     """
     Base class for SAML-related third_party_auth tests
     """
-
-    def setUp(self):
-        super(SAMLTestCase, self).setUp()
-        self.client.defaults['SERVER_NAME'] = 'example.none'  # The SAML lib we use doesn't like testserver' as a domain
-        self.url_prefix = 'http://example.none'
-
     @classmethod
     def _get_public_key(cls, key_name='saml_key'):
         """ Get a public key for use in the test. """

common/djangoapps/third_party_auth/urls.py

@@ -6,7 +6,7 @@ from .views import inactive_user_view, saml_metadata_view, lti_login_and_complet
 
 urlpatterns = patterns(
     '',
-    url(r'^auth/inactive', inactive_user_view),
+    url(r'^auth/inactive', inactive_user_view, name="third_party_inactive_redirect"),
     url(r'^auth/saml/metadata.xml', saml_metadata_view),
     url(r'^auth/login/(?P<backend>lti)/$', lti_login_and_complete_view),
     url(r'^auth/', include('social.apps.django_app.urls', namespace='social')),

common/djangoapps/third_party_auth/views.py

@@ -17,8 +17,13 @@ URL_NAMESPACE = getattr(settings, setting_name('URL_NAMESPACE'), None) or 'socia
 
 def inactive_user_view(request):
     """
-    A newly registered user has completed the social auth pipeline.
-    Their account is not yet activated, but we let them login this once.
+    A newly or recently registered user has completed the social auth pipeline.
+    Their account is not yet activated, but we let them login since the third party auth
+    provider is trusted to vouch for them. See details in pipeline.py.
+
+    The reason this view exists is that if we don't define this as the
+    SOCIAL_AUTH_INACTIVE_USER_URL, inactive users will get sent to LOGIN_ERROR_URL, which we
+    don't want.
     """
     # 'next' may be set to '/account/finish_auth/.../' if this user needs to be auto-enrolled
     # in a course. Otherwise, just redirect them to the dashboard, which displays a message