CDbAuthManager: checkAccessRecursive makes _many_ SQL queries per request #1533

Closed
jakob-stoeck opened this Issue Oct 4, 2012 · 11 comments

Projects

None yet

6 participants

@jakob-stoeck
Contributor

For each PHP request, this function queries multiple times the row count of the yii_auth_item table. For our mid-sized PHP application those were half of all queries.

What do you think of an authCachingDuration parameter, analogous to the db->schemaCachingDuration (here in a superclass):

class DbAuthManager extends CDbAuthManager {
    protected $authCachingDuration = 3600;

    /**
     * Caches parent
     * @see CDbAuthManager::checkAccessRecursive
     */
    protected function checkAccessRecursive($itemName,$userId,$params,$assignments) {
        // calls getAuthItem and itemChildTable, therefore two queries per call
        // EDIT: if an auth item is missing in the table, it would only be one query, so we should only use a 2 here if we are certain that all used $itemNames are existing in the item_auth table
        $this->db->cache($this->authCachingDuration, null, 2);
        return parent::checkAccessRecursive($itemName,$userId,$params,$assignments);
    }
}

With caching the DB Authentication is way less costly in production environments.

@creocoder
Contributor

You can implement your own cache method in CWebUser::checkAccess().

@jakob-stoeck
Contributor

No, I’m not referring to the caching of an individual user’s access. Those queries are user-independent and build up a tree from yii_auth_item and yii_auth_item_child (not yii_auth_assignment you are referring to). Those two tables should rarely ever change in most setups, are not user-dependent, but make up for most of the SQL queries. Therefore my caching proposal.

@creocoder
Contributor

Please check current CWebUser::checkAccess() implementation. It is place where any RBAC caching can be done. There is some kind of caching already. You can implement your own method. Implement any cache in CDbAuthManager is wrong. Also, almost all queries is user-dependent.

@jakob-stoeck
Contributor

mh I think we’re talking about two different things. In the current call tree:

1 CWebUser::checkAccess()
2   getAuthManager->checkAccess()
3     getAuthAssignments() // this is user-specific
4     checkAccessRecursive() // everything from here on is user-independent
5       getAuthItem() // one SQL query
6       $parents // one SQL query
7       foreach ($parents as $p)
8         checkAccessRecursive() // count($parents)*2 SQL queries

I don’t want to cache the user-specific auth assignments (lines 1-3, they can change and there is no big gain), but I’d like to cache the whole rest (lines 4-8, repeated for every parent, amounted to 8 queries per user per request for me, even though auth items and their parents rarely ever change).

Is this possible with CWebUser?

@creocoder
Contributor

Yes. I check implementation. You give really interesting idea! :-)

@muayyad-alsadi

I've even a better idea, since flat is better than nested, we cache the flat item list

<?php
/**
 * CachingDBAuthManager class file.
 *
 * @author Muayyad Saleh Alsadi <alsadi@gmail.com>
 */
class CachingDbAuthManager extends CDbAuthManager {
    public $cacheID='cache';
    public $prefix='auth.';
    public $expires=86400; // a day;
    protected static $_items=false;
    protected static $_flat_items=false;
    protected static $_parents=false;

    public function checkAccess($itemName,$userId,$params=array())
    {
        $assignments=$this->getAuthAssignments($userId);
        if ($this->_checkAccessSingle($itemName,$userId,$params, $assignments))
            return true;
        $items=self::getParents();
        if (isset($items[$itemName])) {
            foreach($items[$itemName] as $item) {
                if ($this->_checkAccessSingle($item,$userId,$params, $assignments))
                    return true;
            }
        }
        return false;
    }


    public function purgeCache() {
        $this->getParents(false);
    }

    public function getAuthItem($name)
    {
        $this->getParents();
        if (isset(self::$_items[$name]))
            return self::$_items[$name];
        return null;
    }


