Skip to content

Plugin management endpoints#9

Merged
claudiulodro merged 3 commits into
masterfrom
add/plugin-endpoints
Apr 15, 2019
Merged

Plugin management endpoints#9
claudiulodro merged 3 commits into
masterfrom
add/plugin-endpoints

Conversation

@claudiulodro
Copy link
Copy Markdown
Contributor

@claudiulodro claudiulodro commented Mar 25, 2019

All Submissions:

Changes proposed in this Pull Request:

Closes #4.

This PR adds the following REST API endpoints for users with the correct capabilities:

  • GET newspack/v1/plugins
  • GET newspack/v1/plugins/{some-plugin}
  • POST newspack/v1/plugins/{some-plugin}/activate
  • POST newspack/v1/plugins/{some-plugin}/deactivate
  • POST newspack/v1/plugins/{some-plugin}/install
  • POST newspack/v1/plugins/{some-plugin}/uninstall

The GET endpoints retrieve info about plugins and their installation status. The POST endpoints can be used to activate/deactivate/etc. a plugin. See the unit tests for usage.

I've also added a minimal way to get information about the managed plugins, since that was required for the GET endpoints. See my comment below, and please let me know if there's a better way you know (loading from a config file?).

How to test the changes in this Pull Request:

  1. Run/examine unit tests.
  2. Examine source code.

Sorry about the length of this PR; the way endpoints are registered and handled is very verbose in WP. Most of the relevant logic is in the Plugins_Controller::{something}_item methods. It should all be straightforward and following the controller design pattern recommended for REST API endpoints.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@claudiulodro claudiulodro added the [Status] Needs Review The issue or pull request needs to be reviewed label Mar 25, 2019
'name' => __( 'AMP', 'newspack' ),
'download' => 'wporg',
],
];
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What sort of information do we need available about the plugins? I've kicked it off with the following info for each plugin:

name - The plugin name.
download - 'wporg' or a URL to a zip file.
status - 'active', 'inactive', or 'uninstalled'

Any additional information we need? Instead of having this information hard-coded in the get_managed_plugins method, is there a better way of storing/retrieving the managed plugin info?

@jeffersonrabb jeffersonrabb self-requested a review April 9, 2019 14:30
Copy link
Copy Markdown
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing with the following output:


1) Newspack_Test_Plugins_Controller::test_activate_deactivate_plugin_authorized
Failed asserting that 500 matches expected 200.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/api-plugins-controller.php:134

2) Newspack_Test_Plugins_Controller::test_install_uninstall_authorized
Failed asserting that 500 matches expected 200.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/api-plugins-controller.php:191

3) Newspack_Test_Plugin_Manager::test_activate_deactivate_wporg_uninstalled
Failed asserting that WP_Error Object &000000002e5748130000000060def72e (
    'errors' => Array &0 (
        'newspack_plugin_failed_install' => Array &1 (
            0 => 'The authenticity of <span class="code">hello-dolly.1.6.zip</span> could not be verified as no signature was found.'
        )
    )
    'error_data' => Array &2 ()
) is true.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/plugin-manager.php:88

4) Newspack_Test_Plugin_Manager::test_activate_deactivate_url_uninstalled
Failed asserting that WP_Error Object &000000002e57482a0000000060def72e (
    'errors' => Array &0 (
        'newspack_plugin_failed_install' => Array &1 (
            0 => 'The authenticity of <span class="code">hello-dolly.1.6.zip</span> could not be verified as no signature was found.'
        )
    )
    'error_data' => Array &2 ()
) is true.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/plugin-manager.php:104

5) Newspack_Test_Plugin_Manager::test_activate_deactivate_installed
Failed asserting that WP_Error Object &000000002e5748120000000060def72e (
    'errors' => Array &0 (
        'newspack_plugin_failed_install' => Array &1 (
            0 => 'The authenticity of <span class="code">hello-dolly.1.6.zip</span> could not be verified as no signature was found.'
        )
    )
    'error_data' => Array &2 ()
) is true.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/plugin-manager.php:134

6) Newspack_Test_Plugin_Manager::test_plugin_install_uninstall_wporg
Failed asserting that WP_Error Object &000000002e574b630000000060def72e (
    'errors' => Array &0 (
        'newspack_plugin_failed_install' => Array &1 (
            0 => 'The authenticity of <span class="code">hello-dolly.1.6.zip</span> could not be verified as no signature was found.'
        )
    )
    'error_data' => Array &2 ()
) is true.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/plugin-manager.php:163

7) Newspack_Test_Plugin_Manager::test_plugin_install_uninstall_url
Failed asserting that WP_Error Object &000000002e574a2f0000000060def72e (
    'errors' => Array &0 (
        'newspack_plugin_failed_install' => Array &1 (
            0 => 'The authenticity of <span class="code">hello-dolly.1.6.zip</span> could not be verified as no signature was found.'
        )
    )
    'error_data' => Array &2 ()
) is true.

/srv/www/wordpress-trunk/public_html/src/wp-content/plugins/newspack-plugin/tests/unit-tests/plugin-manager.php:178

