Merge pull request #603 from willrossi/trac7296

FIX: ensure permissions_for_member() accounts for denied permissions

security/Permission.php

@@ -230,21 +230,22 @@ class Permission extends DataObject implements TemplateGlobalProvider {
 
 	/**
 	 * Get all the 'any' permission codes available to the given member.
-	 * @return array();
+	 *
+	 * @return array
 	 */
 	public static function permissions_for_member($memberID) {
 		$groupList = self::groupList($memberID);
+
 		if($groupList) {
 			$groupCSV = implode(", ", $groupList);
 
-			// Raw SQL for efficiency
+			$allowed = array_unique(DB::query("
-			return array_unique(DB::query("
 				SELECT \"Code\"
 				FROM \"Permission\"
 				WHERE \"Type\" = " . self::GRANT_PERMISSION . " AND \"GroupID\" IN ($groupCSV)
-				
+
 				UNION
-				
+
 				SELECT \"Code\"
 				FROM \"PermissionRoleCode\" PRC
 				INNER JOIN \"PermissionRole\" PR ON PRC.\"RoleID\" = PR.\"ID\"
@@ -252,9 +253,16 @@ class Permission extends DataObject implements TemplateGlobalProvider {
 				WHERE \"GroupID\" IN ($groupCSV)
 			")->column());
 
-		} else {
+			$denied = array_unique(DB::query("
-			return array();
+				SELECT \"Code\"
+				FROM \"Permission\"
+				WHERE \"Type\" = " . self::DENY_PERMISSION . " AND \"GroupID\" IN ($groupCSV)
+			")->column());                        
+			
+			return array_diff($allowed, $denied);         
 		}
+		
+		return array();
 	}
 
 

tests/security/PermissionTest.php

@@ -1,6 +1,11 @@
 <?php
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class PermissionTest extends SapphireTest {
+
 	static $fixture_file = 'PermissionTest.yml';
 	
 	function testGetCodesGrouped() {
@@ -33,6 +38,23 @@ class PermissionTest extends SapphireTest {
 		$this->assertTrue(Permission::checkMember($member, "EDIT_PERMISSIONS"));
 		$this->assertFalse(Permission::checkMember($member, "SITETREE_VIEW_ALL"));
 	}
+
+	function testPermissionsForMember() {
+		$member = $this->objFromFixture('Member', 'access');
+		$permissions = Permission::permissions_for_member($member->ID);
+		$this->assertEquals(4, count($permissions));
+		$this->assertTrue(in_array('CMS_ACCESS_MyAdmin', $permissions));
+		$this->assertTrue(in_array('CMS_ACCESS_AssetAdmin', $permissions));
+		$this->assertTrue(in_array('CMS_ACCESS_SecurityAdmin', $permissions));
+		$this->assertTrue(in_array('EDIT_PERMISSIONS', $permissions));
+
+		$group = $this->objFromFixture("Group", "access");
+
+		Permission::deny($group->ID, "CMS_ACCESS_MyAdmin");
+		$permissions = Permission::permissions_for_member($member->ID);
+		$this->assertEquals(3, count($permissions));
+		$this->assertFalse(in_array('CMS_ACCESS_MyAdmin', $permissions));
+	}
 	
 	function testRolesAndPermissionsFromParentGroupsAreInherited() {
 		$member = $this->objFromFixture('Member', 'globalauthor');
@@ -76,5 +98,5 @@ class PermissionTest extends SapphireTest {
 		
 		Permission::remove_from_hidden_permissions('CMS_ACCESS_LeftAndMain');
 		$this->assertContains('CMS_ACCESS_LeftAndMain', $permissionCheckboxSet->Field());
-	}	
+	}
 }

tests/security/PermissionTest.yml

@@ -3,7 +3,7 @@ PermissionRole:
       Title: Author
    access:
       Title: Access Administrator
-   
+
 PermissionRoleCode:
    author1:
       Role: =>PermissionRole.author
@@ -28,7 +28,7 @@ Member:
    globalauthor:
       FirstName: Test
       Surname: Global Author
-      
+
 Group:
    author:
       Title: Authors