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

Merge pull request #29768 from openedx/timmc/strict-by-default

Browse Source 
feat: Enable ENFORCE_SAFE_SESSIONS by default
This commit is contained in:
Tim McCormack
2022-01-25 17:19:11 +00:00
committed by GitHub
parent c66498db36 9827a077aa
commit ddc55483da
13 changed files with 92 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
lms/djangoapps/bulk_user_retirement/tests/test_views.py
View File

@@ -13,21 +13,21 @@ class BulkUserRetirementViewTests(APITestCase):
"""
def setUp(self):
super().setUp()
self.client = APIClient()
login_client = APIClient()
self.user1 = UserFactory.create(
username='testuser1',
email='test1@example.com',
password='test1_password',
profile__name="Test User1"
)
self.client.login(username=self.user1.username, password='test1_password')
login_client.login(username=self.user1.username, password='test1_password')
self.user2 = UserFactory.create(
username='testuser2',
email='test2@example.com',
password='test2_password',
profile__name="Test User2"
)
self.client.login(username=self.user2.username, password='test2_password')
login_client.login(username=self.user2.username, password='test2_password')
self.user3 = UserFactory.create(
username='testuser3',
email='test3@example.com',
@@ -47,6 +47,8 @@ class BulkUserRetirementViewTests(APITestCase):
required=True
)
self.pending_state = RetirementState.objects.get(state_name='PENDING')
# Use a separate client for retirement worker (don't mix cookie state)
self.client = APIClient()
self.client.force_authenticate(user=self.user1)
def test_gdpr_user_retirement_api(self):

24
lms/djangoapps/discussion/rest_api/tests/test_views.py
View File

@@ -528,6 +528,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
self.retirement.save()
self.superuser = SuperuserFactory()
self.superuser_client = APIClient()
self.retired_username = get_retired_username_by_username(self.user.username)
self.url = reverse("retire_discussion_user")
@@ -555,7 +556,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
self.register_get_user_retire_response(self.user)
headers = self.build_jwt_headers(self.superuser)
data = {'username': self.user.username}
response = self.client.post(self.url, data, **headers)
response = self.superuser_client.post(self.url, data, **headers)
self.assert_response_correct(response, 204, b"")
def test_downstream_forums_error(self):
@@ -565,7 +566,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
self.register_get_user_retire_response(self.user, status=500, body="Server error")
headers = self.build_jwt_headers(self.superuser)
data = {'username': self.user.username}
response = self.client.post(self.url, data, **headers)
response = self.superuser_client.post(self.url, data, **headers)
self.assert_response_correct(response, 500, '"Server error"')
def test_nonexistent_user(self):
@@ -576,7 +577,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
self.retired_username = get_retired_username_by_username(nonexistent_username)
data = {'username': nonexistent_username}
headers = self.build_jwt_headers(self.superuser)
response = self.client.post(self.url, data, **headers)
response = self.superuser_client.post(self.url, data, **headers)
self.assert_response_correct(response, 404, None)
def test_not_authenticated(self):
@@ -594,8 +595,9 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
"""Tests for ReplaceUsernamesView"""
def setUp(self):
super().setUp()
self.client_user = UserFactory()
self.client_user.username = "test_replace_username_service_worker"
self.worker = UserFactory()
self.worker.username = "test_replace_username_service_worker"
self.worker_client = APIClient()
self.new_username = "test_username_replacement"
self.url = reverse("replace_discussion_username")
@@ -616,11 +618,11 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
headers = {'HTTP_AUTHORIZATION': 'JWT ' + token}
return headers
def call_api(self, user, data):
def call_api(self, user, client, data):
""" Helper function to call API with data """
data = json.dumps(data)
headers = self.build_jwt_headers(user)
return self.client.post(self.url, data, content_type='application/json', **headers)
return client.post(self.url, data, content_type='application/json', **headers)
@ddt.data(
[{}, {}],
@@ -632,7 +634,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
data = {
"username_mappings": mapping_data
}
response = self.call_api(self.client_user, data)
response = self.call_api(self.worker, self.worker_client, data)
assert response.status_code == 400
def test_auth(self):
@@ -650,11 +652,11 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
# Test non-service worker
random_user = UserFactory()
response = self.call_api(random_user, data)
response = self.call_api(random_user, APIClient(), data)
assert response.status_code == 403
# Test service worker
response = self.call_api(self.client_user, data)
response = self.call_api(self.worker, self.worker_client, data)
assert response.status_code == 200
def test_basic(self):
@@ -669,7 +671,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
'successful_replacements': data["username_mappings"]
}
self.register_get_username_replacement_response(self.user)
response = self.call_api(self.client_user, data)
response = self.call_api(self.worker, self.worker_client, data)
assert response.status_code == 200
assert response.data == expected_response

2
lms/djangoapps/lti_provider/users.py
View File

@@ -16,6 +16,7 @@ from django.db import IntegrityError, transaction
from common.djangoapps.student.models import UserProfile
from lms.djangoapps.lti_provider.models import LtiUser
from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected
def authenticate_lti_user(request, lti_user_id, lti_consumer):
@@ -96,6 +97,7 @@ def switch_user(request, lti_user, lti_consumer):
# users by this point, but just in case we can return a 403.
raise PermissionDenied()
login(request, edx_user)
mark_user_change_as_expected(edx_user.id)
def generate_random_edx_username():

2
openedx/core/djangoapps/auth_exchange/views.py
View File

@@ -153,5 +153,5 @@ class LoginWithAccessTokenView(APIView):
login(request, request.user) # login generates and stores the user's cookies in the session
response = HttpResponse(status=204) # cookies stored in the session are returned with the response
mark_user_change_as_expected(response, request.user.id)
mark_user_change_as_expected(request.user.id)
return response

7
openedx/core/djangoapps/content/learning_sequences/views.py
View File

@@ -213,7 +213,12 @@ class CourseOutlineView(APIView):
target_username = request.GET.get("user")
if target_username is not None:
return self._get_target_user(request, course_key, has_staff_access, target_username)
target_user = self._get_target_user(request, course_key, has_staff_access, target_username)
# Just like in masquerading, set real_user so that the
# SafeSessions middleware can see that the user didn't
# change unexpectedly.
target_user.real_user = request.user
return target_user
_course_masquerade, user = setup_masquerade(request, course_key, has_staff_access)
return user

2
openedx/core/djangoapps/content_libraries/views.py
View File

@@ -1022,7 +1022,7 @@ class LtiToolLaunchView(TemplateResponseMixin, LtiToolView):
# Render context and response.
context = self.get_context_data()
response = self.render_to_response(context)
mark_user_change_as_expected(response, edx_user.id)
mark_user_change_as_expected(edx_user.id)
return response
def handle_ags(self):

5
openedx/core/djangoapps/credit/tests/test_views.py
View File

@@ -171,6 +171,7 @@ class CreditCourseViewSetTests(AuthMixin, UserMixin, TestCase):
def test_oauth(self):
""" Verify the endpoint supports OAuth, and only allows authorization for staff users. """
client = Client() # avoid mixing cookies
user = UserFactory(is_staff=False)
oauth_client = ApplicationFactory.create()
access_token = AccessTokenFactory.create(user=user, application=oauth_client).token
@@ -179,13 +180,13 @@ class CreditCourseViewSetTests(AuthMixin, UserMixin, TestCase):
}
# Non-staff users should not have access to the API
response = self.client.get(self.path, **headers)
response = client.get(self.path, **headers)
assert response.status_code == 403
# Staff users should have access to the API
user.is_staff = True
user.save()
response = self.client.get(self.path, **headers)
response = client.get(self.path, **headers)
assert response.status_code == 200
def assert_course_created(self, course_id, response):

29
openedx/core/djangoapps/enrollments/tests/test_views.py
View File

@@ -1349,6 +1349,7 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
""" Create a course and user, then log in. """
super().setUp()
self.superuser = SuperuserFactory()
self.superuser_client = Client()
# Pass emit_signals when creating the course so it would be cached
# as a CourseOverview. Enrollments require a cached CourseOverview.
self.first_org_course = CourseFactory.create(emit_signals=True, org="org", course="course", run="run")
@@ -1404,7 +1405,7 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
def test_deactivate_enrollments(self):
self._assert_active()
self._create_test_retirement(self.user)
response = self._submit_unenroll(self.superuser, self.user.username)
response = self._submit_unenroll(self.user.username)
assert response.status_code == status.HTTP_200_OK
data = json.loads(response.content.decode('utf-8'))
# order doesn't matter so compare sets
@@ -1413,18 +1414,18 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
def test_deactivate_enrollments_no_retirement_status(self):
self._assert_active()
response = self._submit_unenroll(self.superuser, self.user.username)
response = self._submit_unenroll(self.user.username)
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_deactivate_enrollments_unauthorized(self):
self._assert_active()
response = self._submit_unenroll(self.user, self.user.username)
response = self._submit_unenroll(self.user.username, submitting_user=self.user, client=self.client)
assert response.status_code == status.HTTP_403_FORBIDDEN
self._assert_active()
def test_deactivate_enrollments_no_username(self):
self._assert_active()
response = self._submit_unenroll(self.superuser, None)
response = self._submit_unenroll(None)
assert response.status_code == status.HTTP_404_NOT_FOUND
data = json.loads(response.content.decode('utf-8'))
assert data == 'Username not specified.'
@@ -1433,23 +1434,23 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
def test_deactivate_enrollments_empty_username(self):
self._assert_active()
self._create_test_retirement(self.user)
response = self._submit_unenroll(self.superuser, "")
response = self._submit_unenroll("")
assert response.status_code == status.HTTP_404_NOT_FOUND
self._assert_active()
def test_deactivate_enrollments_invalid_username(self):
self._assert_active()
self._create_test_retirement(self.user)
response = self._submit_unenroll(self.superuser, "a made up username")
response = self._submit_unenroll("a made up username")
assert response.status_code == status.HTTP_404_NOT_FOUND
self._assert_active()
def test_deactivate_enrollments_called_twice(self):
self._assert_active()
self._create_test_retirement(self.user)
response = self._submit_unenroll(self.superuser, self.user.username)
response = self._submit_unenroll(self.user.username)
assert response.status_code == status.HTTP_200_OK
response = self._submit_unenroll(self.superuser, self.user.username)
response = self._submit_unenroll(self.user.username)
assert response.status_code == status.HTTP_204_NO_CONTENT
assert response.content.decode('utf-8') == ''
self._assert_inactive()
@@ -1465,14 +1466,22 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
_, is_active = CourseEnrollment.enrollment_mode_for_user(self.user, course.id)
assert not is_active
def _submit_unenroll(self, submitting_user, unenrolling_username):
def _submit_unenroll(self, unenrolling_username, submitting_user=None, client=None):
""" Submit enrollment, by default as superuser. """
# Provide both or neither of the overrides
assert (submitting_user is None) == (client is None)
# Avoid mixing cookies between two users
client = client or self.superuser_client
submitting_user = submitting_user or self.superuser
data = {}
if unenrolling_username is not None:
data['username'] = unenrolling_username
url = reverse('unenrollment')
headers = self.build_jwt_headers(submitting_user)
return self.client.post(url, json.dumps(data), content_type='application/json', **headers)
return client.post(url, json.dumps(data), content_type='application/json', **headers)
@ddt.ddt

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

@@ -90,7 +90,7 @@ from django.core.cache import cache
from django.http import HttpResponse
from django.utils.crypto import get_random_string
from django.utils.deprecation import MiddlewareMixin
from edx_django_utils.cache import RequestCache
from edx_django_utils.monitoring import set_custom_attribute
from edx_toggles.toggles import SettingToggle
@@ -133,26 +133,40 @@ LOG_REQUEST_USER_CHANGE_HEADERS_DURATION = getattr(settings, 'LOG_REQUEST_USER_C
# .. toggle_name: ENFORCE_SAFE_SESSIONS
# .. toggle_implementation: SettingToggle
# .. toggle_default: False
# .. toggle_description: Turn this toggle on to enforce safe-sessions policy.
# .. toggle_default: True
# .. toggle_description: Invalidate session and response if mismatch detected.
# That is, when the `user` attribute of the request object gets changed or
# no longer matches the session, the session will be invalidated and the
# response cancelled (changed to an error). This is intended as a backup
# safety measure in case an attacker (or bug) is able to change the user
# on a session in an unexpected way. The behavior will be available for
# the Nutmeg named release and will become permanent in Olive.
# .. toggle_warnings: Before enabling, confirm that incidences of the string
# "SafeCookieData user at request" in the logs only show false positives,
# such as people logging in while in possession of an already-valid session
# cookie.
# .. toggle_use_cases: temporary
# on a session in an unexpected way.
# .. toggle_warnings: Should be disabled if debugging mismatches using the
# LOG_REQUEST_USER_CHANGE_HEADERS toggle, otherwise series of mismatching
# requests from the same user cannot be investigated. Additionally, if
# enabling for the first time, confirm that incidences of the string
# "SafeCookieData user at request" in the logs are very rare; if they are
# not, it is likely that there is either a bug or that a login or
# registration endpoint needs to call ``mark_user_change_as_expected``.
# .. toggle_use_cases: opt_out
# .. toggle_creation_date: 2021-12-01
# .. toggle_target_removal_date: 2022-08-01
# .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1861
ENFORCE_SAFE_SESSIONS = SettingToggle('ENFORCE_SAFE_SESSIONS', default=False)
ENFORCE_SAFE_SESSIONS = SettingToggle('ENFORCE_SAFE_SESSIONS', default=True)
log = getLogger(__name__)
# RequestCache for conveying information from views back up to the
# middleware -- specifically, information about expected changes to
# request.user
#
# Rejected alternatives for where to place the annotation:
#
# - request object: Different request objects are presented to middlewares
# and views, so the attribute would be lost.
# - response object: Doesn't help in cases where an exception is thrown
# instead of a response returned. Still want to validate that users don't
# change unexpectedly on a 404, for example.
request_cache = RequestCache(namespace="safe-sessions")
class SafeCookieError(Exception):
"""
@@ -662,7 +676,7 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
#
# The relevant views set a flag to indicate the exemption.
if getattr(response, 'safe_sessions_expected_user_change', None):
if request_cache.get_cached_response('expected_user_change').is_found:
return no_mismatch_dict
if not hasattr(request, 'safe_cookie_verified_user_id'):
@@ -672,6 +686,10 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
if hasattr(request.user, 'real_user'):
# If a view overrode the request.user with a masqueraded user, this will
# revert/clean-up that change during response processing.
# Known places this is set:
#
# - lms.djangoapps.courseware.masquerade::setup_masquerade
# - openedx.core.djangoapps.content.learning_sequences.views::CourseOutlineView
request.user = request.user.real_user
# determine if the request.user is different now than it was on the initial request
@@ -887,7 +905,7 @@ def track_request_user_changes(request):
request.__class__ = SafeSessionRequestWrapper
def mark_user_change_as_expected(response, new_user_id):
def mark_user_change_as_expected(new_user_id):
"""
Indicate to the safe-sessions middleware that it is expected that
the user is changing between the request and response phase of
@@ -896,4 +914,4 @@ def mark_user_change_as_expected(response, new_user_id):
The new_user_id may be None or an LMS user ID, and may be the same
as the previous user ID.
"""
response.safe_sessions_expected_user_change = {'new_user_id': new_user_id}
request_cache.set('expected_user_change', new_user_id)

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

