From 0da12be223da5ad897ebf875006a690a91f3af12 Mon Sep 17 00:00:00 2001
From: Andrew Ruthven <andrew@etc.gen.nz>
Date: Sun, 18 Dec 2022 02:10:09 +1300
Subject: [PATCH] Stop caching bound collections.

This was causing binding/1038-PROPFIND-Depth-2 to fail as the getctag that
was found didn't match what was expected. Looking at how bound collections
are handled, there is a lot of metadata that we're missing.

Preferably we'd cache or otherwise restore that metadata, but I'm going
to leave that as a future enhancement. ;)
---
 inc/DAVResource.php | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/inc/DAVResource.php b/inc/DAVResource.php
index 1f2145a6..c5d646f9 100644
--- a/inc/DAVResource.php
+++ b/inc/DAVResource.php
@@ -155,6 +155,15 @@ class DAVResource
   private $parent_container_type;
   private $sync_token;
 
+  /**
+  * @var boolean A flag to indicate if the current collection is eligible for caching.
+  *
+  * This feels like a hack, but we store some metadata about a collection in
+  * $this which doesn't get cached, reconstructing that without the info from
+  * the database is fraught with pain.
+  */
+  private $_collection_is_cacheable;
+
   /**
   * Constructor
   * @param mixed $parameters If null, an empty Resourced is created.
@@ -186,7 +195,8 @@ class DAVResource
     $this->_is_binding       = false;
     $this->_is_external      = false;
     $this->_is_addressbook   = false;
-    $this->_is_proxy_resource = false;
+    $this->_is_proxy_resource       = false;
+    $this->_collection_is_cacheable = false;
 
     if ( isset($prefetched_collection) ) {
       $this->collection = $prefetched_collection;
@@ -391,6 +401,7 @@ class DAVResource
     if ( $qry->Exec('DAVResource') && $qry->rows() == 1 && ($row = $qry->Fetch()) ) {
       $this->collection = $row;
       $this->collection->exists = true;
+
       if ( $row->is_calendar == 't' )
         $this->collection->type = 'calendar';
       else if ( $row->is_addressbook == 't' )
@@ -399,6 +410,9 @@ class DAVResource
         $this->collection->type = 'schedule-'. $matches[3]. 'box';
       else
         $this->collection->type = 'collection';
+
+      # Vanilla collection, safe to cache.
+      $this->_collection_is_cacheable = true;
     }
     else if ( preg_match( '{^( ( / ([^/]+) / ) \.(in|out)/ ) [^/]*$}x', $this->dav_name, $matches ) ) {
       // The request is for a scheduling inbox or outbox (or something inside one) and we should auto-create it
@@ -423,6 +437,9 @@ EOSQL;
         $this->collection = $row;
         $this->collection->exists = true;
         $this->collection->type = $this->collection_type;
+
+        # Vanilla collection, safe to cache.
+        $this->_collection_is_cacheable = true;
       }
     }
     else if ( preg_match( '#^(/([^/]+)/calendar-proxy-(read|write))/?[^/]*$#', $this->dav_name, $matches ) ) {
@@ -434,6 +451,9 @@ EOSQL;
       $this->collection->dav_displayname = sprintf( '%s proxy %s', $matches[2], $matches[3] );
       $this->collection->exists = true;
       $this->collection->parent_container = '/' . $matches[2] . '/';
+
+      # Proxy collection, fetch from cache can handle the _is_proxy_resource and proxy_type metadata.
+      $this->_collection_is_cacheable = true;
     }
     else if ( preg_match( '#^(/[^/]+)/?$#', $this->dav_name, $matches)
            || preg_match( '#^((/principals/[^/]+/)[^/]+)/?$#', $this->dav_name, $matches) ) {
@@ -441,6 +461,9 @@ EOSQL;
       $this->FetchPrincipal();
       $this->collection->is_principal = true;
       $this->collection->type = 'principal';
+
+      # We don't cache principals as a collection.
+      $this->_collection_is_cacheable = false;
    }
     else if ( $this->dav_name == '/' ) {
       $this->collection->dav_name = '/';
@@ -449,6 +472,9 @@ EOSQL;
       $this->collection->displayname = $c->system_name;
       $this->collection->default_privileges = (1 | 16 | 32);
       $this->collection->parent_container = '/';
+
+      # Vanilla collection, safe to cache.
+      $this->_collection_is_cacheable = true;
     }
     else {
       $sql = <<<EOSQL
@@ -502,11 +528,18 @@ EOSQL;
           if ( !isset($this->tickets) ) $this->tickets = array();
           $this->tickets[] = new DAVTicket($row->access_ticket_id);
         }
+
+
+        # A lot of metadata is stored outside of the Collection, and the
+        # Collection is what we cache. Caching this type isn't safe as we
+        # can't restore all the metadata.
+        $this->_collection_is_cacheable = false;
       }
       else {
         dbg_error_log( 'DAVResource', 'No collection for path "%s".', $this->dav_name );
         $this->collection->exists = false;
         $this->collection->dav_name = preg_replace('{/[^/]*$}', '/', $this->dav_name);
+        $this->_collection_is_cacheable = false;
       }
     }
 
@@ -538,7 +571,7 @@ EOSQL;
     $this->collection = $cache->get( $cache_ns, $cache_key );
     if ( $this->collection === false ) {
       $this->ReadCollectionFromDatabase();
-      if ( $this->collection->type != 'principal' ) {
+      if ( $this->collection->type != 'principal' && $this->_collection_is_cacheable ) {
         $cache_ns = 'collection-'.$this->collection->dav_name;
         @dbg_error_log( 'Cache', ':FetchCollection: Setting cache ns "%s" key "%s". Type: %s', $cache_ns, $cache_key, $this->collection->type );
         $cache->set( $cache_ns, $cache_key, $this->collection );