diff --git a/AGENTS.md b/AGENTS.md index 653b680..05ae3c2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,7 +200,7 @@ that drives the whole of it against a real WordPress is `tests/unit/Scenario/`. | `src/Registry/` | `Registrar` (holds registered `Sub_Plugin` objects), `Reader` (the registration buffer, drained into the registrar on the way past; the object every pass reads the registry through), `Contracts\Registrar_Interface`. | | `src/Activator.php` | Runs a sub-plugin's activation callback once ever, recorded in one option. | | `src/Conflict/` | `Detector` (whether a standalone is in the way), `Resolver` (which policy branch to take), `Gatekeeper` (which requests, and which users, may have one resolved), `Redirector` (where the user lands afterwards), `Rewriter` (rewrites the activation-error screen for a registered standalone), `Contracts\Resolver_Interface`. | -| `src/Traits/` | `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing). Cross-cutting only: a trait used by one folder lives in that folder. | +| `src/Traits/` | `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing), `Guards_Plugin_Capability` (which capability a plugin act asks for, shared by the conflict gate and the notice queue). Cross-cutting only: a trait used by one folder lives in that folder. | | `src/Notices/` | `Writer` (what a notice says, stored under `slug:type` — `merge`, `conflict`, `stranding`, `dependency`), `Presenter` (who may consume it, render-then-clear), `Store` (keeps it), `Renderer` (draws it, `notice-error` for `dependency` and `notice-warning` for the rest), `Contracts\Writer_Interface`. | | `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Activator_Interface`, `Config_Exception`. | diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index fd284ea..1f1e00b 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -56,8 +56,9 @@ multisite and `activate_plugins` otherwise, since deactivating a standalone is n a network exists. The check matters because `plugins_loaded` fires well before `auth_redirect()`, so an unauthenticated GET of an admin URL reaches this code on its way to the login screen. -These gates apply whatever the policy is — the non-destructive policies only queue a notice, and a -notice is neither shown nor cleared for a user without the same capability. +These gates apply whatever the policy is — the non-destructive policies only queue a notice, and the +[notice queue](notices.md#who-sees-them) asks for that same capability by name before it shows a +notice or clears one, so nothing a policy queued is consumed by someone the gate would have refused. The deactivation itself is silent, and covers both scopes on multisite. Silent because the standalone's own deactivation hook would otherwise run this early: a routine `flush_rewrite_rules()` diff --git a/docs/notices.md b/docs/notices.md index 348bd62..73ec670 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -12,16 +12,21 @@ that is raised exactly once and never re-queued. ## Who sees them -Rendering prints the queue and then clears it, gated on the `activate_plugins` capability. Since -rendering consumes the queue, a user who cannot act on a notice must not be shown one — a -subscriber loading their profile page would otherwise silently swallow the only warning an -administrator was ever going to get. - -On multisite that is usually a network administrator rather than the site administrator who -installed the plugin: core maps `activate_plugins` through `manage_network_plugins` unless the -network has enabled the plugins menu for individual sites. Conflict resolution does not rely on that -mapping and asks for `manage_network_plugins` by name — see -[conflict handling](conflict-handling.md#when-resolution-runs). +Rendering prints the queue and then clears it, gated on the capability +[conflict resolution](conflict-handling.md#when-resolution-runs) asks for: `manage_network_plugins` +on multisite and `activate_plugins` otherwise. Since rendering consumes the queue, a user who cannot +act on a notice must not be shown one — a subscriber loading their profile page would otherwise +silently swallow the only warning an administrator was ever going to get. + +On multisite that means a network administrator rather than the site administrator who installed the +plugin, and the network capability is asked for by name rather than left to core's mapping of +`activate_plugins`, which only widens into it while the network keeps the Plugins menu off for +individual sites. On a network that has turned that menu on, every site administrator holds +`activate_plugins` outright: any one of them opening any admin screen would otherwise print a notice +raised for the network and clear it network-wide, and the queue is one option shared by every site, +so it would be gone for everyone else. That covers the dependency notice too — a site administrator +no longer consumes one, which is no loss, since a queue shared by the whole network was never theirs +alone to consume. ## One message, two places @@ -56,8 +61,8 @@ use Nexcess\PluginAbsorber\Absorber; add_action( 'admin_init', function () { // Gates the read, not just the delete: `admin_init` fires for every logged-in user, and // draining the queue for one who cannot act on it destroys the only warning an - // administrator was going to get. - if ( ! current_user_can( 'activate_plugins' ) ) { + // administrator was going to get. The same capability the built-in rendering asks for. + if ( ! current_user_can( is_multisite() ? 'manage_network_plugins' : 'activate_plugins' ) ) { return; } diff --git a/src/Conflict/Gatekeeper.php b/src/Conflict/Gatekeeper.php index d974a6f..9701acb 100644 --- a/src/Conflict/Gatekeeper.php +++ b/src/Conflict/Gatekeeper.php @@ -8,6 +8,7 @@ namespace Nexcess\PluginAbsorber\Conflict; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; +use Nexcess\PluginAbsorber\Traits\Guards_Plugin_Capability; /** * Whether this request may have a conflict resolved at all. @@ -31,6 +32,7 @@ */ class Gatekeeper { use Guards_Hook_Prefix; + use Guards_Plugin_Capability; /** * Admin scripts that exist only to perform work. @@ -103,22 +105,18 @@ public function request_may_resolve(): bool { * unauthenticated GET of any admin URL gets this far. Without this check a stranger could turn * the standalone off site-wide by requesting a page they are about to be bounced off. * - * The capability asked for matches what resolution can do, which is why the two differ. The - * deactivation is network-wide: Deactivator leaves deactivate_plugins()'s $network_wide - * at its default, and core reads that as both scopes, so the standalone comes out of the - * network's active plugins whichever site the request arrived on. That is authority a single - * site's administrator does not hold, and asking for activate_plugins would not establish it -- - * core only widens that capability into the network one while a network setting says to, so on a - * network that has said otherwise every subsite administrator would pass. Where a network exists, - * the network-scoped capability is the one whose reach matches the action's. + * Which capability that is belongs to Traits\Guards_Plugin_Capability, because the notice + * presenter has to ask the identical question: rendering the queue clears it for everybody, so a + * user who may not have a conflict resolved may not consume the report of one either. Spelling + * the capability here as well is what let the two answers drift apart. * * Here rather than inside the default resolver, because it is the one thing about conflict * resolution that must survive a host binding its own: whoever cannot activate a plugin must not * be able to deactivate one, and a replacement that forgot to re-check would reopen exactly that. * * It gates every policy, not only the destructive one, and that costs nothing. The other - * policies queue a notice, and Notices\Presenter::render() will not render -- or clear -- for a user - * with no plugin capability at all. Queuing on a request that cannot act only parks the notice + * policies queue a notice, and Notices\Presenter::render() will not render -- or clear -- for a + * user this same guard turns away. Queuing on a request that cannot act only parks the notice * until an administrator who can act arrives, which is the request this gate lets resolution run * on anyway. Nothing is consumed or suppressed by waiting: the standalone is still there to * detect. @@ -128,7 +126,7 @@ public function request_may_resolve(): bool { * @return bool */ public function user_may_resolve(): bool { - return current_user_can( is_multisite() ? 'manage_network_plugins' : 'activate_plugins' ); + return self::user_may_manage_plugins(); } /** diff --git a/src/Notices/Presenter.php b/src/Notices/Presenter.php index a4807ed..977ad1f 100644 --- a/src/Notices/Presenter.php +++ b/src/Notices/Presenter.php @@ -8,6 +8,7 @@ namespace Nexcess\PluginAbsorber\Notices; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; +use Nexcess\PluginAbsorber\Traits\Guards_Plugin_Capability; /** * Who may consume the queue, and what happens when they do. @@ -18,7 +19,11 @@ * * The capability check lives here rather than in `Renderer` because it guards the clearing as much as * the drawing: the two have to be decided together, or a user who may not see the queue could still - * destroy it. + * destroy it. Which capability is `Traits\Guards_Plugin_Capability`'s answer, shared with the gate + * that decides who may have a conflict resolved at all — the conflict notices report what that gate + * let happen, so consuming one is the same authority as causing it. The dependency notice is queued + * off the load path, behind no gate at all, and is held to the same capability for the reason above: + * whoever consumes the queue consumes all of it. * * The queue is single-consumer. Rendering consumes it for everybody, so the first eligible * administrator to load any admin screen is the only person who ever sees a given notice — @@ -32,18 +37,7 @@ * @since 1.0.0 */ class Presenter { - /** - * Capability required to see, and thereby consume, the queue. - * - * Rendering clears the queue, so a user who cannot act on a notice must not be shown one: - * a subscriber loading their profile page would otherwise silently swallow the only warning - * an administrator was ever going to get. - * - * @since 1.0.0 - * - * @var string - */ - private const CAPABILITY = 'activate_plugins'; + use Guards_Plugin_Capability; /** * @since 1.0.0 @@ -84,7 +78,11 @@ public function __construct( Store $store, Renderer $renderer ) { * @return void */ public function render(): void { - if ( ! current_user_can( self::CAPABILITY ) ) { + // Rendering clears the queue, so a user who cannot act on a notice must not be shown one: a + // subscriber loading their profile page would otherwise silently swallow the only warning an + // administrator was ever going to get, and on multisite the queue they swallowed it out of is + // shared by every site on the network. + if ( ! self::user_may_manage_plugins() ) { return; } diff --git a/src/Traits/Guards_Plugin_Capability.php b/src/Traits/Guards_Plugin_Capability.php new file mode 100644 index 0000000..695918d --- /dev/null +++ b/src/Traits/Guards_Plugin_Capability.php @@ -0,0 +1,48 @@ +become_plugin_administrator(); // someone who may resolve a conflict $this->create_user( 'subscriber' ); // someone who may not ``` -`become_plugin_administrator()` is not just `create_user( 'administrator' )`. On -multisite `activate_plugins` maps through `manage_network_plugins`, which a site -administrator does not have — so it grants super admin there and sets the current -user either way. A test about *that* difference creates the administrator itself. +`become_plugin_administrator()` is not just `create_user( 'administrator' )`. Both +gates name `manage_network_plugins` on multisite, which a site administrator does +not have — so it grants super admin there and sets the current user either way. A +test about *that* difference creates the administrator itself. ## Stubbing functions @@ -353,7 +354,7 @@ None of it means anything unless all four hold, and setUp establishes all four: `set_request_method( 'GET' )`. `Conflict\Gatekeeper` turns away anything else, so without both of these every policy scenario would pass while resolving nothing at all. -- **A user who can `activate_plugins`** — `become_plugin_administrator()`. The +- **A user who may manage plugins** — `become_plugin_administrator()`. The gatekeeper checks the capability before anything is resolved, and the queue checks the same one before it renders, so as nobody the suite would be asserting that a no-op is a no-op. diff --git a/tests/unit/Notices/PresenterTest.php b/tests/unit/Notices/PresenterTest.php index 4bb9c2d..b5617f2 100644 --- a/tests/unit/Notices/PresenterTest.php +++ b/tests/unit/Notices/PresenterTest.php @@ -34,6 +34,18 @@ class PresenterTest extends WPTestCase { private const OPTION = 'give_plugin_absorber_notices'; + /** + * Whatever the network had under `menu_items` before a test opened the Plugins menu. + * + * @var mixed + */ + private $menu_items = false; + + /** + * @var bool + */ + private $menu_items_changed = false; + public function setUp(): void { parent::setUp(); @@ -41,13 +53,18 @@ public function setUp(): void { Config::set_hook_prefix( 'give' ); $this->clear_queue(); - // render() consumes the queue, so it is gated on a capability. Most tests care about the - // queue rather than the gate, so they run as someone who has it — which on multisite is a - // network administrator, see test_a_site_administrator_on_multisite_cannot_consume_the_queue(). + // render() consumes the queue, so it is gated on a capability — the same one conflict + // resolution asks for, which on multisite is the network-scoped one. Most tests care about the + // queue rather than the gate, so they run as someone who holds it either way. $this->become_plugin_administrator(); } public function tearDown(): void { + // In tearDown rather than at the end of the test body: a failed assertion would otherwise + // leave the network's Plugins menu open for every test that runs after it, and the capability + // a site administrator holds is exactly what those tests turn on. + $this->close_the_network_plugins_menu(); + $this->clear_queue(); Config_State::reset(); parent::tearDown(); @@ -313,10 +330,9 @@ public static function users_who_cannot_activate_plugins(): Generator { } /** - * Surprising but intended: on multisite `activate_plugins` maps through - * `manage_network_plugins`, which only a super admin has unless the network has opened the - * plugins menu to site admins. So the person who installed the plugin on their own site is - * not the person who sees the notice — a network administrator is. + * Surprising but intended: on multisite the queue is one network option, and the notice a merge + * raises is raised exactly once and never re-queued. So the person who sees it is a network + * administrator, not the site administrator who installed the plugin on their own site. */ public function test_a_site_administrator_on_multisite_cannot_consume_the_queue(): void { if ( ! is_multisite() ) { @@ -334,6 +350,58 @@ public function test_a_site_administrator_on_multisite_cannot_consume_the_queue( $this->assertTrue( $this->queue_exists(), 'The queue must survive for the network administrator.' ); } + /** + * And the same site administrator still cannot, on a network that opened the Plugins menu. + * + * The case above passes on a mapping rather than on this gate: with the menu off, core folds + * `activate_plugins` into `manage_network_plugins` and a site administrator holds neither. Turn + * the menu on -- a setting the network administrator owns and one plenty of networks use -- and + * the fold stops, so a site administrator holds `activate_plugins` outright while holding no + * network capability at all. Asking for `activate_plugins` here would then admit every + * administrator of every site: one of them opens any admin screen, prints a merge notice raised + * by a super admin's deactivation, and clears it for the whole network, leaving the only person + * who could undo the deactivation never told it happened. + * + * Both halves are asserted, because the clearing is the damage. Nothing was rendered is a notice + * postponed; nothing survived is a notice destroyed. + */ + public function test_a_site_administrator_cannot_consume_the_queue_when_the_network_opens_the_plugins_menu(): void { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Outside multisite there is no network setting to open.' ); + } + + $this->open_the_network_plugins_menu(); + + $this->queue_notice( + 'queue_merge_notice', + [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] + ); + + wp_set_current_user( $this->create_user( 'administrator' ) ); + + // The capabilities are asserted rather than assumed: without this pair the test would pass on + // a network that never opened the menu, which is the case above and not this one. + $this->assertTrue( + current_user_can( 'activate_plugins' ), + 'An open Plugins menu has to leave a site administrator holding activate_plugins.' + ); + $this->assertFalse( + current_user_can( 'manage_network_plugins' ), + 'And holding it must not be the same as holding the network capability.' + ); + + $this->assertSame( '', $this->render_to_string( $this->make_presenter() ) ); + $this->assertTrue( $this->queue_exists(), 'The queue must survive the site administrator.' ); + + // The positive control, on the same open menu and the same queue: a presenter that refused + // everybody would satisfy the two assertions above for a reason that has nothing to do with + // which capability is asked for. + $this->become_plugin_administrator(); + + $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( $this->make_presenter() ) ); + $this->assertFalse( $this->queue_exists(), 'A network administrator still consumes it.' ); + } + /** * @dataProvider malformed_queues * @@ -391,6 +459,42 @@ public static function malformed_queues(): Generator { yield 'a whitespace-only message' => [ [ 'a:merge' => " \n\t" ], null, [ 'notice' ] ]; } + /** + * Open the per-site Plugins menu the way a network administrator does. + * + * The site option core reads in map_meta_cap(), written directly rather than through the Network + * Settings screen: what decides the capability is the stored value, and going through the screen + * would drag a form submission and a nonce into a test about who may consume a notice. + * + * @return void + */ + private function open_the_network_plugins_menu(): void { + $this->menu_items = get_site_option( 'menu_items', false ); + $this->menu_items_changed = true; + + update_site_option( 'menu_items', [ 'plugins' => 1 ] ); + } + + /** + * Put the network's menu settings back exactly as they were found. + * + * @return void + */ + private function close_the_network_plugins_menu(): void { + if ( ! $this->menu_items_changed ) { + return; + } + + if ( $this->menu_items === false ) { + delete_site_option( 'menu_items' ); + } else { + update_site_option( 'menu_items', $this->menu_items ); + } + + $this->menu_items = false; + $this->menu_items_changed = false; + } + /** * Queue a notice through a plain Writer over the default, option-backed Store — the same one * a Presenter built with no store of its own reads. diff --git a/tests/unit/Scenario/Bootstrap_Test_Case.php b/tests/unit/Scenario/Bootstrap_Test_Case.php index ec0c402..3d6f88e 100644 --- a/tests/unit/Scenario/Bootstrap_Test_Case.php +++ b/tests/unit/Scenario/Bootstrap_Test_Case.php @@ -144,9 +144,9 @@ public function setUp(): void { set_current_screen( 'plugins' ); $this->set_request_method( 'GET' ); - // Two capabilities, and one user who holds both: Conflict\Gatekeeper asks for - // manage_network_plugins on multisite and activate_plugins everywhere else, while - // Notices\Presenter asks for activate_plugins wherever it runs. + // One capability, asked twice: Conflict\Gatekeeper before a standalone is deactivated and + // Notices\Presenter before the queue reporting it is printed and cleared, both of them + // manage_network_plugins on multisite and activate_plugins everywhere else. $this->become_plugin_administrator(); // Where a resolved conflict sends the user is read off the current request rather than off the