Merge pull request #637 from ZeitOnline/ZO-4267-contract-lock

ZO-4267: Clarify unlock() contract, and prevent unlocking a resource that was locked externally

core/docs/changelog/ZO-4267-unlock.change

@@ -0,0 +1 @@
+ZO-4267: Prevent unlocking a resource that was locked externally

core/src/zeit/connector/connector.py

@@ -395,16 +395,16 @@ def lock(self, id, principal, until):
         self._invalidate_cache(id)
         return token
 
-    def unlock(self, id, locktoken=None, invalidate=True):
-        if invalidate:
-            self._invalidate_cache(id)
-        url = self._id2loc(self._get_cannonical_id(id))
-        locktoken = locktoken or self._get_dav_lock(id).get('locktoken')
+    def unlock(self, id):
+        self._invalidate_cache(id)
+        locktoken = self._get_my_locktoken(id)
         if locktoken:
-            self.get_connection().unlock(url, locktoken)
-        if invalidate:
+            self._unlock(id, locktoken)
             self._invalidate_cache(id)
-        return locktoken
+
+    def _unlock(self, id, locktoken):
+        url = self._id2loc(self._get_cannonical_id(id))
+        self.get_connection().unlock(url, locktoken)
 
     def locked(self, id):
         id = self._get_cannonical_id(id)
@@ -414,11 +414,9 @@ def locked(self, id):
             # The resource does not exist on the server. This means it *cannot*
             # be locked.
             davlock = {}
-        owner = davlock.get('owner')
-        timeout = davlock.get('timeout')
-        token = davlock.get('locktoken')
-        mylock = None
 
+        owner = davlock.get('owner')
+        mylock = False
         if davlock and owner:
             # Let's see if the principal is one we know.
             try:
@@ -435,16 +433,17 @@ def locked(self, id):
                 except zope.authentication.interfaces.PrincipalLookupError:
                     pass
                 else:
-                    mylock = (token, owner, timeout)
+                    mylock = True
 
+        timeout = davlock.get('timeout')
         if timeout == 'Infinite':
             timeout = TIME_ETERNITY
         if timeout and timeout < datetime.datetime.now(pytz.UTC):
             # The lock has timed out
             self._invalidate_cache(id)
             return self.locked(id)
 
-        return (owner, timeout, mylock is not None)
+        return (owner, timeout, mylock)
 
     def search(self, attrlist, expr):
         """Search repository behind this connector according to <expr>.
@@ -568,9 +567,8 @@ def _internal_add(self, id, resource, verify_etag=True):
             self.changeProperties(id, properties, locktoken=locktoken)
         finally:
             if autolock and locktoken:  # This was _our_ lock. Cleanup:
-                self.unlock(id, locktoken=locktoken)
-            else:
-                self._invalidate_cache(id)
+                self._unlock(id, locktoken)
+            self._invalidate_cache(id)
 
     def _add_collection(self, id):
         # NOTE id is the collection's id. Trailing slash is appended if needed.

core/src/zeit/connector/generation/evolve1.py

@@ -2,15 +2,13 @@
 import zope.component.hooks
 import zope.generations.utility
 
-import zeit.connector.generation.install
-import zeit.connector.interfaces
-import zeit.connector.lockinfo
-
 
 generation = 1
 
 
 def update(root):
+    import zeit.connector.lockinfo
+
     site_manager = zope.component.getSiteManager()
     # Install lockinfo and move the storage from the body cache
     lockinfo = zeit.connector.generation.install.installLocalUtility(

core/src/zeit/connector/generation/install.py

@@ -5,7 +5,6 @@
 import zeit.connector.cache
 import zeit.connector.interfaces
 import zeit.connector.invalidator
-import zeit.connector.lockinfo
 
 
 def installLocalUtility(root, factory, name, interface, utility_name=''):

core/src/zeit/connector/interfaces.py

@@ -66,7 +66,6 @@ class IConnector(zope.interface.Interface):
     """Connects the cms to the backend.
 
     This utility interfaces the CMSConnector provided by Thomas, Ralf, etc.
