WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 31 hours ago

#44176 assigned defect (bug)

Un-map Privacy Capabilities

Reported by: desrosj Owned by: xkon
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-screenshots early needs-dev-note needs-testing
Focuses: Cc:
PR Number:

Description

Three new capabilities were added in WordPress 4.9.6 to allow granular control over the new privacy features:

  • erase_others_personal_data
  • export_others_personal_data
  • manage_privacy_options

If a site activates a role management plugin (such as Members), the capability is not listed under the Administrator role (which receives these capabilities by default), and is not available to be assigned to other roles. Adding this role as a custom capability does nothing because the capabilities are mapped to manage_options and manage_network.

A site could create a custom role specifically for someone to manage privacy requests that should not have access to other manage_options related functionality.

Related: #44079, ticket:44055#comment:5.

Attachments (8)

44176.diff (4.9 KB) - added by xkon 6 weeks ago.
44176.2.diff (2.8 KB) - added by xkon 6 weeks ago.
members_caps.jpg (110.8 KB) - added by xkon 6 weeks ago.
44176.3.diff (4.8 KB) - added by pbiron 5 weeks ago.
44176.4.diff (4.5 KB) - added by xkon 10 days ago.
44176.5.diff (29.0 KB) - added by garrett-eclipse 32 hours ago.
Updated patch to fix tests and minor adjustments
Screen Shot 2020-01-27 at 1.07.58 AM.png (42.4 KB) - added by garrett-eclipse 32 hours ago.
Privacy Policy Page now editable by editors
Screen Shot 2020-01-27 at 2.13.04 AM.png (260.5 KB) - added by garrett-eclipse 32 hours ago.
Privacy Settings available and saveable by editors.

Download all attachments as: .zip

Change History (48)

#1 @desrosj
21 months ago

  • Summary changed from Missing Privacy Capabilities to Un-map Privacy Capabilities

#2 @johnbillion
21 months ago

  • Keywords 2nd-opinion added

These are meta capabilities that map to a primitive capability in the map_meta_cap() function.

The same problem is true for the other meta capabilities that have been introduced recently, for example the term management capabilities introduced in 4.7: https://make.wordpress.org/core/2016/10/28/fine-grained-capabilities-for-taxonomy-terms-in-4-7/ . There's really no way around this other than an update to the role management plugin to add support for the newly added capabilities.

It's not really possible to add new primitive capabilities any more because there will always be custom user roles that don't get those caps and therefore can't perform an expected action.

cc @flixos90

#3 @flixos90
21 months ago

There's really no way around this other than an update to the role management plugin to add support for the newly added capabilities.

I fully agree with that statement. The only issue I see with the current core implementation of the new capabilities is that those are not meta capabilities. They are primitive capabilities that can't / shouldn't be part of the database.

Due to that, I'd recommend to grant them via user_has_cap, based on the same capabilities that they are currently mapped to in map_meta_cap. Such a filter can easily be unhooked if someone wants to add the capabilities in another way. With them being part of map_meta_cap() it's harder to tweak the behavior because they are hard-coded in the switch clause in the actual function.

What do you think @johnbillion? See https://core.trac.wordpress.org/ticket/41332#comment:3 for more thoughts on primitive capabilities in the database vs primitive capabilities via user_has_cap vs meta capabilities.

#4 follow-ups: @juliobox
21 months ago

Excuse me, I'm looking for the ticket that started this discussion.
I'm explaining.

You're already talking about the admin capa that is not sharable, ok.
But, since this is a page, why an editor can't just edit it like any other page, it's their job after all!
At the opposite, admin should not add content to the front website, this is not their role, only administration stuff.

Example 1: Many clients are not administrators on their site because of you know what (meeeeess). So they are editor based role with some more capa if needed. So, they can't even edit their own privacy page. Me, as admin, as site maintener, as webmaster, I don't know what to add in this page for them, it's their job, not mine.