    protected function _checkAccessSingle($itemName,$userId,$params,$assignments)
    {
                if(($item=$this->getAuthItem($itemName))===null)
                        return false;
                Yii::trace('Checking permission "'.$item->getName().'"','system.web.auth.CDbAuthManager');
                if($this->executeBizRule($item->getBizRule(),$params,$item->getData()))
                {
                        if(in_array($itemName,$this->defaultRoles))
                                return true;
                        if(isset($assignments[$itemName]))
                        {
                                $assignment=$assignments[$itemName];
                                if($this->executeBizRule($assignment->getBizRule(),$params,$assignment->getData()))
                                        return true;
                        }
                }
                return false;
    }


    public function getParents($use_cache=true) {
        $cache=Yii::app()->getComponent($this->cacheID);
        if ($use_cache) {
            if (self::$_flat_items!==false && self::$_items!==false && self::$_parents!==false)
                return self::$_parents;
            else {
                $items=$cache->get($this->prefix.'items');
                $children=$cache->get($this->prefix.'children');
                $parents=$cache->get($this->prefix.'parents');
                if ($children!==false && $items!==false && $parents!=false) {
                    self::$_items=$items;
                    self::$_parents=$parents;
                    self::$_flat_items=$children;
                    return self::$_parents;
                }
            }
        }
        $rows=$this->db->createCommand()
                    ->select('name, type, description, bizrule, data, parent')
                    ->from($this->itemTable)
                    ->leftJoin($this->itemChildTable, 'name=child')
                    ->queryAll();
        $items=array();
        $children=array();
        foreach($rows as $row)
        {
            if(($data=@unserialize($row['data']))===false)
            $data=null;
            $items[$row['name']]=new CAuthItem($this,$row['name'],$row['type'],$row['description'],$row['bizrule'],$data);
            if (!isset($children[$row['parent']])) $children[$row['parent']]=array();
            $children[$row['parent']][]=$row['name'];
        }
        foreach($children as $parent=>$item) {
            self::_recursive_flaten($children, $parent);
        }
        // FIXME: use array_unique on children and parents
        self::$_parents=array();
        foreach($children as $parent=>$a) {
            foreach($a as $item) {
                if (!isset(self::$_parents[$item])) self::$_parents[$item]=array($parent);
                else self::$_parents[$item][]=$parent;
            }
        }
        $cache->set($this->prefix.'items', $items, $this->expires);
        $cache->set($this->prefix.'children', $children, $this->expires);
        $cache->set($this->prefix.'parents', self::$_parents, $this->expires);
        self::$_items=$items;
        self::$_flat_items=$children;
         return self::$_parents;
    }

    protected static function _recursive_flaten(&$children, $parent) {
        if (!isset($children[$parent])) return;
        foreach($children[$parent] as $item) {
            if (isset($children[$item])) {
                foreach($children[$item] as $i) {
                    $children[$parent][]=$i;
                    self::_recursive_flaten($children, $i);
                }
            } else {
                $children[$item]=array();
            }
        }
    }

}
@muayyad-alsadi

@jakob-stoeck I believe my implementation does exactly what you was talking about, as it cache the user independent hierarchy of AuthItems.

@Sarke
Sarke commented Nov 5, 2012

My suggested changes. It doesn't use caching, but it's more Yii code ready. By default acts the same as CDbAuthManager, but setting the options will enable the optimizations.

checkAccessRecursive() is identical to the CDbAuthManager one, except I moved the part where the parents are loaded to getAuthParents(). I also overwrote getAuthItem() and getAuthAssignments() to keep the values in memory and only use one query per table.

class EDbAuthManager extends CDbAuthManager
{
    public $singleLoadAuthItems = true;
    public $singleLoadAuthParents = true;
    public $keepAuthAssignments = false;

    protected $_items;
    protected $_parents;
    protected $_assignments = array();

