diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index e231cb7ab1..f6099f360c 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -2,6 +2,7 @@ import logging import mimetypes import urllib +import urlparse from datetime import datetime from django.conf import settings @@ -16,6 +17,7 @@ from pytz import UTC import third_party_auth from course_modes.models import CourseMode from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationDeadline +from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming.helpers import get_themes # Enumeration of per-course verification statuses @@ -240,6 +242,8 @@ def get_next_url_for_login_page(request): Otherwise, we go to the ?next= query param or to the dashboard if nothing else is specified. + + If THIRD_PARTY_AUTH_HINT is set, then `tpa_hint=` is added as a query parameter. """ redirect_to = get_redirect_to(request) if not redirect_to: @@ -247,6 +251,7 @@ def get_next_url_for_login_page(request): redirect_to = reverse('dashboard') except NoReverseMatch: redirect_to = reverse('home') + if any(param in request.GET for param in POST_AUTH_PARAMS): # Before we redirect to next/dashboard, we need to handle auto-enrollment: params = [(param, request.GET[param]) for param in POST_AUTH_PARAMS if param in request.GET] @@ -255,6 +260,23 @@ def get_next_url_for_login_page(request): # Note: if we are resuming a third party auth pipeline, then the next URL will already # be saved in the session as part of the pipeline state. That URL will take priority # over this one. + + # Append a tpa_hint query parameter, if one is configured + tpa_hint = configuration_helpers.get_value( + "THIRD_PARTY_AUTH_HINT", + settings.FEATURES.get("THIRD_PARTY_AUTH_HINT", '') + ) + if tpa_hint: + # Don't add tpa_hint if we're already in the TPA pipeline (prevent infinite loop), + # and don't overwrite any existing tpa_hint params (allow tpa_hint override). + running_pipeline = third_party_auth.pipeline.get(request) + (scheme, netloc, path, query, fragment) = list(urlparse.urlsplit(redirect_to)) + if not running_pipeline and 'tpa_hint' not in query: + params = urlparse.parse_qs(query) + params['tpa_hint'] = [tpa_hint] + query = urllib.urlencode(params, doseq=True) + redirect_to = urlparse.urlunsplit((scheme, netloc, path, query, fragment)) + return redirect_to diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index 708f5bd2c5..dd71d3d577 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -4,12 +4,16 @@ import logging import ddt from django.conf import settings +from django.contrib.sessions.middleware import SessionMiddleware from django.core.urlresolvers import reverse from django.test import TestCase from django.test.client import RequestFactory +from django.test.utils import override_settings +from mock import patch from testfixtures import LogCapture from student.helpers import get_next_url_for_login_page +from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration_context LOGGER_NAME = "student.helpers" @@ -23,6 +27,13 @@ class TestLoginHelper(TestCase): super(TestLoginHelper, self).setUp() self.request = RequestFactory() + @staticmethod + def _add_session(request): + """Annotate the request object with a session""" + middleware = SessionMiddleware() + middleware.process_request(request) + request.session.save() + @ddt.data( ("https://www.amazon.com", "text/html", None, "Unsafe redirect parameter detected after login page: u'https://www.amazon.com'"), @@ -56,3 +67,38 @@ class TestLoginHelper(TestCase): req.META["HTTP_ACCEPT"] = "text/html" # pylint: disable=no-member next_page = get_next_url_for_login_page(req) self.assertEqual(next_page, u'/dashboard') + + @patch('student.helpers.third_party_auth.pipeline.get') + @ddt.data( + # Test requests outside the TPA pipeline - tpa_hint should be added. + (None, '/dashboard', '/dashboard', False), + ('', '/dashboard', '/dashboard', False), + ('', '/dashboard?tpa_hint=oa2-google-oauth2', '/dashboard?tpa_hint=oa2-google-oauth2', False), + ('saml-idp', '/dashboard', '/dashboard?tpa_hint=saml-idp', False), + # THIRD_PARTY_AUTH_HINT can be overridden via the query string + ('saml-idp', '/dashboard?tpa_hint=oa2-google-oauth2', '/dashboard?tpa_hint=oa2-google-oauth2', False), + + # Test requests inside the TPA pipeline - tpa_hint should not be added, preventing infinite loop. + (None, '/dashboard', '/dashboard', True), + ('', '/dashboard', '/dashboard', True), + ('', '/dashboard?tpa_hint=oa2-google-oauth2', '/dashboard?tpa_hint=oa2-google-oauth2', True), + ('saml-idp', '/dashboard', '/dashboard', True), + # OK to leave tpa_hint overrides in place. + ('saml-idp', '/dashboard?tpa_hint=oa2-google-oauth2', '/dashboard?tpa_hint=oa2-google-oauth2', True), + ) + @ddt.unpack + def test_third_party_auth_hint(self, tpa_hint, next_url, expected_url, running_pipeline, mock_running_pipeline): + mock_running_pipeline.return_value = running_pipeline + + def validate_login(): + req = self.request.get(reverse("login") + "?next={url}".format(url=next_url)) + req.META["HTTP_ACCEPT"] = "text/html" # pylint: disable=no-member + self._add_session(req) + next_page = get_next_url_for_login_page(req) + self.assertEqual(next_page, expected_url) + + with override_settings(FEATURES=dict(settings.FEATURES, THIRD_PARTY_AUTH_HINT=tpa_hint)): + validate_login() + + with with_site_configuration_context(configuration=dict(THIRD_PARTY_AUTH_HINT=tpa_hint)): + validate_login() diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 0944ebc49c..b07ead89fe 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -458,15 +458,63 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi response = self.client.get(reverse('signin_user'), params, HTTP_ACCEPT="text/html") self.assertNotIn(response.content, tpa_hint) - def test_hinted_login_dialog_disabled(self): + @ddt.data( + ('signin_user', 'login'), + ('register_user', 'register'), + ) + @ddt.unpack + def test_hinted_login_dialog_disabled(self, url_name, auth_entry): """Test that the dialog doesn't show up for hinted logins when disabled. """ self.google_provider.skip_hinted_login_dialog = True self.google_provider.save() params = [("next", "/courses/something/?tpa_hint=oa2-google-oauth2")] - response = self.client.get(reverse('signin_user'), params, HTTP_ACCEPT="text/html") + response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html") self.assertRedirects( response, - 'auth/login/google-oauth2/?auth_entry=login&next=%2Fcourses%2Fsomething%2F%3Ftpa_hint%3Doa2-google-oauth2', + 'auth/login/google-oauth2/?auth_entry={}&next=%2Fcourses%2Fsomething%2F%3Ftpa_hint%3Doa2-google-oauth2'.format(auth_entry), + target_status_code=302 + ) + + @override_settings(FEATURES=dict(settings.FEATURES, THIRD_PARTY_AUTH_HINT='oa2-google-oauth2')) + @ddt.data( + 'signin_user', + 'register_user', + ) + def test_settings_tpa_hinted_login(self, url_name): + """ + Ensure that settings.FEATURES['THIRD_PARTY_AUTH_HINT'] can set third_party_auth_hint. + """ + params = [("next", "/courses/something/")] + response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html") + self.assertContains(response, '"third_party_auth_hint": "oa2-google-oauth2"') + + # THIRD_PARTY_AUTH_HINT can be overridden via the query string + tpa_hint = self.hidden_enabled_provider.provider_id + params = [("next", "/courses/something/?tpa_hint={0}".format(tpa_hint))] + response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html") + self.assertContains(response, '"third_party_auth_hint": "{0}"'.format(tpa_hint)) + + # Even disabled providers in the query string will override THIRD_PARTY_AUTH_HINT + tpa_hint = self.hidden_disabled_provider.provider_id + params = [("next", "/courses/something/?tpa_hint={0}".format(tpa_hint))] + response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html") + self.assertNotIn(response.content, tpa_hint) + + @override_settings(FEATURES=dict(settings.FEATURES, THIRD_PARTY_AUTH_HINT='oa2-google-oauth2')) + @ddt.data( + ('signin_user', 'login'), + ('register_user', 'register'), + ) + @ddt.unpack + def test_settings_tpa_hinted_login_dialog_disabled(self, url_name, auth_entry): + """Test that the dialog doesn't show up for hinted logins when disabled via settings.THIRD_PARTY_AUTH_HINT. """ + self.google_provider.skip_hinted_login_dialog = True + self.google_provider.save() + params = [("next", "/courses/something/")] + response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html") + self.assertRedirects( + response, + 'auth/login/google-oauth2/?auth_entry={}&next=%2Fcourses%2Fsomething%2F%3Ftpa_hint%3Doa2-google-oauth2'.format(auth_entry), target_status_code=302 ) diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 697c172bf1..84240d2b9d 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -86,13 +86,17 @@ def login_and_registration_form(request, initial_mode="login"): if tpa_hint_provider.skip_hinted_login_dialog: # Forward the user directly to the provider's login URL when the provider is configured # to skip the dialog. + if initial_mode == "register": + auth_entry = pipeline.AUTH_ENTRY_REGISTER + else: + auth_entry = pipeline.AUTH_ENTRY_LOGIN return redirect( - pipeline.get_login_url(provider_id, pipeline.AUTH_ENTRY_LOGIN, redirect_url=redirect_to) + pipeline.get_login_url(provider_id, auth_entry, redirect_url=redirect_to) ) third_party_auth_hint = provider_id initial_mode = "hinted_login" - except (KeyError, ValueError, IndexError): - pass + except (KeyError, ValueError, IndexError) as ex: + log.error("Unknown tpa_hint provider: %s", ex) # If this is a themed site, revert to the old login/registration pages. # We need to do this for now to support existing themes.