-
Notifications
You must be signed in to change notification settings - Fork 826
Forms: Add plugin install permissions check #45063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
@@ -788,6 +788,8 @@ public static function load_editor_scripts() { | |||
'assetsUrl' => Jetpack_Forms::assets_url(), | |||
'preferredView' => $preferred_view, | |||
'isMailPoetEnabled' => Jetpack_Forms::is_mailpoet_enabled(), | |||
'canInstallPlugins' => current_user_can( 'install_plugins' ), | |||
'canActivatePlugins' => current_user_can( 'activate_plugins' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we are adding these values here, but also in class-dashboard.php below. Right now, we have three different ways in which we pass data to the frontend - one only for the editor and two for the dashboard here and here. We should try to consolidate that to avoid duplication. I started on PR, but it's a separate undertaking that doesn't fit in this PR.
For now, because the PluginActionButton shows in both the dashboard integrations panel, and the form integrations modal, we need that component to check both methods for passing data to the frontend.
> | ||
{ getButtonText() } | ||
</Button> | ||
</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to wrap the Button in a span here because if Tooltip directly wraps Button, and Button is disabled, the tooltip will not show. Tooltips do not show on disabled buttons.
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked well with my editor user and admin showed no regressions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR worked as expected for me in the wp-admin side of things. As an admin and as the editor.
Nice work!
9a36795
to
f049915
Compare
Can you elaborate on choice to pass permission checks from backend with custom data, instead of using core data store and Design and copy looks great! |
@simison That is actually what I did for the first implementation. But I could not get the checks to work correctly. I should say that There are also some benefits to checking in PHP and localizing to JS, mostly around avoiding latency and jumps/flickr in the interface as things load. And we already have established mechanisms for passing data from PHP. Even so, I'd be happy to continue digging on using the core data store if we prefer. |
@simison - To add more context on the point above about core-data and the canUser check. I re-set this up locally and confirmed again it's not working. Here's the explanation for Cursor:
So it's possible but it gets a bit funky. I think we'd need to pass the pluginFile path to the component and do something like:
So it's a check per-plugin, not a general permissions check. Seems like a weird architectural choice not to have a general activate_plugins check, but I think that's the situation. |
Thanks for digging into it! Everything in WP is headed so much headless/JS SPA way that my gut tells it's generally better if things are API driven and data is pulled by JS, hence the question. In this case effort and efficiency from making the initial loading feel smooth begs for the backend solution, so let's go with it. 👍 Thanks again! |
f049915
to
a5fcd2c
Compare
a5fcd2c
to
63d2b66
Compare
63d2b66
to
fe5c912
Compare
I have updated the approach in this PR. We were checking for plugin install/activate permissions in PHP and passing those to JS via the 'config' object on the dashboard and the jpFormsBlocks object in the editor. But in #45091, we added a new 'forms config' endpoint that is preloaded on both dashboard and editor, and includes the permissions checks. So the current PR has been updated to use data from that endpoint. This also fixes the build error were getting on this PR due to JS bundle size. The old approach was effectively loading the whole dashboard dependency tree in the editor because of how the config object is setup. The new way resolves that by using a simple endpoint. @simison - You asked above about why we retrieve permissions in PHP. Just tagging you here because that's now changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely! Thanks!
Proposed changes:
Disable the plugin install or activate buttons in the form integrations dashboard and block modal when the user does not have needed permissions.
Why? We need this because in an upcoming PR, we are going to start exposing the integrations panels in the VIP environment where users do not have plugin install rights. But it will also have an impact, for example, in WordPress multisite environments where users can activate but not install plugins. In short, there are environments where users should see the integrations panel, but not be able to click buttons to install/activate plugins.
Specific changes: The PluginActionButton component now retrieves plugin install/activate permissions from the forms config endpoint, and disables buttons if appropriate.
Screenshot: User who can install but not activate plugins will see this.

Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
NOTE: Make sure that you are not separate setting the jetpack_forms_enable_integrations_tab to false, which would hide the integrations dashboard. That's set to true by default here. Note that this filter will be removed and replaced in a separate PR.
Prepare plugins. Deactviate and delete the creative mail plugin. Then deactivate either the Jetpack CRM or MailPoet plugins, but leave them installed. This will give you some plugins in different states to test.
Create new user. Add a new user to your Jetpack dev site with an
editor
role. By default, editors cannot install or activate plugins.Confirm cannot install or activate. Login to your Jetpack dev site with that new user. Go to Dashboard > Jetpack > Forms > Integrations. Confirm plugin Install or Activate buttons are disabled and if you hover, you see the correct tooltip text telling you the issue.
Update user. You now need to update your new user so they can activate, but still not install plugins. You can do that by adding this to your themes functions.php file:
Confirm can activate but not install.. Go to Dashboard > Jetpack > Forms > Integrations again. Confirm plugin Install button is still disabled, but Activate button is now enabled and works.
Confirm admins can still install/activate. Finally, confirm no regressions by logging in with your admin user and confirm that user can always install and activate plugins.