Example 2: In the company we have 1 IT perso and 3 editors, they are hired to fill content in the website. Blog, landing pages, privacy page, HO WAIT, they can't do that.

Do you see what I mean? So, I'm looking for the "why" of this decision, in a ticket, a discussion, somewhere.

Then this ticket could have sense here, but not now. See you soon.

This ticket was mentioned in Slack in #core-multisite by earnjam. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by iandunn. View the logs.


21 months ago

#7 @desrosj
21 months ago

#44194 was marked as a duplicate.

#8 in reply to: ↑ 4 @lakenh
21 months ago

Replying to juliobox:

Excuse me, I'm looking for the ticket that started this discussion.
I'm explaining.

You're already talking about the admin capa that is not sharable, ok.
But, since this is a page, why an editor can't just edit it like any other page, it's their job after all!
At the opposite, admin should not add content to the front website, this is not their role, only administration stuff.

Example 1: Many clients are not administrators on their site because of you know what (meeeeess). So they are editor based role with some more capa if needed. So, they can't even edit their own privacy page. Me, as admin, as site maintener, as webmaster, I don't know what to add in this page for them, it's their job, not mine.

Example 2: In the company we have 1 IT perso and 3 editors, they are hired to fill content in the website. Blog, landing pages, privacy page, HO WAIT, they can't do that.

Do you see what I mean? So, I'm looking for the "why" of this decision, in a ticket, a discussion, somewhere.

Then this ticket could have sense here, but not now. See you soon.

Here you go, pulled these up after a <5-minute search:

https://core.trac.wordpress.org/ticket/43935

https://core.trac.wordpress.org/ticket/44055

https://core.trac.wordpress.org/ticket/44079

#9 in reply to: ↑ 4 @Ov3rfly
21 months ago

Replying to juliobox:

Example 1: Many clients are not administrators on their site ..., I don't know what to add in this page for them, it's their job, not mine.

Example 2: In the company we have ... 3 editors, they are hired to fill content in ... privacy page, HO WAIT, they can't do that.

Exactly the same situation here. This needs to be changed without rolling out extra capabilities to editor roles of hundreds of sites. Otherwise most of our clients won't be able to use the new and shiny "privacy policy page" feature, see our +1 here: #44194

#10 @Ov3rfly
20 months ago

For interested readers, as long as editor role users can't edit the privacy policy page, instead of the core feature we are now using a simple custom solution for login and registration pages, which supports

  • multiple links,
  • the real post titles,
  • configurable order,
  • any post types

Find details and code in #44194

#11 @garrett-eclipse
18 months ago

#44810 was marked as a duplicate.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


17 months ago

#13 @ianatkins
14 months ago

Agree that giving the DPO admin rights is a recipe for disaster. Am trying to enable the privacy tools for the editor role.

@flixos90 you mention it's an architectural issue. Have tried the filter you suggested ( code below ), this now gives an editor the 'tools' option in the menu that only redirects back to the dashboard and has no sub menus. Correct me if I'm missing something, or that's not the right filter.

I can only get it this to work by granting manage_options via the members plugin, which is less than ideal.

function editor_cap_filter( $caps, $cap, $args ) {
        
        // Bail out for users who can't publish posts:
        if ( !isset( $caps['publish_posts'] ) or !$caps['publish_posts'] )
                return $caps;

        // Add Privacy Capabilties
        $caps[] ='erase_others_personal_data';  
        $caps[] ='export_others_personal_data'; 
        $caps[] ='manage_privacy_options';      
        
        return $caps;

}

add_filter( 'user_has_cap', 'editor_cap_filter', 10, 3 );

#14 @swissspidy
14 months ago

  • Milestone changed from Awaiting Review to 5.1

It'd be great to get this fixed.

#15 @garrett-eclipse
13 months ago

  • Milestone changed from 5.1 to Future Release

