From 56e6f8698c49099f0c78d77e1d99b9eec2f13df9 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Tue, 13 Feb 2018 16:29:55 +0100 Subject: [permissionmanager] Slightly more efficient queries, wildcard support, debug mode, comments --- .../permissionmanager/inc/permissionutil.inc.php | 148 ++++++++++++++++----- modules-available/permissionmanager/page.inc.php | 32 ++++- 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,23 +127,52 @@ 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)); } } } return array_keys($result); } + /** + * 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. * 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, -- cgit v1.2.3-55-g7522