-
     """
 
     def listCollection(id):
@@ -92,21 +91,18 @@ def __getitem__(id):
         returns an IResource instance.
 
         raises KeyError if the resource could not be found.
-
         """
 
     def __delitem__(id):
         """Remove the resource from the repository.
 
         raises KeyError if the resource could not be found.
-
         """
 
     def __setitem__(id, object):
         """Add the given `object` to the document store under the given name.
 
         The object must be adaptable to IResource.
-
         """
 
     def __contains__(id):
@@ -124,31 +120,27 @@ def add(object, verify_etag=True):
         Raises PreconditionFailedError if verify_etag is True and the etags do
         not match or the resource's lock state is different from the expected
         state.
-
         """
 
     def copy(old_id, new_id):
         """Copy the resource old_id to new_id.
 
         raises KeyError if ther is no resource `old_id`
         raises CopyError if there was a problem with copying itself.
-
         """
 
     def move(old_id, new_id):
         """Move the resource with id `old_id` to `new_id`.
 
         raises KeyError if ther is no resource `old_id`
         raises MoveError if there was a problem with moving itself.
-
         """
 
     def changeProperties(id, properties):
         """Change webdav properties of object.
 
         id: unique id
         properties: see IWebDAVProperties.properties
-
         """
 
     def lock(id, principal, until):
@@ -158,17 +150,13 @@ def lock(id, principal, until):
         until: datetime until the lock is valid.
 
         returns locktoken.
-
         """
 
-    def unlock(id, locktoken=None):
-        """Unlock resource.
-
-        Use `locktoken` if given. Otherwise use stored locktoken.
+    def unlock(id):
+        """Unlock resource using the stored locktoken."""
 
-        returns used locktoken.
-
-        """
+    def _unlock(id, locktoken):
+        """Unlock resource using the given locktoken. For tests."""
 
     def locked(id):
         """Return whether a resource is locked or not.
@@ -181,7 +169,6 @@ def locked(id):
 
         returns None, None, None if the resource is not locked.
         raises KeyError if the resource does not exist.
-
         """
 
     def search(attributes, search_expression):
@@ -191,7 +178,6 @@ def search(attributes, search_expression):
         of the requested `attributes`:
 
             (unique_id, attributes[0], attributes[1], ...)
-
         """
 
 
@@ -214,7 +200,6 @@ class IWebDAVReadProperties(zope.interface.common.mapping.IEnumerableMapping):
 
     value can be the special value of DeleteProperty to indicate the property
     should be removed.
-
     """
 
 
@@ -273,7 +258,6 @@ def getData(unique_id, dav_resource):
 
         The cache will be automatically invalidated if the etag of the resouce
         changes.
-
         """
 
     def sweep():
@@ -290,15 +274,13 @@ def keys(include_deleted=False):
 
         If ``include_deleted`` is True entries marked as deleted are also
         returned.
-
         """
 
     def remove(key):
         """Really remove "key" from cache.
 
         __delitem__ marks an entry as deleted so the cache behaves as if it was
         deleted. ``remove`` really removes the entry.
-
         """
 
 
@@ -314,30 +296,6 @@ class IChildNameCache(ICache):
     """A cache for child names of collections."""
 
 
-# BBB obsolete, left only until removed from all ZODBs
-class ILockInfoStorage(zope.interface.Interface):
-    """Storage for locktokens."""
-
-    def get(id):
-        """Return lockinfo for given id.
-
-        returns (token, principal, time)
-
-        """
-
-    def set(id, lockinfo):
-        """Add lockinfo to storage.
-
-        lockinfo: triple of (token, principal, time)
-
-        """
-
-    def remove(id):
-        """Remove lockinfo for id.
-
-        It is not an error to remove no existing lockinfos."""
-
-
 class IResourceInvalidatedEvent(zope.interface.Interface):
     """A resource has been invalidated."""