diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 88ff4562b04d..83be9b644843 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -38,14 +38,6 @@ def test_logout(self): """ self.logout() - def test_request_attr_on_logout(self): - """ - Test request object after logging out to see whether it - has 'is_from_log_out' attribute set to true. - """ - response = self.client.get(reverse('logout')) - assert getattr(response.wsgi_request, 'is_from_logout', False) - class PageLoaderTestCase(LoginEnrollmentTestCase): """ diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index 9031598036f2..ad2bd2cb8a19 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -3,6 +3,17 @@ SafeCookieData that cryptographically binds the user to the session id in the cookie. +The primary goal is to avoid and detect situations where a session is +corrupted and the client becomes logged in as the wrong user. This +could happen via cache corruption (which we've seen before) or a +request handling bug. It's unlikely to happen again, but would be a +critical issue, so we've built in some checks to make sure the user on +the session doesn't change over the course of the session or between +the request and response phases. + +The secondary goal is to improve upon Django's session handling by +including cryptographically enforced expiration. + The implementation is inspired in part by the proposal in the paper but deviates in a number of ways; mostly it just uses the technique @@ -66,9 +77,8 @@ import inspect from base64 import b64encode -from contextlib import contextmanager from hashlib import sha256 -from logging import ERROR, getLogger +from logging import getLogger from django.conf import settings from django.contrib.auth import SESSION_KEY @@ -278,7 +288,8 @@ def process_request(self, request): Step 4. Once the session is retrieved, verify that the user bound in the safe_cookie_data matches the user attached to the - server's session information. + server's session information. Otherwise, reject the request + (bypass the view and return an error or redirect). Step 5. If all is successful, the now verified user_id is stored separately in the request object so it is available for another @@ -313,6 +324,8 @@ def process_request(self, request): if LOG_REQUEST_USER_CHANGES: log_request_user_changes(request) else: + # Return an error or redirect, and don't continue to + # the underlying view. return self._on_user_authentication_failed(request) def process_response(self, request, response): @@ -345,13 +358,12 @@ def process_response(self, request, response): if not _is_cookie_marked_for_deletion(request) and _is_cookie_present(response): try: user_id_in_session = self.get_user_id_from_session(request) - with controlled_logging(request, log): - self._verify_user(request, user_id_in_session) # Step 2 + self._verify_user(request, response, user_id_in_session) # Step 2 - # Use the user_id marked in the session instead of the - # one in the request in case the user is not set in the - # request, for example during Anonymous API access. - self.update_with_safe_session_cookie(response.cookies, user_id_in_session) # Step 3 + # Use the user_id marked in the session instead of the + # one in the request in case the user is not set in the + # request, for example during Anonymous API access. + self.update_with_safe_session_cookie(response.cookies, user_id_in_session) # Step 3 except SafeCookieError: _mark_cookie_for_deletion(request) @@ -378,12 +390,23 @@ def _on_user_authentication_failed(request): return redirect_to_login(request.path) @staticmethod - def _verify_user(request, userid_in_session): + def _verify_user(request, response, userid_in_session): """ Logs an error if the user marked at the time of process_request does not match either the current user in the request or the given userid_in_session. """ + # It's expected that a small number of views may change the + # user over the course of the request. We have exemptions for + # the user changing to/from None, but the login view can + # sometimes change the user from one value to another between + # the request and response phases, specifically when the login + # page is used during an active session. + # + # The relevant views set a flag to indicate the exemption. + if getattr(response, 'safe_sessions_expected_user_change', None): + return + if hasattr(request, 'safe_cookie_verified_user_id'): if hasattr(request.user, 'real_user'): # If a view overrode the request.user with a masqueraded user, this will @@ -431,6 +454,7 @@ def get_user_id_from_session(request): except KeyError: return None + # TODO move to test code, maybe rename, get rid of old Django compat stuff @staticmethod def set_user_id_in_session(request, user): """ @@ -573,28 +597,13 @@ def __setattr__(self, name, value): request.__class__ = SafeSessionRequestWrapper -def _is_from_logout(request): +def mark_user_change_as_expected(response, new_user_id): """ - Returns whether the request has come from logout action to see if - 'is_from_logout' attribute is present. - """ - return getattr(request, 'is_from_logout', False) - + Indicate to the safe-sessions middleware that it is expected that + the user is changing between the request and response phase of + the current request. -@contextmanager -def controlled_logging(request, logger): - """ - Control the logging by changing logger's level if - the request is from logout. + The new_user_id may be None or an LMS user ID, and may be the same + as the previous user ID. """ - default_level = None - from_logout = _is_from_logout(request) - if from_logout: - default_level = logger.getEffectiveLevel() - logger.setLevel(ERROR) - - try: - yield - finally: - if from_logout: - logger.setLevel(default_level) + response.safe_sessions_expected_user_change = {'new_user_id': new_user_id} diff --git a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py index 5686086152f7..ce49f5bd9568 100644 --- a/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py +++ b/openedx/core/djangoapps/safe_sessions/tests/test_middleware.py @@ -17,7 +17,7 @@ from openedx.core.djangolib.testing.utils import get_mock_request from common.djangoapps.student.tests.factories import UserFactory -from ..middleware import SafeCookieData, SafeSessionMiddleware, log_request_user_changes +from ..middleware import SafeCookieData, SafeSessionMiddleware, log_request_user_changes, mark_user_change_as_expected from .test_utils import TestSafeSessionsLogMixin @@ -263,9 +263,9 @@ def cookies_from_request_to_response(self): settings.SESSION_COOKIE_NAME ] - def verify_success(self): + def set_up_for_success(self): """ - Verifies success path. + Set up request for success path -- everything up until process_response(). """ self.client.login(username=self.user.username, password='test') self.request.user = self.user @@ -281,6 +281,12 @@ def verify_success(self): assert self.request.safe_cookie_verified_user_id == self.user.id self.cookies_from_request_to_response() + def verify_success(self): + """ + Verifies success path. + """ + self.set_up_for_success() + with self.assert_not_logged(): response = SafeSessionMiddleware().process_response(self.request, self.client.response) assert response.status_code == 200 @@ -322,6 +328,41 @@ def test_error_from_mobile_app(self): self.request.META = {'HTTP_USER_AGENT': 'open edX Mobile App Version 2.1'} self.verify_error(401) + def test_warn_on_user_change(self): + """ + Verifies that warnings are emitted and custom attributes set if + the user changes unexpectedly between request and response. + """ + self.set_up_for_success() + + # But then user changes unexpectedly + self.request.user = UserFactory.create() + + with self.assert_logged_for_request_user_mismatch(self.user.id, self.request.user.id, 'warning', '/'): + with patch('openedx.core.djangoapps.safe_sessions.middleware.set_custom_attribute') as mock_attr: + response = SafeSessionMiddleware().process_response(self.request, self.client.response) + assert response.status_code == 200 + mock_attr.assert_called_with("safe_sessions.user_mismatch", "request-response-mismatch") + + def test_no_warn_on_expected_user_change(self): + """ + Verifies that no warnings is emitted when the user change is expected. + This might happen on a login, for example. + """ + self.set_up_for_success() + + # User changes... + new_user = UserFactory.create() + self.request.user = new_user + # ...but so does session, and view sets a flag to say it's OK. + mark_user_change_as_expected(self.client.response, new_user.id) + + with self.assert_no_warning_logged(): + with patch('openedx.core.djangoapps.safe_sessions.middleware.set_custom_attribute') as mock_attr: + response = SafeSessionMiddleware().process_response(self.request, self.client.response) + assert response.status_code == 200 + mock_attr.assert_not_called() + @ddt.ddt class TestLogRequestUserChanges(TestCase): diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index e536be42ab66..c72b28154d7a 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -41,6 +41,7 @@ from common.djangoapps.util.json_request import JsonResponse from common.djangoapps.util.password_policy_validators import normalize_password from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance +from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_authn.config.waffle import ENABLE_LOGIN_USING_THIRDPARTY_AUTH_ONLY from openedx.core.djangoapps.user_authn.cookies import get_response_with_refreshed_jwt_cookies, set_logged_in_cookies @@ -593,6 +594,7 @@ def login_user(request, api_version='v1'): set_custom_attribute('login_user_auth_failed_error', False) set_custom_attribute('login_user_response_status', response.status_code) set_custom_attribute('login_user_redirect_url', redirect_url) + mark_user_change_as_expected(response, user.id) return response except AuthFailedError as error: response_content = error.get_response() diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index bbbbdae7ced8..ef8e3bf59afb 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -11,6 +11,7 @@ from django.views.generic import TemplateView from oauth2_provider.models import Application +from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected from openedx.core.djangoapps.user_authn.cookies import delete_logged_in_cookies from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect from common.djangoapps.third_party_auth import pipeline as tpa_pipeline @@ -69,7 +70,6 @@ def target(self): def dispatch(self, request, *args, **kwargs): # We do not log here, because we have a handler registered to perform logging on successful logouts. - request.is_from_logout = True # Get third party auth provider's logout url self.tpa_logout_url = tpa_pipeline.get_idp_logout_url_from_running_pipeline(request) @@ -81,6 +81,7 @@ def dispatch(self, request, *args, **kwargs): # Clear the cookie used by the edx.org marketing site delete_logged_in_cookies(response) + mark_user_change_as_expected(response, None) return response def _build_logout_url(self, url):