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.
This commit is contained in:
Douglas Hall
committed by
Douglas Hall
Douglas Hall
parent
29bacacaa3
commit
d8494483c1
@@ -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))
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user