I'm moving this ticket to 'Future Release' as it hasn't seen much attention. Once a patch has been worked on this can be re-milestoned.

#16 @dingo_bastard
12 months ago

What is necessary to move export_others_personal_data, erase_others_personal_data and manage_privacy_options to 'primitive' capabilities? Just add a custom capability on line 545 in wp-includes/capabilities.php like

case 'export_others_personal_data':
case 'erase_others_personal_data':
case 'manage_privacy_options':
  $caps[] = 'privacy_management';
  break;

or would more work need to be done? Is there something more that we need to modify when creating the primitive capability?

This really is a blocker if you want to create a new user role that will only handle user management, as the export and erasure of private user data is something that should fall under user management.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


6 months ago

#19 @thomaswp
3 months ago

Agree with @Ov3rfly Same here, still looking for a working solution.
The code @ianatkins posted makes the page editable, but getting a error while saving this page.

#20 @garrett-eclipse
3 months ago

  • Milestone changed from Future Release to 5.4

#21 @steveo2000
3 months ago

I can't get any of the above including @ianatkins code to grant permission to editors to edit this page - does anyone have any other temporary solutions until this is fixed? This is real problem as it means editors can't edit one of the most important pages in the site.

Last edited 3 months ago by steveo2000 (previous) (diff)

@xkon
6 weeks ago

#22 follow-up: @xkon
6 weeks ago

  • Keywords has-patch dev-feedback added; needs-patch removed

Hey all,

Please ignore the first attachment:44176.diff, I was a bit confused with the primitive/add_cap as I wasn't fully aware of how it's working and it seems that I did double work :D.

The attachment:44176.2.diff :

Creates a new schema as populate_roles_540() that adds the 3 privacy caps to the administrator role by default & removes them from map_meta_cap.

The 'Tools' is already opening for this role as-is, but I had to adjust the Settings so the Privacy setting could be accessed if someone only had the manage_privacy_options. I would love feedback on how to tackle the menu problem as I don't like much the if statement there and not really sure if it's ok to do it that way.

The caps after this are accessible from plugins like the "Member" ( see screenshot ) that was mentioned here and they can be assigned to different roles also.

As an example I created a custom_dpo for testing everything & to break down all of the capabilities that I eventually added are these:

// These are the privacy-related caps so the role can have access to the Exporter/Eraser.

export_others_personal_data
erase_others_personal_data
manage_privacy_options


// These are page editing for the Privacy page purposes.
edit_pages
edit_others_pages
edit_published_pages
publish_pages
delete_pages
delete_others_pages
delete_published_pages
delete_private_pages
edit_private_pages
read_private_pages

Do tell me if I'm missing anything as I got really lost trying to figure out how caps are supposed to be in core.

The filters that we usually use on plugins are straightforward but core seems a totally different game on this aspect :D .

Thanks!

Last edited 6 weeks ago by xkon (previous) (diff)

This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.


6 weeks ago

@xkon
6 weeks ago

@xkon
6 weeks ago

#24 @xkon
6 weeks ago

  • Keywords has-screenshots added

This ticket was mentioned in Slack in #core-privacy by pbiron. View the logs.


6 weeks ago

#26 in reply to: ↑ 22 @pbiron
6 weeks ago

Replying to xkon:

The attachment:44176.2.diff :

Creates a new schema as populate_roles_540() that adds the 3 privacy caps to the administrator role by default & removes them from map_meta_cap.

The other thing it should do is replace the 2 occurrences in map_meta_cap() (at the end of the case statements for delete_{post,page} and edit_{post,page}) of:

<?php
if ( (int) get_option( 'wp_page_for_privacy_policy' ) === $post->ID ) {
        $caps = array_merge( $caps, map_meta_cap( 'manage_privacy_options', $user_id ) );
}

with:

<?php
if ( (int) get_option( 'wp_page_for_privacy_policy' ) === $post->ID ) {
        $caps[] = 'manage_privacy_options';
}

