From 3c8700bda9277b98f5bd84974c249e3f5ae6e15e Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Wed, 24 Jul 2019 12:09:36 +0300 Subject: [PATCH] Use hashed keys for safe cache keys (#1662) * Use hashed keys for safe cache keys Prevents maximum recursion depth error. Fixes #1661 * Update tests to use safe_key() for cache keys --- .../tests/viewsets/test_connect_viewset.py | 29 ++++++++++--------- onadata/libs/tests/utils/test_cache_tools.py | 8 ++--- onadata/libs/utils/cache_tools.py | 14 ++++----- 3 files changed, 25 insertions(+), 26 deletions(-) diff --git a/onadata/apps/api/tests/viewsets/test_connect_viewset.py b/onadata/apps/api/tests/viewsets/test_connect_viewset.py index 03b8e70d99..0a559e1375 100644 --- a/onadata/apps/api/tests/viewsets/test_connect_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_connect_viewset.py @@ -22,6 +22,7 @@ from onadata.apps.api.viewsets.project_viewset import ProjectViewSet from onadata.libs.authentication import DigestAuthentication from onadata.libs.serializers.project_serializer import ProjectSerializer +from onadata.libs.utils.cache_tools import safe_key class TestConnectViewSet(TestAbstractViewSet): @@ -369,10 +370,10 @@ def test_login_attempts(self, send_account_lockout_email): authentication_classes=(DigestAuthentication,)) auth = DigestAuth('bob', 'bob') # clear cache - cache.delete('login_attempts-bob') - cache.delete('lockout_user-bob') - self.assertIsNone(cache.get('login_attempts-bob')) - self.assertIsNone(cache.get('lockout_user-bob')) + cache.delete(safe_key("login_attempts-bob")) + cache.delete(safe_key("lockout_user-bob")) + self.assertIsNone(cache.get(safe_key('login_attempts-bob'))) + self.assertIsNone(cache.get(safe_key('lockout_user-bob'))) request = self._get_request_session_with_auth(view, auth) @@ -383,7 +384,7 @@ def test_login_attempts(self, send_account_lockout_email): u"Invalid username/password. For security reasons, " u"after 9 more failed login attempts you'll have to " u"wait 30 minutes before trying again.") - self.assertEqual(cache.get('login_attempts-bob'), 1) + self.assertEqual(cache.get(safe_key('login_attempts-bob')), 1) # cache value increments with subsequent attempts response = view(request) @@ -392,30 +393,30 @@ def test_login_attempts(self, send_account_lockout_email): u"Invalid username/password. For security reasons, " u"after 8 more failed login attempts you'll have to " u"wait 30 minutes before trying again.") - self.assertEqual(cache.get('login_attempts-bob'), 2) + self.assertEqual(cache.get(safe_key('login_attempts-bob')), 2) # login_attempts doesn't increase with correct login auth = DigestAuth('bob', 'bobbob') request = self._get_request_session_with_auth(view, auth) response = view(request) self.assertEqual(response.status_code, 200) - self.assertEqual(cache.get('login_attempts-bob'), 2) + self.assertEqual(cache.get(safe_key('login_attempts-bob')), 2) # lockout_user cache created upon fifth attempt auth = DigestAuth('bob', 'bob') request = self._get_request_session_with_auth(view, auth) self.assertFalse(send_account_lockout_email.called) - cache.set('login_attempts-bob', 9) - self.assertIsNone(cache.get('lockout_user-bob')) + cache.set(safe_key('login_attempts-bob'), 9) + self.assertIsNone(cache.get(safe_key('lockout_user-bob'))) response = view(request) self.assertEqual(response.status_code, 401) self.assertEqual(response.data['detail'], u"Locked out. Too many wrong username/password " u"attempts. Try again in 30 minutes.") - self.assertEqual(cache.get('login_attempts-bob'), 10) - self.assertIsNotNone(cache.get('lockout_user-bob')) + self.assertEqual(cache.get(safe_key('login_attempts-bob')), 10) + self.assertIsNotNone(cache.get(safe_key('lockout_user-bob'))) lockout = datetime.strptime( - cache.get('lockout_user-bob'), '%Y-%m-%dT%H:%M:%S') + cache.get(safe_key('lockout_user-bob')), '%Y-%m-%dT%H:%M:%S') self.assertIsInstance(lockout, datetime) # email sent upon limit being reached with right arguments @@ -436,5 +437,5 @@ def test_login_attempts(self, send_account_lockout_email): u"Locked out. Too many wrong username/password " u"attempts. Try again in 30 minutes.") # clear cache - cache.delete('login_attempts-bob') - cache.delete('lockout_user-bob') + cache.delete(safe_key("login_attempts-bob")) + cache.delete(safe_key("lockout_user-bob")) diff --git a/onadata/libs/tests/utils/test_cache_tools.py b/onadata/libs/tests/utils/test_cache_tools.py index eb885816b0..8059d248de 100644 --- a/onadata/libs/tests/utils/test_cache_tools.py +++ b/onadata/libs/tests/utils/test_cache_tools.py @@ -11,7 +11,7 @@ class TestCacheTools(TestCase): """Test onadata.libs.utils.cache_tools module class""" def test_safe_key(self): - """Test safe_key() functions returns a valid key""" - self.assertEqual(safe_key("hello_world"), "hello_world") - self.assertEqual(safe_key("hello world"), "hello-world") - self.assertEqual(safe_key("hello@world"), "hello@world") + """Test safe_key() function returns a hashed key""" + self.assertEqual( + safe_key("hello world"), + "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9") diff --git a/onadata/libs/utils/cache_tools.py b/onadata/libs/utils/cache_tools.py index e4ffc8f71d..1a53d75941 100644 --- a/onadata/libs/utils/cache_tools.py +++ b/onadata/libs/utils/cache_tools.py @@ -1,6 +1,7 @@ -import re +import hashlib from django.core.cache import cache +from django.utils.encoding import force_bytes # Cache names used in project serializer PROJ_PERM_CACHE = "ps-project_permissions-" @@ -31,13 +32,10 @@ def safe_delete(key): - cache.get(key) and cache.delete(key) + """Safely deletes a given key from the cache.""" + _ = cache.get(key) and cache.delete(key) def safe_key(key): - """Return a valid cache key""" - valid_key_chars_re = re.compile("[\x21-\x7e\x80-\xff]+$") - if not valid_key_chars_re.match(key): - return safe_key(key.replace(" ", "-")) - - return key + """Return a hashed key.""" + return hashlib.sha256(force_bytes(key)).hexdigest()