    protected function checkAccessRecursive($itemName, $userId, $params, $assignments)
    {
        if (($item = $this->getAuthItem($itemName)) === null)
            return false;

        Yii::trace('Checking permission "'.$item->getName().'"','system.web.auth.CDbAuthManager');

        if (!isset($params['userId']))
            $params['userId'] = $userId;

        if ($this->executeBizRule($item->getBizRule(), $params, $item->getData()))
        {
            if (in_array($itemName, $this->defaultRoles))
                return true;

            if (isset($assignments[$itemName]))
            {
                $assignment = $assignments[$itemName];
                if ($this->executeBizRule($assignment->getBizRule(), $params, $assignment->getData()))
                    return true;
            }

            foreach ($this->getAuthParents($itemName) as $parent)
            {
                if ($this->checkAccessRecursive($parent, $userId, $params, $assignments))
                    return true;
            }
        }

        return false;
    }

    public function getAuthItem($name)
    {
        if ($this->singleLoadAuthItems)
            return parent::getAuthItem($name);

        if (!isset($this->_items))
        {
            $rows = $this->db->createCommand()
                ->select()
                ->from($this->itemTable)
                ->queryAll();

            $this->_items = array();
            foreach($rows as $row)
            {
                if (($data = @unserialize($row['data'])) === false)
                    $data = null;

                $this->_items[$row['name']] = new CAuthItem($this, $row['name'], $row['type'], $row['description'], $row['bizrule'], $data);
            }
        }

        if (isset($this->_items[$name]))
            return $this->_items[$name];
        else
            return null;
    }

    public function getAuthAssignments($userId)
    {
        if ($this->keepAuthAssignments)
        {
            if (!isset($this->_assignments[$userId]))
                $this->_assignments[$userId] = parent::getAuthAssignments($userId);

            $assignments = $this->_assignments[$userId];
        }
        else
        {
            $assignments = parent::getAuthAssignments($userId);
        }

        return $assignments;
    }

    public function getAuthParents($itemName)
    {
        if ($this->singleLoadAuthParents)
        {
            return $this->db->createCommand()
                ->select('parent')
                ->from($this->itemChildTable)
                ->where('child=:name', array(':name'=>$itemName))
                ->queryColumn();
        }

        if (!isset($this->_parents))
        {
            $relations = $this->db->createCommand()
                ->select()
                ->from($this->itemChildTable)
                ->queryAll();

            $this->_parents = array();
            foreach ($relations as $row)
                $this->_parents[$row['child']][$row['parent']] = $row['parent'];
        }

        if (isset($this->_parents[$itemName]))
            return array_values($this->_parents[$itemName]);
        else
            return array();
    }

}

Read more:
http://www.yiiframework.com/forum/index.php/topic/37216-holy-cdbauthmanager-queries-batman/page__view__findpost__p__179459

@muayyad-alsadi

my parent logic was reversed in my code, I've updated my code with a fix.

@creocoder
Contributor

Guys, may be its good idea to disscuss such things at forum? Here are a controversial changes and no specifics.

@KJLJon KJLJon added a commit to KJLJon/yii that referenced this issue Jun 19, 2013
@KJLJon KJLJon Enh #1533: CDbAuthManager: checkAccessRecursive makes _many_ SQL quer…
…ies per request

added caching to AuthItem and AuthItemChild tables
dab9053
@nineinchnick
Contributor

I agree with @Sarke that CDbAuthManager should load everything in one query, without any caching by default. Some time ago I created my own version (http://www.yiiframework.com/extension/singledbauthmanager/) that follows the same logic as CPhpAuthManager, which in turn loads a whole file at once and updates it on each change.
It passes all tests for CDbAuthManager.

I don't think that loading each item separately is in any way better than loading all of them at once. I doubt that anybody got a large number of auth items and only makes one or two checkAccess() per page in total, including recursive calls.

I would create a pull request if some of you agree with that. This is a big change and it would require a few reviews.

@samdark samdark closed this Nov 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment