Skip to content

Commit

Permalink
feat!: Adding fall back method for sha1 in case default algo is `sh…
Browse files Browse the repository at this point in the history
…a256` (openedx#33345)

* feat!: `sha1` has been deprecated in django32 and removed in django42.

* test: fix quality failure

* fixup! update custom attribute tests (openedx#33436)

I was wondering about all the cases, so I
updated the test to reflect this. I also
made some other minor adjustments.

---------

Co-authored-by: Muhammad Soban Javed <[email protected]>
Co-authored-by: Robert Raposa <[email protected]>
Co-authored-by: Muhammad Soban Javed <[email protected]>
  • Loading branch information
4 people authored Oct 11, 2023
1 parent 14b61ae commit 2549035
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 19 deletions.
2 changes: 1 addition & 1 deletion cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1086,8 +1086,8 @@
}

DEFAULT_AUTO_FIELD = 'django.db.models.AutoField'
# This will be overridden through CMS config
DEFAULT_HASHING_ALGORITHM = 'sha1'

#################### Python sandbox ############################################

CODE_JAIL = {
Expand Down
2 changes: 1 addition & 1 deletion lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1734,8 +1734,8 @@ def _make_mako_template_dirs(settings):


DEFAULT_AUTO_FIELD = 'django.db.models.AutoField'
# This will be overridden through LMS config
DEFAULT_HASHING_ALGORITHM = 'sha1'

#################### Python sandbox ############################################

CODE_JAIL = {
Expand Down
34 changes: 28 additions & 6 deletions openedx/core/djangoapps/cache_toolbox/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user
from django.utils.crypto import constant_time_compare
from django.utils.deprecation import MiddlewareMixin
from edx_django_utils.monitoring import set_custom_attribute

from openedx.core.djangoapps.safe_sessions.middleware import SafeSessionMiddleware, _mark_cookie_for_deletion

Expand All @@ -112,6 +113,7 @@ def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

def process_request(self, request):
set_custom_attribute('DEFAULT_HASHING_ALGORITHM', settings.DEFAULT_HASHING_ALGORITHM)
try:
# Try and construct a User instance from data stored in the cache
session_user_id = SafeSessionMiddleware.get_user_id_from_session(request)
Expand Down Expand Up @@ -141,9 +143,29 @@ def _verify_session_auth(self, request):
auto_auth_enabled = settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING', False)
if not auto_auth_enabled and hasattr(request.user, 'get_session_auth_hash'):
session_hash = request.session.get(HASH_SESSION_KEY)
if not (session_hash and constant_time_compare(session_hash, request.user.get_session_auth_hash())):
# The session hash has changed due to a password
# change. Log the user out.
request.session.flush()
request.user = AnonymousUser()
_mark_cookie_for_deletion(request)
session_hash_verified = session_hash and constant_time_compare(
session_hash, request.user.get_session_auth_hash())

# session hash is verified from the default algo, so skip legacy check
if session_hash_verified:
set_custom_attribute('session_hash_verified', "default")
return

if (
session_hash and
hasattr(request.user, '_legacy_get_session_auth_hash') and
constant_time_compare(
session_hash,
request.user._legacy_get_session_auth_hash() # pylint: disable=protected-access
)
):
# session hash is verified from legacy hashing algorithm.
set_custom_attribute('session_hash_verified', "fallback")
return

# The session hash has changed due to a password
# change. Log the user out.
request.session.flush()
request.user = AnonymousUser()
_mark_cookie_for_deletion(request)
set_custom_attribute('failed_session_verification', True)
103 changes: 92 additions & 11 deletions openedx/core/djangoapps/cache_toolbox/tests/test_middleware.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
"""Tests for cached authentication middleware."""
from unittest.mock import patch
from unittest.mock import call, patch

import django
from django.conf import settings
from django.contrib.auth.models import User, AnonymousUser # lint-amnesty, pylint: disable=imported-auth-user
from django.urls import reverse
from django.test import TestCase
from django.contrib.auth import SESSION_KEY
from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user
from django.http import HttpResponse, SimpleCookie
from django.test import TestCase
from django.urls import reverse

from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangoapps.cache_toolbox.middleware import CacheBackedAuthenticationMiddleware
from openedx.core.djangoapps.safe_sessions.middleware import SafeCookieData, SafeSessionMiddleware
from openedx.core.djangolib.testing.utils import skip_unless_cms, skip_unless_lms, get_mock_request
from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangolib.testing.utils import get_mock_request, skip_unless_cms, skip_unless_lms


class CachedAuthMiddlewareTestCase(TestCase):
Expand All @@ -36,9 +37,68 @@ def _test_change_session_hash(self, test_url, redirect_url, target_status_code=2
"""
response = self.client.get(test_url)
assert response.status_code == 200
with patch.object(User, 'get_session_auth_hash', return_value='abc123'):
response = self.client.get(test_url)
self.assertRedirects(response, redirect_url, target_status_code=target_status_code)

with patch(
"openedx.core.djangoapps.cache_toolbox.middleware.set_custom_attribute"
) as mock_set_custom_attribute:
with patch.object(User, 'get_session_auth_hash', return_value='abc123', autospec=True):
# Django 3.2 has _legacy_get_session_auth_hash, and Django 4 does not
# Remove once we reach Django 4
if hasattr(User, '_legacy_get_session_auth_hash'):
with patch.object(User, '_legacy_get_session_auth_hash', return_value='abc123'):
response = self.client.get(test_url)
else:
response = self.client.get(test_url)

self.assertRedirects(response, redirect_url, target_status_code=target_status_code)
mock_set_custom_attribute.assert_any_call('failed_session_verification', True)

def _test_custom_attribute_after_changing_hash(self, test_url, mock_set_custom_attribute):
"""verify that set_custom_attribute is called with expected values"""
password = 'test-password'

# Test DEFAULT_HASHING_ALGORITHM of 'sha1' for both login and client get
with self.settings(DEFAULT_HASHING_ALGORITHM='sha1'):
self.client.login(username=self.user.username, password=password)
self.client.get(test_url)
# For Django 3.2, the setting 'sha1' applies and is the "default".
# For Django 4, the setting no longer applies, and 'sha256' will be used for both as the "default".
mock_set_custom_attribute.assert_has_calls([
call('DEFAULT_HASHING_ALGORITHM', 'sha1'),
call('session_hash_verified', "default"),
])
mock_set_custom_attribute.reset_mock()

# Test DEFAULT_HASHING_ALGORITHM of 'sha1' for login and switch to 'sha256' for client get.
with self.settings(DEFAULT_HASHING_ALGORITHM='sha1'):
self.client.login(username=self.user.username, password=password)
with self.settings(DEFAULT_HASHING_ALGORITHM='sha256'):
self.client.get(test_url)
if django.VERSION < (4, 0):
# For Django 3.2, the setting 'sha1' applies to login, and uses 'she256' for client get,
# and should "fallback" to 'sha1".
mock_set_custom_attribute.assert_has_calls([
call('DEFAULT_HASHING_ALGORITHM', 'sha256'),
call('session_hash_verified', "fallback"),
])
else:
# For Django 4, the setting no longer applies, and again 'sha256' will be used for both as the "default".
mock_set_custom_attribute.assert_has_calls([
call('DEFAULT_HASHING_ALGORITHM', 'sha256'),
call('session_hash_verified', "default"),
])
mock_set_custom_attribute.reset_mock()

# Test DEFAULT_HASHING_ALGORITHM of 'sha256' for both login and client get
with self.settings(DEFAULT_HASHING_ALGORITHM='sha256'):
self.client.login(username=self.user.username, password=password)
self.client.get(test_url)
# For Django 3.2, the setting 'sha256' applies and is the "default".
# For Django 4, the setting no longer applies, and 'sha256' will be used for both as the "default".
mock_set_custom_attribute.assert_has_calls([
call('DEFAULT_HASHING_ALGORITHM', 'sha256'),
call('session_hash_verified', "default"),
])

@skip_unless_lms
def test_session_change_lms(self):
Expand All @@ -53,6 +113,20 @@ def test_session_change_cms(self):
# Studio login redirects to LMS login
self._test_change_session_hash(home_url, settings.LOGIN_URL + '?next=' + home_url, target_status_code=302)

@skip_unless_lms
@patch("openedx.core.djangoapps.cache_toolbox.middleware.set_custom_attribute")
def test_custom_attribute_after_changing_hash_lms(self, mock_set_custom_attribute):
"""Test set_custom_attribute is called with expected values in LMS"""
test_url = reverse('dashboard')
self._test_custom_attribute_after_changing_hash(test_url, mock_set_custom_attribute)

@skip_unless_cms
@patch("openedx.core.djangoapps.cache_toolbox.middleware.set_custom_attribute")
def test_custom_attribute_after_changing_hash_cms(self, mock_set_custom_attribute):
"""Test set_custom_attribute is called with expected values in CMS"""
test_url = reverse('home')
self._test_custom_attribute_after_changing_hash(test_url, mock_set_custom_attribute)

def test_user_logout_on_session_hash_change(self):
"""
Verify that if a user's session auth hash and the request's hash
Expand All @@ -75,8 +149,15 @@ def test_user_logout_on_session_hash_change(self):
assert self.client.response.cookies.get(settings.SESSION_COOKIE_NAME).value == session_id
assert self.client.response.cookies.get('edx-jwt-cookie-header-payload').value == 'test-jwt-payload'

with patch.object(User, 'get_session_auth_hash', return_value='abc123'):
CacheBackedAuthenticationMiddleware(get_response=lambda request: None).process_request(self.request)
with patch.object(User, 'get_session_auth_hash', return_value='abc123', autospec=True):
# Django 3.2 has _legacy_get_session_auth_hash, and Django 4 does not
# Remove once we reach Django 4
if hasattr(User, '_legacy_get_session_auth_hash'):
with patch.object(User, '_legacy_get_session_auth_hash', return_value='abc123'):
CacheBackedAuthenticationMiddleware(get_response=lambda request: None).process_request(self.request)

else:
CacheBackedAuthenticationMiddleware(get_response=lambda request: None).process_request(self.request)
SafeSessionMiddleware(get_response=lambda request: None).process_response(
self.request, self.client.response
)
Expand Down
10 changes: 10 additions & 0 deletions openedx/core/djangoapps/safe_sessions/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,16 @@ def test_update_cookie_data_at_step_3(self):
assert safe_cookie_data.session_id == 'some_session_id'
assert safe_cookie_data.verify(self.user.id)

def test_update_cookie_data_at_step_3_with_sha256(self):
""" first encode cookie with default algo sha1 and then check with sha256"""
self.assert_response(set_request_user=True, set_session_cookie=True)
serialized_cookie_data = self.client.response.cookies[settings.SESSION_COOKIE_NAME].value
safe_cookie_data = SafeCookieData.parse(serialized_cookie_data)
assert safe_cookie_data.version == SafeCookieData.CURRENT_VERSION
assert safe_cookie_data.session_id == 'some_session_id'
with self.settings(DEFAULT_HASHING_ALGORITHM='sha256'):
assert safe_cookie_data.verify(self.user.id)

def test_cant_update_cookie_at_step_3_error(self):
self.client.response.cookies[settings.SESSION_COOKIE_NAME] = None
with self.assert_invalid_session_id():
Expand Down

0 comments on commit 2549035

Please sign in to comment.