Make plugins installable over the API - #1360
darksidemilk wants to merge 2 commits into
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.
darksidemilk
changed the title
Make plugins installable over the API (fixes #1360)
|
Superseded by #1363, now merged to I ran it against a server before merging, which was the outstanding bit. Four things came out of that: 1. The blocker gate was defeated by an unrelated key in the request body. Fixed in 2. 3. The error arms sent a bare string in a body labelled 4. Leaving Also tagged the operation |
2 participants
Makes plugins installable over the API, and stops the API being able to claim an install that never happened. Those are the same problem from two sides, so they are one PR.
The problem
Install was reachable only as a management-page POST behind a session and CSRF. The REST API exposed the plugin row as a generic entity, so a client could write
installedandschemadirectly — the two columns that record what the installer did.PluginManagementPage::installPost()shows why that ordering matters: it setsstate, then callsPlugin::installdb()to create the tables, and writesinstalled = 1last.Measured against a real server, on the bundled
locationplugin:Setting the column registers the plugin's classes — enough for its routes and schemas to appear in the document — while
schemastays0and no table is ever created. The server's own OpenAPI document then advertises routes that cannot answer, and a client generated from it gets commands guaranteed to fail.It also hides from the obvious repair:
installPost()filters oninstalled IN ('', 0, '0'), so the Install button skips a row that already claims to be installed.1.
POST /plugin/{id}/installThe same three steps as
installPost(), in the same order — activate,installdb(), theninstalled = 1last. 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) andSchema::applyUpdates()resumes from the stored count, so calling it on an installed plugin applies only steps it has not seen — which is what the UI calls Upgrade. One route therefore installs, and brings a plugin whose code ships newerschema()steps up to date, without a drop and recreate.That also makes it the repair for a row broken the way the one above was — which the Install button structurally cannot fix.
Permission is
plugin.install, the node the web Install button checks, rather thanplugin.edit: running a plugin's schema migrations is a different authority from editing its row. Declared inAPI_ROUTE_PERMISSIONSrather than relying on the fail-closed default inresolveApiPermission().2.
plugins.installedandplugins.schemabecome server-ownedOne entry in
Route::$serverOwnedFields. That list 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.phpreads the same list, so the document marks bothreadOnlywith the standard server-owned description.stateis deliberately not included. Activating is a column write and nothing else —installPost()and the Activate action both just setstate = 1— so a client writing it does the whole job rather than half of it. Activation over the API keeps working exactly as before.Verification
Regenerated the document with
spec/tools/dump-openapi.php:373 paths, up from 372.
php -lclean on all three files; added lines within the 80-column style.Not yet exercised at runtime. I have not run
POST /plugin/{id}/installagainst a server built from this branch — the reproduction above was done against a stock server before the fix. The refusal half rides on the pre-existing_refuseServerOwned()path, unchanged here and already exercised by thetask/host/userentries; the install handler is new code and deserves a real run. Happy to do that once it is deployed somewhere, or tell me if you would rather it come with a test.