code cleanup, added some tests

lib/Cake/Controller/Component/Acl/PhpAcl.php

@@ -148,12 +148,12 @@ class PhpAcl extends Object implements AclInterface {
  * @param string $aco_action Action
  * @return boolean true if access is granted, false otherwise
  */
-	public function check($aro, $aco, $aco_action = null) {
+	public function check($aro, $aco, $aco_action = "*") {
 		$allow = $this->options['policy'];
 		$prioritizedAros = $this->Aro->roles($aro);
 
-		if ($aco_action) {
+		if ($aco_action && $aco_action != "*") {
-			$aco .= (strpos($aco, '.') ? '.' : '/') . $aco_action;
+			$aco .= '/' . $aco_action;
 		}
 
 		$path = $this->Aco->path($aco);	
@@ -202,7 +202,7 @@ class PhpAco {
 
 	public function __construct(array $rules = array()) {
 		foreach (array('allow', 'deny') as $type) {
-			if(empty($rules[$type])) {
+			if (empty($rules[$type])) {
 				$rules[$type] = array();
 			}
 		}
@@ -230,7 +230,7 @@ class PhpAco {
 			}
 
 			foreach ($root as $node => $elements) {
-				$pattern = '#^'.str_replace(array_keys(self::$modifiers), array_values(self::$modifiers), $node).'$#';
+				$pattern = '/^'.str_replace(array_keys(self::$modifiers), array_values(self::$modifiers), $node).'$/';
 
 				if ($node == $aco[$level] || preg_match($pattern, $aco[$level])) {
 					// merge allow/denies with $path of current level
@@ -299,7 +299,11 @@ class PhpAco {
 			return array_map('strtolower', $aco);
 		}
 
-		return array_map('trim', explode('/', ltrim(strtolower($aco), '/')));
+		// strip multiple occurences of '/'
+		$aco = preg_replace('#/+#', '/', $aco);
+		// make case insensitive
+		$aco = ltrim(strtolower($aco), '/');
+		return array_map('trim', explode('/', $aco));
 	}
 
 /**
@@ -381,7 +385,10 @@ class PhpAro {
 	protected $tree = array();
 
 	public function __construct(array $aro = array(), array $map = array(), array $aliases = array()) {
-		!empty($map) && $this->map = $map;
+		if (!empty($map)) {
+			$this->map = $map;
+		}
+
 		$this->aliases = $aliases;
 		$this->build($aro);
 	}
@@ -415,7 +422,7 @@ class PhpAro {
 
 		// everybody inherits from the default role
 		if ($aro != self::DEFAULT_ROLE) {
-			$aros[]= array(self::DEFAULT_ROLE);
+			$aros[] = array(self::DEFAULT_ROLE);
 		}
 		return array_reverse($aros);
 	}
@@ -430,7 +437,7 @@ class PhpAro {
  */
 	public function resolve($aro) {
 		foreach ($this->map as $aroGroup => $map) {
-			list ($model, $field) = explode('/', $map);
+			list ($model, $field) = explode('/', $map, 2);
 			$mapped = '';
 
 			if (is_array($aro)) {
@@ -447,7 +454,7 @@ class PhpAro {
 				if (strpos($aro, '/') === false) {
 					$mapped = $aroGroup . '/' . $aro;
 				} else {
-					list($aroModel, $aroValue) =  explode('/', $aro);
+					list($aroModel, $aroValue) =  explode('/', $aro, 2);
 
 					$aroModel = Inflector::camelize($aroModel);
 

lib/Cake/Test/Case/Controller/Component/Acl/PhpAclTest.php

@@ -54,12 +54,14 @@ class PhpAclTest extends CakeTestCase {
 		$this->assertEquals(array('User/hardy'), $roles[4]);
 	}
 
+
 	public function testAddRole() {
 		$this->assertEquals(array(array(PhpAro::DEFAULT_ROLE)), $this->Acl->Aro->roles('foobar'));
 		$this->Acl->Aro->addRole(array('User/foobar' => 'Role/accounting'));
 		$this->assertEquals(array(array('Role/default'), array('Role/accounting'), array('User/foobar')), $this->Acl->Aro->roles('foobar'));
 	}
 
+
 	public function testAroResolve() {
 		$map = $this->Acl->Aro->map;
 		$this->Acl->Aro->map = array(
@@ -82,7 +84,9 @@ class PhpAclTest extends CakeTestCase {
 		$this->assertEquals(PhpAro::DEFAULT_ROLE, $this->Acl->Aro->resolve(array('FooModel' => array('role' => 'hardy'))));
 	}
 
-
+/**
+ * test correct resolution of defined aliases
+ */
 	public function testAroAliases() {
 		$this->Acl->Aro->map = array(
 			'User' => 'User/username',
@@ -130,8 +134,9 @@ class PhpAclTest extends CakeTestCase {
 		$this->assertTrue($this->Acl->check($user, '/controllers/invoices/send'));
 	}
 
+
 /**
- * testPhpCheck method
+ * test check method
  *
  * @return void
  */
@@ -182,6 +187,9 @@ class PhpAclTest extends CakeTestCase {
 	}
 
 
+/**
+ * lhs of defined rules are case insensitive
+ */
 	public function testCheckIsCaseInsensitive() {
 		$this->assertTrue($this->Acl->check('hardy', 'controllers/forms/new'));
 		$this->assertTrue($this->Acl->check('Role/data_acquirer', 'controllers/forms/new'));
@@ -190,6 +198,9 @@ class PhpAclTest extends CakeTestCase {
 	}
 
 
+/**
+ * allow should work in-memory
+ */
 	public function testAllow() {
 		$this->assertFalse($this->Acl->check('jeff', 'foo/bar'));
 
@@ -208,6 +219,9 @@ class PhpAclTest extends CakeTestCase {
 	}
 
 
+/**
+ * deny should work in-memory
+ */
 	public function testDeny() {
 		$this->assertTrue($this->Acl->check('stan', 'controllers/baz/manager_foo'));
 
@@ -220,6 +234,9 @@ class PhpAclTest extends CakeTestCase {
 	}
 
 
+/**
+ * test that a deny rule wins over an equally specific allow rule
+ */
 	public function testDenyRuleIsStrongerThanAllowRule() {	
 		$this->assertFalse($this->Acl->check('peter', 'baz/bam'));
 		$this->Acl->allow('peter', 'baz/bam');
@@ -241,7 +258,10 @@ class PhpAclTest extends CakeTestCase {
 		$this->assertFalse($this->Acl->check('stan', 'controllers/reports/delete'));
 	}
 
-	
+
+/**
+ * test that an invalid configuration throws exception
+ */
 	public function testInvalidConfigWithAroMissing() {
 		$this->setExpectedException(
 			'AclException',
@@ -265,13 +285,28 @@ class PhpAclTest extends CakeTestCase {
 		$this->PhpAcl->build($config);
 	}
 
+/**
+ * test resolving of ACOs
+ */
 	public function testAcoResolve() {
 		$this->assertEquals(array('foo', 'bar'), $this->Acl->Aco->resolve('foo/bar'));
 		$this->assertEquals(array('foo', 'bar'), $this->Acl->Aco->resolve('foo/bar'));
 		$this->assertEquals(array('foo', 'bar', 'baz'), $this->Acl->Aco->resolve('foo/bar/baz'));
 		$this->assertEquals(array('foo', '*-bar', '?-baz'), $this->Acl->Aco->resolve('foo/*-bar/?-baz'));
+		
+		$this->assertEquals(array('foo', 'bar', '[a-f0-9]{24}', '*_bla', 'bla'), $this->Acl->Aco->resolve('foo/bar/[a-f0-9]{24}/*_bla/bla'));
+
+		// multiple slashes will be squashed to a single, then exploded
+		$this->assertEquals(array('foo', 'bar'), $this->Acl->Aco->resolve('foo//bar'));
+		$this->assertEquals(array('foo', 'bar'), $this->Acl->Aco->resolve('//foo//bar'));
+		$this->assertEquals(array('foo', 'bar'), $this->Acl->Aco->resolve('/foo//bar'));
+		$this->assertEquals(array('foo', 'bar'), $this->Acl->Aco->resolve('/foo // bar'));
+		$this->assertEquals(array(''), $this->Acl->Aco->resolve('/////'));
 	}
 
+/**
+ * test that declaring cyclic dependencies should give an error when building the tree
+ */
 	public function testAroDeclarationContainsCycles() {
 		$config = array(
 			'roles' => array(