I can do a revised patch is you'd like.

@pbiron
5 weeks ago

#27 follow-up: @pbiron
5 weeks ago

44176.3.diff is the same as 44176.2.diff with the following additions:

  1. the calls to map_meta_cap( 'manage_privacy_options', $user_id ) in map_meta_cap() are replaced as decribed in comment 26
  2. an upgrade_540() function is added so that existing sites get the caps added to the administrator role

I'm not sure how things have been handled in the past with patches that depend on a specific $db_version (i.e., commit number that hasn't happend yet), but for the purposes of this patch I just made the check (for whether upgrade_540() is called) use the $db_version in 5.3.2 + 1. Note: with that, you won't have to do the "trick" described in this slack thread in order to test that the primitive caps have been added...but it means that upgrade_540() gets called on every admin page load (it's inexpensive, so you'll probably never notice).

This ticket was mentioned in Slack in #core-privacy by pbiron. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core by xkon. View the logs.


3 weeks ago

#30 @xkon
3 weeks ago

Thanks for the update @pbiron! And yes this seems to do the trick instead of the grumpy resetting for tests all the time.

I'm not sure either regarding the db_version so I'll leave that to the more experienced reviewers :).

#31 @xkon
10 days ago

  • Owner set to xkon
  • Status changed from new to assigned

#32 in reply to: ↑ 27 ; follow-up: @Ov3rfly
10 days ago

Replying to pbiron:

... is added so that existing sites get the caps added to the administrator role

Please make sure that the caps also get added to the editor role. We need a solution ..

.. as explained e.g. here Ov3rfly ..

... without (manually) rolling out extra capabilities to editor roles of hundreds of sites.

Last edited 10 days ago by Ov3rfly (previous) (diff)

#33 in reply to: ↑ 32 ; follow-up: @xkon
10 days ago

  • Keywords reporter-feedback added

Replying to Ov3rfly:

Please make sure that the caps also get added to the editor role. We need a solution ..

Editors never had these caps so I'm not sure why you would want that or why would there be an issue as in comment 9?

I'm also not in favor of granting every Editor out there the Privacy capabilities by default especially since they didn't exist up to now.

The purpose of doing this move is that the caps are then free to be assigned to any role so any website owner has the free choice to add these caps to their Editors if they like or create an entirely new role for DPO purposes.

--

44176.3.diff applies fine for me and we have a couple of extra tickets that are waiting for this change so we can re-configured the caps needed.

@desrosj, @SergeyBiryukov can you add any feedback here if there is anything else that might be needed regarding this change on caps? I'm OK for a commit if this does the trick as expected, so we can work on the extra tickets that are waiting.

