From 3b400f0d0fa55cebeabe27e8ee9f9febfc07fa90 Mon Sep 17 00:00:00 2001 From: John Cox Date: Wed, 12 Mar 2014 12:38:58 -0700 Subject: [PATCH 01/18] Remove Mozilla Persona provider now that Mozilla stopped Persona development --- common/djangoapps/third_party_auth/provider.py | 7 ------- common/djangoapps/third_party_auth/tests/test_provider.py | 2 +- common/djangoapps/third_party_auth/tests/test_settings.py | 4 ++-- .../third_party_auth/tests/test_settings_integration.py | 5 ----- 4 files changed, 3 insertions(+), 15 deletions(-) diff --git a/common/djangoapps/third_party_auth/provider.py b/common/djangoapps/third_party_auth/provider.py index 1b1e17796d43..a82396f767fd 100644 --- a/common/djangoapps/third_party_auth/provider.py +++ b/common/djangoapps/third_party_auth/provider.py @@ -54,13 +54,6 @@ class LinkedInOauth2(BaseProvider): } -class MozillaPersona(BaseProvider): - """Provider for Mozilla's Persona auth system.""" - - AUTHENTICATION_BACKEND = 'social.backends.persona.PersonaAuth' - NAME = 'Mozilla Persona' - - class Registry(object): """Singleton registry of third-party auth providers. diff --git a/common/djangoapps/third_party_auth/tests/test_provider.py b/common/djangoapps/third_party_auth/tests/test_provider.py index 18774eb61a74..0bd3e5c90389 100644 --- a/common/djangoapps/third_party_auth/tests/test_provider.py +++ b/common/djangoapps/third_party_auth/tests/test_provider.py @@ -68,4 +68,4 @@ def test_get_returns_enabled_provider(self): def test_get_returns_none_if_provider_not_enabled(self): provider.Registry.configure_once([]) - self.assertIsNone(provider.Registry.get(provider.MozillaPersona.NAME)) + self.assertIsNone(provider.Registry.get(provider.LinkedInOauth2.NAME)) diff --git a/common/djangoapps/third_party_auth/tests/test_settings.py b/common/djangoapps/third_party_auth/tests/test_settings.py index 05f502928430..48db3fb3a99b 100644 --- a/common/djangoapps/third_party_auth/tests/test_settings.py +++ b/common/djangoapps/third_party_auth/tests/test_settings.py @@ -50,9 +50,9 @@ def test_apply_settings_initializes_stubs_and_merges_settings_from_auth_info(sel def test_apply_settings_prepends_auth_backends(self): self.assertEqual(_ORIGINAL_AUTHENTICATION_BACKENDS, self.settings.AUTHENTICATION_BACKENDS) - settings.apply_settings({provider.GoogleOauth2.NAME: {}, provider.MozillaPersona.NAME: {}}, self.settings) + settings.apply_settings({provider.GoogleOauth2.NAME: {}, provider.LinkedInOauth2.NAME: {}}, self.settings) self.assertEqual(( - provider.GoogleOauth2.AUTHENTICATION_BACKEND, provider.MozillaPersona.AUTHENTICATION_BACKEND) + + provider.GoogleOauth2.AUTHENTICATION_BACKEND, provider.LinkedInOauth2.AUTHENTICATION_BACKEND) + _ORIGINAL_AUTHENTICATION_BACKENDS, self.settings.AUTHENTICATION_BACKENDS) diff --git a/common/djangoapps/third_party_auth/tests/test_settings_integration.py b/common/djangoapps/third_party_auth/tests/test_settings_integration.py index 15cff5605a18..372fd48e6bfd 100644 --- a/common/djangoapps/third_party_auth/tests/test_settings_integration.py +++ b/common/djangoapps/third_party_auth/tests/test_settings_integration.py @@ -32,8 +32,3 @@ def test_can_enable_linkedin_oauth2(self): auth_settings.apply_settings({'LinkedIn': {'SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY': 'linkedin_key'}}, settings) self.assertEqual([provider.LinkedInOauth2], provider.Registry.enabled()) self.assertEqual('linkedin_key', settings.SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY) - - @mock.patch.dict(settings.FEATURES, {'ENABLE_THIRD_PARTY_ATUH': True}) - def test_can_enable_mozilla_persona(self): - auth_settings.apply_settings({'Mozilla Persona': {}}, settings) - self.assertEqual([provider.MozillaPersona], provider.Registry.enabled()) From b4b3d9f6ba4e0642b955d4e56a82fae6a59917e4 Mon Sep 17 00:00:00 2001 From: John Cox Date: Wed, 19 Mar 2014 14:29:42 -0700 Subject: [PATCH 02/18] Add new user creation feature --- common/djangoapps/student/views.py | 36 +++++- .../djangoapps/third_party_auth/pipeline.py | 117 +++++++++++++++++- .../djangoapps/third_party_auth/provider.py | 90 +++++++++++++- .../djangoapps/third_party_auth/settings.py | 44 +++++-- .../third_party_auth/tests/test_pipeline.py | 39 ++++++ .../third_party_auth/tests/test_provider.py | 17 ++- .../third_party_auth/tests/test_settings.py | 17 ++- .../tests/test_settings_integration.py | 7 +- .../third_party_auth/tests/testutil.py | 3 + common/djangoapps/third_party_auth/urls.py | 5 +- lms/templates/register.html | 52 +++++++- 11 files changed, 398 insertions(+), 29 deletions(-) create mode 100644 common/djangoapps/third_party_auth/tests/test_pipeline.py diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index d2e8b32d6b25..7e86ab1440c4 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -85,6 +85,9 @@ validate_password_dictionary ) +from third_party_auth import pipeline +from third_party_auth import provider + log = logging.getLogger("edx.student") AUDIT_LOG = logging.getLogger("audit") @@ -383,19 +386,37 @@ def register_user(request, extra_context=None): # and registration is disabled. return redirect(reverse('root')) + pipeline_running = pipeline.running(request) + context = { 'course_id': request.GET.get('course_id'), + 'email': '', 'enrollment_action': request.GET.get('enrollment_action'), + 'name': '', + 'pipeline_running': pipeline_running, 'platform_name': microsite.get_value( 'platform_name', settings.PLATFORM_NAME ), + 'selected_provider': '', + 'username': '', } + if extra_context is not None: context.update(extra_context) if context.get("extauth_domain", '').startswith(external_auth.views.SHIBBOLETH_DOMAIN_PREFIX): return render_to_response('register-shib.html', context) + + # If third-party auth is enabled, prepopulate the form with data from the + # selected provider. + if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline_running: + running_pipeline = pipeline.get(request) + current_provider = provider.Registry.get_by_backend_name(running_pipeline.get('backend')) + overrides = current_provider.get_register_form_data(running_pipeline.get('kwargs')) + overrides['selected_provider'] = current_provider.NAME + context.update(overrides) + return render_to_response('register.html', context) @@ -1037,6 +1058,11 @@ def create_account(request, post_override=None): post_vars = post_override if post_override else request.POST extra_fields = getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) + if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request): + post_vars = dict(post_vars.items()) + overrides = {'password': pipeline.make_random_password()} + post_vars.update(overrides) + # if doing signup for an external authorization, then get email, password, name from the eamap # don't use the ones from the form, since the user could have hacked those # unless originally we didn't get a valid email or name from the external auth @@ -1234,9 +1260,17 @@ def create_account(request, post_override=None): login_user.save() AUDIT_LOG.info(u"Login activated on extauth account - {0} ({1})".format(login_user.username, login_user.email)) + dog_stats_api.increment("common.student.account_created") + redirect_url = try_change_enrollment(request) + + # Resume the third-party-auth pipeline if necessary. + if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request): + running_pipeline = pipeline.get(request) + redirect_url = pipeline.get_complete_url(running_pipeline['backend']) + response = JsonResponse({ 'success': True, - 'redirect_url': try_change_enrollment(request), + 'redirect_url': redirect_url, }) # set the login cookie for the edx marketing site diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 1f9c56906c19..b7d82c078ed5 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -1,9 +1,118 @@ -"""Auth pipeline definitions.""" +"""Auth pipeline definitions. +Gotcha alert!: + +Bear in mind that when pausing and resuming a pipeline function decorated with +@partial.partial, execution resumes by re-invoking the decorated function +instead of invoking the next function in the pipeline stack. For example, if +you have a pipeline of + + A + B + C + +with an implementation of + + @partial.partial + def B(*args, **kwargs): + [...] + +B will be invoked twice: once when initially proceeding through the pipeline +before it is paused, and and once when other code finishes and the pipeline +resumes. Consequently, many decorated functions will first invoke a predicate +to determine if they are in their first or second execution (usually by +checking side-effects from the first run). + +This is surprising but important behavior, since it allows a single function in +the pipeline to consolidate all the operations needed to establish invariants +rather than spreading them across two functions in the pipeline. + +See http://psa.matiasaguirre.net/docs/pipeline.html for more docs. +""" + +import random +# string is deprecated, but it's still the surest way to get these charsets. +# pylint: disable-msg=deprecated-module +import string + +from django.core.urlresolvers import reverse +from django.shortcuts import redirect from social.pipeline import partial +from . import provider + + +# Int. Default size for randomly-generated passwords. +_DEFAULT_RANDOM_PASSWORD_LENGTH = 12 +# String. The characters available to the random password generator. Must pass +# the ORM's validation routines. +_PASSWORD_CHARSET = string.letters + string.digits + + +def get(request): + """Gets the running pipeline from `request`.""" + return request.session.get('partial_pipeline') + + +def _get_url(view_name, backend_name): + """Protected wrapper for reverse().""" + return reverse(view_name, kwargs={'backend': backend_name}) + +def get_complete_url(backend_name): + """Gets URL for the endpoint that returns control to the auth pipeline. + + Arg is `backend_name`, the name of the python-social-auth backend from the + running pipeline (for example, 'google-oauth2'). Returns string. + """ + return _get_url('social:complete', backend_name) + + +def get_login_url(provider_name): + """Gets the login URL for the endpoint that kicks of auth with a provider. + + Args are `provider_name`, a string containing the name of a + provider.Provider that has been enabled. Returns string. + """ + enabled_provider = provider.Registry.get(provider_name) + if not enabled_provider: + raise ValueError('Provider %s not enabled' % provider_name) + return _get_url('social:begin', enabled_provider.BACKEND_CLASS.name) + + +def make_random_password(length=None, seed=None): + """Makes a random password. + + When a user creates an account via a social provider, we need to create a + placeholder password for them to satisfy the ORM's consistency and + validation requirements. Users don't know (and hence cannot sign in with) + this password; that's OK because they can always use the reset password + flow to set it to a known value. + + Args are `length`, an int that determines the number of chars in the + returned value, and `seed`, a hashable object used to initialize the RNG. + `seed` is for tests only; do not pass it otherwise. + """ + if length is None: + length = _DEFAULT_RANDOM_PASSWORD_LENGTH + + if seed is not None: + random.seed(seed) + + return ''.join(random.choice(_PASSWORD_CHARSET) for _ in xrange(length)) + + +def running(request): + """Returns True iff `request` is running a third-party auth pipeline.""" + return request.session.get('partial_pipeline') is not None # Avoid False for {}. + + +# Signature set by framework; prepending 'unused_' causes TypeError on dispatch +# to the auth backend's authenticate(). pylint: disable-msg=unused-argument @partial.partial -def step(*args, **kwargs): - """Fake pipeline step; just throws loudly for now.""" - raise NotImplementedError('%s, %s' % (args, kwargs)) +def redirect_to_supplementary_form(strategy, details, response, uid, user=None, *args, **kwargs): + """Cut point that dispatches user to a create account or sign in form.""" + if user is not None: + return + + return redirect('/register', name='register_user') diff --git a/common/djangoapps/third_party_auth/provider.py b/common/djangoapps/third_party_auth/provider.py index a82396f767fd..598601791127 100644 --- a/common/djangoapps/third_party_auth/provider.py +++ b/common/djangoapps/third_party_auth/provider.py @@ -4,6 +4,9 @@ invoke the Django armature. """ +from social.backends import google +from social.backends import linkedin + class BaseProvider(object): """Abstract base class for third-party auth providers. @@ -12,11 +15,10 @@ class BaseProvider(object): in the provider Registry. """ - # String. Dot-delimited module.Class. The name of the backend - # implementation to load. - AUTHENTICATION_BACKEND = None + # Class. The provider's backing social.backends.base.BaseAuth child. + BACKEND_CLASS = None # String. User-facing name of the provider. Must be unique across all - # enabled providers. + # enabled providers. Will be presented in the UI. NAME = None # Dict of string -> object. Settings that will be merged into Django's @@ -25,6 +27,54 @@ class BaseProvider(object): # settings instance during application initialization. SETTINGS = {} + @classmethod + def get_authentication_backend(cls): + """Gets associated Django settings.AUTHENTICATION_BACKEND string.""" + return '%s.%s' % (cls.BACKEND_CLASS.__module__, cls.BACKEND_CLASS.__name__) + + @classmethod + def get_email(cls, unused_provider_details): + """Gets email address string from `provider_details` dict. + + Provider responses can contain arbitrary data. This method can be + overridden to extract an email address from the provider details + extracted by the social_details pipeline step. + """ + return None + + @classmethod + def get_name(cls, unused_provider_details): + """Gets name string from `provider_details` dict. + + Provider responses can contain arbitrary data. This method can be + overridden to extract a full name for a user from the provider details + extracted by the social_details pipeline step. + """ + return None + + @classmethod + def get_register_form_data(cls, pipeline_kwargs): + """Gets dict of data to display on the register form. + + common.djangoapps.student.views.register_user uses this to populate the + new account creation form with values supplied by the user's chosen + provider, preventing duplicate data entry. + """ + # Details about the user sent back from the provider. + details = pipeline_kwargs.get('details') + # Get the username separately to take advantage of the de-duping logic + # built into the pipeline. The provider cannot de-dupe because it can't + # check the state of taken usernames in our system. Note that there is + # technically a data race between the creation of this value and the + # creation of the user object, so it is still possible for users to get + # an error on submit. + suggested_username = pipeline_kwargs.get('username') + return { + 'email': cls.get_email(details) or '', + 'name': cls.get_name(details) or '', + 'username': suggested_username, + } + @classmethod def merge_onto(cls, settings): """Merge class-level settings onto a django `settings` module.""" @@ -35,24 +85,40 @@ def merge_onto(cls, settings): class GoogleOauth2(BaseProvider): """Provider for Google's Oauth2 auth system.""" - AUTHENTICATION_BACKEND = 'social.backends.google.GoogleOAuth2' + BACKEND_CLASS = google.GoogleOAuth2 NAME = 'Google' SETTINGS = { 'SOCIAL_AUTH_GOOGLE_OAUTH2_KEY': None, 'SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET': None, } + @classmethod + def get_email(cls, provider_details): + return provider_details.get('email') + + @classmethod + def get_name(cls, provider_details): + return provider_details.get('fullname') + class LinkedInOauth2(BaseProvider): """Provider for LinkedIn's Oauth2 auth system.""" - AUTHENTICATION_BACKEND = 'social.backends.linkedin.LinkedinOAuth2' + BACKEND_CLASS = linkedin.LinkedinOAuth2 NAME = 'LinkedIn' SETTINGS = { 'SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY': None, 'SOCIAL_AUTH_LINKEDIN_OAUTH2_SECRET': None, } + @classmethod + def get_email(cls, provider_details): + return provider_details.get('email') + + @classmethod + def get_name(cls, provider_details): + return provider_details.get('fullname') + class Registry(object): """Singleton registry of third-party auth providers. @@ -111,6 +177,18 @@ def get(cls, provider_name): cls._check_configured() return cls._ENABLED.get(provider_name) + @classmethod + def get_by_backend_name(cls, backend_name): + """Gets provider (or None) by backend name. + + Arg is string `backend_name`, a python-social-auth + backends.base.BaseAuth.name (for example, 'google-oauth2'). + """ + cls._check_configured() + for enabled in cls._ENABLED.values(): + if enabled.BACKEND_CLASS.name == backend_name: + return enabled + @classmethod def _reset(cls): """Returns the registry to an unconfigured state; for tests only.""" diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index 0ff55e1213d8..f1ca00eb3846 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -45,6 +45,11 @@ from . import provider +# Tuple of string. Additional Django middleware classes. +_MIDDLEWARE_CLASSES = ( + 'social.apps.django_app.middleware.SocialAuthExceptionMiddleware', +) + def _merge_auth_info(django_settings, auth_info): """Merge `auth_info` dict onto `django_settings` module.""" @@ -71,14 +76,39 @@ def _set_global_settings(django_settings): 'social.apps.django_app.default', 'third_party_auth', ) - django_settings.TEMPLATE_CONTEXT_PROCESSORS += ( - 'social.apps.django_app.context_processors.backends', - 'social.apps.django_app.context_processors.login_redirect', - ) + # Inject exception middleware to make redirects fire. + django_settings.MIDDLEWARE_CLASSES += _MIDDLEWARE_CLASSES + # Where to send the user if there's an error during social authentication. + # Details about the error will be added to this URL as args so they will be + # logged. + django_settings.SOCIAL_AUTH_LOGIN_ERROR_URL = '/register' + # Where to send the user once social authentication is successful. + django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard' # Inject our customized auth pipeline. All auth backends must work with # this pipeline. django_settings.SOCIAL_AUTH_PIPELINE = ( - 'third_party_auth.pipeline.step', + 'social.pipeline.social_auth.social_details', + 'social.pipeline.social_auth.social_uid', + 'social.pipeline.social_auth.auth_allowed', + 'social.pipeline.social_auth.social_user', + 'social.pipeline.user.get_username', + 'third_party_auth.pipeline.redirect_to_supplementary_form', + 'social.pipeline.user.create_user', + 'social.pipeline.social_auth.associate_user', + 'social.pipeline.social_auth.load_extra_data', + 'social.pipeline.user.user_details', + ) + # We let the user specify their email address during signup. + django_settings.SOCIAL_AUTH_PROTECTED_USER_FIELDS = ['email'] + # Disable exceptions by default for prod so you get redirect behavior + # instead of a Django error page. During development you may want to + # enable this when you want to get stack traces rather than redirections. + django_settings.SOCIAL_AUTH_RAISE_EXCEPTIONS = False + # Context processors required under Django. + django_settings.SOCIAL_AUTH_UUID_LENGTH = 4 + django_settings.TEMPLATE_CONTEXT_PROCESSORS += ( + 'social.apps.django_app.context_processors.backends', + 'social.apps.django_app.context_processors.login_redirect', ) @@ -86,14 +116,14 @@ def _set_provider_settings(django_settings, enabled_providers, auth_info): """Set provider-specific settings.""" # Must prepend here so we get called first. django_settings.AUTHENTICATION_BACKENDS = ( - tuple(enabled_provider.AUTHENTICATION_BACKEND for enabled_provider in enabled_providers) + + tuple(enabled_provider.get_authentication_backend() for enabled_provider in enabled_providers) + django_settings.AUTHENTICATION_BACKENDS) # Merge settings from provider classes, and configure all placeholders. for enabled_provider in enabled_providers: enabled_provider.merge_onto(django_settings) - # Merge settings from .auth.json. + # Merge settings from .auth.json, overwriting placeholders. _merge_auth_info(django_settings, auth_info) diff --git a/common/djangoapps/third_party_auth/tests/test_pipeline.py b/common/djangoapps/third_party_auth/tests/test_pipeline.py new file mode 100644 index 000000000000..f4ca008a9e1d --- /dev/null +++ b/common/djangoapps/third_party_auth/tests/test_pipeline.py @@ -0,0 +1,39 @@ +""" +Unit tests for third_party_auth/pipeline.py. +""" + +import random + +from third_party_auth import pipeline +from third_party_auth.tests import testutil + + +# Allow tests access to protected methods (or module-protected methods) under +# test. pylint: disable-msg=protected-access + + +class MakeRandomPasswordTest(testutil.TestCase): + """Tests formation of random placeholder passwords.""" + + def tearDown(self): + random.seed() # Make random random again. + super(MakeRandomPasswordTest, self).tearDown() + + def test_custom_length(self): + custom_length = 20 + self.assertEqual(custom_length, len(pipeline.make_random_password(length=custom_length))) + + def test_default_length(self): + self.assertEqual(pipeline._DEFAULT_RANDOM_PASSWORD_LENGTH, len(pipeline.make_random_password())) + + def test_probably_only_uses_charset(self): + # This is ultimately probablistic since we could randomly select a good character 100000 consecutive times. + for char in pipeline.make_random_password(length=100000): + self.assertIn(char, pipeline._PASSWORD_CHARSET) + + def test_pseudorandomly_picks_chars_from_charset(self): + seed = 1 + random.seed(seed) + expected = ''.join( + random.choice(pipeline._PASSWORD_CHARSET) for _ in xrange(pipeline._DEFAULT_RANDOM_PASSWORD_LENGTH)) + self.assertEqual(expected, pipeline.make_random_password(seed=seed)) diff --git a/common/djangoapps/third_party_auth/tests/test_provider.py b/common/djangoapps/third_party_auth/tests/test_provider.py index 0bd3e5c90389..9f9320dbcc2a 100644 --- a/common/djangoapps/third_party_auth/tests/test_provider.py +++ b/common/djangoapps/third_party_auth/tests/test_provider.py @@ -10,8 +10,7 @@ class RegistryTest(testutil.TestCase): """Tests registry discovery and operation.""" # Allow access to protected methods (or module-protected methods) under - # test. - # pylint: disable-msg=protected-access + # test. pylint: disable-msg=protected-access def test_calling_configure_once_twice_raises_value_error(self): provider.Registry.configure_once([provider.GoogleOauth2.NAME]) @@ -69,3 +68,17 @@ def test_get_returns_enabled_provider(self): def test_get_returns_none_if_provider_not_enabled(self): provider.Registry.configure_once([]) self.assertIsNone(provider.Registry.get(provider.LinkedInOauth2.NAME)) + + def test_get_by_backend_name_raises_runtime_error_if_not_configured(self): + with self.assertRaisesRegexp(RuntimeError, '^.*not configured$'): + provider.Registry.get_by_backend_name('') + + def test_get_by_backend_name_returns_enabled_provider(self): + provider.Registry.configure_once([provider.GoogleOauth2.NAME]) + self.assertIs( + provider.GoogleOauth2, + provider.Registry.get_by_backend_name(provider.GoogleOauth2.BACKEND_CLASS.name)) + + def test_get_by_backend_name_returns_none_if_provider_not_enabled(self): + provider.Registry.configure_once([]) + self.assertIsNone(provider.Registry.get_by_backend_name(provider.GoogleOauth2.BACKEND_CLASS.name)) diff --git a/common/djangoapps/third_party_auth/tests/test_settings.py b/common/djangoapps/third_party_auth/tests/test_settings.py index 48db3fb3a99b..eda1cd4a1bb7 100644 --- a/common/djangoapps/third_party_auth/tests/test_settings.py +++ b/common/djangoapps/third_party_auth/tests/test_settings.py @@ -9,10 +9,12 @@ _ORIGINAL_AUTHENTICATION_BACKENDS = ('first_authentication_backend',) _ORIGINAL_INSTALLED_APPS = ('first_installed_app',) +_ORIGINAL_MIDDLEWARE_CLASSES = ('first_middleware_class',) _ORIGINAL_TEMPLATE_CONTEXT_PROCESSORS = ('first_template_context_preprocessor',) _SETTINGS_MAP = { 'AUTHENTICATION_BACKENDS': _ORIGINAL_AUTHENTICATION_BACKENDS, 'INSTALLED_APPS': _ORIGINAL_INSTALLED_APPS, + 'MIDDLEWARE_CLASSES': _ORIGINAL_MIDDLEWARE_CLASSES, 'TEMPLATE_CONTEXT_PROCESSORS': _ORIGINAL_TEMPLATE_CONTEXT_PROCESSORS, } @@ -20,6 +22,8 @@ class SettingsUnitTest(testutil.TestCase): """Unit tests for settings management code.""" + # Allow access to protected methods (or module-protected methods) under + # test. pylint: disable-msg=protected-access # Suppress sprurious no-member warning on fakes. # pylint: disable-msg=no-member @@ -27,6 +31,11 @@ def setUp(self): super(SettingsUnitTest, self).setUp() self.settings = testutil.FakeDjangoSettings(_SETTINGS_MAP) + 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) + def test_apply_settings_adds_third_party_auth_to_installed_apps(self): settings.apply_settings({}, self.settings) self.assertIn('third_party_auth', self.settings.INSTALLED_APPS) @@ -52,7 +61,7 @@ def test_apply_settings_prepends_auth_backends(self): self.assertEqual(_ORIGINAL_AUTHENTICATION_BACKENDS, self.settings.AUTHENTICATION_BACKENDS) settings.apply_settings({provider.GoogleOauth2.NAME: {}, provider.LinkedInOauth2.NAME: {}}, self.settings) self.assertEqual(( - provider.GoogleOauth2.AUTHENTICATION_BACKEND, provider.LinkedInOauth2.AUTHENTICATION_BACKEND) + + provider.GoogleOauth2.get_authentication_backend(), provider.LinkedInOauth2.get_authentication_backend()) + _ORIGINAL_AUTHENTICATION_BACKENDS, self.settings.AUTHENTICATION_BACKENDS) @@ -66,3 +75,9 @@ def test_apply_settings_raises_value_error_if_provider_contains_uninitialized_se } with self.assertRaisesRegexp(ValueError, '^.*not initialized$'): settings.apply_settings(auth_info, self.settings) + + def test_apply_settings_turns_off_raising_social_exceptions(self): + # Guard against submitting a conf change that's convenient in dev but + # bad in prod. + settings.apply_settings({}, self.settings) + self.assertFalse(self.settings.SOCIAL_AUTH_RAISE_EXCEPTIONS) diff --git a/common/djangoapps/third_party_auth/tests/test_settings_integration.py b/common/djangoapps/third_party_auth/tests/test_settings_integration.py index 372fd48e6bfd..a544569a4b32 100644 --- a/common/djangoapps/third_party_auth/tests/test_settings_integration.py +++ b/common/djangoapps/third_party_auth/tests/test_settings_integration.py @@ -11,15 +11,14 @@ from third_party_auth import settings as auth_settings from third_party_auth.tests import testutil -_AUTH_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH' - class SettingsIntegrationTest(testutil.TestCase): """Integration tests of auth settings pipeline.""" - @unittest.skipUnless(_AUTH_FEATURES_KEY in settings.FEATURES, _AUTH_FEATURES_KEY + ' not in settings.FEATURES') + @unittest.skipUnless( + testutil.AUTH_FEATURES_KEY in settings.FEATURES, testutil.AUTH_FEATURES_KEY + ' not in settings.FEATURES') def test_enable_third_party_auth_is_disabled_by_default(self): - self.assertIs(False, settings.FEATURES.get(_AUTH_FEATURES_KEY)) + self.assertIs(False, settings.FEATURES.get(testutil.AUTH_FEATURES_KEY)) @mock.patch.dict(settings.FEATURES, {'ENABLE_THIRD_PARTY_AUTH': True}) def test_can_enable_google_oauth2(self): diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index a431aaca02fd..1800c3c62de6 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -8,6 +8,9 @@ from third_party_auth import provider +# String. settings.FEATURES key for the boolean that enables third_party_auth. +AUTH_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH' + class FakeDjangoSettings(object): """A fake for Django settings.""" diff --git a/common/djangoapps/third_party_auth/urls.py b/common/djangoapps/third_party_auth/urls.py index 05322a680d2a..461ec8fc3be8 100644 --- a/common/djangoapps/third_party_auth/urls.py +++ b/common/djangoapps/third_party_auth/urls.py @@ -2,6 +2,9 @@ from django.conf.urls import include, patterns, url +# String. Base of URL path component. +_PATH_BASE = 'auth/' + urlpatterns = patterns( - '', url(r'^auth/', include('social.apps.django_app.urls', namespace='social')), + '', url(r'^' + _PATH_BASE, include('social.apps.django_app.urls', namespace='social')), ) diff --git a/lms/templates/register.html b/lms/templates/register.html index e8c818f2a583..56d48c270c01 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -12,6 +12,8 @@ <%! from django.utils.translation import ugettext as _ %> <%! from student.models import UserProfile %> <%! from datetime import date %> +<%! from third_party_auth import pipeline %> +<%! from third_party_auth import provider %> <%! import calendar %> <%block name="pagetitle">${_("Register for {platform_name}").format(platform_name=platform_name)} @@ -110,11 +112,42 @@

