From d8494483c10b71385180120aa0c328ea63ca930e Mon Sep 17 00:00:00 2001 From: Douglas Hall Date: Wed, 20 Dec 2017 15:46:39 -0500 Subject: [PATCH] ENT-779 Remove code references to ProviderConfig.drop_existing_session. We no longer need the drop_existing_session flag on IdP configurations because dropping the existing session should actually be the only behavior for certain view in the edx-enterprise code. --- common/djangoapps/course_modes/views.py | 3 - .../djangoapps/third_party_auth/decorators.py | 54 ------------ common/djangoapps/third_party_auth/models.py | 8 -- .../third_party_auth/tests/test_decorators.py | 84 +------------------ common/djangoapps/util/tests/test_db.py | 8 +- requirements/edx/base.txt | 2 +- 6 files changed, 9 insertions(+), 150 deletions(-) diff --git a/common/djangoapps/course_modes/views.py b/common/djangoapps/course_modes/views.py index 8209154acb..eae83a3810 100644 --- a/common/djangoapps/course_modes/views.py +++ b/common/djangoapps/course_modes/views.py @@ -28,7 +28,6 @@ from lms.djangoapps.experiments.utils import get_experiment_user_metadata_contex from openedx.core.djangoapps.catalog.utils import get_currency_data from openedx.core.djangoapps.embargo import api as embargo_api from student.models import CourseEnrollment -from third_party_auth.decorators import tpa_hint_ends_existing_session from util.db import outer_atomic from xmodule.modulestore.django import modulestore @@ -55,7 +54,6 @@ class ChooseModeView(View): """ return super(ChooseModeView, self).dispatch(*args, **kwargs) - @method_decorator(tpa_hint_ends_existing_session) @method_decorator(login_required) @method_decorator(transaction.atomic) def get(self, request, course_id, error=None): @@ -197,7 +195,6 @@ class ChooseModeView(View): pass return render_to_response("course_modes/choose.html", context) - @method_decorator(tpa_hint_ends_existing_session) @method_decorator(transaction.non_atomic_requests) @method_decorator(login_required) @method_decorator(outer_atomic(read_committed=True)) diff --git a/common/djangoapps/third_party_auth/decorators.py b/common/djangoapps/third_party_auth/decorators.py index 74ef1f3024..5f0f3e6b40 100644 --- a/common/djangoapps/third_party_auth/decorators.py +++ b/common/djangoapps/third_party_auth/decorators.py @@ -33,57 +33,3 @@ def xframe_allow_whitelisted(view_func): resp['X-Frame-Options'] = x_frame_option return resp return wraps(view_func, assigned=available_attrs(view_func))(wrapped_view) - - -def tpa_hint_ends_existing_session(func): - """ - Modifies a view function so that, if a tpa_hint URL parameter is received, the user is - already logged in, and the hinted SSO provider is so configured, the user is redirected - to a logout view and then back here. When they're directed back here, a URL parameter - called "session_cleared" will be attached to indicate that, even though a user is now - logged in, they should be passed through without intervention. - """ - - @wraps(func) - def inner(request, *args, **kwargs): - """ - Check for a TPA hint in combination with a logged in user, and log the user out - if the hinted provider specifies that they should be, and if they haven't already - been redirected to a logout by this decorator. - """ - sso_provider = None - provider_id = request.GET.get('tpa_hint') - decorator_already_processed = request.GET.get('session_cleared') == 'yes' - if provider_id and not decorator_already_processed: - # Check that there is a provider and that we haven't already processed this view. - if request.user and request.user.is_authenticated(): - try: - sso_provider = Registry.get(provider_id=provider_id) - except ValueError: - sso_provider = None - if sso_provider and sso_provider.drop_existing_session: - # Do the redirect only if the configured provider says we ought to. - return redirect( - '{}?{}'.format( - request.build_absolute_uri(reverse('logout')), - urlencode( - { - 'redirect_url': '{}?{}'.format( - request.path, - urlencode( - [ - ('tpa_hint', provider_id), - ('session_cleared', 'yes') - ] - ) - ) - } - ) - ) - ) - - else: - # Otherwise, pass everything through to the wrapped view. - return func(request, *args, **kwargs) - - return inner diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 45925b3f74..10e03a52c2 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -154,14 +154,6 @@ class ProviderConfig(ConfigurationModel): "authentication using the correct link is still possible." ), ) - drop_existing_session = models.BooleanField( - default=False, - help_text=_( - "Whether to drop an existing session when accessing a view decorated with " - "third_party_auth.decorators.tpa_hint_ends_existing_session when a tpa_hint " - "URL query parameter mapping to this provider is included in the request." - ) - ) max_session_length = models.PositiveIntegerField( null=True, blank=True, diff --git a/common/djangoapps/third_party_auth/tests/test_decorators.py b/common/djangoapps/third_party_auth/tests/test_decorators.py index 1dfa6e723d..0bc1b8bd60 100644 --- a/common/djangoapps/third_party_auth/tests/test_decorators.py +++ b/common/djangoapps/third_party_auth/tests/test_decorators.py @@ -11,7 +11,7 @@ from django.test import RequestFactory from mock import MagicMock from six.moves.urllib.parse import urlencode -from third_party_auth.decorators import tpa_hint_ends_existing_session, xframe_allow_whitelisted +from third_party_auth.decorators import xframe_allow_whitelisted from third_party_auth.tests.testutil import TestCase @@ -21,14 +21,6 @@ def mock_view(_request): return HttpResponse() -@tpa_hint_ends_existing_session -def mock_hinted_view(request): - """ - Another test view for testing purposes. - """ - return JsonResponse({"tpa_hint": request.GET.get('tpa_hint')}) - - # remove this decorator once third_party_auth is enabled in CMS @unittest.skipIf( 'third_party_auth' not in settings.INSTALLED_APPS, @@ -68,77 +60,3 @@ class TestXFrameWhitelistDecorator(TestCase): request = self.construct_request(url) response = mock_view(request) self.assertEqual(response['X-Frame-Options'], 'DENY') - - -@unittest.skipIf( - 'third_party_auth' not in settings.INSTALLED_APPS, - 'third_party_auth is not currently installed in CMS' -) -@ddt.ddt -class TestTpaHintSessionEndingDecorator(TestCase): - """ - In cases where users may be sharing computers, we want to ensure - that a hinted link (has a tpa_hint query parameter) from an external site - will always force the user to login again with SSO, even if the user is already - logged in. (For SSO providers that enable this option.) - - This aims to prevent a situation where user 1 forgets to logout, - then user 2 logs in to the external site and takes a link to the - Open edX instance, but gets user 1's session. - """ - - url = '/protected_view' - - def setUp(self): - super(TestTpaHintSessionEndingDecorator, self).setUp() - self.enable_saml() - self.factory = RequestFactory() - - def get_user_mock(self, authenticated=False): - user = MagicMock() - user.is_authenticated.return_value = authenticated - return user - - def get_request_mock(self, authenticated=False, hinted=False, done=False): - user = self.get_user_mock(authenticated) - params = {} - if hinted: - params['tpa_hint'] = 'saml-realprovider' - if done: - params['session_cleared'] = 'yes' - url = '{}?{}'.format(self.url, urlencode(params)) - request = self.factory.get(url) - request.user = user - return request - - def create_provider(self, protected=False): - self.configure_saml_provider( - enabled=True, - name="Fancy real provider", - idp_slug="realprovider", - backend_name="tpa-saml", - drop_existing_session=protected, - ) - - @ddt.unpack - @ddt.data( - (True, True, False, False, False), - (True, True, True, False, True), - (True, True, True, True, False), - (True, True, False, True, False), - (False, True, True, False, False), - (True, False, True, False, False), - ) - def test_redirect_when_logged_in_with_hint(self, protected, authenticated, hinted, done, redirects): - self.create_provider(protected) - request = self.get_request_mock(authenticated, hinted, done) - response = mock_hinted_view(request) - if redirects: - self.assertRedirects( - response, - '/logout?redirect_url=%2Fprotected_view%3Ftpa_' - 'hint%3Dsaml-realprovider%26session_cleared%3Dyes', - fetch_redirect_response=False, - ) - else: - self.assertEqual(json.loads(response.content)['tpa_hint'], ('saml-realprovider' if hinted else None)) diff --git a/common/djangoapps/util/tests/test_db.py b/common/djangoapps/util/tests/test_db.py index 2509d22052..3a2ed47bb2 100644 --- a/common/djangoapps/util/tests/test_db.py +++ b/common/djangoapps/util/tests/test_db.py @@ -237,4 +237,10 @@ class MigrationTests(TestCase): out = StringIO() call_command('makemigrations', dry_run=True, verbosity=3, stdout=out) output = out.getvalue() - self.assertIn('No changes detected', output) + # Temporarily disable this check so we can remove + # the ProviderConfig.drop_existing_session field. + # We will restore this check once the code referencing + # this field has been deleted/released and a migration + # for field removal has been added. + if 'Remove field' not in output: + self.assertIn('No changes detected', output) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 55bc792430..328421d144 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -49,7 +49,7 @@ git+https://github.com/cpennington/pylint-django@fix-field-inference-during-monk enum34==1.1.6 edx-django-oauth2-provider==1.2.5 edx-django-sites-extensions==2.3.0 -edx-enterprise==0.56.5 +edx-enterprise==0.57.0 edx-oauth2-provider==1.2.2 edx-organizations==0.4.9 edx-rest-api-client==1.7.1