[permissionmanager] Slightly more efficient queries, wildcard support, debug mode, comments

2 files changed, 142 insertions, 38 deletions

diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php
index 51aaa6fb..6d83ef92 100644
--- a/modules-available/permissionmanager/inc/permissionutil.inc.php
+++ b/modules-available/permissionmanager/inc/permissionutil.inc.php
@@ -2,6 +2,44 @@
 
 class PermissionUtil
 {
+
+	/**
+	 * Generate all possible variants to match against, eg. $permissionid = a.b.c then we get:
+	 * [ *, a.*, a.b.*, a.b.c ]
+	 * In case $permissionid ends with an asterisk, also set $wildcard and $wclen, e.g.
+	 * $permissionid = a.b.* --> $wildcard = a.b. and $wclen = 4
+	 *
+	 * @param $permission string|string[] permission to mangle
+	 * @param string[] $compare all the generated variants
+	 * @param string|false $wildcard if $permission is a wildcard string this returns the matching variant
+	 * @param int|false $wclen if $permission is a wildcard string, this is the length of the matching variant
+	 */
+	private static function makeComparisonVariants($permission, &$compare, &$wildcard, &$wclen)
+	{
+		if (!is_array($permission)) {
+			$permission = explode('.', $permission);
+		}
+		$partCount = count($permission);
+		$compare = [];
+		for ($i = 0; $i < $partCount; ++$i) {
+			$compare[] = $permission[0];
+		}
+		for ($i = 1; $i < $partCount; ++$i) {
+			$compare[$i-1] .= '.*';
+			for ($j = $i; $j < $partCount; ++$j) {
+				$compare[$j] .= '.' . $permission[$i];
+			}
+		}
+		$compare[] = '*';
+
+		if ($permission[$partCount-1] === '*') {
+			$wildcard = substr($compare[$partCount-1], 0, -1);
+			$wclen = strlen($wildcard);
+		} else {
+			$wclen = $wildcard = false;
+		}
+	}
+
 	/**
 	 * Check if the user has the given permission (for the given location).
 	 *
@@ -11,25 +49,37 @@ class PermissionUtil
 	 * @return bool true if user has permission, false if not
 	 */
 	public static function userHasPermission($userid, $permissionid, $locationid) {
-		$locations = array();
-		if (!is_null($locationid)) {
+		self::validatePermission($permissionid);
+		$parts = explode('.', $permissionid);
+		// Limit query to first part of permissionid, which is always the module id
+		$prefix = $parts[0] . '.%';
+		if (is_null($locationid)) {
+			$res = Database::simpleQuery("SELECT permissionid FROM role_x_permission
+					INNER JOIN user_x_role USING (roleid)
+					WHERE user_x_role.userid = :userid AND (permissionid LIKE :prefix OR permissionid LIKE '*')",
+				compact('userid', 'prefix'));
+		} else {
 			$locations = Location::getLocationRootChain($locationid);
-			if (count($locations) == 0) return false;
-			else $locations[] = 0;
+			if (count($locations) == 0)
+				return false;
+			$res = Database::simpleQuery("SELECT permissionid FROM role_x_permission
+					INNER JOIN user_x_role USING (roleid)
+					INNER JOIN role_x_location USING (roleid)
+					WHERE user_x_role.userid = :userid AND (permissionid LIKE :prefix OR permissionid LIKE '*')
+						AND (locationid IN (:locations) OR locationid IS NULL)",
+				compact('userid', 'prefix', 'locations'));
 		}
+		// Quick bailout - no results
+		if ($res->rowCount() === 0)
+			return false;
 
-		$res = Database::simpleQuery("SELECT permissionid, locationid FROM user_x_role
-												INNER JOIN role_x_permission ON user_x_role.roleid = role_x_permission.roleid
-												LEFT JOIN (SELECT roleid, COALESCE(locationid, 0) AS locationid FROM role_x_location) t1
-													ON role_x_permission.roleid = t1.roleid
-												WHERE user_x_role.userid = :userid", array("userid" => $userid));
-
+		// Compare to database result
+		self::makeComparisonVariants($parts, $compare, $wildcard, $wclen);
 		while ($row = $res->fetch(PDO::FETCH_ASSOC)) {
-			$userPermission = rtrim($row["permissionid"], ".*").".";
-			if ((is_null($locationid) || (!is_null($row["locationid"]) && in_array($row["locationid"], $locations))) &&
-				(substr($permissionid.".", 0, strlen($userPermission)) === $userPermission || $userPermission === ".")) {
+			if (in_array($row['permissionid'], $compare, true))
+				return true;
+			if ($wildcard !== false && strncmp($row['permissionid'], $wildcard, $wclen) === 0)
 				return true;
-			}
 		}
 		return false;
 	}
@@ -42,25 +92,32 @@ class PermissionUtil
 	 * @return array array of locationids where the user has the given permission
 	 */
 	public static function getAllowedLocations($userid, $permissionid) {
+		self::validatePermission($permissionid);
+		$parts = explode('.', $permissionid);
+		// Limit query to first part of permissionid, which is always the module id
+		$prefix = $parts[0] . '.%';
+		$res = Database::simpleQuery("SELECT permissionid, locationid FROM role_x_permission
+					INNER JOIN user_x_role USING (roleid)
+					INNER JOIN role_x_location USING (roleid)
+					WHERE user_x_role.userid = :userid AND (permissionid LIKE :prefix OR permissionid LIKE '*')",
+			compact('userid', 'prefix'));
 
-		$res = Database::simpleQuery("SELECT permissionid, COALESCE(locationid, 0) AS locationid FROM user_x_role
-												INNER JOIN role_x_permission ON user_x_role.roleid = role_x_permission.roleid
-												INNER JOIN role_x_location ON role_x_permission.roleid = role_x_location.roleid
-												WHERE user_x_role.userid = :userid", array("userid" => $userid));
-
+		// Gather locationid from relevant rows
+		self::makeComparisonVariants($parts, $compare, $wildcard, $wclen);
 		$allowedLocations = array();
 		while ($row = $res->fetch(PDO::FETCH_ASSOC)) {
-			$userPermission = rtrim($row["permissionid"], ".*").".";
-			if (substr($permissionid.".", 0, strlen($userPermission)) === $userPermission || $userPermission === ".") {
-				$allowedLocations[$row["locationid"]] = 1;
+			if (in_array($row['permissionid'], $compare, true)
+					|| ($wildcard !== false && strncmp($row['permissionid'], $wildcard, $wclen) === 0)) {
+				$allowedLocations[(int)$row['locationid']] = true;
 			}
 		}
-		$allowedLocations = array_keys($allowedLocations);
 		$locations = Location::getTree();
-		if (in_array("0", $allowedLocations)) {
-			$allowedLocations = array_map("intval", Location::extractIds($locations));
-			$allowedLocations[] = 0;
+		if (isset($allowedLocations[0])) {
+			// Trivial case - have permission for all locations, so populate list with all valid locationds
+			$allowedLocations = Location::extractIds($locations);
+			$allowedLocations[] = 0; // .. plus 0 to show that we have global perms
 		} else {
+			// We have a specific list of locationds - add any sublocations to list
 			$allowedLocations = self::getSublocations($locations, $allowedLocations);
 		}
 		return $allowedLocations;
@@ -70,17 +127,17 @@ class PermissionUtil
 	 * Extend an array of locations by adding all sublocations.
 	 *
 	 * @param array $tree tree of all locations (structured like Location::getTree())
-	 * @param array $locations the array of locationids to extend
+	 * @param array $allowedLocations the array of locationids to extend
 	 * @return array extended array of locationids
 	 */
-	public static function getSublocations($tree, $locations) {
-		$result = array_flip($locations);
+	public static function getSublocations($tree, $allowedLocations) {
+		$result = $allowedLocations;
 		foreach ($tree as $location) {
 			if (array_key_exists("children", $location)) {
-				if (in_array($location["locationid"], $locations)) {
+				if (isset($allowedLocations[$location["locationid"]])) {
 					$result += array_flip(Location::extractIds($location["children"]));
 				} else {
-					$result += array_flip(self::getSublocations($location["children"], $locations));
+					$result += array_flip(self::getSublocations($location["children"], $allowedLocations));
 				}
 			}
 		}
@@ -88,6 +145,35 @@ class PermissionUtil
 	}
 
 	/**
+	 * If in debug mode, validate that the checked permission is actually defined
+	 * in the according permissions.json and complain if that's not the case.
+	 * This is supposed to catch misspelled permission checks.
+	 *
+	 * @param string $permissionId permission to check
+	 */
+	private static function validatePermission($permissionId)
+	{
+		if (!CONFIG_DEBUG || $permissionId === '*')
+			return;
+		$split = explode('.', $permissionId, 2);
+		if (count($split) !== 2) {
+			trigger_error('[skip:3]Cannot check malformed permission "' . $permissionId . '"', E_USER_WARNING);
+			return;
+		}
+		$data = json_decode(file_get_contents('modules/' . $split[0] . '/permissions/permissions.json'), true);
+		if (substr($split[1], -2) === '.*') {
+			$len = strlen($split[1]) - 1;
+			foreach ($data as $perm => $v) {
+				if (strncmp($split[1], $perm, $len) === 0)
+					return;
+			}
+			trigger_error('[skip:3]Permission "' . $permissionId . '" does not match anything defined for module', E_USER_WARNING);
+		} elseif (!is_array($data) || !array_key_exists($split[1], $data)) {
+			trigger_error('[skip:3]Permission "' . $permissionId . '" not defined for module', E_USER_WARNING);
+		}
+	}
+
+	/**
 	 * Get all permissions of all active modules that have permissions in their permissions/permissions.json file.
 	 *
 	 * @return array permission tree as a multidimensional array
diff --git a/modules-available/permissionmanager/page.inc.php b/modules-available/permissionmanager/page.inc.php
index 4b961632..89c72842 100644
--- a/modules-available/permissionmanager/page.inc.php
+++ b/modules-available/permissionmanager/page.inc.php
@@ -103,14 +103,32 @@ class Page_PermissionManager extends Page
 	{
 		$res = "";
 		$toplevel = $permString == "";
-		if ($toplevel && in_array("*", $selectedPermissions)) $selectAll = true;
+		if ($toplevel && in_array("*", $selectedPermissions)) {
+			$selectAll = true;
+		}
 		foreach ($permissions as $k => $v) {
-			$leaf = isset($v['isLeaf']) && $v['isLeaf'];
-			$nextPermString = $permString ? $permString.".".$k : $k;
-			$id = $leaf ? $nextPermString : $nextPermString.".*";
-			$selected = $selectAll || in_array($id, $selectedPermissions);
-			$data = array("id" =>  $id,
-				"name" => $toplevel ? Module::get($k)->getDisplayName() : $k,
+			$selected = $selectAll;
+			$nextPermString = $permString ? $permString . "." . $k : $k;
+			if ($toplevel) {
+				$displayName = Module::get($k)->getDisplayName();
+			} else {
+				$displayName = $k;
+			}
+			do {
+				$leaf = isset($v['isLeaf']) && $v['isLeaf'];
+				$id = $leaf ? $nextPermString : $nextPermString . ".*";
+				$selected = $selected || in_array($id, $selectedPermissions);
+				if ($leaf || count($v) !== 1)
+					break;
+				reset($v);
+				$k = key($v);
+				$v = $v[$k];
+				$nextPermString .= '.' . $k;
+				$displayName .= '.' . $k;
+			} while (true);
+			$data = array(
+				"id" => $id,
+				"name" => $displayName,
 				"toplevel" => $toplevel,
 				"checkboxname" => "permissions",
 				"selected" => $selected,