Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: Don't warn about expected user changes in safe-sessions (#28983)

Browse Source 
This is intended to silence a rare false positive that seems to happen
when someone logs in on a browser that already has an active session
for another user. We believe there should be no further positives once
this case is handled.

- login and logout views annotate the response to indicate the session
  user should be changing between the request and response phases
- safe-sessions middleware skips the verify-user check when this
  annotation is present

Also:

- Adds a test around existing behavior for unexpected user-changes
- Remove logging control based on `is_from_log_out`. This reverts most
  of af9e26f/PR #11479 for two reasons:
  - The safe-sessions `_verify_user` code has since changed to check for
    `request.user.id == None`
  - A commit later in the PR changes the login and logout pages to
    signal that the user/session change is expected
This commit is contained in:
Tim McCormack
2021-10-13 15:53:16 +00:00
committed by GitHub
parent 3cc1389d68
commit fe3d855986
5 changed files with 89 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
lms/djangoapps/courseware/tests/tests.py
View File

@@ -38,14 +38,6 @@ class ActivateLoginTest(LoginEnrollmentTestCase):
"""
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):
"""

73
openedx/core/djangoapps/safe_sessions/middleware.py
View File

@@ -3,6 +3,17 @@ This module defines SafeSessionMiddleware that makes use of a
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
<http://www.cse.msu.edu/~alexliu/publications/Cookie/cookie.pdf>
but deviates in a number of ways; mostly it just uses the technique
@@ -66,9 +77,8 @@ Custom Attributes:
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 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
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 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
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 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
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 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
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 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
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 log_request_user_changes(request):
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):
The new_user_id may be None or an LMS user ID, and may be the same
as the previous user ID.
"""
Control the logging by changing logger's level if
the request is from logout.
"""
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}

47
openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
View File

@@ -17,7 +17,7 @@ from django.test.utils import override_settings
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 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, TestCase):
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 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, TestCase):
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 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, TestCase):
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):

2
openedx/core/djangoapps/user_authn/views/login.py
View File

@@ -41,6 +41,7 @@ from common.djangoapps.track import segment
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()

3
openedx/core/djangoapps/user_authn/views/logout.py
View File

@@ -11,6 +11,7 @@ from django.utils.http import urlencode
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 @@ class LogoutView(TemplateView):
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 @@ class LogoutView(TemplateView):
# 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):