Capabilities Improvements

[GaryJones]

Having watched Capability-Driven Development WCEU talk by @felixarntz, I think we could improve the capabilities situation in SatisPress. We already have custom capabilities for viewing or downloading packages, but I think they could be applied to Administrators in a different way, and we could use a custom capability of the admin screen.

Admin Screen

Currently, we register the admin screen with manage_options.

Instead, we could use a new capability like manage_satispress_options, and then filter user_has_cap so that anyone with manage_options can see the SatisPress page, but site owners have more flexibility about who else might be granted that capability - see 21:25 in the video.

Plugins Toggle

If manage_satispress_options was added, then it should be checked against when displaying the plugins checkbox, and also when saving the result of the Ajax call.

Mapping Meta Cap

All seems good here, though adding a mapping for manage_satispress_option would also be needed if the above was done.

Avoid adding new role?

09:30 of the video says that a guideline is to "Never add capabilities to the database, unless you introduce an entire new role".

Right now, I think we add download packages and view packages capabilities directly to the administrator role, breaking this guideline.

I think if we filtered user_has_cap again to add the download packages and view packages capabilities, then we could avoid breaking this guideline, and allow site owners to remove our filter if needed.

Alternatively, we could add a new "SatisPress Manager" role for managing the SatisPress settings + Plugins checkboxes - see 24:00 of the video.

Thoughts? (@bradyvercher, @felixarntz or anyone else)

[bradyvercher]

Admin Screen

It can't hurt anything to use a custom capability for managing SatisPress options and it provides more flexibility, so I think that's worthwhile.

Toggling Plugins

Another thing to consider is that the activate_plugins capability is required to view the screen with the checkbox.

Registering vs Filtering Capabilities for Admins

As for using user_has_cap rather than assigning the capabilities to the administrator role during activation, I could go either way. My main goals would be:

• Require as little configuration as possible to get started using SatisPress
• Rely on existing plugins as much as possible for managing capabilities rather require code
• Leave it up to admins to create roles if their requirements call for them

With the filter, it should at least be possible to disable it with code, whereas the current approach would restore the capabilities if SatisPress is reactivated (WooCommerce does this, too).

One downside with the filter approach is that most capability management plugins won't display the SatisPress capabilities since they're not registered in the database or they won't accurately reflect whether or not administrators have a specific capability.

---

Actually, after thinking it through, I think I'm leaning towards creating the capabilities in the database on the initial install/upgrade to 0.3.0 and removing registration from the activation routine. These are the benefits I see:

• Prevents capabilities from being reset during activation
• Doesn't require code to deactivate for administrators
• Allows capabilities to be visible to capability management plugins

I watched that talk, but didn't see the reason to never add capabilities to the database without a custom role. It's probably a bad practice for restricted core roles (non-admins), especially if the capabilities are restored on reactivation, but I think it's safe to add capabilities to the administrator role, especially if they help improve UX and can be managed via a plugin or code.

Let me know if there's something I'm missing regarding that guideline.

[felixarntz]

@GaryJones @bradyvercher

Instead, we could use a new capability like manage_satispress_options, and then filter user_has_cap so that anyone with manage_options can see the SatisPress page, but site owners have more flexibility about who else might be granted that capability

Precisely, that would definitely be an improvement.

09:30 of the video says that a guideline is to "Never add capabilities to the database, unless you introduce an entire new role".

It's probably a bad practice for restricted core roles (non-admins), especially if the capabilities are restored on reactivation, but I think it's safe to add capabilities to the administrator role, especially if they help improve UX and can be managed via a plugin or code.

Granting custom capabilities via the user_has_cap filter allows easier and more straightforward tweaking and is less error-prone. Activation hooks can be problematic with large multisites, or custom setups that load plugins in some manual way (sure, both of those are rather edge-case), so when using user_has_cap you don't need to rely on those. It just works. Another benefit is that projects using the plugin can simply unhook your user_has_cap filter and then implement it in a custom way. Instead, if you have those capabilities in the database and someone wants to further restrict access or generally handle them in a different way, they'd need to revoke those capabilities first in a less intuitive way, either with an extra user_has_cap filter that removes those or (worse) a map_meta_cap filter that maps them to do_not_allow.

You make a point that those capabilities handled by filters are not included by capability management plugins, which is correct. I'd say this is the only argument for adding capabilities to the database.

I guess the decision then depends on whether one favors the technical benefits or those for users of such plugins. I personally consider capability handling something that should happen on the code level (apart from roles), so I have never bothered about those capability management plugins with UI (they can easily create a big mess too).

Alternatively, we could add a new "SatisPress Manager" role for managing the SatisPress settings + Plugins checkboxes

That would be a good way to make those capabilities available to those management plugins, if needed. However the issue then would be that initially nobody would have the capabilities, since nobody would have the role. And if you then use user_has_cap to also grant them to administrators, it would still be confusing for those management plugins. I think user_has_cap is a good idea as well as a custom role as an extra, but that those two things conflict in some way with having a capability management plugin confirms my hesitation about those.

[GaryJones]

Using user_has_cap works in terms of restricting access. What the issue seems to be is integration with Members et al.

For Members, when I wp_roles()->remove_cap... and re-activate the plugin, the checkboxes in Members are empty, even though administrators do have the right capabilities:

... via the temporary filter callback I added:

add_filter( 'user_has_cap', function( $allcaps ) {
    if ( isset( $allcaps['manage_options'] ) ) {
        $allcaps[ 'manage_satispress_options' ]     = $allcaps[ 'manage_options' ];
        $allcaps[ Capabilities::DOWNLOAD_PACKAGES ] = $allcaps[ 'manage_options' ];
        $allcaps[ Capabilities::VIEW_PACKAGES ]     = $allcaps[ 'manage_options' ];
    }

    return $allcaps;
});

