From b8ba687a2eeb495ed81683d0eb93ff232d17618d Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Tue, 29 Sep 2020 14:38:48 +0200 Subject: [PATCH] misc: fix locking in clean_nonces (#47122) --- tests/test_api.py | 23 ++++++++++++++++------- wcs/qommon/publisher.py | 24 ++++++++++-------------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 984f781e..fd9be53a 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -296,7 +296,7 @@ def test_get_user(pub, local_user): assert [x['slug'] for x in output.json['user_roles']] == ['foo-bar'] -def test_is_url_signed_check_nonce(pub, local_user): +def test_is_url_signed_check_nonce(pub, local_user, freezer): ORIG = 'xxx' KEY = 'xxx' @@ -306,7 +306,9 @@ def test_is_url_signed_check_nonce(pub, local_user): # test clean_nonces do not bark when nonces directory is empty if os.path.exists(os.path.join(pub.app_dir, 'nonces')): shutil.rmtree(os.path.join(pub.app_dir, 'nonces')) - pub.clean_nonces() + pub.clean_nonces(now=0) + nonce_dir = os.path.join(pub.app_dir, 'nonces') + assert not os.path.exists(nonce_dir) or not os.listdir(nonce_dir) signed_url = sign_url('?format=json&orig=%s&email=%s' % (ORIG, urllib.quote(local_user.email)), KEY) req = HTTPRequest(None, {'SCRIPT_NAME': '/', @@ -322,11 +324,18 @@ def test_is_url_signed_check_nonce(pub, local_user): is_url_signed() assert exc_info.value.public_msg == 'nonce already used' # test that clean nonces works - assert os.listdir(os.path.join(pub.app_dir, 'nonces')) - pub.clean_nonces(delta=0) - assert os.listdir(os.path.join(pub.app_dir, 'nonces')) - pub.clean_nonces(delta=0, now=time.time() + 2 * DEFAULT_DURATION) - assert not os.listdir(os.path.join(pub.app_dir, 'nonces')) + pub.clean_nonces() + assert os.listdir(nonce_dir) + + # 80 seconds in the future, nothing should be cleaned + freezer.move_to(datetime.timedelta(seconds=80)) + pub.clean_nonces() + assert os.listdir(nonce_dir) + + # 90 seconds in the future, nonces should be removed + freezer.move_to(datetime.timedelta(seconds=10)) + pub.clean_nonces() + assert not os.listdir(nonce_dir) def test_get_user_compat_endpoint(pub, local_user): diff --git a/wcs/qommon/publisher.py b/wcs/qommon/publisher.py index 4db97206..353f929d 100644 --- a/wcs/qommon/publisher.py +++ b/wcs/qommon/publisher.py @@ -62,6 +62,7 @@ import logging import logging.handlers from . import logger from . import storage +from .vendor import locket class ImmediateRedirectException(Exception): @@ -571,24 +572,19 @@ class QommonPublisher(Publisher, object): def clean_nonces(self, delta=60, now=None): nonce_dir = os.path.join(get_publisher().app_dir, 'nonces') - now = now or time.time() if not os.path.exists(nonce_dir): return cleaning_lock_file = os.path.join(self.app_dir, 'cleaning_nonces.lock') - fd = os.open(cleaning_lock_file, os.O_RDONLY | os.O_CREAT, 0o666) try: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - except IOError: - # lock is currently held, that is fine. - return - try: - for nonce in os.listdir(nonce_dir): - nonce_path = os.path.join(nonce_dir, nonce) - # we need delta so that os.utime in is_url_signed has time to update file timestamp - if delta < now - os.stat(nonce_path)[8]: - os.unlink(nonce_path) - finally: - os.close(fd) + now = now or time.time() + with locket.lock_file(cleaning_lock_file, timeout=0): + for nonce in os.listdir(nonce_dir): + nonce_path = os.path.join(nonce_dir, nonce) + # we need delta so that os.utime in is_url_signed has time to update file timestamp + if delta < now - os.stat(nonce_path)[8]: + os.unlink(nonce_path) + except locket.LockError: + pass def clean_sessions(self): cleaning_lock_file = os.path.join(self.app_dir, 'cleaning_sessions.lock') -- 2.28.0