[Tickets #6060] Incorrect return type from cache miss

[bugs at horde.org]

DO NOT REPLY TO THIS MESSAGE. THIS EMAIL ADDRESS IS NOT MONITORED.

Ticket URL: http://dev.horde.org/horde/whups/ticket/?id=6060
-----------------------------------------------------------------------
 Ticket             | 6060
 Created By         | Ben Klang <ben at alkaloid.net>
 Summary            | Incorrect return type from cache miss
 Queue              | Horde Framework Packages
 Version            | FRAMEWORK_3
 Type               | Bug
 State              | Unconfirmed
 Priority           | 1. Low
 Milestone          | 
 Patch              | 
 Owners             | 
-----------------------------------------------------------------------


Ben Klang <ben at alkaloid.net> (2008-01-01 22:43) wrote:

I have Horde FW_3 from CVS as of about 2 hours ago.  My installation is
configured to use the MySQL cache backend.  While calling $perms->exists()
I see that a cache miss results in an erroneous $perms result.  Fortunately
it is a "fail-closed" so no extra permissions are granted, but the
unsuspecting user will not see the application to which he has been
granted.

>From framework/Perms/Perms/datatree.php line 139:
        $perm = $this->_cache->get('perm_' . $name,
$GLOBALS['conf']['cache']['default_lifetime']);
        if ($perm === false) {
$this->_cache->set('perm_' . $name, serialize($perm),
$GLOBALS['conf']['cache']['default_lifetime']);
            $permsCache[$name] = $perm;
        }

This code expects a cache miss to return a false.  Inspection of the code
seems to take some care to ensure this happens (for instance on a DB
error).  However the return type for DB::common::getOne() is NULL if the
row does not exist.  This causes entries which have not yet been cached to
return a type of NULL.  From line 150 of framework/Cache/Cache/sql.php:

        $result = $this->_db->getOne($query, $values);
        if (is_a($result, 'PEAR_Error')) {
            Horde::logMessage($result, __FILE__, __LINE__, PEAR_LOG_ERR);
            return false;
        } else {
            if ($this->_mc) {
                $this->_mc->set($key, $result);
            }
            return $result;
        }

I *think* the correct action here would be to check for a return type of
NULL from getOne() and then return false, indicating a cache miss. 
However, I'm not very coherent on the bigger picture for the Cache system
yet, so I'll leave it for discussion here.  Did I miss something?

Practical example:  In my case I have a module called "site" which has
granted "show/read" perms to Guests.  When a guest goes into that app they
are prompted with a login screen.  Inserting debug statements into the code
shows that while I have indeed defined a "site" permission, it is never
checked directly because the Perms/datatree code receives a NULL from the
Cache driver.