Add missing nonce check to uninstall action #1431
Conversation
| // Setup Stream uninstall with full data purge. | ||
| add_action( | ||
| 'wp_ajax_wp_stream_uninstall', | ||
| array( $this, 'action_wp_stream_uninstall' ) |
There was a problem hiding this comment.
This admin controller is now responsible for handling the uninstall route including all the permission checks and the actual trigger. Previously it was bundled with the DB driver which had no insight into the nonce names, etc.
| add_action( 'wp_ajax_wp_stream_uninstall', array( $uninstall, 'uninstall' ) ); | ||
|
|
||
| return $uninstall; | ||
| $uninstall->uninstall(); |
There was a problem hiding this comment.
We expect that calling purge_storage() will actually trigger all the purge steps so we call the uninstall() logic here.
We also expected that whoever is calling the purge_storage() method has done all the necessary input validation and permission checks. This decouples the routing knowledge from the actual business logic of purging the data.
|
|
||
| $edd_options = get_option( 'edd_settings' ); | ||
|
|
||
| $current_user = new WP_User(1); |
There was a problem hiding this comment.
This was setting up the test environment as if an admin user had logged in. It should be the responsibility of each test to setup and tear-down the environment state as necessary.
| @@ -14,10 +14,6 @@ public function setUp() { | |||
| $this->mock->register(); | |||
| } | |||
|
|
|||
| public function tearDown() { | |||
There was a problem hiding this comment.
These are not needed since they call the parent with no additions.
| ); | ||
| wp_set_current_user( $administrator_id ); | ||
| public function test_logged_out_by_default() { | ||
| $this->assertFalse( is_user_logged_in(), 'Run tests as logged-out user by default' ); |
There was a problem hiding this comment.
Ensure that default tests run without having a logged-in user.
| @@ -13,18 +13,10 @@ public function setUp() { | |||
| parent::setUp(); | |||
|
|
|||
| $this->admin = $this->plugin->admin; | |||
| $this->assertNotEmpty( $this->admin ); | |||
There was a problem hiding this comment.
This is already being tested in test_construct() below.
| 'email' => '[email protected]', | ||
| ) | ||
| ); | ||
| wp_set_current_user( $administrator_id ); |
There was a problem hiding this comment.
Don't assume that we want to be logged-in for every test here.
| $this->_handleAjax( 'wp_stream_uninstall' ); | ||
| } | ||
|
|
||
| public function test_action_wp_stream_uninstall_not_available_to_non_admins() { |
There was a problem hiding this comment.
All the tests related to the stream_uninstall_nonce action are now part of the admin test suite since that is where the controller logic for this lives.
| * | ||
| * @return void | ||
| */ | ||
| private function allow_drop_table() { |
There was a problem hiding this comment.
Moved this into a local helper to allow calling it whenever needed.
Note that _drop_temporary_table filter is added back on new transaction start https://github.com/wp-phpunit/wp-phpunit/blob/411a5e68d54fedb1fa4f07ce4e3d9cad1d27f379/includes/abstract-testcase.php#L443-L449 so we don't need to do it.
There was a problem hiding this comment.
I've tested all possible scenarios with the plugin Uninstall on a network and a single site, and everything appears to be working.
@schlessera Could you please review my comments inline the pull request and the major issue I discovered with multisite uninstalls. Thanks!
| */ | ||
| if ( ! is_multisite() || $this->plugin->is_network_activated() ) { | ||
| if ( ! is_multisite() || ( $this->plugin->is_network_activated() && is_main_site() && is_super_admin() ) ) { |
There was a problem hiding this comment.
@schlessera This fixes a really important issue where the plugin being uninstalled from a blog on a multisite would delete ALL Stream data, if the Stream plugin was activated at the network level.
We now purge all plugin records and tables only:
- if the uninstall is happening on a single site, or
- if the uninstall is happening on the main site of the multisite and is triggered by the super admin.
There was a problem hiding this comment.
Hmm, doesn't that mean that it won't clean up anymore if you only installed it on a subsite?
There was a problem hiding this comment.
The Stream tables are shared between all sites on a multisite.
Currently the tables are completely removed only if:
- The plugin is "Uninstalled" from the main blog of the multisite and it was enabled at the network level.
- The plugin is "Uninstalled" from a non-multisite setup.
All other uninstalls simply delete the data associated with the particular blog_id. So in case of uninstalling the plugin from a blog on a network (when the plugin is not activated globally) it would preserve the tables and remove the data. Uninstalling it from the main blog will delete the tables only if the plugin was installed globally.
This changeset is attempting to prevent full table removal when a plugin is uninstalled from any single blog on the network when the plugin is activated at the network level.
There was a problem hiding this comment.
Hmm, reflecting on this a bit more, I think there are two concepts intermingled here.
What it currently does, is it removes all tables when single site or ( multisite & network-activated & main site & super-admin).
On multisite, what is currently being missed completely is the $network flag that gets passed to activate() and deactivate() handlers; it is just not being forwarded.
The condition where it would remove all tables on multisite would be when $network => true was passed to the deactivate() handler. The conditions you've added tried to get close to that case, but you could still just deactivate the plugin for the main site only and leave it active on all other sites.
So we'd need to forward the $network flag from the deactivation handler first before being able to correctly state the conditions here.
@kasparsd thoughts?
👉 Superseded by #1435.
Fixes #1426.
This PR includes the following changes:
'wp_ajax_wp_stream_uninstall'AJAX action.Checklist
contributing.md).