Note, I still need to do a "wp role reset administrator" for any plugin to re-read the caps freely, not sure if that would work by itself in a case of the release update as @pbiron says at comment 32 with caring to the $db_version (maybe this is because I'm working from /src directly instead of /build) :).

Last edited 10 days ago by xkon (previous) (diff)

#34 in reply to: ↑ 33 @Ov3rfly
10 days ago

Replying to xkon:

Editors never had these caps ...

I'm also not in favor of granting every Editor out there the Privacy capabilities by default especially since they didn't exist up to now.

See rest of thread, e.g. "Example 1" in comment 4.

The purpose of doing this move is that the caps are then free to be assigned to any role so any website owner has the free choice to add these caps to their Editors if they like ..

..which would require manual changes to hundreds of sites and extensive testing and additional cost for clients to provide the features to those site.

In realworld "WordPress as CMS" usage, nobody in charge of public visible content of the website is an administrator role. All these sites currently can not use the privacy features or even the "privacy page" functions.

My request was/is: If an "auto-assign" for administrator roles is provided in the patch, see comment 27, also an "auto-assign" of the caps to editor roles should be added.

#35 follow-up: @xkon
10 days ago

Let me take a step back & make a recap of the above in full @Ov3rfly with some extra additions & explanations to see if we can agree to what you're proposing as I think we got lost somewhere in the context (maybe I didn't read something correctly or I don't know..).

---

Editors never had this choice so this doesn't actually bring changes to hundreds of sites. Instead it brings the option to do it from now on and bind these caps to roles as needed. That's totally different and if that's what you mean then personally I'm fine with it as that's the way it should've been since the start and we, unfortunately, missed it and that's why this task was created, to address this issue.

---

Now regarding the "caps". Editors by default (IMHO) do not in any way need access to erase_others_personal_data & export_others_personal_data. It's not within the scope of their default role to freely download any other persons Personal Data Exports in any way or remove them. It's like saying that Editors all of a sudden have access to delete other users.

If you meant only the manage_privacy_options cap then sorry if I missed it but it wasn't clear at least to me as "caps" was mentioned I assumed you meant all 3 that this ticket takes care of.

What we could do since we are splitting these caps "on their own" is that we can have a new ticket if you like and propose Editors to have the manage_privacy_options cap only which is connected to the Privacy Policy page + the Privacy Settings admin screen ( that's why I mentioned in my comment that more tickets after this will be following since we have to re-adjust who has access and where).

#36 in reply to: ↑ 35 @Ov3rfly
10 days ago

Replying to xkon:

... is that we can have a new ticket if you like and propose Editors to have the manage_privacy_options cap only which is connected to the Privacy Policy page + the Privacy Settings admin screen.

Obviously you missed the references to #44194 in above comments, I have opened such ticket almost 2 years ago, it was closed as duplicate and I was asked to use this ticket instead by desrosj ..

Changing the way this capability is managed is being discussed in #44176. Please add any information you have to that ticket.

.. and that's what I did.

@xkon
10 days ago

#37 @xkon
10 days ago

Sorry for this @Ov3rfly, yes I didn't wander through the ticket linked at your earlier comment so I totally missed that.

44176.4.diff also adds manage_privacy_options to Editor role so it will be able to alter the pages contents & settings.

#38 @xkon
7 days ago

  • Keywords early added

Sorry forgot to add an early here as this will have to go first before other tickets that might need re-adjustment due to caps and as we'll have to see that everything works as expected on an early point :).

#39 @audrasjb
7 days ago

  • Keywords needs-dev-note added

@garrett-eclipse
32 hours ago

Updated patch to fix tests and minor adjustments

@garrett-eclipse
32 hours ago

Privacy Policy Page now editable by editors

@garrett-eclipse
32 hours ago

Privacy Settings available and saveable by editors.

#40 @garrett-eclipse
31 hours ago

  • Keywords needs-testing added; 2nd-opinion dev-feedback reporter-feedback removed

Thanks for the patches @xkon & @pbiron and for the flag there @Ov3rfly this is working nicely and would be great to have addressed early in 5.4 here for testing.

I've made a minor adjustment in 44176.5.diff to provide the following updates;

  1. Added the @global to the upgrade_540 method.
  2. Updated the expected db_version to match the current in trunk 47018. This allows testing by setting your db_version in the options table to be less than 47018. This will need to be updated during 5.4 release.
  3. Updated the Settings taking it out of the conditional to not change existing functionality and just replace it when the user has manage_privacy_options but not manage_options. Also updated to options-privacy.php in this scenario so the link directs to the Privacy Settings.
  4. Addressing the PHP Unit failures I've updated the test coverage for the capabilities moving them to primitive capabilities and setting them to support administrator and for the manage_privacy_options also added editor

It would be nice to get some testing on multisite as the change from meta caps to primitive ones means we're adding the capabilities to the administrator and editor and there's no longer the is_network conditional;
$caps[] = is_multisite() ? 'manage_network' : 'manage_options';

I've uploaded some screenshots illustrating the privacy page and settings are editable by editors.

Note: See TracTickets for help on using tickets.