Skip to content

Commit

Permalink
Fix incorrectly inheriting permissions.
Browse files Browse the repository at this point in the history
When child inherits from a deny parent the '*' permission should reflect
permissions on all nodes not just the leaf node. Previously once a node
with all permissions set to inherit was found, the check would pass.
Instead it should cascade to the parent nodes and look for explicit
allow/deny.

Refs #8450
  • Loading branch information
markstory committed Mar 12, 2016
1 parent 18b0334 commit fef3090
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 10 deletions.
23 changes: 13 additions & 10 deletions lib/Cake/Model/Permission.php
Expand Up @@ -129,19 +129,18 @@ public function check($aro, $aco, $action = '*') {
$perms = Hash::extract($perms, '{n}.' . $this->alias);
foreach ($perms as $perm) {
if ($action === '*') {
if (empty($perm)) {
continue;
}
foreach ($permKeys as $key) {
if (!empty($perm)) {
if ($perm[$key] == -1) {
return false;
} elseif ($perm[$key] == 1 || $perm[$key] == 0) {
$inherited[$key] = $perm[$key];
}
if ($perm[$key] == -1 && !(isset($inherited[$key]) && $inherited[$key] == 1)) {
// Deny, but only if a child node didnt't explicitly allow
return false;
} elseif ($perm[$key] == 1) {
// Allow & inherit from parent nodes
$inherited[$key] = $perm[$key];
}
}

if (count($inherited) === count($permKeys)) {
return true;
}
} else {
switch ($perm['_' . $action]) {
case -1:
Expand All @@ -153,6 +152,10 @@ public function check($aro, $aco, $action = '*') {
}
}
}

if ($action === '*' && count($inherited) === count($permKeys)) {
return true;
}
}
return false;
}
Expand Down
16 changes: 16 additions & 0 deletions lib/Cake/Test/Case/Controller/Component/Acl/DbAclTest.php
Expand Up @@ -452,6 +452,12 @@ public function testInheritParentDeny() {
$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();

Expand All @@ -463,8 +469,18 @@ public function testInheritParentDeny() {
$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->assertTrue($this->Acl->check('Jane', 'bizzaro_town', 'create'), 'Should have access due explicit allow');
$this->assertFalse($this->Acl->check('Jane', 'bizzaro_town', '*'), 'Should not have access due to inherit');
}

/**
Expand Down

0 comments on commit fef3090

Please sign in to comment.