current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite
1

Is there a better way to accomplish the following?

/**
 * Performs an access check given a user.
 *
 * @param Cas_Acl_Sid $user The user or SID being checked.
 * @param Cas_Acl_Privilege $privilege The privilege to check.
 * @return int|null 1 if user access allowed, 2 if group access allowed, false if access is denied, null if access cannot be determined.
 */
public function accessCheck(Cas_Acl_Sid $user, Cas_Acl_Privilege $privilege)
{
    $db = Zend_Db_Table_Abstract::getDefaultAdapter();
    $usersQuery = $db->select()->from('AccessControlEntries', array('Allowed', new Zend_Db_Expr('1 AS Type')))
        ->where('Acl = ?', $this->_id)
        ->where('Sid = ?', $user->GetGuid())
        ->where('Privilege = ?', $privilege->GetId());

    $groupsQuery = $db->select()->from('AccessControlEntries', array('Allowed', new Zend_Db_Expr('2 AS Type')))
        ->join('GroupMembers', $db->quoteIdentifier(array('GroupMembers', 'Group')) . ' = ' .
                               $db->quoteIdentifier(array('AccessControlEntries', 'Sid')), array())
        ->where('Acl = ?', $this->_id)
        ->where($db->quoteIdentifier(array('GroupMembers', 'User')) . ' = ?', $user->GetGuid())
        ->where('Privilege = ?', $privilege->GetId());

    $query = $db->select()
        ->union(array($usersQuery, $groupsQuery), Zend_Db_Select::SQL_UNION_ALL)
        ->order('Type')
        ->order('Allowed')
        ->limit(1);

    $dbResult = $db->fetchAll($query);

    if (!count($dbResult))
    {
        return null;
    }
    else {
        if ($dbResult[0]['Allowed'])
        {
            return (int)$dbResult[0]['Type'];
        }
        else
        {
            return false;
        }
    }
}
share|improve this question
3  
could you supply a basic schema of the tables involved? –  xzyfer Feb 11 '11 at 6:43
1  
Any reason your method starts with an uppercase letter? This is against Zend coding standards. I always follow the convention of the framework I'm using, even if I don't like it. –  Cobby May 15 '12 at 23:34
    
@Cobby: I was not aware there was a convention on this. (Doesn't matter much now, as I've thrown Zend by the wayside a long time ago now, thank <insert deity>) –  Billy ONeal May 16 '12 at 17:25
    
I'm a big fan of Zend Framework, but Zend_Db wasn't a very good library. That being said, it was still a good move changing from Zend Framework; given that PHP v5.3+ frameworks are wayy better. –  Cobby May 17 '12 at 2:11
    
@Cobby: Well, really, what I did was dump PHP itself :) –  Billy ONeal May 17 '12 at 2:12

1 Answer 1

up vote 2 down vote

Stored procedure

The query above is enough big to move it from the application layer to the database in the form of a stored procedure. It will be clearer and faster, the only disadvantage is that an SP has a hard dependency on the database type (MySQL, MSSQL, other).

Return value(s)

You are returning 3 types in your method: null, integer and boolean. Keep your logic clean, and return always one type if you can (yes, PHP has it's type juggling "issue" but still, clean code talks).

public function accessCheck(Cas_Acl_Sid $user, Cas_Acl_Privilege $privilege)
{
    //execute the query in a stored procedure

    $dbResult = // stored procedure result

    if (empty($dbResult) || !$dbResult[0]['Allowed'])
    {
        return 0;
    }

    return (int)$dbResult[0]['Type'];
}
share|improve this answer
    
If you can accept a hard dependency on a single database type, there's little reason to use Zend_Db at all. As for faster, there is little evidence of that. As for returning different types; the value 0 is a valid primary key value, so your "return 0" example above is not correct. –  Billy ONeal Feb 9 '13 at 23:42
    
In most cases the primary key seed is 1 so 0 is invalid. Second if you are dealing with an ACL problem, then the valid values should be the nth power of second: 1, 2, 4, 8 and so on plus the 0. Why? You can apply a logical and to get who can read or write something ( if ($result & WRITE) { /* can write */ } ). If the result is 0 then the if statement will be false. –  Peter Kiss Feb 10 '13 at 7:04
    
Only if you can represent all your permissions in a bit field. –  Billy ONeal Feb 10 '13 at 7:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.