#8279 Inherited Permissions breaks inherits from denied parents in 2.8.1

[houseoftech]

This is a (multiple allowed):

• bug
• enhancement
• feature-discussion (RFC)
• CakePHP Version: 2.8.1
• Platform and Target: LAMP

What you did

Checked permission check($aro, $aco, '*') on an inherited (0) child ACO whose parent has everything denied (-1).

Expected Behavior

The check should have returned false for denied.

Actual Behavior

The check returned true.

Since 2.8.1 all logged in users have been able to access all actions. If I revert only the [Fix inherited permissions when checking the '' permission.](Fix inherited permissions when checking the '' permission.) commit, then authorization behaves as expected.

Here is an appendage to the original test case provided which also tests inheritable permissions on denied parents. The last two asserts fail in 2.8.1.

public function testInheritParentDeny() {
    $this->Acl->Aco->create(array('parent_id' => null, 'alias' => 'world'));
    $this->Acl->Aco->save();

    $this->Acl->Aco->create(array('parent_id' => $this->Acl->Aco->id, 'alias' => 'town'));
    $this->Acl->Aco->save();

    $this->Acl->Aco->create(array('parent_id' => null, 'alias' => 'bizzaro_world'));
    $this->Acl->Aco->save();

    $this->Acl->Aco->create(array('parent_id' => $this->Acl->Aco->id, 'alias' => 'bizzaro_town'));
    $this->Acl->Aco->save();

    $this->Acl->Aro->create(array('parent_id' => null, 'alias' => 'Jane'));
    $this->Acl->Aro->save();

    // Setup deny on create for parent
    $this->Acl->allow('Jane', 'world', '*');
    $this->Acl->deny('Jane', 'world', 'create');

    // Setup inherit and specify allow for create on child.
    $this->Acl->inherit('Jane', 'town', '*');
    $this->Acl->allow('Jane', 'town', 'create');

    // Setup deny on create for parent
    $this->Acl->deny('Jane', 'bizzaro_world', '*');
    $this->Acl->allow('Jane', 'bizzaro_world', 'create');

    // Setup inherit.
    $this->Acl->inherit('Jane', 'bizzaro_town', '*');

    $this->assertTrue($this->Acl->check('Jane', 'town', 'create'), 'Should have access due to override');
    $this->assertTrue($this->Acl->check('Jane', 'town', '*'), 'Should have access due to inherit');
    $this->assertFalse($this->Acl->check('Jane', 'bizzaro_town', 'create'), 'Should not have access due to inherit');
    $this->assertFalse($this->Acl->check('Jane', 'bizzaro_town', '*'), 'Should not have access due to inherit');
}

[markstory]

I'll take a look, thank you for the test case 👍

[markstory]

The third assertion

$this->assertFalse($this->Acl->check('Jane', 'bizzaro_town', 'create'), 'Should not have access due to inherit');

Shouldn't that be true? Jane was granted create on bizzaro_world, and bizzaro_town inherits the ALLOW create rule.

[houseoftech]

Yes, you are correct. That should be assertTrue.

[houseoftech]

This looks like it might work.

$allowed = $inherited = array();
for ($i = 0; $i < $count; $i++) {
    $permAlias = $this->alias;

    $perms = $this->find('all', array(
        'conditions' => array(
            "{$permAlias}.aro_id" => $aroPath[$i][$this->Aro->alias]['id'],
            "{$permAlias}.aco_id" => $acoIDs
        ),
        'order' => array($this->Aco->alias . '.lft' => 'desc'),
        'recursive' => 0
    ));

    if (empty($perms)) {
        continue;
    }
    $perms = Hash::extract($perms, '{n}.' . $this->alias);
    foreach ($perms as $perm) {
        if ($action === '*') {
            foreach ($permKeys as $key) {
                if (!empty($perm)) {
                    if ($perm[$key] == -1 && !(isset($inherited[$key]) && $inherited[$key] == 1)) {
                        return false;
                    } elseif ($perm[$key] == 1) {
                        $allowed[$key] = 1;
                    }

                    if ($perm[$key] == 1 || $perm[$key] == 0) {
                        $inherited[$key] = $perm[$key];
                    }
                }
            }

            if (count($allowed) === count($permKeys)) {
                return true;
            }
        } else {
            switch ($perm['_' . $action]) {
                case -1:
                    return false;
                case 0:
                    continue;
                case 1:
                    return true;
            }
        }
    }
}

[markstory]

Thanks for the diff @houseoftech. I took a slightly different approach with #8453. I figured that the root cause was that we were checking $inherited too early. Instead of on each permission node, it should be checked at the very end to ensure all the inherit/allow overrides are accounted for.