FAILURES!
Tests: 21, Assertions: 50, Failures: 7.```

@claudiulodro claudiulodro added [Status] Needs Changes or Feedback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 9, 2019
@claudiulodro
Copy link
Copy Markdown
Contributor Author

The previous errors seem to be caused by the wordpress.org repo not being ready for WP 5.2 yet. WP 5.2 adds file signature checking to prevent infrastructure attacks on wordpress.org, but it's not quite ready yet. When the signature files are added to wordpress.org, things should work correctly. In the meantime, I recommend WP 5.1 for testing this PR.

@jeffersonrabb
Copy link
Copy Markdown
Contributor

I've been testing these endpoints via CURL, using this plugin to generate an access token (thank you @claudiulodro for the idea and @georgestephanis for the plugin!). All the endpoints are currently failing with the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function Newspack\get_plugins() in /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php:148
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(39): Newspack\Plugin_Manager::get_installed_plugins()
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(156): Newspack\Plugin_Manager::get_managed_plugins()
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;get_items(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP in <b>/srv/www/repos/newspack-plugin/includes/class-plugin-manager.php</b> on line <b>148</b><br />

4 similar comments
@jeffersonrabb
Copy link
Copy Markdown
Contributor

I've been testing these endpoints via CURL, using this plugin to generate an access token (thank you @claudiulodro for the idea and @georgestephanis for the plugin!). All the endpoints are currently failing with the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function Newspack\get_plugins() in /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php:148
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(39): Newspack\Plugin_Manager::get_installed_plugins()
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(156): Newspack\Plugin_Manager::get_managed_plugins()
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;get_items(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP in <b>/srv/www/repos/newspack-plugin/includes/class-plugin-manager.php</b> on line <b>148</b><br />

@jeffersonrabb
Copy link
Copy Markdown
Contributor

I've been testing these endpoints via CURL, using this plugin to generate an access token (thank you @claudiulodro for the idea and @georgestephanis for the plugin!). All the endpoints are currently failing with the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function Newspack\get_plugins() in /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php:148
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(39): Newspack\Plugin_Manager::get_installed_plugins()
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(156): Newspack\Plugin_Manager::get_managed_plugins()
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;get_items(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP in <b>/srv/www/repos/newspack-plugin/includes/class-plugin-manager.php</b> on line <b>148</b><br />

@jeffersonrabb
Copy link
Copy Markdown
Contributor

I've been testing these endpoints via CURL, using this plugin to generate an access token (thank you @claudiulodro for the idea and @georgestephanis for the plugin!). All the endpoints are currently failing with the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function Newspack\get_plugins() in /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php:148
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(39): Newspack\Plugin_Manager::get_installed_plugins()
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(156): Newspack\Plugin_Manager::get_managed_plugins()
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;get_items(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP in <b>/srv/www/repos/newspack-plugin/includes/class-plugin-manager.php</b> on line <b>148</b><br />

@jeffersonrabb
Copy link
Copy Markdown
Contributor

I've been testing these endpoints via CURL, using this plugin to generate an access token (thank you @claudiulodro for the idea and @georgestephanis for the plugin!). All the endpoints are currently failing with the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function Newspack\get_plugins() in /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php:148
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(39): Newspack\Plugin_Manager::get_installed_plugins()
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(156): Newspack\Plugin_Manager::get_managed_plugins()
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;get_items(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP in <b>/srv/www/repos/newspack-plugin/includes/class-plugin-manager.php</b> on line <b>148</b><br />

@jeffersonrabb
Copy link
Copy Markdown
Contributor

I've been testing these endpoints via CURL, using this plugin to generate an access token (thank you @claudiulodro for the idea). All the endpoints are currently failing with the following error:

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function Newspack\get_plugins() in /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php:148
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(39): Newspack\Plugin_Manager::get_installed_plugins()
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(156): Newspack\Plugin_Manager::get_managed_plugins()
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;get_items(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP in <b>/srv/www/repos/newspack-plugin/includes/class-plugin-manager.php</b> on line <b>148</b><br />

@jeffersonrabb
Copy link
Copy Markdown
Contributor

After throwing in a quick fix for the above problem I'm seeing this error when hitting the uninstall endpoint (e.g. wp-json/newspack/v1/plugins/amp/uninstall):

<br />
<b>Fatal error</b>:  Uncaught Error: Call to undefined function request_filesystem_credentials() in /srv/www/hechinger-newspack/public_html/wp-admin/includes/plugin.php:867
Stack trace:
#0 /srv/www/repos/newspack-plugin/includes/class-plugin-manager.php(229): delete_plugins(Array)
#1 /srv/www/repos/newspack-plugin/includes/api/class-plugins-controller.php(267): Newspack\Plugin_Manager::uninstall('amp')
#2 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(946): Newspack\API\Plugins_Controller-&gt;uninstall_item(Object(WP_REST_Request))
#3 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api/class-wp-rest-server.php(329): WP_REST_Server-&gt;dispatch(Object(WP_REST_Request))
#4 /srv/www/hechinger-newspack/public_html/wp-includes/rest-api.php(309): WP_REST_Server-&gt;serve_request('/newspack/v1/pl...')
#5 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(286): rest_api_loaded(Object(WP))
#6 /srv/www/hechinger-newspack/public_html/wp-includes/class-wp-hook.php(310): WP_Hook-&gt;apply in <b>/srv/www/hechinger-newspack/public_html/wp-admin/includes/plugin.php</b> on line <b>867</b><br />

@jeffersonrabb
Copy link
Copy Markdown
Contributor

This may be a question for a future PR, but I'm curious if Newspack should keep a log of which plugins it has installed and uninstalled. Specifically I'm wondering about a situation where we take a plugin that Newspack typically manages (e.g. Jetpack) and in some flow we programmatically uninstall or deactivate it. If the customer had installed Jetpack on their own, either before or during the time they were using Newspack, it might be inappropriate for us to uninstall it. If we maintained a list of which plugins we had explicitly installed, it would be easier to assess whether we have the authority to uninstall.

Copy link
Copy Markdown
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments about some errors and a couple of questions.

Comment thread includes/api/class-plugins-controller.php Outdated
@claudiulodro
Copy link
Copy Markdown
Contributor Author

I've fixed the issues around functions not being defined. Those were happening because the API runs outside of the WP Admin context, so those files weren't getting included. Good catch! Thanks for the thorough testing.

This may be a question for a future PR, but I'm curious if Newspack should keep a log of which plugins it has installed and uninstalled. Specifically I'm wondering about a situation where we take a plugin that Newspack typically manages (e.g. Jetpack) and in some flow we programmatically uninstall or deactivate it. If the customer had installed Jetpack on their own, either before or during the time they were using Newspack, it might be inappropriate for us to uninstall it. If we maintained a list of which plugins we had explicitly installed, it would be easier to assess whether we have the authority to uninstall.

I agree; this could definitely be a valid concern. I think we'll need to define our flows a little better to see if that's going to be an issue. I'm not sure what sort of flow would deactivate or uninstall a plugin (maybe if there is some sort of plugin incompatibility with a new plugin being activated?). Let's implement this feature in a separate PR when/if we need it.

@claudiulodro claudiulodro added [Status] Needs Review The issue or pull request needs to be reviewed and removed [Status] Needs Changes or Feedback The issue or pull request needs action from the original creator labels Apr 12, 2019
@jeffersonrabb
Copy link
Copy Markdown
Contributor

Looks great! I only found one other weird case. VIA these endpoints if install a plugin, then activate, then uninstall (without deactivating first), I get this PHP Notice:

<b>Notice</b>: Constant JETPACK__PLUGIN_DIR already defined in <b>/srv/www/hechinger-newspack/public_html/wp-content/plugins/jetpack/uninstall.php</b> on line <b>14</b><br />

@jeffersonrabb jeffersonrabb added [Status] Needs Changes or Feedback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 12, 2019
@claudiulodro
Copy link
Copy Markdown
Contributor Author

The latest issue is a bug in Jetpack, and shouldn't be an issue with other plugins. I've submitted a patch at Automattic/jetpack#12056

@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Changes or Feedback The issue or pull request needs action from the original creator labels Apr 15, 2019
@claudiulodro claudiulodro merged commit 15608e5 into master Apr 15, 2019
@claudiulodro claudiulodro deleted the add/plugin-endpoints branch April 15, 2019 20:28
kmwilkerson added a commit that referenced this pull request May 15, 2026
- Add isEligible guard on Reset: excluded for woocommerce-source emails
  and emails without a registry_slug (finding #1)
- Capture previous status before optimistic update so rollback doesn't
  assume binary publish/draft (finding #2)
- Drop view_category from enrichment response — dead data not consumed
  by the frontend (finding #3)
- Add @todo NPPD-1532 comment on reset endpoint coupling (finding #4)
- Add NPPD-1531 comment on optimistic update / store consolidation (#5)
- Replace brittle PHPUnit count assertions with structural invariants:
  required keys, valid source/recipient, exclusive type keys, no
  duplicates, sort order (findings #6, #7, #8)
- Add optimistic-rollback failure test and reset eligibility test (#9, #11)
- Add 6th mockEmail fixture (WC draft) for cleaner activate test (#10)
- Coerce pluginsReady with Boolean() to handle undefined (#12)
- Use noticeText prop consistently on Notice (#13)
- Fix preview placeholder to use neutral gray tokens (#14)
- Trim EmailItem interface to consumed fields, add source (#15)
- Add source-of-truth comment on category_order strings (#16)
- Consolidate three duplicate TODO comments into one (#17)
- Add NPPD-1525 ticket reference on EmailPreview TODO (#18)
- Extract duplicated screen-reader h1 into PageHeading component (#19)
- Add text-overflow ellipsis on trigger description column (#20)
- Return localized string from recipient getValue for search (#21)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugin installer endpoint

2 participants