---

I dived into Members, and as you probably know, it reads from $wp_roles. While there are ways to amend what that global returns without affecting what's stored in the DB, it's likely to cause more confusion.

I guess the decision then depends on whether one favors the technical benefits or those for users of such plugins.

I agree with this. SatisPress isn't aimed at the general user - it's squarely aimed at developers, and while some may choose to use Members, User Role Editor etc., I think it would be a rare case for capability management plugins to be used on an install used for SatisPress; I think those developers would cope with documentation on how to further amend user_has_cap if needed, and not need Members et al in the first place.

Personally, I'd be in favour of using the filter.

---

As another general capability improvement (however it's assigned), the link to the packages.json could be behind a check for Capabilities::VIEW_PACKAGES as well.

[bradyvercher]

Thanks for stopping by and weighing in @felixarntz!

Activation hooks can be problematic with large multisites, or custom setups that load plugins in some manual way (sure, both of those are rather edge-case), so when using user_has_cap you don't need to rely on those.

I'm suggesting removing the capability registration in the activation hook, so that should mitigate any problems caused by different setups or plugin loading methods.

Capabilities will only be registered when the plugin is installed for the first time or when upgrading from a version prior to 0.3.0.

Granting custom capabilities via the user_has_cap filter allows easier and more straightforward tweaking and is less error-prone[...] Another benefit is that projects using the plugin can simply unhook your user_has_cap filter and then implement it in a custom way. Instead, if you have those capabilities in the database and someone wants to further restrict access or generally handle them in a different way, they'd need to revoke those capabilities first in a less intuitive way...

I think this is only valid if every plugin implements the user_has_cap filter in the same way, which isn't at all guaranteed. Otherwise, a developer would need to grok multiple plugins, determine whether the filters should be unhooked or augmented in some way, then write custom logic for each plugin. That seems less straightforward and more error-prone to me. It also requires writing and maintaining code that may be unnecessary.

You make a point that those capabilities handled by filters are not included by capability management plugins, which is correct. I'd say this is the only argument for adding capabilities to the database.

Aside from plugins, it occurred to me that WP CLI is another interface that wouldn't work without the capabilities in the database, and while I haven't looked into how the REST API works with capabilities, it seems like it would need to read from the database as well.

With all that in mind, I think using a plugin to manage capabilities, the core Role API, or WP CLI are all more intuitive and consistent than unhooking or overriding filters. Registering the capabilities in the database doesn't prevent developers from implementing a custom user_has_cap filter, either.

[felixarntz]

@GaryJones

For Members, when I wp_roles()->remove_cap... and re-activate the plugin, the checkboxes in Members are empty, even though administrators do have the right capabilities

Right, that's exactly the issue. If you use a custom role, that role will show up in Members with the custom capabilities, but if you grant custom capabilities to an existing role, they only show up there if granted via the database and not via user_has_cap (or any code in general).

@bradyvercher

Aside from plugins, it occurred to me that WP CLI is another interface that wouldn't work without the capabilities in the database, and while I haven't looked into how the REST API works with capabilities, it seems like it would need to read from the database as well.

With all that in mind, I think using a plugin to manage capabilities, the core Role API, or WP CLI are all more intuitive and consistent than unhooking or overriding filters. Registering the capabilities in the database doesn't prevent developers from implementing a custom user_has_cap filter, either.

It seems our views differ here. :)

Capabilities to me are something "internal", something that is managed by a developer. A person interacting with an external interface, such as the WP-Admin GUI, the REST API or WP-CLI shouldn't have to worry about capabilities, them seeing a "You don't have sufficient permissions." etc as a result (or where possible not see a UI element to even try to perform the action) is sufficient IMO. That's also why core exposes roles that group capabilities to the user, but not the capabilities themselves.

The REST API uses core's regular capability handling, which takes the DB, user_has_cap and map_meta_cap into account. Most notably, it doesn't expose capabilities either; in case where JS needs to know in advance whether a user can perform an action, it uses the targetSchema (see https://github.com/WordPress/gutenberg/pull/6529 for first work in that regard).

I consider WP-CLIs capabilities command and plugins such as Members nice utilities to quickly experiment with capabilities, but they cannot replace actual capability management from the codebase, as even core itself uses those filters for several capabilities.

[bradyvercher]

@GaryJones I missed your comment last night.

I agree with this. SatisPress isn't aimed at the general user - it's squarely aimed at developers, and while some may choose to use Members, User Role Editor etc., I think it would be a rare case for capability management plugins to be used on an install used for SatisPress; I think those developers would cope with documentation on how to further amend user_has_cap if needed, and not need Members et al in the first place.

I'm a developer that would prefer to use a plugin or WP CLI to manage capabilities rather than write code for it 😉

@felixarntz

It seems our views differ here. :)

There's nothing wrong with that!

I agree that the way capabilities are reported to the end-user, whatever interface they're using, is sufficient. I think the disagreement is limited to how to register the capabilities and expose them for management.

My intention in bringing up the REST API was in regards to how it exposes capabilities to clients. It looks like it's possible to list capabilities if they're registered, but they won't show if using a filter (similar to @GaryJones screenshot above). The same goes for running wp cap list administrator.

---

I think user_has_cap provides a lot of power and granularity when it's needed, and in some cases it may be unavoidable, but I just don't see the benefit of using it by default when registered capabilities provide greater compatibility with existing tools.

[bradyvercher]

After removing capability registration during activation and introducing a new satispress_manage_options capability, I think the way they're registered should be sufficient and will allow them to continue to work with the Members plugin and WP-CLI.