From a8b0095b335780ae0bb950bc44021215d43a6b2d Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Mon, 12 Feb 2018 14:17:07 +0100 Subject: [permissionmanager] Introduce "location-aware" flag for permissions This flag tells wether the permission can be restricted to certain locations in a meaningful way. This flag has to be set in the permissions.json of the according module. For example, the permission to reboot the server cannot be limited to certain locations in a meaningful way, while the view of the client log can be filtered to only show log entries for clients in specific locations. --- modules-available/permissionmanager/inc/permissionutil.inc.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php index 5ff41046..3daf422e 100644 --- a/modules-available/permissionmanager/inc/permissionutil.inc.php +++ b/modules-available/permissionmanager/inc/permissionutil.inc.php @@ -100,9 +100,9 @@ class PermissionUtil if (!is_array($data)) continue; preg_match('#^modules/([^/]+)/#', $file, $out); - foreach( $data as $p ) { + foreach( $data as $p => $data) { $description = Dictionary::translateFileModule($out[1], "permissions", $p); - self::putInPermissionTree($out[1].".".$p, $description, $permissions); + self::putInPermissionTree($out[1].".".$p, $data['location-aware'], $description, $permissions); } } ksort($permissions); @@ -120,10 +120,11 @@ class PermissionUtil * Place a permission into the given permission tree. * * @param string $permission the permission to place in the tree + * @param bool $locationAware whether this permissions can be restricted to specific locations only * @param string $description the description of the permission * @param array $tree the permission tree to modify */ - private static function putInPermissionTree($permission, $description, &$tree) + private static function putInPermissionTree($permission, $locationAware, $description, &$tree) { $subPermissions = explode('.', $permission); foreach ($subPermissions as $subPermission) { @@ -134,6 +135,6 @@ class PermissionUtil $tree =& $tree[$subPermission]; } } - $tree = $description; + $tree = array('description' => $description, 'location-aware' => $locationAware, 'isLeaf' => true); } } \ No newline at end of file -- cgit v1.2.3-55-g7522 From ace4ece3cc294fbe5b2cdfa53557d312d0057d76 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Mon, 12 Feb 2018 18:16:24 +0100 Subject: [permissionmanager] getPermissions(): Only consider permissions for active modules --- .../permissionmanager/inc/permissionutil.inc.php | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php index 3daf422e..51aaa6fb 100644 --- a/modules-available/permissionmanager/inc/permissionutil.inc.php +++ b/modules-available/permissionmanager/inc/permissionutil.inc.php @@ -100,17 +100,26 @@ class PermissionUtil if (!is_array($data)) continue; preg_match('#^modules/([^/]+)/#', $file, $out); - foreach( $data as $p => $data) { - $description = Dictionary::translateFileModule($out[1], "permissions", $p); - self::putInPermissionTree($out[1].".".$p, $data['location-aware'], $description, $permissions); + $moduleId = $out[1]; + if (Module::get($moduleId) === false) + continue; + foreach($data as $perm => $permissionFlags) { + $description = Dictionary::translateFileModule($moduleId, "permissions", $perm); + self::putInPermissionTree($moduleId . "." . $perm, $permissionFlags['location-aware'], $description, $permissions); } } ksort($permissions); global $MENU_CAT_OVERRIDE; $sortingOrder = $MENU_CAT_OVERRIDE; - foreach ($permissions as $module => $v) $sortingOrder[Module::get($module)->getCategory()][] = $module; + foreach ($permissions as $module => $v) { + $sortingOrder[Module::get($module)->getCategory()][] = $module; + } $permissions = array_replace(array_flip(call_user_func_array('array_merge', $sortingOrder)), $permissions); - foreach ($permissions as $module => $v) if (is_int($v)) unset($permissions[$module]); + foreach ($permissions as $module => $v) { + if (is_int($v)) { + unset($permissions[$module]); + } + } return $permissions; -- cgit v1.2.3-55-g7522 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(-) (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') 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 From e39bc13cee5db3d4905498e0fe3ac89fd3ccbfe7 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Wed, 14 Feb 2018 13:54:45 +0100 Subject: [permissionmanager] Apply formatting --- modules-available/permissionmanager/api.inc.php | 7 --- .../inc/permissiondbupdate.inc.php | 37 +++++++++------- .../permissionmanager/inc/permissionutil.inc.php | 17 +++++--- modules-available/permissionmanager/page.inc.php | 51 ++++++++++++---------- 4 files changed, 60 insertions(+), 52 deletions(-) delete mode 100644 modules-available/permissionmanager/api.inc.php (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') diff --git a/modules-available/permissionmanager/api.inc.php b/modules-available/permissionmanager/api.inc.php deleted file mode 100644 index 0d84ebce..00000000 --- a/modules-available/permissionmanager/api.inc.php +++ /dev/null @@ -1,7 +0,0 @@ - 'value', - 'number' => 123, - 'list' => array(1,2,3,4,5,6,'foo') -)); diff --git a/modules-available/permissionmanager/inc/permissiondbupdate.inc.php b/modules-available/permissionmanager/inc/permissiondbupdate.inc.php index 0f37a053..f2e7a366 100644 --- a/modules-available/permissionmanager/inc/permissiondbupdate.inc.php +++ b/modules-available/permissionmanager/inc/permissiondbupdate.inc.php @@ -1,6 +1,7 @@ $users, "roles" => $roles)); } @@ -35,7 +38,8 @@ class PermissionDbUpdate { * * @param string $roleid roleid */ - public static function deleteRole($roleid) { + public static function deleteRole($roleid) + { Database::exec("DELETE FROM role WHERE roleid = :roleid", array("roleid" => $roleid)); } @@ -47,28 +51,31 @@ class PermissionDbUpdate { * @param array $permissions array of permissions * @param string|null $roleid roleid or null if the role does not exist yet */ - public static function saveRole($rolename, $locations, $permissions, $roleid = NULL) { + public static function saveRole($rolename, $locations, $permissions, $roleid = null) + { if ($roleid) { Database::exec("UPDATE role SET rolename = :rolename WHERE roleid = :roleid", - array("rolename" => $rolename, "roleid" => $roleid)); + array("rolename" => $rolename, "roleid" => $roleid)); Database::exec("DELETE FROM role_x_location - WHERE roleid = :roleid AND locationid NOT IN (:locations)", array("roleid" => $roleid, 'locations' => $locations)); + WHERE roleid = :roleid AND locationid NOT IN (:locations)", + array("roleid" => $roleid, 'locations' => $locations)); Database::exec("DELETE FROM role_x_permission - WHERE roleid = :roleid AND permissionid NOT IN (:permissions)", array("roleid" => $roleid, 'permissions' => $permissions)); + WHERE roleid = :roleid AND permissionid NOT IN (:permissions)", + array("roleid" => $roleid, 'permissions' => $permissions)); } else { Database::exec("INSERT INTO role (rolename) VALUES (:rolename)", array("rolename" => $rolename)); $roleid = Database::lastInsertId(); } - $arg = array_map(function($loc) use ($roleid) { + + $arg = array_map(function ($loc) use ($roleid) { return compact('roleid', 'loc'); }, $locations); - Database::exec("INSERT IGNORE INTO role_x_location (roleid, locationid) VALUES :arg", - ['arg' => $arg]); - $arg = array_map(function($perm) use ($roleid) { + Database::exec("INSERT IGNORE INTO role_x_location (roleid, locationid) VALUES :arg", ['arg' => $arg]); + + $arg = array_map(function ($perm) use ($roleid) { return compact('roleid', 'perm'); }, $permissions); - Database::exec("INSERT IGNORE INTO role_x_permission (roleid, permissionid) VALUES :arg", - ['arg' => $arg]); + Database::exec("INSERT IGNORE INTO role_x_permission (roleid, permissionid) VALUES :arg", ['arg' => $arg]); } } diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php index 6d83ef92..d3948ebd 100644 --- a/modules-available/permissionmanager/inc/permissionutil.inc.php +++ b/modules-available/permissionmanager/inc/permissionutil.inc.php @@ -25,15 +25,15 @@ class PermissionUtil $compare[] = $permission[0]; } for ($i = 1; $i < $partCount; ++$i) { - $compare[$i-1] .= '.*'; + $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); + if ($permission[$partCount - 1] === '*') { + $wildcard = substr($compare[$partCount - 1], 0, -1); $wclen = strlen($wildcard); } else { $wclen = $wildcard = false; @@ -48,7 +48,8 @@ class PermissionUtil * @param int|null $locationid locationid to check or null if the location should be disregarded * @return bool true if user has permission, false if not */ - public static function userHasPermission($userid, $permissionid, $locationid) { + public static function userHasPermission($userid, $permissionid, $locationid) + { self::validatePermission($permissionid); $parts = explode('.', $permissionid); // Limit query to first part of permissionid, which is always the module id @@ -91,7 +92,8 @@ class PermissionUtil * @param string $permissionid permissionid to check * @return array array of locationids where the user has the given permission */ - public static function getAllowedLocations($userid, $permissionid) { + 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 @@ -130,7 +132,8 @@ class PermissionUtil * @param array $allowedLocations the array of locationids to extend * @return array extended array of locationids */ - public static function getSublocations($tree, $allowedLocations) { + public static function getSublocations($tree, $allowedLocations) + { $result = $allowedLocations; foreach ($tree as $location) { if (array_key_exists("children", $location)) { @@ -189,7 +192,7 @@ class PermissionUtil $moduleId = $out[1]; if (Module::get($moduleId) === false) continue; - foreach($data as $perm => $permissionFlags) { + foreach ($data as $perm => $permissionFlags) { $description = Dictionary::translateFileModule($moduleId, "permissions", $perm); self::putInPermissionTree($moduleId . "." . $perm, $permissionFlags['location-aware'], $description, $permissions); } diff --git a/modules-available/permissionmanager/page.inc.php b/modules-available/permissionmanager/page.inc.php index 89c72842..1c2d56bf 100644 --- a/modules-available/permissionmanager/page.inc.php +++ b/modules-available/permissionmanager/page.inc.php @@ -95,8 +95,8 @@ class Page_PermissionManager extends Page * * @param array $permissions the permission tree * @param array $selectedPermissions permissions that should be preselected - * @param array $selectAll true if all pemrissions should be preselected, false if only those in $selectedPermissions - * @param array $permString the prefix permission string with which all permissions in the permission tree should start + * @param bool $selectAll true if all permissions should be preselected, false if only those in $selectedPermissions + * @param string $permString the prefix permission string with which all permissions in the permission tree should start * @return string generated html code */ private static function generatePermissionHTML($permissions, $selectedPermissions = array(), $selectAll = false, $permString = "") @@ -142,10 +142,10 @@ class Page_PermissionManager extends Page if ($toplevel) { $res = Render::parse("treepanel", array("id" => "*", - "name" => Dictionary::translateFile("template-tags", "lang_permissions"), - "checkboxname" => "permissions", - "selected" => $selectAll, - "HTML" => $res)); + "name" => Dictionary::translateFile("template-tags", "lang_permissions"), + "checkboxname" => "permissions", + "selected" => $selectAll, + "HTML" => $res)); } return $res; } @@ -162,25 +162,27 @@ class Page_PermissionManager extends Page private static function generateLocationHTML($locations, $selectedLocations = array(), $selectAll = false, $toplevel = true) { $res = ""; - if ($toplevel && in_array(0, $selectedLocations)) $selectAll = true; + if ($toplevel && in_array(0, $selectedLocations)) { + $selectAll = true; + } foreach ($locations as $location) { $selected = $selectAll || in_array($location["locationid"], $selectedLocations); $res .= Render::parse("treenode", - array("id" => $location["locationid"], - "name" => $location["locationname"], - "toplevel" => $toplevel, - "checkboxname" => "locations", - "selected" => $selected, - "HTML" => array_key_exists("children", $location) ? - self::generateLocationHTML($location["children"], $selectedLocations, $selected, false) : "")); + array("id" => $location["locationid"], + "name" => $location["locationname"], + "toplevel" => $toplevel, + "checkboxname" => "locations", + "selected" => $selected, + "HTML" => array_key_exists("children", $location) ? + self::generateLocationHTML($location["children"], $selectedLocations, $selected, false) : "")); } if ($toplevel) { $res = Render::parse("treepanel", array("id" => 0, - "name" => Dictionary::translateFile("template-tags", "lang_locations"), - "checkboxname" => "locations", - "selected" => $selectAll, - "HTML" => $res)); + "name" => Dictionary::translateFile("template-tags", "lang_locations"), + "checkboxname" => "locations", + "selected" => $selectAll, + "HTML" => $res)); } return $res; } @@ -193,12 +195,14 @@ class Page_PermissionManager extends Page */ private static function processLocations($locations) { - if (in_array(0, $locations)) return array(NULL); + if (in_array(0, $locations)) + return array(null); $result = array(); foreach ($locations as $location) { $rootchain = array_reverse(Location::getLocationRootChain($location)); foreach ($rootchain as $l) { - if (in_array($l, $result)) break; + if (in_array($l, $result)) + break; if (in_array($l, $locations)) { $result[] = $l; break; @@ -216,7 +220,8 @@ class Page_PermissionManager extends Page */ private static function processPermissions($permissions) { - if (in_array("*", $permissions)) return array("*"); + if (in_array("*", $permissions)) + return array("*"); $result = array(); foreach ($permissions as $permission) { $x =& $result; @@ -239,10 +244,10 @@ class Page_PermissionManager extends Page foreach ($permissions as $permission => $a) { if (is_array($a)) { if (array_key_exists("*", $a)) { - $result[] = $permission.".*"; + $result[] = $permission . ".*"; } else { foreach (self::extractPermissions($a) as $subPermission) { - $result[] = $permission.".".$subPermission; + $result[] = $permission . "." . $subPermission; } } } else { -- cgit v1.2.3-55-g7522 From 76cd19df64210d7512a53216b8b173233a923fc2 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Wed, 14 Feb 2018 14:13:40 +0100 Subject: [permissionmanager] Fix: False positive for perm validity check --- modules-available/permissionmanager/inc/permissionutil.inc.php | 2 ++ 1 file changed, 2 insertions(+) (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php index d3948ebd..f1385bc2 100644 --- a/modules-available/permissionmanager/inc/permissionutil.inc.php +++ b/modules-available/permissionmanager/inc/permissionutil.inc.php @@ -163,6 +163,8 @@ class PermissionUtil trigger_error('[skip:3]Cannot check malformed permission "' . $permissionId . '"', E_USER_WARNING); return; } + if ($split[1] === '*') + return; $data = json_decode(file_get_contents('modules/' . $split[0] . '/permissions/permissions.json'), true); if (substr($split[1], -2) === '.*') { $len = strlen($split[1]) - 1; -- cgit v1.2.3-55-g7522 From 7bde027d280e3e08758d95213559677099cd3819 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Fri, 16 Feb 2018 12:20:25 +0100 Subject: [permissionmanager] Force lowercase permissions, handle locId 0 properly --- .../permissionmanager/inc/permissiondbupdate.inc.php | 4 ++++ .../permissionmanager/inc/permissionutil.inc.php | 13 ++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') diff --git a/modules-available/permissionmanager/inc/permissiondbupdate.inc.php b/modules-available/permissionmanager/inc/permissiondbupdate.inc.php index f2e7a366..8a67bf24 100644 --- a/modules-available/permissionmanager/inc/permissiondbupdate.inc.php +++ b/modules-available/permissionmanager/inc/permissiondbupdate.inc.php @@ -53,6 +53,10 @@ class PermissionDbUpdate */ public static function saveRole($rolename, $locations, $permissions, $roleid = null) { + foreach ($permissions as &$permission) { + $permission = strtolower($permission); + } + unset($permission); if ($roleid) { Database::exec("UPDATE role SET rolename = :rolename WHERE roleid = :roleid", array("rolename" => $rolename, "roleid" => $roleid)); diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php index f1385bc2..b4d54055 100644 --- a/modules-available/permissionmanager/inc/permissionutil.inc.php +++ b/modules-available/permissionmanager/inc/permissionutil.inc.php @@ -50,6 +50,7 @@ class PermissionUtil */ public static function userHasPermission($userid, $permissionid, $locationid) { + $permissionid = strtolower($permissionid); self::validatePermission($permissionid); $parts = explode('.', $permissionid); // Limit query to first part of permissionid, which is always the module id @@ -60,9 +61,14 @@ class PermissionUtil 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; + if ($locationid === 0) { + $locations = [0]; + } else { + $locations = Location::getLocationRootChain($locationid); + if (empty($locations)) { // Non-existent location, still continue as user might have global perms + $locations = [0]; + } + } $res = Database::simpleQuery("SELECT permissionid FROM role_x_permission INNER JOIN user_x_role USING (roleid) INNER JOIN role_x_location USING (roleid) @@ -94,6 +100,7 @@ class PermissionUtil */ public static function getAllowedLocations($userid, $permissionid) { + $permissionid = strtolower($permissionid); self::validatePermission($permissionid); $parts = explode('.', $permissionid); // Limit query to first part of permissionid, which is always the module id -- cgit v1.2.3-55-g7522 From 7afe5a3ffee64ff5c1ee7692a2ac4c83d46d6a78 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Mon, 19 Feb 2018 13:36:35 +0100 Subject: [permissionmanager] Implement permissions: Permissinmanager is now protected by permissions. In order to prevent complete lockout, the user with userid == 1 will always be able to edit and assign permissions. (TODO: Communicate this somehow) --- inc/permission.inc.php | 2 +- .../inc/getpermissiondata.inc.php | 22 +++++++- .../permissionmanager/inc/permissionutil.inc.php | 34 ++++++++----- .../permissionmanager/lang/de/permissions.json | 7 +++ .../permissionmanager/lang/de/template-tags.json | 6 ++- .../permissionmanager/lang/en/permissions.json | 7 +++ .../permissionmanager/lang/en/template-tags.json | 6 ++- modules-available/permissionmanager/page.inc.php | 59 +++++++++++++++------- .../permissionmanager/permissions/permissions.json | 17 +++++++ modules-available/permissionmanager/style.css | 4 ++ .../permissionmanager/templates/header-menu.html | 6 +-- .../permissionmanager/templates/roleeditor.html | 5 +- .../permissionmanager/templates/rolestable.html | 25 ++++++--- .../permissionmanager/templates/treenode.html | 2 +- .../permissionmanager/templates/treepanel.html | 2 +- .../roomplanner/permissions/permissions.json | 10 ++++ 16 files changed, 164 insertions(+), 50 deletions(-) create mode 100644 modules-available/permissionmanager/lang/de/permissions.json create mode 100644 modules-available/permissionmanager/lang/en/permissions.json create mode 100644 modules-available/permissionmanager/permissions/permissions.json create mode 100644 modules-available/roomplanner/permissions/permissions.json (limited to 'modules-available/permissionmanager/inc/permissionutil.inc.php') diff --git a/inc/permission.inc.php b/inc/permission.inc.php index e61c7423..aaef6ba6 100644 --- a/inc/permission.inc.php +++ b/inc/permission.inc.php @@ -33,7 +33,7 @@ class Permission } $temp =& $array; foreach (explode('.', $perm) as $sub) { - if (empty($sub)) + if (empty($sub) || $sub === '*') continue; $temp =& $temp[$sub]; } diff --git a/modules-available/permissionmanager/inc/getpermissiondata.inc.php b/modules-available/permissionmanager/inc/getpermissiondata.inc.php index dd100d42..496c8224 100644 --- a/modules-available/permissionmanager/inc/getpermissiondata.inc.php +++ b/modules-available/permissionmanager/inc/getpermissiondata.inc.php @@ -3,6 +3,9 @@ class GetPermissionData { + const WITH_USER_COUNT = 1; + const WITH_LOCATION_COUNT = 2; + /** * Get data for all users. * @@ -64,11 +67,26 @@ class GetPermissionData /** * Get all roles. * + * @param int $flags Bitmask specifying additional data to fetch (WITH_* constants of this class) * @return array array roles (each with roleid and rolename) */ - public static function getRoles() + public static function getRoles($flags = 0) { - return Database::queryAll("SELECT roleid, rolename FROM role ORDER BY rolename ASC"); + $cols = $joins = ''; + if ($flags & self::WITH_USER_COUNT) { + $cols .= ', Count(DISTINCT rxu.userid) AS users'; + $joins .= ' LEFT JOIN user_x_role rxu ON (r.roleid = rxu.roleid)'; + } + if ($flags & self::WITH_LOCATION_COUNT) { + $cols .= ', Count(DISTINCT rxl.locationid) AS locations'; + $joins .= ' LEFT JOIN role_x_location rxl ON (r.roleid = rxl.roleid)'; + } + if (!empty($joins)) { + $joins .= ' GROUP BY r.roleid'; + } + return Database::queryAll("SELECT r.roleid, r.rolename $cols FROM role r + $joins + ORDER BY rolename ASC"); } /** diff --git a/modules-available/permissionmanager/inc/permissionutil.inc.php b/modules-available/permissionmanager/inc/permissionutil.inc.php index b4d54055..bc42c5a0 100644 --- a/modules-available/permissionmanager/inc/permissionutil.inc.php +++ b/modules-available/permissionmanager/inc/permissionutil.inc.php @@ -53,6 +53,9 @@ class PermissionUtil $permissionid = strtolower($permissionid); self::validatePermission($permissionid); $parts = explode('.', $permissionid); + // Special case: To prevent lockout, userid === 1 always has permissionmanager.* + if ($parts[0] === 'permissionmanager' && User::getId() === 1) + return true; // Limit query to first part of permissionid, which is always the module id $prefix = $parts[0] . '.%'; if (is_null($locationid)) { @@ -103,21 +106,26 @@ class PermissionUtil $permissionid = strtolower($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')); + // Special case: To prevent lockout, userid === 1 always has permissionmanager.* + if ($parts[0] === 'permissionmanager' && User::getId() === 1) { + $allowedLocations = [true]; + } else { + // 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')); - // Gather locationid from relevant rows - self::makeComparisonVariants($parts, $compare, $wildcard, $wclen); - $allowedLocations = array(); - while ($row = $res->fetch(PDO::FETCH_ASSOC)) { - if (in_array($row['permissionid'], $compare, true) + // Gather locationid from relevant rows + self::makeComparisonVariants($parts, $compare, $wildcard, $wclen); + $allowedLocations = array(); + while ($row = $res->fetch(PDO::FETCH_ASSOC)) { + if (in_array($row['permissionid'], $compare, true) || ($wildcard !== false && strncmp($row['permissionid'], $wildcard, $wclen) === 0)) { - $allowedLocations[(int)$row['locationid']] = true; + $allowedLocations[(int)$row['locationid']] = true; + } } } $locations = Location::getTree(); diff --git a/modules-available/permissionmanager/lang/de/permissions.json b/modules-available/permissionmanager/lang/de/permissions.json new file mode 100644 index 00000000..444c1e8c --- /dev/null +++ b/modules-available/permissionmanager/lang/de/permissions.json @@ -0,0 +1,7 @@ +{ + "locations.view": "Orte mit zugewiesenen Rollen sehen.", + "roles.edit": "Rollen bearbeiten.", + "roles.view": "Rollen sehen.", + "users.edit-roles": "Benutzern Rollen zuweisen oder entziehen.", + "users.view": "Benutzer mit zugewiesenen Rolen sehen." +} \ No newline at end of file diff --git a/modules-available/permissionmanager/lang/de/template-tags.json b/modules-available/permissionmanager/lang/de/template-tags.json index 2ec25b37..52073dee 100644 --- a/modules-available/permissionmanager/lang/de/template-tags.json +++ b/modules-available/permissionmanager/lang/de/template-tags.json @@ -1,18 +1,20 @@ { "lang_addRole": "Rollen erteilen", "lang_addRoleHeading": "Neue Rolle hinzuf\u00fcgen", - "lang_deleteCheck": "Sind Sie sich sicher, dass Sie diese Rolle l\u00f6schen wollen?", "lang_editRoleHeading": "Rolle bearbeiten", "lang_locationAwareDesc": "Berechtigungen mit diesem Symbol k\u00f6nnen auf bestimmte R\u00e4ume\/Orte beschr\u00e4nkt werden. Alle anderen Berechtigungen sind unabh\u00e4ngig von den f\u00fcr diese Rolle ausgew\u00e4hlten Orten.", "lang_locations": "R\u00e4ume", "lang_moduleName": "Rechtemanager", "lang_name": "Name", "lang_newRole": "Rolle anlegen", + "lang_numAssignedUsers": "Benutzer mit dieser Rolle", "lang_permissions": "Rechte", "lang_removeRole": "Rollen entziehen", + "lang_roleDeleteConfirm": "Sind Sie sich sicher, dass Sie diese Rolle l\u00f6schen m\u00f6chten? Benutzer, denen diese Rolle zugewiesen ist, werden die entsprechenden Berechtigungen verlieren.", "lang_roles": "Rollen", "lang_searchPlaceholder": "Nach Rollen suchen...", "lang_selected": "ausgew\u00e4hlte", "lang_selectizePlaceholder": "Nach Rollen filtern...", - "lang_users": "Nutzer" + "lang_users": "Nutzer", + "lang_view": "Anzeigen" } \ No newline at end of file diff --git a/modules-available/permissionmanager/lang/en/permissions.json b/modules-available/permissionmanager/lang/en/permissions.json new file mode 100644 index 00000000..9f7263db --- /dev/null +++ b/modules-available/permissionmanager/lang/en/permissions.json @@ -0,0 +1,7 @@ +{ + "locations.view": "See locations with assigned roles.", + "roles.edit": "Edit roles.", + "roles.view": "Show roles.", + "users.edit-roles": "Assign or remove roles from users.", + "users.view": "See users with assigned roles." +} \ No newline at end of file diff --git a/modules-available/permissionmanager/lang/en/template-tags.json b/modules-available/permissionmanager/lang/en/template-tags.json index cef23422..b7a1d77a 100644 --- a/modules-available/permissionmanager/lang/en/template-tags.json +++ b/modules-available/permissionmanager/lang/en/template-tags.json @@ -1,18 +1,20 @@ { "lang_addRole": "Grant Roles", "lang_addRoleHeading": "Add new role", - "lang_deleteCheck": "Are you sure you want to delete this role?", "lang_editRoleHeading": "Edit role", "lang_locationAwareDesc": "Permissions with this symbol can be restricted to certain locations. All other permissions are independent of the locations selected for this role.", "lang_locations": "Locations", "lang_moduleName": "Permission Manager", "lang_name": "Name", "lang_newRole": "New Role", + "lang_numAssignedUsers": "Users with this role", "lang_permissions": "Permissions", "lang_removeRole": "Revoke Roles", + "lang_roleDeleteConfirm": "Are you sure you want to delete this role? Users currently assigned to this role will lose the according permissions.", "lang_roles": "Roles", "lang_searchPlaceholder": "Search for roles...", "lang_selected": "selected", "lang_selectizePlaceholder": "Filter for roles...", - "lang_users": "Users" + "lang_users": "Users", + "lang_view": "View" } \ No newline at end of file diff --git a/modules-available/permissionmanager/page.inc.php b/modules-available/permissionmanager/page.inc.php index 1c2d56bf..a78f935f 100644 --- a/modules-available/permissionmanager/page.inc.php +++ b/modules-available/permissionmanager/page.inc.php @@ -17,17 +17,21 @@ class Page_PermissionManager extends Page $action = Request::any('action', 'show', 'string'); if ($action === 'addRoleToUser') { + User::assertPermission('users.edit-roles'); $users = Request::post('users', ''); $roles = Request::post('roles', ''); PermissionDbUpdate::addRoleToUser($users, $roles); } elseif ($action === 'removeRoleFromUser') { + User::assertPermission('users.edit-roles'); $users = Request::post('users', ''); $roles = Request::post('roles', ''); PermissionDbUpdate::removeRoleFromUser($users, $roles); } elseif ($action === 'deleteRole') { + User::assertPermission('roles.edit'); $id = Request::post('deleteId', false, 'string'); PermissionDbUpdate::deleteRole($id); } elseif ($action === 'saveRole') { + User::assertPermission('roles.edit'); $roleID = Request::post("roleid", false); $rolename = Request::post("rolename"); $locations = self::processLocations(Request::post("locations")); @@ -44,33 +48,51 @@ class Page_PermissionManager extends Page */ protected function doRender() { - $show = Request::get("show", "roles"); + $show = Request::get("show", false, 'string'); + + if ($show === false) { + foreach (['roles', 'users', 'locations'] as $show) { + if (User::hasPermission($show . '.*')) + break; + } + } // switch between tables, but always show menu to switch tables // get menu button colors - $buttonColors = array(); + $data = array(); if ($show === "roleEditor") { - $buttonColors['groupClass'] = 'slx-fade'; - $buttonColors['rolesButtonClass'] = 'active'; + $data['groupClass'] = 'btn-group-muted'; + $data['rolesButtonClass'] = 'active'; } else { - $buttonColors[$show . 'ButtonClass'] = 'active'; + $data[$show . 'ButtonClass'] = 'active'; } + Permission::addGlobalTags($data['perms'], null, ['roles.*', 'users.*', 'locations.*']); - Render::addtemplate('header-menu', $buttonColors); + Render::addtemplate('header-menu', $data); if ($show === "roles") { - $data = array("roles" => GetPermissionData::getRoles()); + User::assertPermission('roles.*'); + $data = array("roles" => GetPermissionData::getRoles(GetPermissionData::WITH_USER_COUNT)); + Permission::addGlobalTags($data['perms'], null, ['roles.edit']); Render::addTemplate('rolestable', $data); } elseif ($show === "users") { - $data = array("user" => GetPermissionData::getUserData(), "allroles" => GetPermissionData::getRoles()); + User::assertPermission('users.*'); + $data = array("user" => GetPermissionData::getUserData()); + if (User::hasPermission('users.edit-roles')) { + $data['allroles'] = GetPermissionData::getRoles(); + } + Permission::addGlobalTags($data['perms'], null, ['users.edit-roles']); Render::addTemplate('role-filter-selectize', $data); Render::addTemplate('userstable', $data); } elseif ($show === "locations") { + User::assertPermission('locations.*'); $data = array("location" => GetPermissionData::getLocationData(), "allroles" => GetPermissionData::getRoles()); Render::addTemplate('role-filter-selectize', $data); Render::addTemplate('locationstable', $data); } elseif ($show === "roleEditor") { + User::assertPermission('roles.*'); $data = array("cancelShow" => Request::get("cancel", "roles")); + Permission::addGlobalTags($data['perms'], null, ['roles.edit']); $selectedPermissions = array(); $selectedLocations = array(); @@ -83,8 +105,10 @@ class Page_PermissionManager extends Page $selectedLocations = $roleData["locations"]; } - $data["permissionHTML"] = self::generatePermissionHTML(PermissionUtil::getPermissions(), $selectedPermissions); - $data["locationHTML"] = self::generateLocationHTML(Location::getTree(), $selectedLocations); + $data["permissionHTML"] = self::generatePermissionHTML(PermissionUtil::getPermissions(), $selectedPermissions, + false, '', ['perms' => $data['perms']]); + $data["locationHTML"] = self::generateLocationHTML(Location::getTree(), $selectedLocations, + false, '', ['perms' => $data['perms']]); Render::addTemplate('roleeditor', $data); } @@ -99,7 +123,7 @@ class Page_PermissionManager extends Page * @param string $permString the prefix permission string with which all permissions in the permission tree should start * @return string generated html code */ - private static function generatePermissionHTML($permissions, $selectedPermissions = array(), $selectAll = false, $permString = "") + private static function generatePermissionHTML($permissions, $selectedPermissions = array(), $selectAll = false, $permString = "", $tags = []) { $res = ""; $toplevel = $permString == ""; @@ -132,12 +156,12 @@ class Page_PermissionManager extends Page "toplevel" => $toplevel, "checkboxname" => "permissions", "selected" => $selected, - "HTML" => $leaf ? "" : self::generatePermissionHTML($v, $selectedPermissions, $selected, $nextPermString), + "HTML" => $leaf ? "" : self::generatePermissionHTML($v, $selectedPermissions, $selected, $nextPermString, $tags), ); if ($leaf) { $data += $v; } - $res .= Render::parse("treenode", $data); + $res .= Render::parse("treenode", $data + $tags); } if ($toplevel) { $res = Render::parse("treepanel", @@ -145,7 +169,7 @@ class Page_PermissionManager extends Page "name" => Dictionary::translateFile("template-tags", "lang_permissions"), "checkboxname" => "permissions", "selected" => $selectAll, - "HTML" => $res)); + "HTML" => $res) + $tags); } return $res; } @@ -159,7 +183,7 @@ class Page_PermissionManager extends Page * @param array $toplevel true if the location tree are the children of the root location, false if not * @return string generated html code */ - private static function generateLocationHTML($locations, $selectedLocations = array(), $selectAll = false, $toplevel = true) + private static function generateLocationHTML($locations, $selectedLocations = array(), $selectAll = false, $toplevel = true, $tags = []) { $res = ""; if ($toplevel && in_array(0, $selectedLocations)) { @@ -174,7 +198,8 @@ class Page_PermissionManager extends Page "checkboxname" => "locations", "selected" => $selected, "HTML" => array_key_exists("children", $location) ? - self::generateLocationHTML($location["children"], $selectedLocations, $selected, false) : "")); + self::generateLocationHTML($location["children"], $selectedLocations, $selected, false, $tags) : "") + + $tags); } if ($toplevel) { $res = Render::parse("treepanel", @@ -182,7 +207,7 @@ class Page_PermissionManager extends Page "name" => Dictionary::translateFile("template-tags", "lang_locations"), "checkboxname" => "locations", "selected" => $selectAll, - "HTML" => $res)); + "HTML" => $res) + $tags); } return $res; } diff --git a/modules-available/permissionmanager/permissions/permissions.json b/modules-available/permissionmanager/permissions/permissions.json new file mode 100644 index 00000000..3981a2c3 --- /dev/null +++ b/modules-available/permissionmanager/permissions/permissions.json @@ -0,0 +1,17 @@ +{ + "roles.view": { + "location-aware": false + }, + "roles.edit": { + "location-aware": false + }, + "users.view": { + "location-aware": false + }, + "users.edit-roles": { + "location-aware": false + }, + "locations.view": { + "location-aware": false + } +} \ No newline at end of file diff --git a/modules-available/permissionmanager/style.css b/modules-available/permissionmanager/style.css index 8285fdd2..6169b26f 100644 --- a/modules-available/permissionmanager/style.css +++ b/modules-available/permissionmanager/style.css @@ -55,3 +55,7 @@ td > .label { column-count: 3; } } + +.btn-group-muted > button { + color: #aaa; +} \ No newline at end of file diff --git a/modules-available/permissionmanager/templates/header-menu.html b/modules-available/permissionmanager/templates/header-menu.html index ce31d237..91bfa3af 100644 --- a/modules-available/permissionmanager/templates/header-menu.html +++ b/modules-available/permissionmanager/templates/header-menu.html @@ -4,17 +4,17 @@
- - - diff --git a/modules-available/permissionmanager/templates/roleeditor.html b/modules-available/permissionmanager/templates/roleeditor.html index d50f2145..8524427b 100644 --- a/modules-available/permissionmanager/templates/roleeditor.html +++ b/modules-available/permissionmanager/templates/roleeditor.html @@ -22,7 +22,10 @@
{{lang_cancel}} - +
- {{lang_deleteCheck}} +

{{lang_roleDeleteConfirm}}

+ {{lang_numAssignedUsers}}:
- +
@@ -56,12 +66,13 @@
- {{lang_newRole}} + {{lang_newRole}}