diff --git a/packages/web/lib/plugins/site/class/site.class.php b/packages/web/lib/plugins/site/class/site.class.php index 6f075cc3da..02e562eb5b 100755 --- a/packages/web/lib/plugins/site/class/site.class.php +++ b/packages/web/lib/plugins/site/class/site.class.php @@ -323,4 +323,119 @@ public function assocSetter($assocItem, $alterItem = '', $implicitCall = false) } return $this; } + /** + * Whether this user's view is bounded by site membership. + * + * @param int $userID The user to test. + * + * @return bool + */ + public static function userIsRestricted($userID) + { + $userID = (int)$userID; + if ($userID < 1) { + return false; + } + $flags = self::getSubObjectIDs( + 'SiteUserRestriction', + array('userID' => $userID), + 'isRestricted' + ); + return (bool)(isset($flags[0]) ? $flags[0] : false); + } + /** + * The sites this user belongs to. + * + * @param int $userID The user to look up. + * + * @return array + */ + public static function userSiteIDs($userID) + { + return (array)self::getSubObjectIDs( + 'SiteUserAssociation', + array('userID' => (int)$userID), + 'siteID' + ); + } + /** + * The hosts belonging to any of these sites. + * + * @param array $siteIDs The sites. + * + * @return array + */ + public static function hostIDsForSites($siteIDs) + { + return (array)self::getSubObjectIDs( + 'SiteHostAssociation', + array('siteID' => (array)$siteIDs), + 'hostID' + ); + } + /** + * The groups holding one or more hosts of these sites. + * + * @param array $siteIDs The sites. + * + * @return array + */ + public static function groupIDsForSites($siteIDs) + { + $hostIDs = self::hostIDsForSites($siteIDs); + if (count($hostIDs) < 1) { + return array(); + } + return (array)self::getSubObjectIDs( + 'GroupAssociation', + array('hostID' => $hostIDs), + 'groupID' + ); + } + /** + * The object ids $userID may see for $classname. + * + * THE RETURN IS A TRI-STATE and the distinction is the whole point: + * + * null no boundary applies -- leave the caller's set alone + * array(...) narrow to exactly these ids + * array() a real answer meaning "nothing", NOT "no boundary" + * + * null is the only value that means "unbounded". Treating an empty + * array as unbounded -- which is what any `if (!$ids)` test does -- is + * how a user entitled to nothing ends up seeing everything, so callers + * must test `null ===` and nothing looser. + * + * This is the single statement of the membership rule. The management + * pages reach it through AddSiteFilterSearch and the API reaches it + * through AddSiteAPI; if the two ever disagree about who may see what, + * the boundary is decorative. + * + * @param string $classname The class being listed or fetched. + * @param int $userID The acting user. + * + * @return array|null + */ + public static function scopedObjectIDs($classname, $userID) + { + $classname = strtolower((string)$classname); + // Only what the plugin actually associates. Everything else -- + // images, snapins, storage nodes, the association tables -- has no + // site boundary to apply, and returning an id list for one would + // narrow lookups the plugin knows nothing about. + if (!in_array($classname, array('host', 'group'), true)) { + return null; + } + $userID = (int)$userID; + if (!self::userIsRestricted($userID)) { + return null; + } + $siteIDs = self::userSiteIDs($userID); + if (count($siteIDs) < 1) { + return array(); + } + return 'group' === $classname + ? self::groupIDsForSites($siteIDs) + : self::hostIDsForSites($siteIDs); + } } diff --git a/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php b/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php index 95a804862a..f10e367d0f 100644 --- a/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php +++ b/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php @@ -81,6 +81,13 @@ public function __construct() $this, 'adjustMassInfo' ) + ) + ->register( + 'API_SCOPE_IDS', + array( + $this, + 'scopeIDs' + ) ); } /** @@ -189,6 +196,53 @@ public function adjustMassInfo($arguments) break; } } + /** + * Narrows an API read to the acting user's sites. + * + * Until this existed the plugin's boundary was a management-page + * feature: the only filtering hook is AddSiteFilterSearch, registered + * on HOST_DATA and GROUP_DATA, and both handlers switch on the global + * $node/$sub the pages set. Nothing under api/ fires those events, so + * a site-restricted user saw their site in the grid and every host on + * the server through /fog/host/list -- on the same credentials, and + * without an API token, because Route skips API auth entirely when a + * management session is already valid. + * + * Sets $arguments['ids'] only when a boundary actually applies. Left + * alone it stays null, which is the caller's "no narrowing" value; an + * EMPTY array set here is a real answer meaning the user may see + * nothing. See Site::scopedObjectIDs() for why those must not be + * collapsed. + * + * @param mixed $arguments The arguments to modify. + * + * @return void + */ + public function scopeIDs($arguments) + { + if (!in_array($this->node, (array)self::$pluginsinstalled)) { + return; + } + // No acting user means no boundary to apply -- the service daemons + // and the status endpoints reach Route::ids()/names() with nobody + // logged in, and narrowing those to a site would break imaging + // rather than protect anything. + if (!self::$FOGUser || !self::$FOGUser->isValid()) { + return; + } + $scope = Site::scopedObjectIDs( + $arguments['classname'], + self::$FOGUser->get('id') + ); + if (null === $scope) { + return; + } + $arguments['ids'] = array_values( + array_unique( + array_map('intval', (array)$scope) + ) + ); + } /** * This function changes the getter to enact on this particular item. * diff --git a/packages/web/lib/plugins/site/hooks/addsitefiltersearch.hook.php b/packages/web/lib/plugins/site/hooks/addsitefiltersearch.hook.php index 121d9ebf0c..30ebce4ef2 100755 --- a/packages/web/lib/plugins/site/hooks/addsitefiltersearch.hook.php +++ b/packages/web/lib/plugins/site/hooks/addsitefiltersearch.hook.php @@ -85,11 +85,17 @@ public function hostData($arguments) case 'host': switch ($sub) { case 'search': + // Narrowed against $siteHosts rather than by a + // second SiteHostAssociation lookup of its own: + // the membership rule lives in Site now, and two + // statements of who may see what is a boundary + // that is decorative the first time they differ. $hostsID = self::getClass('HostManager')->search(''); - $hosts = self::getSubObjectIDs( - 'SiteHostAssociation', - array('hostID' => $hostsID,'siteID'=>$siteIDbyUser), - 'hostID' + $hosts = array_values( + array_intersect( + array_map('intval', (array)$hostsID), + array_map('intval', (array)$siteHosts) + ) ); break; case 'list': @@ -212,12 +218,7 @@ public function groupData($arguments) */ public function isRestricted($userid) { - $userRestrictions = self::getSubObjectIDs( - 'SiteUserRestriction', - array('userID' => $userid), - 'isRestricted' - ); - return $userRestrictions[0]; + return Site::userIsRestricted($userid); } /** * Get site IDs where the user is associated. @@ -228,12 +229,7 @@ public function isRestricted($userid) */ public function getSiteIDbyUser($userID) { - $find = array('userID' => $userID); - return self::getSubObjectIDs( - 'SiteUserAssociation', - $find, - 'siteID' - ); + return Site::userSiteIDs($userID); } /** @@ -245,12 +241,7 @@ public function getSiteIDbyUser($userID) */ public function getHostIDbySite($siteIDs) { - $find = array('siteID' => $siteIDs); - return self::getSubObjectIDs( - 'SiteHostAssociation', - $find, - 'hostID' - ); + return Site::hostIDsForSites($siteIDs); } /** * Get the group IDs which have one or more hosts of the user locations. @@ -261,11 +252,6 @@ public function getHostIDbySite($siteIDs) */ public function getGroupIDbySite($siteIDbyUser) { - $siteHosts = $this->getHostIDbySite($siteIDbyUser); - return self::getSubObjectIDs( - 'GroupAssociation', - array('hostID' => $siteHosts), - 'groupID' - ); + return Site::groupIDsForSites($siteIDbyUser); } } diff --git a/packages/web/lib/router/route.class.php b/packages/web/lib/router/route.class.php index 1980e3ddbd..4dd30b04a6 100644 --- a/packages/web/lib/router/route.class.php +++ b/packages/web/lib/router/route.class.php @@ -447,6 +447,18 @@ public static function runMatches() ? self::$matches['params']['class'] : '' ); + /** + * Object boundary for a per-object route. Inert unless a + * plugin answers API_SCOPE_IDS. + */ + self::_requireObjectScope( + isset(self::$matches['params']['class']) + ? self::$matches['params']['class'] + : '', + isset(self::$matches['params']['id']) + ? self::$matches['params']['id'] + : 0 + ); call_user_func_array( self::$matches['target'], array_values(self::$matches['params']) @@ -512,6 +524,103 @@ private static function _requireAuthorized($name, $class) HTTPResponseCodes::HTTP_FORBIDDEN ); } + /** + * The object ids the acting user may see for this class, or null when + * no boundary applies. + * + * THE RETURN IS A TRI-STATE and every caller below depends on it: + * + * null no boundary -- leave the result set alone + * array(...) narrow to exactly these ids + * array() a real answer meaning "nothing" + * + * null is the ONLY value meaning unbounded. `if (!$ids)` is true for + * both null and array(), so a caller written that way shows every + * object to the one user entitled to none. Test `null ===`. + * + * Inert in core: nothing here knows what a site is. The site plugin + * answers the event; with the plugin absent or the user unrestricted + * the value stays null and the read behaves exactly as it always did. + * + * @param string $classname The class being read. + * + * @return array|null + */ + private static function _scopeIDs($classname) + { + $ids = null; + self::$HookManager + ->processEvent( + 'API_SCOPE_IDS', + array( + 'classname' => &$classname, + 'ids' => &$ids + ) + ); + return is_array($ids) ? array_values($ids) : null; + } + /** + * Narrows a filter set to the ids the acting user may see. + * + * Folded into the WHERE rather than applied to the rows afterwards, so + * a route that only ever produces ids -- names() and ids() -- is + * bounded by the query itself. An intersection that comes out empty is + * passed through as an empty array on purpose: _buildWhere() compiles + * that to `WHERE 1=0` rather than dropping the term, which is the + * difference between "you may see nothing" and "here is everything". + * + * @param string $classname The class being read. + * @param array $whereItems The caller's filter. + * + * @return array + */ + private static function _scopeWhereItems($classname, $whereItems) + { + $scope = self::_scopeIDs($classname); + if (null === $scope) { + return $whereItems; + } + $whereItems = (array)$whereItems; + if (isset($whereItems['id'])) { + $whereItems['id'] = array_values( + array_intersect( + array_map('intval', (array)$whereItems['id']), + $scope + ) + ); + return $whereItems; + } + $whereItems['id'] = $scope; + return $whereItems; + } + /** + * Denies a per-object route whose target is outside the acting user's + * scope. + * + * At dispatch rather than in each handler, for the same reason + * _requireAuthorized() is: one place to audit, and it covers indiv, + * update, delete, task and cancel without each of them remembering. + * Routes carrying no id are unaffected. + * + * @param string $class The class the route is acting on, if any. + * @param int $id The target object id, if any. + * + * @return void + */ + private static function _requireObjectScope($class, $id) + { + $id = (int)$id; + if ($id < 1) { + return; + } + $scope = self::_scopeIDs(strtolower((string)$class)); + if (null === $scope || in_array($id, $scope, true)) { + return; + } + self::sendResponse( + HTTPResponseCodes::HTTP_FORBIDDEN + ); + } /** * Test token information. * @@ -638,6 +747,10 @@ public static function listem( $find, self::getsearchbody($classname) ); + // Object boundary. Applied to the rows rather than the query + // because this route has no LIMIT -- every match is built and + // returned -- so filtering here is exact and keeps 'count' honest. + $scope = self::_scopeIDs($classname); switch ($classname) { case 'plugin': self::$data['count_active'] = 0; @@ -671,6 +784,11 @@ public static function listem( if (!$bypass && false != $test) { continue; } + if (null !== $scope + && !in_array((int)$class->get('id'), $scope, true) + ) { + continue; + } self::$data[$classname.'s'][] = self::getter( $classname, $class, @@ -726,10 +844,16 @@ public static function search($class, $item) self::$data = array(); self::$data['count'] = 0; self::$data[$classname.'s'] = array(); + $scope = self::_scopeIDs($classname); foreach ($classman->search($item, true) as &$class) { if (false != stripos($class->get('name'), '_api_')) { continue; } + if (null !== $scope + && !in_array((int)$class->get('id'), $scope, true) + ) { + continue; + } self::$data[$classname.'s'][] = self::getter( $classname, $class @@ -2101,6 +2225,7 @@ public static function names($class, $whereItems = []) ); $whereItems = self::handleWhereItems($whereItems, $class); + $whereItems = self::_scopeWhereItems($classname, $whereItems); $sql = 'SELECT `' . $classVars['databaseFields']['id'] @@ -2189,6 +2314,8 @@ public static function ids($class, $whereItems = [], $getField = 'id') } } + $whereItems = self::_scopeWhereItems($classname, $whereItems); + $sql = 'SELECT `' . $classVars['databaseFields'][$getField] . '` FROM `' diff --git a/tests/site-api-scope.test.php b/tests/site-api-scope.test.php new file mode 100644 index 0000000000..8ac162571c --- /dev/null +++ b/tests/site-api-scope.test.php @@ -0,0 +1,287 @@ +isValid()'), + $failures, + $checks +); +// SiteHostAssociation is read twice in the management hook and only one +// of those is the boundary. The grid shows each row's site name, which is +// a per-host lookup by hostID and stays. What must not come back is a +// lookup keyed on siteID -- that is the membership question, and Site +// answers it for both paths now. +check( + 'the management hook asks no membership question of its own', + !preg_match("/'siteID'\s*=>/", $uiHook), + $failures, + $checks +); +check( + 'Site owns the SiteHostAssociation membership lookup', + false !== strpos($site, "'SiteHostAssociation'"), + $failures, + $checks +); +foreach ( + [ + 'SiteUserRestriction' => 'Site::userIsRestricted()', + 'SiteUserAssociation' => 'Site::userSiteIDs()', + 'GroupAssociation' => 'Site::groupIDsForSites()' + ] as $table => $owner +) { + check( + "the management hook reads $table through $owner, not its own query", + false === strpos($uiHook, "'$table'"), + $failures, + $checks + ); + check( + "Site owns the $table lookup", + false !== strpos($site, "'$table'"), + $failures, + $checks + ); +} + +if (count($failures)) { + fwrite(STDERR, 'FAIL (' . count($failures) . " of $checks):\n"); + foreach ($failures as $f) { + fwrite(STDERR, " - $f\n"); + } + exit(1); +} +echo "ok $checks checks passed\n";