${_("The following errors occurred while processing yo
+ % if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH'): + + % if not pipeline_running: + +

+ ${_("Register to start learning today!")} +

+ +
    + % for enabled in provider.Registry.enabled(): +
  1. ${_("Sign in with " + enabled.NAME)} + % endfor +
+ +

+ ${_('or create your own {platform_name} account by completing all required* fields below.').format(platform_name=platform_name)} +

+ + % else: + +

+ ${_("You've successfully signed in with {selected_provider}.").format(selected_provider=selected_provider)}
+ ${_("Finish your account registration below to start learning.")} +

+ + % endif + + % else: +

${_("Please complete the following fields to register for an account. ")}
${_('Required fields are noted by bold text and an asterisk (*).')}

+ % endif +

${_('Required Information')}

@@ -123,20 +156,33 @@

${_('Required Information')}

  1. - + +
    2. + + % if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline_running: + +
  2. + +
    3. + + % else: +
    4. + + % endif +
  4. - + ${_('Will be shown in any discussions or forums you participate in')} (${_('cannot be changed later')})
  5. - + ${_("Needed for any certificates you may earn")}
From c937d830b6912b47f95c4bff740f728a98372ce6 Mon Sep 17 00:00:00 2001 From: John Cox Date: Thu, 20 Mar 2014 15:17:22 -0700 Subject: [PATCH 03/18] Update AUTHORS and use .format in template --- AUTHORS | 2 ++ lms/templates/register.html | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 86c77ad8d566..b7f73af4e0d5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -140,3 +140,5 @@ Muhammad Ammar William Desloge Marco Re Jonas Jelten +John Cox + diff --git a/lms/templates/register.html b/lms/templates/register.html index 56d48c270c01..3af58c877664 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -122,7 +122,7 @@

${_("The following errors occurred while processing yo
    % for enabled in provider.Registry.enabled(): -
  1. ${_("Sign in with " + enabled.NAME)} +
  2. ${_("Sign in with {provider_name}").format(provider_name=enabled.NAME)} % endfor
From 972ff0c4a8da61122d3b9045acba3ce6985cbc7b Mon Sep 17 00:00:00 2001 From: John Cox Date: Mon, 24 Mar 2014 13:35:45 -0700 Subject: [PATCH 04/18] Address review comments --- common/djangoapps/student/views.py | 6 +-- .../djangoapps/third_party_auth/pipeline.py | 51 ++++++++++--------- .../djangoapps/third_party_auth/provider.py | 48 +++++++++++++---- .../djangoapps/third_party_auth/settings.py | 13 +++-- .../third_party_auth/tests/test_pipeline.py | 19 +++---- .../third_party_auth/tests/test_settings.py | 3 +- .../third_party_auth/tests/testutil.py | 2 +- common/djangoapps/third_party_auth/urls.py | 4 +- lms/templates/register.html | 3 +- 9 files changed, 90 insertions(+), 59 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 7e86ab1440c4..2067f22e0e2d 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -85,8 +85,7 @@ validate_password_dictionary ) -from third_party_auth import pipeline -from third_party_auth import provider +from third_party_auth import pipeline, provider log = logging.getLogger("edx.student") AUDIT_LOG = logging.getLogger("audit") @@ -1060,8 +1059,7 @@ def create_account(request, post_override=None): if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request): post_vars = dict(post_vars.items()) - overrides = {'password': pipeline.make_random_password()} - post_vars.update(overrides) + post_vars.update({'password': pipeline.make_random_password()}) # if doing signup for an external authorization, then get email, password, name from the eamap # don't use the ones from the form, since the user could have hacked those diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index b7d82c078ed5..75182f4dbe21 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -18,7 +18,7 @@ def B(*args, **kwargs): [...] B will be invoked twice: once when initially proceeding through the pipeline -before it is paused, and and once when other code finishes and the pipeline +before it is paused, and once when other code finishes and the pipeline resumes. Consequently, many decorated functions will first invoke a predicate to determine if they are in their first or second execution (usually by checking side-effects from the first run). @@ -31,9 +31,7 @@ def B(*args, **kwargs): """ import random -# string is deprecated, but it's still the surest way to get these charsets. -# pylint: disable-msg=deprecated-module -import string +import string # pylint: disable-msg=deprecated-module from django.core.urlresolvers import reverse from django.shortcuts import redirect @@ -50,7 +48,7 @@ def B(*args, **kwargs): def get(request): - """Gets the running pipeline from `request`.""" + """Gets the running pipeline from the passed request.""" return request.session.get('partial_pipeline') @@ -62,17 +60,25 @@ def _get_url(view_name, backend_name): def get_complete_url(backend_name): """Gets URL for the endpoint that returns control to the auth pipeline. - Arg is `backend_name`, the name of the python-social-auth backend from the - running pipeline (for example, 'google-oauth2'). Returns string. + Args: + backend_name: string. Name of the python-social-auth backend from the + currently-running pipeline. + + Returns: + String. URL that finishes the auth pipeline for a provider. """ return _get_url('social:complete', backend_name) def get_login_url(provider_name): - """Gets the login URL for the endpoint that kicks of auth with a provider. + """Gets the login URL for the endpoint that kicks off auth with a provider. + + Args: + provider_name: string. The name of the provider.Provider that has been + enabled. - Args are `provider_name`, a string containing the name of a - provider.Provider that has been enabled. Returns string. + Returns: + String. URL that starts the auth pipeline for a provider. """ enabled_provider = provider.Registry.get(provider_name) if not enabled_provider: @@ -80,7 +86,7 @@ def get_login_url(provider_name): return _get_url('social:begin', enabled_provider.BACKEND_CLASS.name) -def make_random_password(length=None, seed=None): +def make_random_password(length=None, choice_fn=random.SystemRandom().choice): """Makes a random password. When a user creates an account via a social provider, we need to create a @@ -89,21 +95,20 @@ def make_random_password(length=None, seed=None): this password; that's OK because they can always use the reset password flow to set it to a known value. - Args are `length`, an int that determines the number of chars in the - returned value, and `seed`, a hashable object used to initialize the RNG. - `seed` is for tests only; do not pass it otherwise. - """ - if length is None: - length = _DEFAULT_RANDOM_PASSWORD_LENGTH + Args: + choice_fn: function or method. Takes an iterable and returns a random + element. + length: int. Number of chars in the returned value. None to use default. - if seed is not None: - random.seed(seed) - - return ''.join(random.choice(_PASSWORD_CHARSET) for _ in xrange(length)) + Returns: + String. The resulting password. + """ + length = length if length is not None else _DEFAULT_RANDOM_PASSWORD_LENGTH + return ''.join(choice_fn(_PASSWORD_CHARSET) for _ in xrange(length)) def running(request): - """Returns True iff `request` is running a third-party auth pipeline.""" + """Returns True iff request is running a third-party auth pipeline.""" return request.session.get('partial_pipeline') is not None # Avoid False for {}. @@ -111,7 +116,7 @@ def running(request): # to the auth backend's authenticate(). pylint: disable-msg=unused-argument @partial.partial def redirect_to_supplementary_form(strategy, details, response, uid, user=None, *args, **kwargs): - """Cut point that dispatches user to a create account or sign in form.""" + """Dispatches user to a create account form if they are new.""" if user is not None: return diff --git a/common/djangoapps/third_party_auth/provider.py b/common/djangoapps/third_party_auth/provider.py index 598601791127..c1b1ed3c808e 100644 --- a/common/djangoapps/third_party_auth/provider.py +++ b/common/djangoapps/third_party_auth/provider.py @@ -4,8 +4,7 @@ invoke the Django armature. """ -from social.backends import google -from social.backends import linkedin +from social.backends import google, linkedin class BaseProvider(object): @@ -34,21 +33,35 @@ def get_authentication_backend(cls): @classmethod def get_email(cls, unused_provider_details): - """Gets email address string from `provider_details` dict. + """Gets user's email address. Provider responses can contain arbitrary data. This method can be overridden to extract an email address from the provider details extracted by the social_details pipeline step. + + Args: + unused_provider_details: dict of string -> string. Data about the + user passed back by the provider. + + Returns: + String or None. The user's email address, if any. """ return None @classmethod def get_name(cls, unused_provider_details): - """Gets name string from `provider_details` dict. + """Gets user's name. Provider responses can contain arbitrary data. This method can be overridden to extract a full name for a user from the provider details extracted by the social_details pipeline step. + + Args: + unused_provider_details: dict of string -> string. Data about the + user passed back by the provider. + + Returns: + String or None. The user's full name, if any. """ return None @@ -59,9 +72,19 @@ def get_register_form_data(cls, pipeline_kwargs): common.djangoapps.student.views.register_user uses this to populate the new account creation form with values supplied by the user's chosen provider, preventing duplicate data entry. + + Args: + pipeline_kwargs: dict of string -> object. Keyword arguments + accumulated by the pipeline thus far. + + Returns: + Dict of string -> string. Keys are names of form fields; values are + values for that field. Where there is no value, the empty string + must be used. """ # Details about the user sent back from the provider. details = pipeline_kwargs.get('details') + # Get the username separately to take advantage of the de-duping logic # built into the pipeline. The provider cannot de-dupe because it can't # check the state of taken usernames in our system. Note that there is @@ -69,6 +92,7 @@ def get_register_form_data(cls, pipeline_kwargs): # creation of the user object, so it is still possible for users to get # an error on submit. suggested_username = pipeline_kwargs.get('username') + return { 'email': cls.get_email(details) or '', 'name': cls.get_name(details) or '', @@ -77,7 +101,7 @@ def get_register_form_data(cls, pipeline_kwargs): @classmethod def merge_onto(cls, settings): - """Merge class-level settings onto a django `settings` module.""" + """Merge class-level settings onto a django settings module.""" for key, value in cls.SETTINGS.iteritems(): setattr(settings, key, value) @@ -143,7 +167,7 @@ def _get_all(cls): @classmethod def _enable(cls, provider): - """Enables a single `provider`.""" + """Enables a single provider.""" if provider.NAME in cls._ENABLED: raise ValueError('Provider %s already enabled' % provider.NAME) cls._ENABLED[provider.NAME] = provider @@ -152,10 +176,12 @@ def _enable(cls, provider): def configure_once(cls, provider_names): """Configures providers. - Takes `provider_names`, a list of string. + Args: + provider_names: list of string. The providers to configure. """ if cls._CONFIGURED: raise ValueError('Provider registry already configured') + # Flip the bit eagerly -- configure() should not be re-callable if one # _enable call fails. cls._CONFIGURED = True @@ -173,7 +199,7 @@ def enabled(cls): @classmethod def get(cls, provider_name): - """Gets provider named `provider_name` string if enabled, else None.""" + """Gets provider named provider_name string if enabled, else None.""" cls._check_configured() return cls._ENABLED.get(provider_name) @@ -181,8 +207,10 @@ def get(cls, provider_name): def get_by_backend_name(cls, backend_name): """Gets provider (or None) by backend name. - Arg is string `backend_name`, a python-social-auth - backends.base.BaseAuth.name (for example, 'google-oauth2'). + Args: + backend_name: string. The python-social-auth + backends.base.BaseAuth.name (for example, 'google-oauth2') to + try and get a provider for. """ cls._check_configured() for enabled in cls._ENABLED.values(): diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index f1ca00eb3846..6b0de281d088 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -52,7 +52,7 @@ def _merge_auth_info(django_settings, auth_info): - """Merge `auth_info` dict onto `django_settings` module.""" + """Merge auth_info dict onto django_settings module.""" enabled_provider_names = [] to_merge = [] @@ -76,14 +76,18 @@ def _set_global_settings(django_settings): 'social.apps.django_app.default', 'third_party_auth', ) + # Inject exception middleware to make redirects fire. django_settings.MIDDLEWARE_CLASSES += _MIDDLEWARE_CLASSES + # Where to send the user if there's an error during social authentication. # Details about the error will be added to this URL as args so they will be # logged. django_settings.SOCIAL_AUTH_LOGIN_ERROR_URL = '/register' + # Where to send the user once social authentication is successful. django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard' + # Inject our customized auth pipeline. All auth backends must work with # this pipeline. django_settings.SOCIAL_AUTH_PIPELINE = ( @@ -98,12 +102,15 @@ def _set_global_settings(django_settings): 'social.pipeline.social_auth.load_extra_data', 'social.pipeline.user.user_details', ) + # We let the user specify their email address during signup. django_settings.SOCIAL_AUTH_PROTECTED_USER_FIELDS = ['email'] + # Disable exceptions by default for prod so you get redirect behavior # instead of a Django error page. During development you may want to # enable this when you want to get stack traces rather than redirections. django_settings.SOCIAL_AUTH_RAISE_EXCEPTIONS = False + # Context processors required under Django. django_settings.SOCIAL_AUTH_UUID_LENGTH = 4 django_settings.TEMPLATE_CONTEXT_PROCESSORS += ( @@ -113,7 +120,7 @@ def _set_global_settings(django_settings): def _set_provider_settings(django_settings, enabled_providers, auth_info): - """Set provider-specific settings.""" + """Sets provider-specific settings.""" # Must prepend here so we get called first. django_settings.AUTHENTICATION_BACKENDS = ( tuple(enabled_provider.get_authentication_backend() for enabled_provider in enabled_providers) + @@ -128,7 +135,7 @@ def _set_provider_settings(django_settings, enabled_providers, auth_info): def apply_settings(auth_info, django_settings): - """Apply settings from `auth_info` dict to `django_settings` module.""" + """Applies settings from auth_info dict to django_settings module.""" provider_names = auth_info.keys() provider.Registry.configure_once(provider_names) enabled_providers = provider.Registry.enabled() diff --git a/common/djangoapps/third_party_auth/tests/test_pipeline.py b/common/djangoapps/third_party_auth/tests/test_pipeline.py index f4ca008a9e1d..dff5b351a553 100644 --- a/common/djangoapps/third_party_auth/tests/test_pipeline.py +++ b/common/djangoapps/third_party_auth/tests/test_pipeline.py @@ -15,15 +15,11 @@ class MakeRandomPasswordTest(testutil.TestCase): """Tests formation of random placeholder passwords.""" - def tearDown(self): - random.seed() # Make random random again. + def setUp(self): super(MakeRandomPasswordTest, self).tearDown() + self.seed = 1 - def test_custom_length(self): - custom_length = 20 - self.assertEqual(custom_length, len(pipeline.make_random_password(length=custom_length))) - - def test_default_length(self): + def test_default_args(self): self.assertEqual(pipeline._DEFAULT_RANDOM_PASSWORD_LENGTH, len(pipeline.make_random_password())) def test_probably_only_uses_charset(self): @@ -32,8 +28,9 @@ def test_probably_only_uses_charset(self): self.assertIn(char, pipeline._PASSWORD_CHARSET) def test_pseudorandomly_picks_chars_from_charset(self): - seed = 1 - random.seed(seed) + random_instance = random.Random(self.seed) expected = ''.join( - random.choice(pipeline._PASSWORD_CHARSET) for _ in xrange(pipeline._DEFAULT_RANDOM_PASSWORD_LENGTH)) - self.assertEqual(expected, pipeline.make_random_password(seed=seed)) + random_instance.choice(pipeline._PASSWORD_CHARSET) + for _ in xrange(pipeline._DEFAULT_RANDOM_PASSWORD_LENGTH)) + random_instance.seed(self.seed) + self.assertEqual(expected, pipeline.make_random_password(choice_fn=random_instance.choice)) diff --git a/common/djangoapps/third_party_auth/tests/test_settings.py b/common/djangoapps/third_party_auth/tests/test_settings.py index eda1cd4a1bb7..9734c01af090 100644 --- a/common/djangoapps/third_party_auth/tests/test_settings.py +++ b/common/djangoapps/third_party_auth/tests/test_settings.py @@ -2,8 +2,7 @@ Unit tests for settings code. """ -from third_party_auth import provider -from third_party_auth import settings +from third_party_auth import provider, settings from third_party_auth.tests import testutil diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index 1800c3c62de6..03e0ef51adae 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -16,7 +16,7 @@ class FakeDjangoSettings(object): """A fake for Django settings.""" def __init__(self, mappings): - """Initializes the fake from `mappings`, a dict.""" + """Initializes the fake from mappings dict.""" for key, value in mappings.iteritems(): setattr(self, key, value) diff --git a/common/djangoapps/third_party_auth/urls.py b/common/djangoapps/third_party_auth/urls.py index 461ec8fc3be8..ffaeac23c4a1 100644 --- a/common/djangoapps/third_party_auth/urls.py +++ b/common/djangoapps/third_party_auth/urls.py @@ -2,9 +2,7 @@ from django.conf.urls import include, patterns, url -# String. Base of URL path component. -_PATH_BASE = 'auth/' urlpatterns = patterns( - '', url(r'^' + _PATH_BASE, include('social.apps.django_app.urls', namespace='social')), + '', url(r'^auth/', include('social.apps.django_app.urls', namespace='social')), ) diff --git a/lms/templates/register.html b/lms/templates/register.html index 3af58c877664..5693ac7366a7 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -12,8 +12,7 @@ <%! from django.utils.translation import ugettext as _ %> <%! from student.models import UserProfile %> <%! from datetime import date %> -<%! from third_party_auth import pipeline %> -<%! from third_party_auth import provider %> +<%! from third_party_auth import pipeline, provider %> <%! import calendar %> <%block name="pagetitle">${_("Register for {platform_name}").format(platform_name=platform_name)} From 4688a85a41f66ab887574e79e7f8f5ecc943d5de Mon Sep 17 00:00:00 2001 From: John Cox Date: Tue, 25 Mar 2014 10:52:59 -0700 Subject: [PATCH 05/18] Remove comments per review; whitespace fix --- AUTHORS | 1 - common/djangoapps/third_party_auth/pipeline.py | 3 --- common/djangoapps/third_party_auth/settings.py | 2 +- common/djangoapps/third_party_auth/tests/testutil.py | 2 +- 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/AUTHORS b/AUTHORS index b7f73af4e0d5..56ad7d6f303a 100644 --- a/AUTHORS +++ b/AUTHORS @@ -141,4 +141,3 @@ William Desloge Marco Re Jonas Jelten John Cox - diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 75182f4dbe21..dcdff0f4e508 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -40,10 +40,7 @@ def B(*args, **kwargs): from . import provider -# Int. Default size for randomly-generated passwords. _DEFAULT_RANDOM_PASSWORD_LENGTH = 12 -# String. The characters available to the random password generator. Must pass -# the ORM's validation routines. _PASSWORD_CHARSET = string.letters + string.digits diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index 6b0de281d088..cdf09e259c9e 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -45,7 +45,7 @@ from . import provider -# Tuple of string. Additional Django middleware classes. + _MIDDLEWARE_CLASSES = ( 'social.apps.django_app.middleware.SocialAuthExceptionMiddleware', ) diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index 03e0ef51adae..6907cea26eb7 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -8,7 +8,7 @@ from third_party_auth import provider -# String. settings.FEATURES key for the boolean that enables third_party_auth. + AUTH_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH' From efc089dc94da927d8694b5f6479ee9b95e8b748b Mon Sep 17 00:00:00 2001 From: John Cox Date: Wed, 26 Mar 2014 12:51:23 -0700 Subject: [PATCH 06/18] Hide password field in pipeline; add translator comments --- lms/templates/register.html | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lms/templates/register.html b/lms/templates/register.html index 5693ac7366a7..ff870ad6f1b6 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -121,6 +121,7 @@

${_("The following errors occurred while processing yo
    % for enabled in provider.Registry.enabled(): + ## Translators: provider_name is the name of an external, third-party user authentication service (like Google or LinkedIn).
  1. ${_("Sign in with {provider_name}").format(provider_name=enabled.NAME)} % endfor
@@ -132,6 +133,7 @@

${_("The following errors occurred while processing yo % else:

+ ## Translators: provider_name is the name of an external, third-party user authentication service (like Google or LinkedIn). ${_("You've successfully signed in with {selected_provider}.").format(selected_provider=selected_provider)}
${_("Finish your account registration below to start learning.")}

@@ -160,9 +162,9 @@

${_('Required Information')}

% if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline_running: -
  • +
    • % else: From ea704860c427c3642789799a8a60bbf450dbc2f4 Mon Sep 17 00:00:00 2001 From: John Cox Date: Thu, 27 Mar 2014 16:04:30 -0700 Subject: [PATCH 07/18] Add sign in to existing account support --- common/djangoapps/student/views.py | 102 +++++++++---- .../djangoapps/third_party_auth/middleware.py | 18 +++ .../djangoapps/third_party_auth/pipeline.py | 139 ++++++++++++++++-- .../djangoapps/third_party_auth/settings.py | 17 ++- .../third_party_auth/tests/test_settings.py | 4 + common/djangoapps/third_party_auth/urls.py | 3 +- lms/templates/login.html | 30 ++++ lms/templates/register.html | 33 ++++- 8 files changed, 293 insertions(+), 53 deletions(-) create mode 100644 common/djangoapps/third_party_auth/middleware.py diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 2067f22e0e2d..bb0d0fe49ea6 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -365,11 +365,13 @@ def signin_user(request): context = { 'course_id': request.GET.get('course_id'), 'enrollment_action': request.GET.get('enrollment_action'), + 'pipeline_running': 'true' if pipeline.running(request) else 'false', 'platform_name': microsite.get_value( 'platform_name', settings.PLATFORM_NAME ), } + return render_to_response('login.html', context) @@ -385,14 +387,12 @@ def register_user(request, extra_context=None): # and registration is disabled. return redirect(reverse('root')) - pipeline_running = pipeline.running(request) - context = { 'course_id': request.GET.get('course_id'), 'email': '', 'enrollment_action': request.GET.get('enrollment_action'), 'name': '', - 'pipeline_running': pipeline_running, + 'running_pipeline': None, 'platform_name': microsite.get_value( 'platform_name', settings.PLATFORM_NAME @@ -409,10 +409,11 @@ def register_user(request, extra_context=None): # If third-party auth is enabled, prepopulate the form with data from the # selected provider. - if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline_running: + if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request): running_pipeline = pipeline.get(request) current_provider = provider.Registry.get_by_backend_name(running_pipeline.get('backend')) overrides = current_provider.get_register_form_data(running_pipeline.get('kwargs')) + overrides['running_pipeline'] = running_pipeline overrides['selected_provider'] = current_provider.NAME context.update(overrides) @@ -710,6 +711,7 @@ def accounts_login(request): return external_auth.views.course_specific_login(request, course_id) context = { + 'pipeline_running': 'false', 'platform_name': settings.PLATFORM_NAME, } return render_to_response('login.html', context) @@ -719,22 +721,60 @@ def accounts_login(request): @ensure_csrf_cookie def login_user(request, error=""): """AJAX request to log in the user.""" - if 'email' not in request.POST or 'password' not in request.POST: - return JsonResponse({ - "success": False, - "value": _('There was an error receiving your login information. Please email us.'), # TODO: User error message - }) # TODO: this should be status code 400 # pylint: disable=fixme - email = request.POST['email'] - password = request.POST['password'] - try: - user = User.objects.get(email=email) - except User.DoesNotExist: - if settings.FEATURES['SQUELCH_PII_IN_LOGS']: - AUDIT_LOG.warning(u"Login failed - Unknown user email") - else: - AUDIT_LOG.warning(u"Login failed - Unknown user email: {0}".format(email)) - user = None + backend_name = None + email = None + password = None + redirect_url = None + response = None + running_pipeline = None + third_party_auth_requested = settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request) + third_party_auth_successful = False + trumped_by_first_party_auth = bool(request.POST.get('email')) or bool(request.POST.get('password')) + user = None + + if third_party_auth_requested and not trumped_by_first_party_auth: + # The user has already authenticated via third-party auth and has not + # asked to do first party auth by supplying a username or password. We + # now want to put them through the same logging and cookie calculation + # logic as with first-party auth. + running_pipeline = pipeline.get(request) + user_id = running_pipeline['kwargs'].get('user') + backend_name = running_pipeline['backend'] + requested_provider = provider.Registry.get_by_backend_name(backend_name) + + try: + user = pipeline.get_authenticated_user(user_id, backend_name) + third_party_auth_successful = True + except User.DoesNotExist: + AUDIT_LOG.warning( + u'Login failed - user with id %s has no social auth with backend_name %s'.format( + user_id=user_id, backend_name=backend_name)) + return JsonResponse({ + "success": False, + ## Translators: provider_name is the name of an external, third-party user authentication service (like + ## Google or LinkedIn). + "value": _('There is no {platform_name} account associated with your {provider_name} account. Please use your {platform_name} credentials or pick another provider.').format( + platform_name=settings.PLATFORM_NAME, provider_name=requested_provider.NAME) + }) # TODO: this should be status code 401 # pylint: disable=fixme + + else: + + if 'email' not in request.POST or 'password' not in request.POST: + return JsonResponse({ + "success": False, + "value": _('There was an error receiving your login information. Please email us.'), # TODO: User error message + }) # TODO: this should be status code 400 # pylint: disable=fixme + + email = request.POST['email'] + password = request.POST['password'] + try: + user = User.objects.get(email=email) + except User.DoesNotExist: + if settings.FEATURES['SQUELCH_PII_IN_LOGS']: + AUDIT_LOG.warning(u"Login failed - Unknown user email") + else: + AUDIT_LOG.warning(u"Login failed - Unknown user email: {0}".format(email)) # check if the user has a linked shibboleth account, if so, redirect the user to shib-login # This behavior is pretty much like what gmail does for shibboleth. Try entering some @stanford.edu @@ -773,14 +813,17 @@ def login_user(request, error=""): # username so that authentication is guaranteed to fail and we can take # advantage of the ratelimited backend username = user.username if user else "" - try: - user = authenticate(username=username, password=password, request=request) - # this occurs when there are too many attempts from the same IP address - except RateLimitException: - return JsonResponse({ - "success": False, - "value": _('Too many failed login attempts. Try again later.'), - }) # TODO: this should be status code 429 # pylint: disable=fixme + + if not third_party_auth_successful: + try: + user = authenticate(username=username, password=password, request=request) + # this occurs when there are too many attempts from the same IP address + except RateLimitException: + return JsonResponse({ + "success": False, + "value": _('Too many failed login attempts. Try again later.'), + }) # TODO: this should be status code 429 # pylint: disable=fixme + if user is None: # tick the failed login counters if the user exists in the database if user_found_by_email_lookup and LoginFailures.is_feature_enabled(): @@ -821,6 +864,9 @@ def login_user(request, error=""): redirect_url = try_change_enrollment(request) + if third_party_auth_successful: + redirect_url = pipeline.get_complete_url(backend_name) + dog_stats_api.increment("common.student.successful_login") response = JsonResponse({ "success": True, @@ -1052,7 +1098,7 @@ def create_account(request, post_override=None): JSON call to create new edX account. Used by form in signup_modal.html, which is included into navigation.html """ - js = {'success': False} + js = {'success': False} # pylint: disable-msg=invalid-name post_vars = post_override if post_override else request.POST extra_fields = getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) diff --git a/common/djangoapps/third_party_auth/middleware.py b/common/djangoapps/third_party_auth/middleware.py new file mode 100644 index 000000000000..19a5ece22a73 --- /dev/null +++ b/common/djangoapps/third_party_auth/middleware.py @@ -0,0 +1,18 @@ +"""Middleware classes for third_party_auth.""" + +from social.apps.django_app.middleware import SocialAuthExceptionMiddleware + +from . import pipeline + + +class ExceptionMiddleware(SocialAuthExceptionMiddleware): + """Custom middleware that handles conditional redirection.""" + + def get_redirect_uri(self, 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 = self.strategy.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) diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index dcdff0f4e508..7c1ffd4fce84 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -1,5 +1,32 @@ """Auth pipeline definitions. +Auth pipelines handle the process of authenticating a user. They involve a +consumer system and a provider service. The general pattern is: + + 1. The consumer system exposes a URL endpoint that starts the process. + 2. When a user visits that URL, the client system redirects the user to a + page served by the provider. The user authenticates with the provider. + The provider handles authentication failure however it wants. + 3. On success, the provider POSTs to a URL endpoint on the consumer to + invoke the pipeline. It sends back an arbitrary payload of data about + the user. + 4. The pipeline begins, executing each function in its stack. The stack is + defined on django's settings object's SOCIAL_AUTH_PIPELINE. This is done + in settings._set_global_settings. + 5. Each pipeline function is variadic. Most pipeline functions are part of + the pythons-social-auth library; our extensions are defined below. The + pipeline is the same no matter what provider is used. + 6. Pipeline functions can return a dict to add arguments to the function + invoked next. They can return None if this is not necessary. + 7. Pipeline functions may be decorated with @partial.partial. This pauses + the pipeline and serializes its state onto the request's session. When + this is done they may redirect to other edX handlers to execute edX + account registration/sign in code. + 8. In that code, redirecting to get_complete_url() resumes the pipeline. + This happens by hitting a handler exposed by the consumer system. + 9. In this way, execution moves between the provider, the pipeline, and + arbitrary consumer system code. + Gotcha alert!: Bear in mind that when pausing and resuming a pipeline function decorated with @@ -33,25 +60,89 @@ def B(*args, **kwargs): import random import string # pylint: disable-msg=deprecated-module +from django.contrib.auth.models import User from django.core.urlresolvers import reverse from django.shortcuts import redirect +from social.exceptions import AuthException from social.pipeline import partial from . import provider +AUTH_ENTRY_KEY = 'auth_entry' +AUTH_ENTRY_LOGIN = 'login' +AUTH_ENTRY_REGISTER = 'register' +_AUTH_ENTRY_CHOICES = frozenset([AUTH_ENTRY_LOGIN, AUTH_ENTRY_REGISTER]) _DEFAULT_RANDOM_PASSWORD_LENGTH = 12 _PASSWORD_CHARSET = string.letters + string.digits +class AuthEntryError(AuthException): + """Raised when auth_entry is missing or invalid on URLs. + + auth_entry tells us whether the auth flow was initiated to register a new + user (in which case it has the value of AUTH_ENTRY_REGISTER) or log in an + existing user (in which case it has the value of AUTH_ENTRY_LOGIN). + + This is necessary because the edX code we hook into the pipeline to + redirect to the existing auth flows needs to know what case we are in in + order to format its output correctly (for example, the register code is + invoked earlier than the login code, and it needs to know if the login flow + was requested to dispatch correctly). + """ + + def get(request): """Gets the running pipeline from the passed request.""" return request.session.get('partial_pipeline') -def _get_url(view_name, backend_name): - """Protected wrapper for reverse().""" - return reverse(view_name, kwargs={'backend': backend_name}) +def get_authenticated_user(user_id, backend_name): + """Gets a saved user authenticated by a particular backend. + + Between pipeline steps User objects are not saved -- only the id. We need + to reconstitute the user and set its .backend, which is ordinarily monkey- + patched on by Django during authenticate(), so it will function like a + user returned by authenticate(). + + Args: + user_id: long. Id of the user to get. + backend_name: string. The name of the third-party auth backend from + the running pipeline. + + Returns: + User if user is found and has a social auth from the passed + backend_name. + + Raises: + User.DoesNotExist: if no user matching user_id is found. + AssertionError: if the user is not authenticated. + """ + match = False + user = User.objects.get(id=user_id) + assert user.is_authenticated() + + for association in user.social_auth.all(): + if association.provider == backend_name: + match = True + break + + if not match: + raise User.DoesNotExist + + user.backend = provider.Registry.get_by_backend_name(backend_name).get_authentication_backend() + return user + + +def _get_url(view_name, backend_name, auth_entry=None): + """Creates a URL to hook into social auth endpoints.""" + kwargs = {'backend': backend_name} + url = reverse(view_name, kwargs=kwargs) + + if auth_entry: + url += '?%s=%s' % (AUTH_ENTRY_KEY, auth_entry) + + return url def get_complete_url(backend_name): @@ -67,20 +158,26 @@ def get_complete_url(backend_name): return _get_url('social:complete', backend_name) -def get_login_url(provider_name): +def get_login_url(provider_name, auth_entry): """Gets the login URL for the endpoint that kicks off auth with a provider. Args: provider_name: string. The name of the provider.Provider that has been enabled. + auth_entry: string. Query argument specifying the desired entry point + for the auth pipeline. Used by the pipeline for later branching. + Must be one of _AUTH_ENTRY_CHOICES. Returns: String. URL that starts the auth pipeline for a provider. """ + assert auth_entry in _AUTH_ENTRY_CHOICES enabled_provider = provider.Registry.get(provider_name) + if not enabled_provider: raise ValueError('Provider %s not enabled' % provider_name) - return _get_url('social:begin', enabled_provider.BACKEND_CLASS.name) + + return _get_url('social:begin', enabled_provider.BACKEND_CLASS.name, auth_entry=auth_entry) def make_random_password(length=None, choice_fn=random.SystemRandom().choice): @@ -109,12 +206,32 @@ def running(request): return request.session.get('partial_pipeline') is not None # Avoid False for {}. -# Signature set by framework; prepending 'unused_' causes TypeError on dispatch -# to the auth backend's authenticate(). pylint: disable-msg=unused-argument +# Pipeline functions. +# Signatures are set by python-social-auth; prepending 'unused_' causes +# TypeError on dispatch to the auth backend's authenticate(). +# pylint: disable-msg=unused-argument + + +def parse_query_params(strategy, response, *args, **kwargs): + """Reads whitelisted query params, transforms them into pipeline args.""" + auth_entry = strategy.session.get(AUTH_ENTRY_KEY) + + if not (auth_entry and auth_entry in _AUTH_ENTRY_CHOICES): + raise AuthEntryError(strategy.backend, 'auth_entry missing or invalid') + + return { + # Whether the auth pipeline entered from /login. + 'is_login': auth_entry == AUTH_ENTRY_LOGIN, + # Whether the auth pipeline entered from /register. + 'is_register': auth_entry == AUTH_ENTRY_REGISTER, + } + + @partial.partial -def redirect_to_supplementary_form(strategy, details, response, uid, user=None, *args, **kwargs): +def redirect_to_supplementary_form(strategy, details, response, uid, is_login=None, is_register=None, user=None, *args, **kwargs): """Dispatches user to a create account form if they are new.""" - if user is not None: - return + if is_login: + return redirect('/login', name='signin_user') - return redirect('/register', name='register_user') + if is_register and user is None: + return redirect('/register', name='register_user') diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index cdf09e259c9e..bdad04033e15 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -46,8 +46,9 @@ from . import provider +_FIELDS_STORED_IN_SESSION = ['auth_entry'] _MIDDLEWARE_CLASSES = ( - 'social.apps.django_app.middleware.SocialAuthExceptionMiddleware', + 'third_party_auth.middleware.ExceptionMiddleware', ) @@ -71,6 +72,11 @@ def _merge_auth_info(django_settings, auth_info): def _set_global_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 + # Register and configure python-social-auth with Django. django_settings.INSTALLED_APPS += ( 'social.apps.django_app.default', @@ -80,10 +86,10 @@ def _set_global_settings(django_settings): # Inject exception middleware to make redirects fire. django_settings.MIDDLEWARE_CLASSES += _MIDDLEWARE_CLASSES - # Where to send the user if there's an error during social authentication. - # Details about the error will be added to this URL as args so they will be - # logged. - django_settings.SOCIAL_AUTH_LOGIN_ERROR_URL = '/register' + # Where to send the user if there's an error during social authentication + # and we cannot send them to a more specific URL + # (see middleware.ExceptionMiddleware). + django_settings.SOCIAL_AUTH_LOGIN_ERROR_URL = '/' # Where to send the user once social authentication is successful. django_settings.SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard' @@ -91,6 +97,7 @@ def _set_global_settings(django_settings): # Inject our customized auth pipeline. All auth backends must work with # this pipeline. django_settings.SOCIAL_AUTH_PIPELINE = ( + 'third_party_auth.pipeline.parse_query_params', 'social.pipeline.social_auth.social_details', 'social.pipeline.social_auth.social_uid', 'social.pipeline.social_auth.auth_allowed', diff --git a/common/djangoapps/third_party_auth/tests/test_settings.py b/common/djangoapps/third_party_auth/tests/test_settings.py index 9734c01af090..a6cd86333c5c 100644 --- a/common/djangoapps/third_party_auth/tests/test_settings.py +++ b/common/djangoapps/third_party_auth/tests/test_settings.py @@ -35,6 +35,10 @@ def test_apply_settings_adds_exception_middleware(self): for middleware_name in settings._MIDDLEWARE_CLASSES: self.assertIn(middleware_name, 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) + def test_apply_settings_adds_third_party_auth_to_installed_apps(self): settings.apply_settings({}, self.settings) self.assertIn('third_party_auth', self.settings.INSTALLED_APPS) diff --git a/common/djangoapps/third_party_auth/urls.py b/common/djangoapps/third_party_auth/urls.py index ffaeac23c4a1..dc02425ef393 100644 --- a/common/djangoapps/third_party_auth/urls.py +++ b/common/djangoapps/third_party_auth/urls.py @@ -4,5 +4,6 @@ urlpatterns = patterns( - '', url(r'^auth/', include('social.apps.django_app.urls', namespace='social')), + '', + url(r'^auth/', include('social.apps.django_app.urls', namespace='social')), ) diff --git a/lms/templates/login.html b/lms/templates/login.html index ec5da5ad5091..fc6bdc2b188a 100644 --- a/lms/templates/login.html +++ b/lms/templates/login.html @@ -4,6 +4,7 @@ <%! from django.core.urlresolvers import reverse %> <%! from django.utils.translation import ugettext as _ %> +<%! from third_party_auth import provider, pipeline %> <%block name="pagetitle">${_("Log into your {platform_name} Account").format(platform_name=platform_name)} @@ -93,6 +94,17 @@ text("${_(u'Processing your account information…')}"); } } + + (function post_form_if_pipeline_running(pipeline_running) { + // If the pipeline is running, the user has already authenticated via a + // third-party provider. We want to invoke /login_ajax to loop in the + // code that does logging and sets cookies on the request. It is most + // consistent to do that by using the same mechanism that is used when + // the use does first-party sign-in: POSTing the sign-in form. + if (pipeline_running) { + $('#login-form').submit(); + } + })(${pipeline_running}) @@ -164,6 +176,24 @@

    ${_('Account Preferences')}

    + + % if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH'): + +
    + +

    + ${_('or, if you have connected one of these providers, log in below.')} +

    + +
      + % for enabled in provider.Registry.enabled(): + ## Translators: provider_name is the name of an external, third-party user authentication provider (like Google or LinkedIn). +
    1. ${_("Sign in with {provider_name}").format(provider_name=enabled.NAME)} + % endfor +
    + + % endif +