Skip to content

E
edx-platform
Commit 8e7f45a5 by Douglas Hall
Options

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.
parent d00697f5
Showing with 8 additions and 149 deletions
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))
......
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
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,
......
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))
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)
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment