Push the API object boundary into the query, keeping the id list as fallback - #1233
Merged
Conversation
…allback The site boundary reached the API in #1229 by asking the plugin which object ids the acting user may see, then narrowing with that list. It is correct and it does not scale: answering "which hosts may this user see" means reading every one of them into PHP on every request, and then either splicing thousands of ids into an IN list or comparing each row against them. A new event, API_SCOPE_WHERE, asks for the same boundary as a SQL fragment instead. It is tried FIRST and API_SCOPE_IDS is the fallback, so a third-party plugin that knows only the old event keeps bounding reads exactly as it did -- both handlers stay registered in the site plugin and only one is ever applied to a request. Two tri-states, and they are not the same one. The id list uses null for "no boundary" and an empty array for "you may see nothing". The fragment has no empty state at all: '' is read as silence and falls through, because an empty fragment is indistinguishable from no answer and would otherwise compile to `WHERE ()`. Deny-all is therefore said in SQL, as '1=0'. Where each applies: names(), ids() Route builds this SQL itself, so the fragment is ANDed onto whatever _buildWhere() produced. listem(), These go through the manager, which had no seam. find() search() and search() take a $scopeWhere argument now; it is ANDed on LAST with the caller's own terms parenthesised, because $whereOperator is a parameter and 'OR' is a value it takes. An OR that can reach outside the boundary is not a boundary. dispatch gate _requireObjectScope() asks whether one row satisfies the same expression -- bounded, and it cannot disagree with what the list showed. Also fixed, found while proving parity and in the same call path: hostIDsForSites() went through SiteHostAssociation's manager, which walks class relationships to build joins and so pulled in Host's MACAddressAssociation relationship. That carries array('primary' => 1), which buildQuery() emits as `hostMAC`.`hmPrimary` = '1' in the WHERE -- turning a LEFT OUTER JOIN into an inner one and silently DROPPING every host with no primary MAC. 95 of 1000 in the lab. It under-returned, so nobody saw anything they should not, but a site-restricted user could not see hosts in their own site, and it made the two boundaries disagree about which hosts a site holds. A membership lookup has no business joining Host; it reads the association table directly now. This also affects the management pages, which share the function: restricted users will now see hosts that were wrongly hidden from them. _boundedSiteIDs() is memoized per user per request. Without it an UNRESTRICTED user pays for the restriction lookup twice on every read -- once for the fragment that declines, once for the id list that declines. Measured 2 -> 3 statements for an administrator before the memo, 2 after. Deliberately not memoized on the public userIsRestricted()/userSiteIDs(), which the management pages call on requests that have just written the rows they read. Verified against the 1.5 lab database, 2079 hosts, a site holding 1000, with the real site plugin: id list id list fragment as merged + join fix (this) names host 95 1000 1000 ids host 95 1000 1000 list host 95 95 95 search host 53 53 53 names group 2 2 2 restricted, no site 0 0 0 logged out (daemon) 2079 2079 2079 administrator 126/2079 126/2079 126/2079 queries, restricted 4 4 3 The fixed id list and the fragment agree on every line, which is the parity that was asked for; the unfixed column is the bug above. Characterization tests were written FIRST, against the id-list implementation, and pass unedited after the port -- 65 checks over three states (no boundary, scoped, deny-all) times four read routes times two classes, plus the dispatch gate out of process because a denial ends the response by exiting. They assert which objects come back, never the SQL that produced them, so they hold across either implementation. The database arm skips rather than degrading into a string match where no 1.5 schema is reachable. Every gate is mutation-verified. Two bugs were caught that way and would not have been caught otherwise: the gate read PDODB's `false` for "no rows" through count((array)$rows), which is 1, so it allowed every object it was built to refuse; and the fragment was dropped whenever a caller had supplied a filter of their own, because only the empty arm of the append helper was being driven. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #1229, which brought the site boundary to the API by asking the plugin
which object ids the acting user may see and narrowing with that list. Correct,
but it answers "which hosts may this user see" by reading every one of them into
PHP on every request.
What changes
A new event,
API_SCOPE_WHERE, asks for the same boundary as a SQL fragment.It is tried first;
API_SCOPE_IDSis the fallback. Both handlers stayregistered in the site plugin and only one is ever applied to a request, so a
third-party plugin that knows only the old event keeps working untouched.
Two tri-states, and they are not the same one.
API_SCOPE_IDSnullarray(1,2,3)array()API_SCOPE_WHEREnull'<sql>''1=0'The fragment has no empty-string state.
''is read as silence and fallsthrough — an empty fragment is indistinguishable from no answer, and would
otherwise compile to
WHERE (). Deny-all has to be said in SQL.Where each applies
names(),ids()_buildWhere()producedlistem(),search()find()/search()take a$scopeWhereargument now_requireObjectScope()asks whether one row satisfies the same expression the lists narrow withThe manager seam ANDs the fragment on last, with the caller's own terms
parenthesised and joined by a literal
AND, never through$whereOperator—that parameter takes
'OR', and an OR that can reach outside the boundary is nota boundary.
Also fixed — same call path, found while proving parity
hostIDsForSites()went throughSiteHostAssociation's manager.find()walksclass relationships to build joins, so it pulled in
Host'sMACAddressAssociationrelationship, which carriesarray('primary' => 1).buildQuery()emits that as`hostMAC`.`hmPrimary` = '1'in the WHERE,turning the LEFT OUTER JOIN into an inner one and silently dropping every host
with no primary MAC — 95 of 1000 in the lab.
It under-returned, so nobody saw anything they should not; but a site-restricted
user could not see hosts in their own site, and the two boundaries disagreed
about which hosts a site holds. A membership lookup has no business joining
Host; it reads the association table directly now.restricted users will now see hosts that were wrongly hidden from them.
Verified against the 1.5 lab, 2079 hosts, a site holding 1000, real site plugin
names hostids hostlist hostsearch hostnames groupThe fixed id list and the fragment agree on every line — that is the parity
this was asked for. The first column is the bug above.
_boundedSiteIDs()is memoized per user per request: without it an unrestricteduser paid for the restriction lookup twice on every read (2 → 3 statements for an
administrator), once for the fragment that declines and once for the id list that
declines. Deliberately not memoized on the public
userIsRestricted()/userSiteIDs(), which the management pages call on requests that have justwritten the rows they read.
Tests
Characterization tests were written first, against the id-list implementation,
and pass unedited after the port. 65 checks: three states (no boundary, scoped,
deny-all) × four read routes × two classes, plus the dispatch gate out of process
because a denial ends the response by exiting. They assert which objects come
back, never the SQL that produced them, so they hold across either
implementation. The database arm skips rather than degrading into a string match
where no 1.5 schema is reachable.
Every gate is mutation-verified. Two bugs were caught that way and by nothing
else:
falsefor "no rows" throughcount((array)$rows), which is 1 — so it allowed every object it was builtto refuse;
because only the empty arm of the append helper was being driven.
Suite: 17 files, 0 failures.
Not in scope
The
hmPrimaryfilter leaking out ofHost's relationship into anything thatjoins through Host is a wider problem than this call path. Fixed here only where
the boundary depends on it.
Downstream
No route class list change, so FogApi's hardcoded
-coreObjectlist is unaffected.🤖 Generated with Claude Code
https://claude.ai/code/session_01GN7hADN5QLzoxXWDA6xeUk