misc: fix locking in clean_nonces (#47122)

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):

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')