Make plugins installable over the API (fixes #1360) - #1363
Merged
Merged
Conversation
Both record what the installer DID, and the installer writes them only
after it succeeded. PluginManagementPage::installPost() sets state, then
calls Plugin::installdb() to create the tables, and writes installed=1
last. Plugin::installdb() writes schema to the number of migration steps
it applied.
The generic edit route accepted both, so a client could assert an install
that never happened. Measured against a real server, on the bundled
location plugin:
PUT /plugin/2/edit {"installed":"1","state":"1"} -> 200
GET system/openapi 58 schemas -> 60
Location and Locationassociation now present
GET /location 406 SQLSTATE[42S02]: Base table or view not found
GET /location/count 406 SQLSTATE[42S02]: ...
GET /locationassociation
406 SQLSTATE[42S02]: ...
So the server's own OpenAPI document advertised routes that could not
answer, and a client generated from it gets commands guaranteed to fail.
That is worse than the plugin being absent: absent is discoverable.
It also hid from the obvious repair. installPost() only calls installdb()
for plugins filtered on installed IN ('', 0, '0'), so the Install button
SKIPS a row that already claims to be installed. upgradePost() filters on
installed = 1 and does call installdb(), so Upgrade is what fixes such a
row -- which is not the button anyone reaches for, and nothing says so.
Uses the existing mechanism rather than a new one: Route::$serverOwnedFields
already drives _refuseServerOwned(), which answers 400 only when the value
would CHANGE, so reading a plugin and sending the object back unmodified
still works. openapi.class.php reads the same list, so the document now
marks both properties readOnly with the standard server-owned description.
state is deliberately NOT included. Activating a plugin is a column write
and nothing else -- installPost() and the Activate action both just set
state=1 -- so a client writing it does the whole job rather than half of
it, and that stays available over the API.
This closes the hole rather than adding a feature. Installing over REST
needs its own explicit route that calls installdb(), because creating
tables should not be a side effect of a generic PUT. Worth doing, and a
separate change.
Makes plugins installable over the API, which they were not: install was
reachable only as a management-page POST behind a session and CSRF, so a
client could set the columns but never create the tables.
The handler performs the same three steps as
PluginManagementPage::installPost(), in the same order, because the order
is the contract: activate, then Plugin::installdb() to apply the plugin's
schema migrations, then installed=1 LAST so the column only ever claims
an install that actually happened. Activation blockers are refused first,
the same gate the page applies, so the API is not a way around a plugin
the server has a reason to refuse.
installdb() is called unconditionally rather than only when the plugin is
not yet installed. Migration steps are append-only and idempotent by
contract (docs/PLUGIN_SCHEMA_MIGRATIONS.md) and Schema::applyUpdates()
resumes from the stored count, so calling it on an installed plugin
applies only steps it has not seen. One route therefore installs, and also
brings a plugin whose code ships newer schema() steps up to date -- what
the UI calls Upgrade -- without a drop and recreate.
That also makes it the repair for a row whose installed flag was set
without tables ever being created. The web Install action cannot do it:
installPost() filters on installed IN ('', 0, '0'), so it skips a plugin
that already claims to be installed.
Permission is plugin.install, the node the web Install button checks,
rather than plugin.edit: running a plugin's schema migrations is a
different authority from editing its row. Declared in
API_ROUTE_PERMISSIONS, so the fail-closed default in
resolveApiPermission() is not relied on.
Documented in openapi.class.php as a fixed route rather than left to the
generic shapes, for the same reason the upload routes are: it is an
action, not CRUD on a row.
Verified by regenerating the document:
/plugin/{id}/install operationId=pluginInstall
responses 204/400/401/403/404/500
x-fog-permission=plugin.install
Plugin.installed readOnly=True Plugin.schema readOnly=True
Plugin.state readOnly=None (activation stays a plain column write)
php -l clean on all three files; added lines within the 80-column style.
$inputoverride is documented as "override php://input to blank", and it
suppressed exactly one of the two places listem() reads php://input. The
other is getsearchbody(), which turns any of the class's own fields found
in the request body into a WHERE clause -- so every internal
Route::getList() call was silently filtered by the body of whichever
request it happened to be running inside.
That is a gate bypass, not an inefficiency. Plugin::activationBlockers()
lists `plugin` through getList() to decide whether a plugin may be
switched on, and it treats "the target id is not in the list" as "nothing
blocks it". Measured against a server built from this branch, on a plugin
whose manifest declares fog_min 9.9.9:
POST /plugin/2/install -> 400 needs FOG 9.9.9
POST /plugin/2/install {"name":"ldap"} -> 204, installed
One unrelated key in a body the route does not even read was enough to
list a single row, miss the target, report no blockers and install a
plugin this server refuses to run. Any gate built on a getList() is
defeatable the same way, which is why this is fixed in listem() rather
than in activationBlockers().
Route::getList() is the only caller in the tree that passes
$inputoverride = true, so the change reaches internal list reads and
nothing else. The routes that legitimately take a JSON search body --
/{class}/list, /count, /names, /ids -- all reach listem() with the
default false and are unaffected; verified against the same server.
Co-Authored-By: Claude <noreply@anthropic.com>
Three things the route needed before it does what it documents. All three
were measured against a server built from this branch, over HTTP, against
a clone of a real 1.6 database -- the run the original commit said it had
not done.
1. Every error arm answered with sendResponse(), which writes a bare
string into a body labelled application/json. The document declares
400, 404 and 500 as the Error schema, so a generated client parsed
`Plugin not found` as JSON and got a syntax error instead of the
reason. setErrorMessage() emits {"error": ...}, which is what the
document already promised.
2. installdb() is idempotent only for a manager that adopted schema().
Without one it falls back to the legacy install(), whose 1.5 shape
opens with uninstall() -- a DROP -- and rebuilds; wolbroadcast
destroyed its own table on every install that way until
fog-plugins#24, and two bundled plugins still ship no schema(). The
web Install button never met this because it filters on installed IN
('', 0, '0'); this route deliberately does not filter, so "install an
installed plugin" silently meant "empty it". A second install of such
a plugin now answers 400 and says so. A first install still works --
there is nothing to lose -- and a hooks-only plugin with no manager at
all has neither method and re-installs as the no-op it should.
3. The blocker gate was one PUT away from being irrelevant. state is
deliberately left writable so activating over the API keeps working,
but nothing gated it, so a plugin the server refuses to run -- an
installed one left deactivated because a FOG upgrade moved past its
fog_max -- could be switched back on with {"state":"1"} and would load
on every boot. edit() now applies the same Plugin::activationBlockers()
check the page and the install route apply, on the transition to
active only: reading a plugin and PUTting it back unchanged, and
editing a description on an already-active plugin, both still work.
Also tags the operation `plugin` rather than `system`. _op() takes its tag
from the class argument, which has to stay empty for a fixed route so the
operation id and the permission lookup key off the route name -- the two
upload routes already override the tag afterwards for the same reason.
Verified: install creates the tables and writes installed last; re-install
of a schema() plugin is a no-op upgrade; 404, blocked, and legacy-refusal
all answer {"error": ...}; plugin.edit gets 403 on the route and
plugin.install gets 204; the JSON search body on /list, /count, /names and
/ids is unchanged. tests/run-all.sh: 149 passed, 0 failed.
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
2 participants
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.
Supersedes #1360. Carries @darksidemilk's two commits unchanged, then fixes
what a real run against a server found. The original said it had not been
exercised at runtime; it has now.
How it was verified
A clone of a live 1.6 database in a throwaway MariaDB container, the branch's
own
packages/webserved over HTTP, driven with realfog-api-token/fog-user-tokenheaders. Nothing was written to a live install.What was wrong
1.
Route::listem()leaked the request body into every internal list read —and that defeated the blocker gate this PR adds.
$inputoverrideis documented as "override php://input to blank" andsuppressed exactly one of the two places
listem()reads it. The other isgetsearchbody(), which turns any of the class's own fields found in the bodyinto a
WHERE.Plugin::activationBlockers()listspluginthroughRoute::getList()and treats "the target id is not in the list" as "nothingblocks it", so on a plugin whose manifest declares
fog_min 9.9.9:One unrelated key in a body the route does not read was enough to walk past
the gate. Fixed in
listem(), not inactivationBlockers(), because any gatebuilt on a
getList()was defeatable the same way.Route::getList()is theonly caller in the tree that passes
$inputoverride = true, so the changereaches internal list reads and nothing else —
/list,/count,/namesand/idsstill honour a JSON search body, verified.2. Every error arm answered with a bare string in a body labelled
application/json. The document declares 400/404/500 as theErrorschema,so a generated client parsed
Plugin not foundand got a syntax error insteadof the reason. Now
setErrorMessage(), i.e.{"error": ...}.3.
installdb()is idempotent only for a manager that adoptedschema().Without one it falls back to the legacy
install(), whose 1.5 shape opens withuninstall()— a DROP — and rebuilds.wolbroadcastdestroyed its own tableon every install that way until FOGProject/fog-plugins#24, and
taskstateeditand
tasktypeeditstill ship noschema(). The web Install button never metthis because it filters on
installed IN ('', 0, '0'); this route deliberatelydoes not filter, so "install an installed plugin" silently meant "empty it". A
second install of such a plugin now answers 400 and says why. A first install
still works — there is nothing to lose — and a hooks-only plugin with no manager
re-installs as the no-op it should.
4. The blocker gate was one PUT away from being irrelevant.
stateisdeliberately left writable so activating over the API keeps working, but
nothing gated it — so an installed plugin left deactivated because a FOG upgrade
moved past its
fog_maxcould be switched back on with{"state":"1"}andwould load on every boot (
getActivePlugins()selectsinstalled=1 AND state=1).edit()now applies the samePlugin::activationBlockers()check,on the transition to active only: an unchanged round-trip PUT, and a
description edit on an already-active plugin, both still work.
5. The operation was tagged
system._op()takes its tag from the classargument, which has to stay empty for a fixed route so the operation id and the
permission lookup key off the route name — the two upload routes already
override the tag afterwards for exactly this. Now
plugin.Behaviour matrix, all measured
POST /plugin/99999/install{"error":"Plugin not found"}{"error":"capone needs FOG 9.9.9 or newer; this server is 1.6.0"}PUT /plugin/2/edit {"state":"1"}on a blocked pluginPUTwritinginstalled/schema... is maintained by the serverPUTunchanged round-tripschema()plugininstalledwritten lastschema()plugin... does not declare schema() migrationsplugin.editonlyplugin.installtests/run-all.sh: 149 passed, 0 failed.Downstream
No change to
Route::$validClasses, so FogApi's hardcoded class list(
Private/Get-DynmicParam.ps1) needs no sync.POST /plugin/{id}/installis anew operation FogApi has no cmdlet for.
Still open, deliberately not in this PR
handleWhereItems()also picks up the dispatched route's?filter=when$whereItemsisfalse, so a nested internalgetList()inside a filterableroute can consume the filter the handler was going to use. Not reachable here —
captureRequestFilter()only arms for filterable route names, andpluginInstallis not one, verified — but it is the same shape as the bugfixed above.