diff --git a/packages/web/lib/fog/fogmanagercontroller.class.php b/packages/web/lib/fog/fogmanagercontroller.class.php index 5b252b0dc2..cb7d4275c4 100644 --- a/packages/web/lib/fog/fogmanagercontroller.class.php +++ b/packages/web/lib/fog/fogmanagercontroller.class.php @@ -151,6 +151,7 @@ public function __construct() * @param mixed $idField what fields to get * @param bool $onecompare second where uses AND * @param string $filter array function for filter + * @param string $scopeWhere an object-boundary SQL fragment to AND on * * @return array */ @@ -164,7 +165,8 @@ public function find( $not = false, $idField = false, $onecompare = true, - $filter = 'array_unique' + $filter = 'array_unique', + $scopeWhere = '' ) { // Fail safe defaults if (empty($findWhere)) { @@ -349,6 +351,64 @@ public function find( $idFields = array_filter($idFields); $idField = $idFields; unset($idFields); + $whereClause = ( + count($whereArray) > 0 ? + sprintf( + ' WHERE %s%s', + implode(" $whereOperator ", (array) $whereArray), + ( + $isEnabled ? + sprintf(' AND %s', $isEnabled) : + '' + ) + ) : + ( + $isEnabled ? + sprintf(' WHERE %s', $isEnabled) : + '' + ) + ); + $andClause = ( + count($whereArrayAnd) > 0 ? + ( + count($whereArray) > 0 ? + sprintf( + 'AND %s', + implode(" $whereOperator ", (array) $whereArrayAnd) + ) : + sprintf( + ' WHERE %s', + implode(" $whereOperator ", (array) $whereArrayAnd) + ) + ) : + '' + ); + // The object boundary, when the caller was given one to apply. + // + // Two properties this has to hold that the obvious splice does not. + // It is ANDed on LAST, after everything the caller asked for, with + // the caller's own terms parenthesised: $whereOperator is a parameter + // and 'OR' is a value it takes, so a term merged in beside the + // caller's could be satisfied INSTEAD of the boundary rather than as + // well as it. And it is joined with a literal ' AND ', never through + // $whereOperator, for the same reason. An OR that can reach outside + // the boundary is not a boundary. + // + // Empty means no boundary, which is every caller that does not pass + // this argument. A caller that means "you may see nothing" passes a + // fragment saying so, such as '1=0' -- NOT an empty string, which + // reads here as unrestricted and would hand back the whole table. + $scopeWhere = trim((string)$scopeWhere); + if ('' !== $scopeWhere) { + $inner = trim($whereClause . ' ' . $andClause); + $inner = preg_replace('#^WHERE\s+#i', '', $inner); + $whereClause = ( + '' === $inner ? + sprintf(' WHERE %s', $scopeWhere) : + sprintf(' WHERE (%s) AND (%s)', $inner, $scopeWhere) + ); + $andClause = ''; + } $query = sprintf( $this->loadQueryTemplate, ( @@ -358,38 +418,8 @@ public function find( ), $this->databaseTable, $join, - ( - count($whereArray) > 0 ? - sprintf( - ' WHERE %s%s', - implode(" $whereOperator ", (array) $whereArray), - ( - $isEnabled ? - sprintf(' AND %s', $isEnabled) : - '' - ) - ) : - ( - $isEnabled ? - sprintf(' WHERE %s', $isEnabled) : - '' - ) - ), - ( - count($whereArrayAnd) > 0 ? - ( - count($whereArray) > 0 ? - sprintf( - 'AND %s', - implode(" $whereOperator ", (array) $whereArrayAnd) - ) : - sprintf( - ' WHERE %s', - implode(" $whereOperator ", (array) $whereArrayAnd) - ) - ) : - '' - ), + $whereClause, + $andClause, $groupBy, $orderBy ); @@ -1002,7 +1032,7 @@ public function exists( * * @return mixe */ - public function search($keyword = '', $returnObjects = false) + public function search($keyword = '', $returnObjects = false, $scopeWhere = '') { $keyword = trim($keyword); if (!$keyword) { @@ -1030,7 +1060,19 @@ public function search($keyword = '', $returnObjects = false) ) ); if (empty($keyword) || $keyword === '%') { - return $this->find(); + return $this->find( + array(), + 'AND', + 'name', + 'ASC', + '=', + false, + false, + false, + true, + 'array_unique', + $scopeWhere + ); } $keyword = preg_replace( '#[%\+\s\+]#', @@ -1330,7 +1372,19 @@ public function search($keyword = '', $returnObjects = false) array('id' => $itemIDs) ); if ($returnObjects) { - return $this->find(array('id' => $itemIDs)); + return $this->find( + array('id' => $itemIDs), + 'AND', + 'name', + 'ASC', + '=', + false, + false, + false, + true, + 'array_unique', + $scopeWhere + ); } return $itemIDs; diff --git a/packages/web/lib/plugins/site/class/site.class.php b/packages/web/lib/plugins/site/class/site.class.php index 02e562eb5b..c56cbaa99a 100755 --- a/packages/web/lib/plugins/site/class/site.class.php +++ b/packages/web/lib/plugins/site/class/site.class.php @@ -367,10 +367,46 @@ public static function userSiteIDs($userID) */ public static function hostIDsForSites($siteIDs) { - return (array)self::getSubObjectIDs( - 'SiteHostAssociation', - array('siteID' => (array)$siteIDs), - 'hostID' + $siteIDs = array_filter( + array_map('intval', array_values((array)$siteIDs)) + ); + if (count($siteIDs) < 1) { + return array(); + } + // Read directly rather than through getSubObjectIDs(). + // + // SiteHostAssociation declares a class relationship to Host, and + // find() walks those relationships to build the joins -- including + // Host's own MACAddressAssociation relationship, which carries the + // filter array('primary' => 1). buildQuery() puts that in the WHERE + // as `hostMAC`.`hmPrimary` = '1', which turns a LEFT OUTER JOIN into + // an inner one and DROPS every host with no primary MAC row. + // + // The effect was silent and it under-returned: a site-restricted user + // did not see hosts in their own site that had no primary MAC. In the + // lab, 95 of 1000. It is not a disclosure -- nobody saw anything they + // should not -- but it is wrong, and it made the SQL boundary in + // scopedObjectWhere() disagree with this one, which is worse: which + // hosts you could see depended on which of the two answered. + // + // A plain membership lookup has no business joining Host at all. The + // table and column names are the same ones scopedObjectWhere() writes + // and are justified there. + $rows = self::$DB + ->query( + sprintf( + 'SELECT DISTINCT `siteHostAssoc`.`shaHostID`' + . ' FROM `siteHostAssoc`' + . ' WHERE `siteHostAssoc`.`shaSiteID` IN (%s)', + implode(',', $siteIDs) + ) + ) + ->fetch(\PDO::FETCH_ASSOC, 'fetch_all') + ->get('shaHostID'); + return array_values( + array_unique( + array_map('intval', (array)$rows) + ) ); } /** @@ -416,25 +452,145 @@ public static function groupIDsForSites($siteIDs) * * @return array|null */ - public static function scopedObjectIDs($classname, $userID) + /** + * Per-request memo of _boundedSiteIDs(), keyed by user id. + * + * Core consults the SQL fragment first and falls back to the id list, so + * on a server where only one of the two is answered -- which is every + * server, since the fragment always wins here -- an UNRESTRICTED user + * pays for the restriction lookup TWICE per read: once for the fragment + * that declines, once for the id list that declines. Measured at 2 -> 3 + * statements for an administrator on `names(host)` before this existed, + * which is a cost the boundary imposes on exactly the people it does not + * apply to. + * + * Deliberately NOT a memo on userIsRestricted() or userSiteIDs(). Those + * are public and the management pages call them directly, including on + * requests that have just written the rows they read; a memo there would + * serve a stale answer to the page that changed it. Scoped to this + * private ladder, the only callers are the two read-side entry points, + * neither of which writes. + * + * @var array + */ + private static $_boundedSites = array(); + /** + * The same boundary as scopedObjectIDs(), expressed as SQL. + * + * THE RETURN IS A TRI-STATE, and it is not the id list's: + * + * null no boundary applies -- the caller must fall back + * '' narrow with this expression + * '1=0' a real answer meaning "nothing" + * + * There is no empty-string state, because the caller reads '' as "no + * listener answered" -- see Route::_scopeWhere(). Deny-all therefore has + * to be said in SQL, and '1=0' is how it is said here. + * + * Why a fragment at all: scopedObjectIDs() answers by reading every + * object the user may see into PHP, on every request, and the caller then + * either splices thousands of ids into an IN list or compares each row + * against them. This costs one expression whatever the fleet size. The + * membership rule is still stated once -- the two functions share the + * ladder below and differ only in what they return -- so the API and the + * management pages cannot drift into two different answers. + * + * Nothing here interpolates user input. $idExpr is built by the caller + * from the model's own $databaseFields, and $userID is cast to int; there + * is no path from a request parameter into this string. A future edit + * that wants to inline anything else needs a parameter, not a cast. + * + * @param string $classname The class being listed or fetched. + * @param string $idExpr The object-id column, quoted and qualified. + * @param int $userID The acting user. + * + * @return string|null + */ + public static function scopedObjectWhere($classname, $idExpr, $userID) + { + $siteIDs = self::_boundedSiteIDs($classname, $userID); + if (null === $siteIDs) { + return null; + } + if (count($siteIDs) < 1) { + return '1=0'; + } + $sites = implode( + ',', + array_map('intval', array_values($siteIDs)) + ); + $hostsInSites = sprintf( + 'SELECT `siteHostAssoc`.`shaHostID` FROM `siteHostAssoc`' + . ' WHERE `siteHostAssoc`.`shaSiteID` IN (%s)', + $sites + ); + if ('group' === strtolower((string)$classname)) { + // A group is in scope when it holds at least one host that is. + // Same rule as groupIDsForSites(), which is the point. + return sprintf( + 'EXISTS (SELECT 1 FROM `groupMembers`' + . ' WHERE `groupMembers`.`gmGroupID` = %s' + . ' AND `groupMembers`.`gmHostID` IN (%s))', + $idExpr, + $hostsInSites + ); + } + return sprintf( + 'EXISTS (SELECT 1 FROM `siteHostAssoc`' + . ' WHERE `siteHostAssoc`.`shaHostID` = %s' + . ' AND `siteHostAssoc`.`shaSiteID` IN (%s))', + $idExpr, + $sites + ); + } + /** + * The sites bounding this user for this class, or null for no boundary. + * + * The shared front half of scopedObjectIDs() and scopedObjectWhere(): + * everything up to the membership lookup itself. Two copies of this + * ladder would be two chances to answer "is this user bounded?" + * differently, in the one place where the two answers must agree. + * + * Returns an EMPTY ARRAY for a restricted user belonging to no site -- + * a real answer meaning "nothing" -- and null when no boundary applies. + * + * @param string $classname The class being listed or fetched. + * @param int $userID The acting user. + * + * @return array|null + */ + private static function _boundedSiteIDs($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. + // site boundary to apply, and narrowing one the plugin knows nothing + // about would break lookups rather than protect anything. if (!in_array($classname, array('host', 'group'), true)) { return null; } $userID = (int)$userID; - if (!self::userIsRestricted($userID)) { + if (array_key_exists($userID, self::$_boundedSites)) { + return self::$_boundedSites[$userID]; + } + self::$_boundedSites[$userID] = self::userIsRestricted($userID) + ? self::userSiteIDs($userID) + : null; + return self::$_boundedSites[$userID]; + } + public static function scopedObjectIDs($classname, $userID) + { + // Same ladder as scopedObjectWhere(), deliberately: these two answer + // the same question in two shapes, and a server where they disagree + // has a boundary that depends on which route you came in through. + $siteIDs = self::_boundedSiteIDs($classname, $userID); + if (null === $siteIDs) { return null; } - $siteIDs = self::userSiteIDs($userID); if (count($siteIDs) < 1) { return array(); } - return 'group' === $classname + return 'group' === strtolower((string)$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 f10e367d0f..d563fa1558 100644 --- a/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php +++ b/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php @@ -88,6 +88,13 @@ public function __construct() $this, 'scopeIDs' ) + ) + ->register( + 'API_SCOPE_WHERE', + array( + $this, + 'scopeWhere' + ) ); } /** @@ -243,6 +250,52 @@ public function scopeIDs($arguments) ) ); } + /** + * The same boundary as scopeIDs(), as a SQL fragment. + * + * Answered in preference to the id list, and the reason is cost: scopeIDs() + * reads every host the user may see into PHP on every request, which on a + * server with thousands of them is the whole expense of the feature. A + * fragment is one expression whatever the fleet size. + * + * Both handlers stay registered. Core tries this one and falls back to the + * id list when nothing answers, so a third-party plugin that knows only + * API_SCOPE_IDS keeps bounding reads exactly as it did. + * + * Sets $arguments['where'] only when a boundary applies. Left alone it + * stays null, which is the caller's "nobody answered" value. Note this + * tri-state is NOT the id list's: an empty string is read as silence, so + * "you may see nothing" is the literal fragment '1=0'. See + * Site::scopedObjectWhere(). + * + * @param mixed $arguments The arguments to modify. + * + * @return void + */ + public function scopeWhere($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. Same guard as scopeIDs(), and it has + // to be here too: this handler is consulted FIRST, so a boundary it + // emitted would apply before the id list was ever asked. + if (!self::$FOGUser || !self::$FOGUser->isValid()) { + return; + } + $where = Site::scopedObjectWhere( + $arguments['classname'], + $arguments['idExpr'], + self::$FOGUser->get('id') + ); + if (null === $where) { + return; + } + $arguments['where'] = $where; + } /** * This function changes the getter to enact on this particular item. * diff --git a/packages/web/lib/router/route.class.php b/packages/web/lib/router/route.class.php index 4dd30b04a6..cbc4fd42de 100644 --- a/packages/web/lib/router/route.class.php +++ b/packages/web/lib/router/route.class.php @@ -559,6 +559,115 @@ private static function _scopeIDs($classname) ); return is_array($ids) ? array_values($ids) : null; } + /** + * The object boundary as a SQL fragment, or null when none applies. + * + * Tried BEFORE _scopeIDs(). A boundary expressed as SQL costs one + * expression whatever the fleet size, where an id list costs a lookup of + * every object the user may see, materialised into PHP, on every request + * -- and then has to be spliced into a query or compared against every + * row. On a server with thousands of hosts that is the whole cost of the + * feature. + * + * THE RETURN IS A TRI-STATE, and it is NOT the same tri-state as + * _scopeIDs(): + * + * null no answer -- fall through to the id list + * '' narrow with this expression + * '1=0' (or similar) a real answer meaning "nothing" + * + * There is deliberately no empty-string state. An empty fragment is + * indistinguishable from silence, so it is read as silence: a listener + * that means "you may see nothing" must say so in SQL, and '1=0' is how. + * If '' were treated as deny-all, a listener that returned '' by accident + * would deny; if it were treated as a boundary, it would ALSO produce + * `WHERE ()`, which is a syntax error. Reading it as no-answer is the + * only option that fails towards the existing id-list path rather than + * towards either a broken query or a silent policy change. + * + * $idExpr is the caller's own id column, already quoted and qualified, so + * a listener can write `EXISTS (... WHERE assoc.hostID = )` and + * not have to know the table name or guess at ambiguity in a joined + * query. + * + * Inert in core: nothing here knows what a site is. + * + * @param string $classname The class being read. + * @param string $idExpr The object-id column, quoted and qualified. + * + * @return string|null + */ + private static function _scopeWhere($classname, $idExpr) + { + $where = null; + self::$HookManager + ->processEvent( + 'API_SCOPE_WHERE', + array( + 'classname' => &$classname, + 'idExpr' => &$idExpr, + 'where' => &$where + ) + ); + if (!is_string($where)) { + return null; + } + $where = trim($where); + return '' === $where ? null : $where; + } + /** + * The boundary fragment for a class, with the id expression worked out. + * + * Every caller needs the same `\`table\`.\`idcol\`` expression, and + * every caller getting it right separately is the sort of duplication + * that stays correct until one of them is edited. + * + * @param string $classname The class being read. + * + * @return string|null + */ + private static function _scopeWhereFor($classname) + { + $classVars = self::getClass($classname, '', true); + if (!isset($classVars['databaseTable'], $classVars['databaseFields']['id'])) { + return null; + } + return self::_scopeWhere( + $classname, + sprintf( + '`%s`.`%s`', + $classVars['databaseTable'], + $classVars['databaseFields']['id'] + ) + ); + } + /** + * ANDs a boundary fragment onto a WHERE clause _buildWhere() produced. + * + * _buildWhere() returns either '' or a complete ' WHERE ...' whose own + * terms are joined with AND, so appending is safe without parentheses + * around what is already there -- but they are added anyway, because + * "the other function only ever emits AND" is a property a later edit + * can remove without anything here noticing. + * + * @param string $where The clause so far, '' or ' WHERE ...'. + * @param string $frag The boundary fragment. + * + * @return string + */ + private static function _andScopeWhere($where, $frag) + { + if (null === $frag || '' === (string)$frag) { + return $where; + } + $where = (string)$where; + if ('' === trim($where)) { + return ' WHERE (' . $frag . ')'; + } + return ' WHERE (' + . preg_replace('#^\s*WHERE\s+#i', '', trim($where)) + . ') AND (' . $frag . ')'; + } /** * Narrows a filter set to the ids the acting user may see. * @@ -613,7 +722,24 @@ private static function _requireObjectScope($class, $id) if ($id < 1) { return; } - $scope = self::_scopeIDs(strtolower((string)$class)); + $classname = strtolower((string)$class); + // The fragment answers this as a bounded existence check -- one row + // at most, and the id list is never materialised. It is also the SAME + // expression the list routes narrow with, which is the property that + // matters: a 403 that disagreed with what the list showed would be + // two statements of who may see what, and the second one to be edited + // would make the boundary decorative. + $scopeWhere = self::_scopeWhereFor($classname); + if (null !== $scopeWhere) { + if (self::_objectInScopeWhere($classname, $id, $scopeWhere)) { + return; + } + self::sendResponse( + HTTPResponseCodes::HTTP_FORBIDDEN + ); + return; + } + $scope = self::_scopeIDs($classname); if (null === $scope || in_array($id, $scope, true)) { return; } @@ -621,6 +747,52 @@ private static function _requireObjectScope($class, $id) HTTPResponseCodes::HTTP_FORBIDDEN ); } + /** + * Does this one object satisfy the boundary fragment? + * + * Deliberately not `SELECT ... LIMIT 1` over the scoped set followed by a + * comparison -- the id is bound as a parameter and the database answers + * yes or no, so the cost does not move with how many objects the user can + * see. + * + * A query that cannot run answers NO. That is the safe direction here: + * this decides whether to serve a single object, and refusing one the + * user was entitled to is a visible, reportable failure, where serving + * one they were not is silent. + * + * @param string $classname The class the route is acting on. + * @param int $id The target object id. + * @param string $frag The boundary fragment. + * + * @return bool + */ + private static function _objectInScopeWhere($classname, $id, $frag) + { + $classVars = self::getClass($classname, '', true); + if (!isset($classVars['databaseTable'], $classVars['databaseFields']['id'])) { + return false; + } + $sql = sprintf( + 'SELECT `%s`.`%s` FROM `%s` WHERE `%s`.`%s` = :scope_id' + . ' AND (%s) LIMIT 1', + $classVars['databaseTable'], + $classVars['databaseFields']['id'], + $classVars['databaseTable'], + $classVars['databaseTable'], + $classVars['databaseFields']['id'], + $frag + ); + $rows = self::$DB + ->query($sql, array(), array('scope_id' => (int)$id)) + ->fetch() + ->get(); + // is_array(), not count((array)$rows). PDODB::get() answers a query + // that matched nothing with `false`, and `count((array)false)` is 1 -- + // so the obvious test reads "no such row" as "in scope" and the gate + // allows everything it was built to refuse. It reported allowed for + // an object outside the boundary until a behavioural test drove it. + return is_array($rows) && count($rows) > 0; + } /** * Test token information. * @@ -747,10 +919,18 @@ 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); + // Object boundary, preferring SQL. + // + // A fragment is pushed into the query, so the rows the boundary + // excludes are never built at all. Nothing answering the fragment + // event falls through to the id list, which is applied to the rows + // instead -- unchanged, because a third-party plugin that only knows + // API_SCOPE_IDS has to keep working exactly as it did. + // + // Either way 'count' stays honest: it counts what is emitted, and + // this route has no LIMIT, so every match is built and returned. + $scopeWhere = self::_scopeWhereFor($classname); + $scope = null === $scopeWhere ? self::_scopeIDs($classname) : null; switch ($classname) { case 'plugin': self::$data['count_active'] = 0; @@ -776,7 +956,20 @@ public static function listem( } break; default: - foreach ((array)$classman->find($find, 'AND', $sortby) as &$class) { + $found = $classman->find( + $find, + 'AND', + $sortby, + 'ASC', + '=', + false, + false, + false, + true, + 'array_unique', + (string)$scopeWhere + ); + foreach ((array)$found as &$class) { $test = stripos( $class->get('name'), '_api_' @@ -844,8 +1037,11 @@ 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) { + // Same two-path boundary as listem(): SQL when a listener supplies + // it, the id list when none does. + $scopeWhere = self::_scopeWhereFor($classname); + $scope = null === $scopeWhere ? self::_scopeIDs($classname) : null; + foreach ($classman->search($item, true, (string)$scopeWhere) as &$class) { if (false != stripos($class->get('name'), '_api_')) { continue; } @@ -2225,7 +2421,13 @@ public static function names($class, $whereItems = []) ); $whereItems = self::handleWhereItems($whereItems, $class); - $whereItems = self::_scopeWhereItems($classname, $whereItems); + // Object boundary. The fragment is preferred and the id list is the + // fallback; only one of the two is ever applied, so a boundary is + // never counted twice and never half-applied. + $scopeWhere = self::_scopeWhereFor($classname); + if (null === $scopeWhere) { + $whereItems = self::_scopeWhereItems($classname, $whereItems); + } $sql = 'SELECT `' . $classVars['databaseFields']['id'] @@ -2235,7 +2437,10 @@ public static function names($class, $whereItems = []) . $classVars['databaseTable'] . '`'; - $sql .= self::_buildWhere($classVars, $whereItems, $params); + $sql .= self::_andScopeWhere( + self::_buildWhere($classVars, $whereItems, $params), + $scopeWhere + ); $sql .= ' ORDER BY `' . ( $classVars['databaseFields']['name'] ?: @@ -2314,7 +2519,11 @@ public static function ids($class, $whereItems = [], $getField = 'id') } } - $whereItems = self::_scopeWhereItems($classname, $whereItems); + // Object boundary; see names() for why only one of the two applies. + $scopeWhere = self::_scopeWhereFor($classname); + if (null === $scopeWhere) { + $whereItems = self::_scopeWhereItems($classname, $whereItems); + } $sql = 'SELECT `' . $classVars['databaseFields'][$getField] @@ -2322,7 +2531,10 @@ public static function ids($class, $whereItems = [], $getField = 'id') . $classVars['databaseTable'] . '`'; - $sql .= self::_buildWhere($classVars, $whereItems, $params); + $sql .= self::_andScopeWhere( + self::_buildWhere($classVars, $whereItems, $params), + $scopeWhere + ); $sql .= ' ORDER BY `' . ( (isset($classVars['databaseFields']['name']) && $classVars['databaseFields']['name']) ? diff --git a/tests/lib/scope-harness.php b/tests/lib/scope-harness.php new file mode 100644 index 0000000000..1b5d142d86 --- /dev/null +++ b/tests/lib/scope-harness.php @@ -0,0 +1,343 @@ +log[] = $sql; + foreach (array('hosts', 'groups') as $t) { + if (false === strpos($sql, '`' . $t . '`')) { + continue; + } + $vars = Route::getClass(rtrim($t, 's'), '', true); + $this->_r = array(); + foreach ($this->rowIds as $i) { + $row = array(); + // Every declared column, so loading an object does not warn + // its way through a row that is missing most of itself. + foreach ($vars['databaseFields'] as $col) { + $row[$col] = ''; + } + $row[$vars['databaseFields']['id']] = $i; + $row[$vars['databaseFields']['name']] = 'h' . $i; + $this->_r[] = $row; + } + return $this; + } + $this->_r = array(); + return $this; + } + + public function fetch($m = null, $t = '', $p = array()) + { + return $this; + } + + public function get($f = '') + { + return $this->_r; + } + + // save() on an unknown hook event reaches these; answering keeps the + // fixture from dying on the first event name this test introduces. + public function insertId() + { + return 1; + } + + public function sqlerror() + { + return ''; + } + + public function affectedRows() + { + return 1; + } +} + +/** + * Boots FOG far enough to drive Route, with the probe registered. + * + * @param string $web packages/web + * @param string $state 'none', 'scoped' or 'deny' -- only used by the + * subprocess arm, which cannot be handed an array + * + * @return void + */ +function scopeHarnessBoot($web, $state = 'none') +{ + static $booted = false; + if ($booted) { + return; + } + $booted = true; + + $tmp = sys_get_temp_dir() . '/fog-scope-behaviour-' . getmypid(); + @mkdir($tmp . '/cache', 0700, true); + @mkdir($tmp . '/log', 0700, true); + @mkdir($tmp . '/plugins', 0700, true); + register_shutdown_function( + function () use ($tmp) { + if (!is_dir($tmp)) { + return; + } + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator( + $tmp, + \FilesystemIterator::SKIP_DOTS + ), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($it as $f) { + $f->isDir() ? @rmdir($f->getPathname()) : @unlink($f->getPathname()); + } + @rmdir($tmp); + } + ); + if (!defined('FOG_CACHE_DIR')) { + define('FOG_CACHE_DIR', $tmp . '/cache'); + } + if (!defined('FOG_LOG_DIR')) { + define('FOG_LOG_DIR', $tmp . '/log'); + } + if (!defined('FOG_PLUGIN_DIR')) { + define('FOG_PLUGIN_DIR', $tmp . '/plugins'); + } + if (!defined('FOG_SCHEMA')) { + define('FOG_SCHEMA', 0); + } + // The row-shaped fixtures below are deliberately partial, so the noise a + // partial row makes is not the output of this test. + error_reporting(E_ALL & ~E_WARNING & ~E_NOTICE & ~E_DEPRECATED); + + require_once $web . '/commons/init.php'; + new Initiator(); + + $set = function ($class, $prop, $value) { + $p = new \ReflectionProperty($class, $prop); + $p->setAccessible(true); + $p->setValue(null, $value); + }; + // LoadGlobals is what normally builds these and it wants a database. + $set('FOGBase', 'HookManager', new HookManager()); + $set('FOGBase', 'EventManager', new EventManager()); + // AFTER the managers: constructing one re-runs FOGBase's own wiring and + // leaves $DB null behind it. + $set('FOGBase', 'DB', new ScopeFakeDB()); + // MACAddress logs a parse failure through FOGCore, and a Host builds one + // on load. Without this the host cases die on a null. + $set( + 'FOGBase', + 'FOGCore', + new class { + public function debug($m = '') + { + } + public function error($m = '') + { + } + } + ); + + // processEvent() writes an unseen event name to the hookevent table before + // dispatching it. Seeded so the fixture is not answering INSERTs. + $known = new \ReflectionProperty('HookManager', 'knownEvents'); + $known->setAccessible(true); + $known->setValue( + null, + array_flip( + array( + 'API_SCOPE_IDS', + 'API_SCOPE_WHERE', + 'API_GETTER', + 'API_MASSDATA_MAPPING', + 'API_INDIVDATA_MAPPING', + 'API_VALID_CLASSES', + 'API_SENSITIVE_FIELDS', + ) + ) + ); + + // After Initiator: ScopeProbe extends Hook, which does not exist until + // the autoloader does. + require_once __DIR__ . '/scopeprobe.php'; + $probe = (new \ReflectionClass('ScopeProbe'))->newInstanceWithoutConstructor(); + $hm = new \ReflectionProperty('FOGBase', 'HookManager'); + $hm->setAccessible(true); + $hm->getValue()->register('API_SCOPE_IDS', array($probe, 'scope')); + $hm->getValue()->register('API_SCOPE_WHERE', array($probe, 'scopeWhere')); + + switch ($state) { + case 'scoped': + ScopeProbe::$answer = array(2, 3); + break; + case 'deny': + ScopeProbe::$answer = array(); + break; + default: + ScopeProbe::$answer = 'none'; + } +} + +/** + * Why the database arm cannot run, or null when it can. + * + * Also defines the DATABASE_* constants when it returns null, reading them + * out of a live install's generated config so no credential is written down + * in the repository. + * + * @return string|null + */ +function scopeHarnessDbReason() +{ + foreach (glob('/var/www/html/fog-1.5*/lib/fog/config.class.php') ?: array() as $cfg) { + $src = @file_get_contents($cfg); + if (false === $src) { + continue; + } + preg_match_all( + "/define\('(DATABASE_[A-Z]+)', *'([^']*)'/", + $src, + $m, + PREG_SET_ORDER + ); + $vals = array(); + foreach ($m as $d) { + $vals[$d[1]] = $d[2]; + } + if (count($vals) < 5) { + continue; + } + foreach ($vals as $k => $v) { + if (!defined($k)) { + define($k, $v); + } + } + return null; + } + return 'no 1.5 install found under /var/www/html to read a database' + . ' configuration from'; +} + +/** + * Two hosts with NO MAC, in a site of their own, and a teardown for both. + * + * Exists because of a bug the group fixture cannot see. SiteHostAssociation + * declares a class relationship to Host, find() walks those to build joins, + * and Host's MACAddressAssociation relationship carries array('primary' => 1) + * -- which buildQuery() emits as `hostMAC`.`hmPrimary` = '1' in the WHERE. + * That turns the LEFT OUTER JOIN into an inner one, so a plain membership + * lookup silently dropped every host with no primary MAC: 95 of 1000 in the + * lab. Hosts with no MAC row at all are the sharpest case, so that is what + * this makes. + * + * @param object $db the real PDODB + * + * @return array array(siteID, array(hostID, hostID)) + */ +function scopeHarnessMaclessFixture($db) +{ + $mark = 'zzmac' . getmypid(); + register_shutdown_function( + function () use ($db, $mark) { + // Associations first: they are what points at the hosts, and a + // row left behind here would be a dangling membership. + $db->query( + "DELETE `siteHostAssoc` FROM `siteHostAssoc`" + . " JOIN `site` ON `site`.`sID` = `siteHostAssoc`.`shaSiteID`" + . " WHERE `site`.`sName` = '" . $mark . "'" + ); + $db->query("DELETE FROM `site` WHERE `sName` = '" . $mark . "'"); + $db->query( + "DELETE FROM `hosts` WHERE `hostName` LIKE '" . $mark . "%'" + ); + } + ); + $db->query( + "INSERT INTO `site` (`sName`,`sDesc`) VALUES ('" . $mark . "','fixture')" + ); + $siteID = (int)$db->insertId(); + $hostIDs = array(); + foreach (array('a', 'b') as $n) { + $db->query( + "INSERT INTO `hosts` (`hostName`,`hostIP`,`hostUseAD`)" + . " VALUES ('" . $mark . $n . "','','0')" + ); + $hostIDs[] = (int)$db->insertId(); + } + foreach ($hostIDs as $hid) { + $db->query( + "INSERT INTO `siteHostAssoc` (`shaName`,`shaSiteID`,`shaHostID`)" + . " VALUES ('', " . $siteID . ", " . $hid . ")" + ); + } + return array($siteID, $hostIDs); +} + +/** + * Swaps the database FOGBase reads through. + * + * @param object $db the connection to install + * + * @return void + */ +function scopeHarnessSetDb($db) +{ + $p = new \ReflectionProperty('FOGBase', 'DB'); + $p->setAccessible(true); + $p->setValue(null, $db); +} + +/** + * Three groups this process owns, and a teardown that removes exactly those. + * + * Marked with the pid so a concurrent run, or an abandoned one, cannot be + * mistaken for this run's rows -- and so the DELETE can name what it removes + * rather than clearing a table this server may be using for something. + * + * @param object $db the real PDODB + * + * @return array the three ids + */ +function scopeHarnessFixture($db) +{ + $mark = 'zz-scopetest-' . getmypid() . '-'; + register_shutdown_function( + function () use ($db, $mark) { + $db->query( + "DELETE FROM `groups` WHERE `groupName` LIKE '" . $mark . "%'" + ); + } + ); + $ids = array(); + foreach (array(1, 2, 3) as $n) { + $db->query( + "INSERT INTO `groups` (`groupName`,`groupDesc`)" + . " VALUES ('" . $mark . $n . "','api scope characterization')" + ); + $ids[] = (int)$db->insertId(); + } + return $ids; +} diff --git a/tests/lib/scopeprobe.php b/tests/lib/scopeprobe.php new file mode 100644 index 0000000000..0ea359aa2d --- /dev/null +++ b/tests/lib/scopeprobe.php @@ -0,0 +1,75 @@ + no boundary, read unrestricted. + * Not an edge case: the service daemons and the status endpoints reach + * Route::ids()/names() logged out, through getIds()/getNames(). A + * boundary that always applies scopes those too, and the symptom is + * imaging breaking, not a 403. + * + * 2. A scoped user with objects in scope -> exactly those objects. + * + * 3. A scoped user with NOTHING in scope -> NOTHING, not everything. + * This is the one that matters. `null` means "no boundary" and `array()` + * means "you may see nothing"; both are falsy, so any `if (!$ids)` test + * collapses deny-all into full disclosure -- for precisely the users the + * boundary exists to restrict, with no error and no log line. + * + * TWO ARMS. + * + * The row-filtered routes -- listem() and search() -- are assertable with no + * database at all, because the boundary is applied to objects the manager + * already built: a fake PDO hands back three rows and the assertion is which + * of them survive. That arm always runs. + * + * names() and ids() are not. They push the boundary into the WHERE clause, so + * "which rows come back" is a question only a real database can answer, and + * asserting on the generated SQL instead would be asserting the mechanism -- + * the thing this file exists not to do. That arm therefore runs against a real + * 1.5 schema when one is reachable and SKIPS when it is not, rather than + * quietly degrading into a string match. + * + * The database arm creates its own rows, marked with this process' pid, and + * asserts only about those. It never asserts a table is empty: "empty of my + * rows" is not "empty", and a server with real data would fail an assertion + * that confused the two. The single exception is deny-all, where a zero TOTAL + * is the assertion. + * + * Usage: php tests/site-api-scope-behaviour.test.php + * Exit status 0 = pass (or skip), 1 = fail. + */ + +$web = dirname(__DIR__) . '/packages/web'; + +// --------------------------------------------------------------- subprocess +// +// _requireObjectScope() answers a denial with sendResponse(), which ends in +// exit -- there is no result to inspect and no exception to catch, so the +// only way to observe it is from outside the process. Re-exec of this same +// file with an argument, and the marker below is what "it did not deny" +// looks like. +if (isset($argv[1]) && '--object-scope' === $argv[1]) { + require __DIR__ . '/lib/scope-harness.php'; + scopeHarnessBoot($web, $argv[2]); + // The fragment states need a real database: the gate answers a fragment + // by asking whether one row satisfies it, which is a question only a + // database has. The rows are the PARENT's fixture -- they exist for as + // long as the parent does -- and their ids arrive on the command line so + // the child does not create a second, unrelated set. + if (0 === strpos($argv[2], 'frag')) { + scopeHarnessDbReason(); + scopeHarnessSetDb(new PDODB()); + $inScope = array_map('intval', array_slice($argv, 4)); + ScopeProbe::$whereAnswer = 'frag-deny' === $argv[2] + ? '1=0' + : function ($idExpr) use ($inScope) { + return $idExpr . ' IN (' . implode(',', $inScope) . ')'; + }; + } + $m = new \ReflectionMethod('Route', '_requireObjectScope'); + $m->setAccessible(true); + $m->invoke(null, 'group', (int)$argv[3]); + echo "ALLOWED\n"; + exit(0); +} + +require __DIR__ . '/lib/scope-harness.php'; + +$failures = []; +$checks = 0; + +function check($label, $cond, array &$failures, &$checks) +{ + $checks++; + if (!$cond) { + $failures[] = $label; + } +} + +// ------------------------------------------------------- arm 1: no database +// +// Three rows in, and the question is which come out. `group` rather than +// `host` for the second class because a Host builds a MACAddress on load, +// which reaches for globals a fake database has no way to supply; both are +// scoped classes and the boundary code does not branch on which. +scopeHarnessBoot($web, 'none'); + +$states = [ + ['none', 'unbounded', [1, 2, 3]], + [[2, 3], 'scoped', [2, 3]], + [[], 'deny-all', []], +]; + +foreach (['host', 'group'] as $class) { + foreach ($states as [$answer, $label, $expect]) { + ScopeProbe::$answer = $answer; + foreach (['listem', 'search'] as $route) { + Route::$data = null; + if ('listem' === $route) { + Route::listem($class); + } else { + Route::search($class, 'h'); + } + $got = array_map( + 'intval', + array_column((array)(Route::$data[$class . 's'] ?? []), 'id') + ); + sort($got); + check( + "$route($class) $label returns [" . implode(',', $expect) . ']', + $got === $expect, + $failures, + $checks + ); + // The count is part of the answer, not decoration: a caller that + // pages on it is told how many objects exist, so a count computed + // over the unscoped set discloses the size of what it hid. + check( + "$route($class) $label reports count " . count($expect), + (int)(Route::$data['count'] ?? -1) === count($expect), + $failures, + $checks + ); + } + } +} + +// ---------------------------------------- arm 2: per-object route, out of process +$objectScope = function ($state, $id, array $extra = []) { + $out = []; + exec( + sprintf( + '%s %s --object-scope %s %d %s 2>&1', + escapeshellarg(PHP_BINARY), + escapeshellarg(__FILE__), + escapeshellarg($state), + $id, + implode(' ', array_map('intval', $extra)) + ), + $out + ); + return in_array('ALLOWED', array_map('trim', $out), true); +}; +foreach ([['none', 1, true], ['scoped', 1, false], ['scoped', 3, true], ['deny', 1, false]] as [$state, $id, $allow]) { + check( + "_requireObjectScope(group, $id) under '$state' " . ($allow ? 'allows' : 'denies'), + $objectScope($state, $id) === $allow, + $failures, + $checks + ); +} + +// ------------------------------------------------------- arm 3: real database +// +// Skipped rather than faked when there is no 1.5 schema to talk to. The +// credentials are read from a live install's generated config at run time and +// never written down here. +$dbSkip = scopeHarnessDbReason(); +if (null !== $dbSkip) { + echo "SKIP (database arm): $dbSkip\n"; +} else { + // Arms 1 and 2 ran against the fake; from here the boundary has to be + // answered by a real database, because "which rows come back" is the + // whole assertion and only a database can answer it. + $real = new PDODB(); + scopeHarnessSetDb($real); + $ids = scopeHarnessFixture($real); + $mine = function ($values) use ($ids) { + $out = array_values( + array_intersect(array_map('intval', (array)$values), $ids) + ); + sort($out); + return $out; + }; + $cases = [ + ['none', 'unbounded', $ids], + [[$ids[1], $ids[2]], 'scoped', [$ids[1], $ids[2]]], + [[], 'deny-all', []], + ]; + foreach ($cases as [$answer, $label, $expect]) { + ScopeProbe::$answer = $answer; + + Route::$data = null; + Route::names('group', []); + $names = (array)Route::$data; + check( + "names(group) $label returns my " . count($expect) . ' group(s)', + $mine(array_column($names, 'id')) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::ids('group', [], 'id'); + $idlist = (array)Route::$data; + check( + "ids(group) $label returns my " . count($expect) . ' group(s)', + $mine($idlist) === $expect, + $failures, + $checks + ); + + // Deny-all is the only state where a TOTAL is assertable, and it is + // the assertion the whole tri-state exists for: not "my rows are + // gone" but "no rows at all", which is what separates a boundary + // that compiled to nothing from one that compiled to everything. + if ('deny-all' === $label) { + check( + 'names(group) deny-all returns NO rows at all', + 0 === count($names), + $failures, + $checks + ); + check( + 'ids(group) deny-all returns NO rows at all', + 0 === count($idlist), + $failures, + $checks + ); + } + } + + /* + * The SQL-fragment path, which is what a site-scoped server actually + * runs -- the id list is now the fallback for plugins that predate the + * fragment event. + * + * Only reachable with a real database, and not because of a harness + * limitation: the whole point of a fragment is that the DATABASE applies + * the boundary, so there is nothing to observe without one. The answers + * below are built from the caller's own $idExpr rather than a hardcoded + * column, so what is under test is the seam and not a string. + */ + ScopeProbe::$answer = 'none'; + $fragCases = array( + array('none', 'unbounded', $ids), + array( + function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }, + 'scoped', + array($ids[1], $ids[2]) + ), + array('1=0', 'deny-all', array()), + ); + foreach ($fragCases as [$answer, $label, $expect]) { + ScopeProbe::$whereAnswer = $answer; + + Route::$data = null; + Route::names('group', []); + $names = (array)Route::$data; + check( + "fragment: names(group) $label returns my " . count($expect), + $mine(array_column($names, 'id')) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::ids('group', [], 'id'); + $idlist = (array)Route::$data; + check( + "fragment: ids(group) $label returns my " . count($expect), + $mine($idlist) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::listem('group'); + check( + "fragment: listem(group) $label returns my " . count($expect), + $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::search('group', 'zz-scopetest-'); + check( + "fragment: search(group) $label returns my " . count($expect), + $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')) === $expect, + $failures, + $checks + ); + + if ('deny-all' === $label) { + check( + 'fragment: names(group) deny-all returns NO rows at all', + 0 === count($names), + $failures, + $checks + ); + check( + 'fragment: listem(group) deny-all returns NO rows at all', + 0 === (int)(Route::$data['count'] ?? -1) + || 0 === count((array)(Route::$data['groups'] ?? [])), + $failures, + $checks + ); + } + } + + /* + * The fragment survives a caller's own filter. + * + * Every case above hands names()/ids() an empty filter, so the boundary + * is the only term in the WHERE and a fragment that got dropped whenever + * a clause already existed would pass all of them. That is not a + * hypothetical: the append helper has two arms, and only the empty one + * was being driven. A caller asking for all three of my groups, narrowed + * by a fragment naming two, has to come back with two. + */ + ScopeProbe::$answer = 'none'; + ScopeProbe::$whereAnswer = function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }; + Route::$data = null; + Route::names('group', ['id' => $ids]); + check( + 'fragment: names(group) narrows a filter the caller supplied', + $mine(array_column((array)Route::$data, 'id')) === [$ids[1], $ids[2]], + $failures, + $checks + ); + Route::$data = null; + Route::ids('group', ['id' => $ids], 'id'); + check( + 'fragment: ids(group) narrows a filter the caller supplied', + $mine((array)Route::$data) === [$ids[1], $ids[2]], + $failures, + $checks + ); + ScopeProbe::$whereAnswer = '1=0'; + Route::$data = null; + Route::names('group', ['id' => $ids]); + check( + 'fragment: deny-all beats a filter the caller supplied', + 0 === count((array)Route::$data), + $failures, + $checks + ); + // And the manager path too: listem() passes the caller's own $find + // alongside the fragment, which is the second arm of the same join. + ScopeProbe::$whereAnswer = function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }; + Route::$data = null; + Route::listem('group', 'name', false, ['id' => $ids]); + check( + 'fragment: listem(group) narrows a filter the caller supplied', + $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')) + === [$ids[1], $ids[2]], + $failures, + $checks + ); + + /* + * Exactly ONE boundary is applied, never both. + * + * Two narrowings ANDed together is not a safer boundary, it is a + * different one nobody stated -- and the failure is invisible, because + * over-restriction looks like the feature working. The fragment says + * "these two"; the id list says "that other one"; if both applied the + * answer would be nothing at all. + */ + ScopeProbe::$whereAnswer = function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }; + ScopeProbe::$answer = array($ids[0]); + foreach (array('names', 'ids', 'listem', 'search') as $route) { + Route::$data = null; + switch ($route) { + case 'names': + Route::names('group', []); + $got = $mine(array_column((array)Route::$data, 'id')); + break; + case 'ids': + Route::ids('group', [], 'id'); + $got = $mine((array)Route::$data); + break; + case 'listem': + Route::listem('group'); + $got = $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')); + break; + default: + Route::search('group', 'zz-scopetest-'); + $got = $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')); + } + check( + "$route(group) applies the fragment ALONE when both events answer", + $got === array($ids[1], $ids[2]), + $failures, + $checks + ); + } + + /* + * And the id list is still reachable. Without this the fall-through could + * be broken in the other direction -- fragment-only -- and every check + * above would still pass while every plugin written before the fragment + * event silently stopped bounding anything. + */ + ScopeProbe::$whereAnswer = 'none'; + ScopeProbe::$answer = array($ids[0]); + Route::$data = null; + Route::names('group', []); + check( + 'the id list still bounds a read when no fragment answers', + $mine(array_column((array)Route::$data, 'id')) === array($ids[0]), + $failures, + $checks + ); + // An empty fragment is silence, not deny-all: it must fall through to + // the id list rather than either denying or reading unbounded. + ScopeProbe::$whereAnswer = ' '; + Route::$data = null; + Route::names('group', []); + check( + 'an empty fragment falls through to the id list', + $mine(array_column((array)Route::$data, 'id')) === array($ids[0]), + $failures, + $checks + ); + ScopeProbe::$whereAnswer = 'none'; + ScopeProbe::$answer = 'none'; + + /* + * The two boundaries must agree about WHICH hosts are in a site. + * + * They did not. A membership lookup through SiteHostAssociation's manager + * drags in Host's joins, including the primary-MAC filter, so it dropped + * every host with no primary MAC -- 95 of 1000 in the lab -- while the + * SQL fragment, which joins nothing, returned all of them. Not a + * disclosure: it under-returned, so a site-restricted user could not see + * hosts in their own site. But it meant which hosts you could see + * depended on which of the two answered, which is the one thing a + * boundary must not do. + * + * Asserted against the database rather than against the other function, + * so it cannot pass by both being wrong in the same way. + */ + [$maclessSite, $maclessHosts] = scopeHarnessMaclessFixture($real); + $got = Site::hostIDsForSites([$maclessSite]); + sort($got); + check( + 'hostIDsForSites() returns hosts that have no primary MAC', + array_map('intval', (array)$got) === $maclessHosts, + $failures, + $checks + ); + // And the fragment agrees, which is the property that actually matters. + $frag = Site::scopedObjectWhere( + 'host', + '`hosts`.`hostID`', + 0 + ); + check( + 'scopedObjectWhere() declines for a user with no restriction row', + null === $frag, + $failures, + $checks + ); + + // The per-object gate under a fragment. Same expression the lists narrow + // with, so a 403 and an absent row are the same decision. + check( + 'fragment: the gate allows an object inside the boundary', + true === $objectScope('frag-scoped', $ids[1], [$ids[1], $ids[2]]), + $failures, + $checks + ); + check( + 'fragment: the gate denies an object outside the boundary', + false === $objectScope('frag-scoped', $ids[0], [$ids[1], $ids[2]]), + $failures, + $checks + ); + check( + 'fragment: the gate denies everything under a deny-all fragment', + false === $objectScope('frag-deny', $ids[1], [$ids[1], $ids[2]]), + $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"; diff --git a/tests/site-api-scope.test.php b/tests/site-api-scope.test.php index 8ac162571c..9c5572ffbb 100644 --- a/tests/site-api-scope.test.php +++ b/tests/site-api-scope.test.php @@ -152,7 +152,9 @@ function methodBody($src, $name) */ $looseScope = []; if (preg_match_all( - '/(?:if\s*\(\s*!\s*\$scope\b|empty\s*\(\s*\$scope\s*\)|if\s*\(\s*\$scope\s*\))/', + '/(?:if\s*\(\s*!\s*\$scope(?:Where)?\b' + . '|empty\s*\(\s*\$scope(?:Where)?\s*\)' + . '|if\s*\(\s*\$scope(?:Where)?\s*\))/', $route, $m )) { @@ -168,15 +170,108 @@ function methodBody($src, $name) // Counted, not merely present. The regex above catches `if (!$scope)` // but not `if ($scope && ...)` spanning two lines, and the whole failure // mode here is a comparison quietly losing its `null` half. +// Word-bounded, because $scopeWhere is a DIFFERENT tri-state with a +// different vocabulary and an unbounded /\$scope/ counts both as one. check( 'the row filters compare against null exactly twice (listem, search)', - 2 === preg_match_all('/null !== \$scope/', $route, $mn), + 2 === preg_match_all('/null !== \$scope\b/', $route, $mn), $failures, $checks ); check( 'the WHERE narrowing and the dispatch gate each short-circuit on null', - 2 === preg_match_all('/null === \$scope/', $route, $mn2), + 2 === preg_match_all('/null === \$scope\b/', $route, $mn2), + $failures, + $checks +); + +/* + * The SQL-fragment boundary, which is tried before the id list. + * + * Its tri-state is not the id list's and must not be written as though it + * were. There is no empty-string state: '' is read as "no listener answered" + * and falls through, because an empty fragment is indistinguishable from + * silence and would otherwise compile to `WHERE ()`. What has to hold is + * that the two are never BOTH applied -- a request narrowed twice is a + * request whose boundary nobody can reason about -- and that the id list is + * still reachable, or every third-party plugin answering only the old event + * silently stops bounding anything. + */ +check( + '_scopeWhere() answers null for anything that is not a string', + false !== strpos(methodBody($route, '_scopeWhere'), 'is_string($where)'), + $failures, + $checks +); +check( + '_scopeWhere() reads an empty fragment as no answer, not as deny-all', + (bool)preg_match( + "/'' === \\\$where \\? null :/", + methodBody($route, '_scopeWhere') + ), + $failures, + $checks +); +foreach (['listem', 'search'] as $fn) { + $body = methodBody($route, $fn); + check( + "$fn() falls through to the id list only when no fragment answered", + (bool)preg_match( + '/\$scope = null === \$scopeWhere \? self::_scopeIDs\(/', + $body + ), + $failures, + $checks + ); + check( + "$fn() hands the fragment to the manager rather than filtering rows", + false !== strpos($body, '(string)$scopeWhere'), + $failures, + $checks + ); +} +foreach (['names', 'ids'] as $fn) { + $body = methodBody($route, $fn); + check( + "$fn() applies the id list only when no fragment answered", + (bool)preg_match( + '/if \(null === \$scopeWhere\) \{\s*\$whereItems = self::_scopeWhereItems\(/', + $body + ), + $failures, + $checks + ); + check( + "$fn() ANDs the fragment onto the clause it built", + false !== strpos($body, '_andScopeWhere('), + $failures, + $checks + ); +} +check( + 'the dispatch gate answers a fragment with a bounded existence check', + false !== strpos( + methodBody($route, '_requireObjectScope'), + '_objectInScopeWhere(' + ), + $failures, + $checks +); +check( + 'a boundary query that cannot run denies rather than allows', + (bool)preg_match( + '/databaseFields..\[.id.\]\)\) \{\s*return false;/', + methodBody($route, '_objectInScopeWhere') + ), + $failures, + $checks +); +check( + 'the object id is bound as a parameter, never concatenated', + false !== strpos( + methodBody($route, '_objectInScopeWhere'), + "array('scope_id' => (int)\$id)" + ), $failures, $checks ); @@ -195,9 +290,117 @@ function methodBody($src, $name) $failures, $checks ); +// The two short circuits moved into _boundedSiteIDs(), which both the id +// list and the SQL fragment now go through -- that sharing is the property +// worth pinning, not which function the `return null` lines sit in. Three +// now: the two in the shared ladder, and the one pass-through in each of the +// public entry points. +// Stated once, in the shared ladder -- asserted by where the two deciding +// terms LIVE rather than by counting `return null`, which the memo turned +// into a ternary and which a later refactor will move again. What must not +// happen is either entry point growing its own copy: two answers to "is this +// user bounded?" is how the API and the pages come to disagree. +$ladder = methodBody($site, '_boundedSiteIDs'); check( - 'scopedObjectIDs() returns null only for an unbounded class or user', - 2 === preg_match_all('/\n\s*return null;/', $site, $m2), + 'the shared ladder holds the scoped-class list', + false !== strpos($ladder, "array('host', 'group')"), + $failures, + $checks +); +check( + 'the shared ladder holds the restriction test', + false !== strpos($ladder, 'self::userIsRestricted('), + $failures, + $checks +); +foreach (['scopedObjectIDs', 'scopedObjectWhere'] as $fn) { + $body = methodBody($site, $fn); + check( + "$fn() does not restate the class list", + false === strpos($body, "array('host', 'group')"), + $failures, + $checks + ); + check( + "$fn() does not restate the restriction test", + false === strpos($body, 'self::userIsRestricted('), + $failures, + $checks + ); +} +// The memo is on the private ladder only. userIsRestricted() and +// userSiteIDs() are public and the management pages call them on requests +// that have just written the rows they read, so a memo there would serve a +// stale answer to the page that changed it. +foreach (['userIsRestricted', 'userSiteIDs'] as $fn) { + check( + "$fn() is not memoized", + false === strpos(methodBody($site, $fn), '_boundedSites'), + $failures, + $checks + ); +} +// A plain membership lookup must not join Host: find() would walk Host's +// MACAddressAssociation relationship, whose array('primary' => 1) filter +// becomes `hmPrimary` = '1' in the WHERE and drops every host without a +// primary MAC. It under-returned for years and made the two boundaries +// disagree about which hosts were in a site. +check( + 'hostIDsForSites() reads the association table without joining Host', + // 'self::' matters: the comment above the code names the function it + // no longer calls, and a bare grep for the name reads its own prose. + false === strpos(methodBody($site, 'hostIDsForSites'), 'self::getSubObjectIDs(') + && false !== strpos(methodBody($site, 'hostIDsForSites'), '`siteHostAssoc`'), + $failures, + $checks +); +check( + 'hostIDsForSites() interpolates only intval-mapped site ids', + (bool)preg_match( + "/array_map\('intval', array_values\(\(array\)\\\$siteIDs\)\)/", + methodBody($site, 'hostIDsForSites') + ), + $failures, + $checks +); +foreach (['scopedObjectIDs', 'scopedObjectWhere'] as $fn) { + check( + "$fn() gets its boundary from the shared ladder", + false !== strpos(methodBody($site, $fn), 'self::_boundedSiteIDs('), + $failures, + $checks + ); + check( + "$fn() passes an unbounded answer straight through as null", + (bool)preg_match( + '/if \(null === \$siteIDs\) \{\s*return null;/', + methodBody($site, $fn) + ), + $failures, + $checks + ); +} +// Deny-all in the SQL shape is a fragment that says so, never an empty +// string -- Route::_scopeWhere() reads '' as "nobody answered" and would +// fall through to an unbounded read for exactly the user entitled to none. +check( + 'scopedObjectWhere() says deny-all in SQL rather than returning empty', + (bool)preg_match( + "/count\\(\\\$siteIDs\\) < 1\\) \\{\\s*return '1=0';/", + methodBody($site, 'scopedObjectWhere') + ), + $failures, + $checks +); +// The fragment is inlined into a statement, so anything interpolated into it +// has to be an int by construction. There is no request path to $userID or +// the site ids, and this is what keeps it that way. +check( + 'scopedObjectWhere() interpolates only intval-mapped site ids', + false !== strpos( + methodBody($site, 'scopedObjectWhere'), + "array_map('intval', array_values(\$siteIDs))" + ), $failures, $checks );