From ee9f632a3dfad988da8788e4cd38d740aa3773e7 Mon Sep 17 00:00:00 2001 From: Jesse Shapiro Date: Tue, 18 Apr 2017 09:07:53 -0400 Subject: [PATCH] Allow per-SSO-provider session expiration limits --- .../migrations/0009_auto_20170415_1144.py | 29 +++++++++++++++++++ common/djangoapps/third_party_auth/models.py | 12 ++++++++ common/djangoapps/third_party_auth/saml.py | 2 +- .../djangoapps/third_party_auth/strategy.py | 12 +++++++- .../third_party_auth/tests/specs/base.py | 16 +++++----- .../tests/specs/test_testshib.py | 24 +++++++++++++++ .../third_party_auth/tests/test_admin.py | 3 ++ requirements/edx/base.txt | 5 +++- 8 files changed, 93 insertions(+), 10 deletions(-) create mode 100644 common/djangoapps/third_party_auth/migrations/0009_auto_20170415_1144.py diff --git a/common/djangoapps/third_party_auth/migrations/0009_auto_20170415_1144.py b/common/djangoapps/third_party_auth/migrations/0009_auto_20170415_1144.py new file mode 100644 index 0000000000..fa969ae5fb --- /dev/null +++ b/common/djangoapps/third_party_auth/migrations/0009_auto_20170415_1144.py @@ -0,0 +1,29 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('third_party_auth', '0008_auto_20170413_1455'), + ] + + operations = [ + migrations.AddField( + model_name='ltiproviderconfig', + name='max_session_length', + field=models.PositiveIntegerField(default=None, help_text='If this option is set, then users logging in using this SSO provider will have their session length limited to no longer than this value. If set to 0 (zero), the session will expire upon the user closing their browser. If left blank, the Django platform session default length will be used.', null=True, verbose_name=b'Max session length (seconds)', blank=True), + ), + migrations.AddField( + model_name='oauth2providerconfig', + name='max_session_length', + field=models.PositiveIntegerField(default=None, help_text='If this option is set, then users logging in using this SSO provider will have their session length limited to no longer than this value. If set to 0 (zero), the session will expire upon the user closing their browser. If left blank, the Django platform session default length will be used.', null=True, verbose_name=b'Max session length (seconds)', blank=True), + ), + migrations.AddField( + model_name='samlproviderconfig', + name='max_session_length', + field=models.PositiveIntegerField(default=None, help_text='If this option is set, then users logging in using this SSO provider will have their session length limited to no longer than this value. If set to 0 (zero), the session will expire upon the user closing their browser. If left blank, the Django platform session default length will be used.', null=True, verbose_name=b'Max session length (seconds)', blank=True), + ), + ] diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 4ddc1bcda5..1398384ebc 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -148,6 +148,18 @@ class ProviderConfig(ConfigurationModel): "URL query parameter mapping to this provider is included in the request." ) ) + max_session_length = models.PositiveIntegerField( + null=True, + blank=True, + default=None, + verbose_name='Max session length (seconds)', + help_text=_( + "If this option is set, then users logging in using this SSO provider will have " + "their session length limited to no longer than this value. If set to 0 (zero), " + "the session will expire upon the user closing their browser. If left blank, the " + "Django platform session default length will be used." + ) + ) prefix = None # used for provider_id. Set to a string value in subclass backend_name = None # Set to a field or fixed value in subclass accepts_logins = True # Whether to display a sign-in button when the provider is enabled diff --git a/common/djangoapps/third_party_auth/saml.py b/common/djangoapps/third_party_auth/saml.py index 918f09a5b1..f3134803fb 100644 --- a/common/djangoapps/third_party_auth/saml.py +++ b/common/djangoapps/third_party_auth/saml.py @@ -32,7 +32,7 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method try: return self._config.get_setting(name) except KeyError: - return self.strategy.setting(name, default) + return self.strategy.setting(name, default, backend=self) def auth_url(self): """ diff --git a/common/djangoapps/third_party_auth/strategy.py b/common/djangoapps/third_party_auth/strategy.py index a0ced2f88b..5f67d3a6ad 100644 --- a/common/djangoapps/third_party_auth/strategy.py +++ b/common/djangoapps/third_party_auth/strategy.py @@ -3,7 +3,8 @@ A custom Strategy for python-social-auth that allows us to fetch configuration f ConfigurationModels rather than django.settings """ from .models import OAuth2ProviderConfig -from .pipeline import AUTH_ENTRY_CUSTOM +from .pipeline import AUTH_ENTRY_CUSTOM, get as get_pipeline_from_request +from .provider import Registry from social.backends.oauth import OAuthAuth from social.strategies.django_strategy import DjangoStrategy @@ -41,6 +42,15 @@ class ConfigurationModelStrategy(DjangoStrategy): if error_url: return error_url + # Special case: we want to get this particular setting directly from the provider database + # entry if possible; if we don't have the information, fall back to the default behavior. + if name == 'MAX_SESSION_LENGTH': + running_pipeline = get_pipeline_from_request(self.request) if self.request else None + if running_pipeline is not None: + provider_config = Registry.get_from_pipeline(running_pipeline) + if provider_config: + return provider_config.max_session_length + # At this point, we know 'name' is not set in a [OAuth2|LTI|SAML]ProviderConfig row. # It's probably a global Django setting like 'FIELDS_STORED_IN_SESSION': return super(ConfigurationModelStrategy, self).setting(name, default, backend) diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index 98566d0763..f879ebf332 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -144,7 +144,7 @@ class IntegrationTestMixin(object): """ raise NotImplementedError - def _test_return_login(self, user_is_activated=True): + def _test_return_login(self, user_is_activated=True, previous_session_timed_out=False): """ Test logging in to an account that is already linked. """ # Make sure we're not logged in: dashboard_response = self.client.get(reverse('dashboard')) @@ -156,12 +156,14 @@ class IntegrationTestMixin(object): # The user should be redirected to the provider: self.assertEqual(try_login_response.status_code, 302) login_response = self.do_provider_login(try_login_response['Location']) - # There will be one weird redirect required to set the login cookie: - self.assertEqual(login_response.status_code, 302) - self.assertEqual(login_response['Location'], self.url_prefix + self.complete_url) - # And then we should be redirected to the dashboard: - login_response = self.client.get(login_response['Location']) - self.assertEqual(login_response.status_code, 302) + # If the previous session was manually logged out, there will be one weird redirect + # required to set the login cookie (it sticks around if the main session times out): + if not previous_session_timed_out: + self.assertEqual(login_response.status_code, 302) + self.assertEqual(login_response['Location'], self.url_prefix + self.complete_url) + # And then we should be redirected to the dashboard: + login_response = self.client.get(login_response['Location']) + self.assertEqual(login_response.status_code, 302) if user_is_activated: url_expected = reverse('dashboard') else: diff --git a/common/djangoapps/third_party_auth/tests/specs/test_testshib.py b/common/djangoapps/third_party_auth/tests/specs/test_testshib.py index b8be7c53b0..31997c6f54 100644 --- a/common/djangoapps/third_party_auth/tests/specs/test_testshib.py +++ b/common/djangoapps/third_party_auth/tests/specs/test_testshib.py @@ -1,11 +1,14 @@ """ Third_party_auth integration tests using a mock version of the TestShib provider """ +import datetime import ddt import unittest import httpretty import json +import time from mock import patch +from freezegun import freeze_time from social.apps.django_app.default.models import UserSocialAuth from unittest import skip @@ -90,6 +93,7 @@ class SamlIntegrationTestUtilities(object): kwargs.setdefault('metadata_source', TESTSHIB_METADATA_URL) kwargs.setdefault('icon_class', 'fa-university') kwargs.setdefault('attr_email', 'urn:oid:1.3.6.1.4.1.5923.1.1.1.6') # eduPersonPrincipalName + kwargs.setdefault('max_session_length', None) self.configure_saml_provider(**kwargs) if fetch_metadata: @@ -207,6 +211,26 @@ class TestShibIntegrationTest(SamlIntegrationTestUtilities, IntegrationTestMixin self.assertEqual(num_failed, 0) self.assertEqual(len(failure_messages), 0) + def test_login_with_testshib_provider_short_session_length(self): + """ + Test that when we have a TPA provider which as an explicit maximum + session length set, waiting for longer than that between requests + results in us being logged out. + """ + # Configure the provider with a 10-second timeout + self._configure_testshib_provider(max_session_length=10) + + now = datetime.datetime.utcnow() + with freeze_time(now): + # Test the login flow, adding the user in the process + super(TestShibIntegrationTest, self).test_login() + + # Wait 30 seconds; longer than the manually-set 10-second timeout + later = now + datetime.timedelta(seconds=30) + with freeze_time(later): + # Test returning as a logged in user; this method verifies that we're logged out first. + self._test_return_login(previous_session_timed_out=True) + @unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, 'third_party_auth not enabled') class SuccessFactorsIntegrationTest(SamlIntegrationTestUtilities, IntegrationTestMixin, testutil.SAMLTestCase): diff --git a/common/djangoapps/third_party_auth/tests/test_admin.py b/common/djangoapps/third_party_auth/tests/test_admin.py index 9ae56b5a86..9c3b2b3ed8 100644 --- a/common/djangoapps/third_party_auth/tests/test_admin.py +++ b/common/djangoapps/third_party_auth/tests/test_admin.py @@ -66,6 +66,9 @@ class Oauth2ProviderConfigAdminTest(testutil.TestCase): # Remove the icon_image from the POST data, to simulate unchanged icon_image post_data = models.model_to_dict(provider1) del post_data['icon_image'] + # Remove max_session_length; it has a default null value which must be POSTed + # back as an absent value, rather than as a "null-like" included value. + del post_data['max_session_length'] # Change the name, to verify POST post_data['name'] = 'Another name' diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 04da1c1eda..059a1b8d8b 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -90,7 +90,10 @@ python-memcached==1.48 django-memcached-hashring==0.1.2 python-openid==2.2.5 python-dateutil==2.1 -python-social-auth==0.2.21 +# We need to be able to set a maximum session length on our third-party auth providers; +# our goal is to upstream these changes and return to the canonical version ASAP. +# python-social-auth==0.2.21 +git+https://github.com/edx/python-social-auth@758985102cee98f440fae44ed99617b7cfef3473#egg=python-social-auth==0.2.21.edx.a pytz==2016.7 pysrt==0.4.7 PyYAML==3.10