@@ -341,9 +341,10 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
self.request.META = {'HTTP_USER_AGENT': 'open edX Mobile App Version 2.1'}
self.verify_error(401)
@override_settings(ENFORCE_SAFE_SESSIONS=False)
def test_warn_on_user_change_before_response(self):
"""
Verifies that warnings are emitted and custom attributes set if
Verifies that when enforcement disabled, warnings are emitted and custom attributes set if
the user changes unexpectedly between request and response.
"""
self.set_up_for_success()
@@ -358,10 +359,9 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
set_attr_call_args = [call.args for call in mock_attr.call_args_list]
assert ("safe_sessions.user_mismatch", "request-response-mismatch") in set_attr_call_args
@override_settings(ENFORCE_SAFE_SESSIONS=True)
def test_enforce_on_user_change_before_response(self):
"""
Copy of test_warn_on_user_change_before_response but with enforcement enabled.
Copy of test_warn_on_user_change_before_response but with enforcement enabled (default).
The differences should be the status code and the session deletion.
"""
self.set_up_for_success()
@@ -379,6 +379,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
set_attr_call_args = [call.args for call in mock_attr.call_args_list]
assert ("safe_sessions.user_mismatch", "request-response-mismatch") in set_attr_call_args
@override_settings(ENFORCE_SAFE_SESSIONS=False)
def test_warn_on_user_change_from_session(self):
"""
Verifies that warnings are emitted and custom attributes set if
@@ -395,6 +396,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
set_attr_call_args = [call.args for call in mock_attr.call_args_list]
assert ("safe_sessions.user_mismatch", "request-session-mismatch") in set_attr_call_args
@override_settings(ENFORCE_SAFE_SESSIONS=False)
def test_warn_on_user_change_in_both(self):
"""
Verifies that warnings are emitted and custom attributes set if
@@ -482,7 +484,7 @@ class TestSafeSessionMiddleware(TestSafeSessionsLogMixin, CacheIsolationTestCase
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)
mark_user_change_as_expected(new_user.id)
with self.assert_no_warning_logged():
with patch('openedx.core.djangoapps.safe_sessions.middleware.set_custom_attribute') as mock_attr:

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

@@ -605,7 +605,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)
mark_user_change_as_expected(user.id)
return response
except AuthFailedError as error:
response_content = error.get_response()

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

@@ -82,7 +82,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)
mark_user_change_as_expected(None)
return response
def _build_logout_url(self, url):

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

@@ -589,7 +589,7 @@ class RegistrationView(APIView):
path='/',
secure=request.is_secure()
) # setting the cookie to show account activation dialogue in platform and learning MFE
mark_user_change_as_expected(response, user.id)
mark_user_change_as_expected(user.id)
return response
def _handle_duplicate_email_username(self, request, data):