Make WordPress Core

Opened 12 years ago

Last modified 20 months ago

#22895 accepted defect (bug)

user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages

Reported by: kevinb's profile kevinB Owned by: johnbillion's profile johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5
Component: Role/Capability Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Utilization of the new separation edit_posts /create_posts capability separation reveals a flaw in admin menu privilege checking.

The issue occurs when:

  1. For any post type other the "post", the user has $type->cap->edit_posts but not $type->cap->create_posts
  1. User also does not have a manage_terms capability for any associated taxonomies

In that situation, access to "edit.php?post_type=whatever" fails unless the user has the "edit_posts" cap for the "post" type.

This occurs because:

  1. wp-admin/includes/menu.php removes solitary submenus that have the same destination as the parent
  1. get_admin_page_parent() returns nullstring if there is no $submenu item
  1. user_can_access_admin_page() performs a type-sensitive capability check only if get_admin_page_parent() returns an existing $submenu key.

For now, my plugin workaround is to hook into 'admin_menu' and add a dummy submenu with nullstring caption.

Attachments (4)

22895.diff (645 bytes) - added by nacin 12 years ago.
22895-2.diff (1.5 KB) - added by bobbingwide 10 years ago.
Don't update the global $pagenow. Use a copy in $pagetest.
22895.3.diff (1.5 KB) - added by joshlevinson 9 years ago.
Patch refresh
22895.3.test.diff (9.1 KB) - added by joshlevinson 9 years ago.
Unit test

Download all attachments as: .zip

Change History (49)

#1 @kevinB
12 years ago

  • Summary changed from user_can_admin_menu() is Type-Insensitive for Users who can't create pages to user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages

#2 @nacin
12 years ago

  • Milestone changed from Awaiting Review to 3.5.1

#3 @kevinB
12 years ago

Note that another condition for this issue:

  • The cap->create_posts must be defined to != $cap->edit_posts
Version 0, edited 12 years ago by kevinB (next)

#4 @johnbillion
12 years ago

  • Cc johnbillion added

#5 @georgestephanis
12 years ago

Blocking off some time on Saturday to wrangle this if noone else beats me to it.

#6 @georgestephanis
12 years ago

I'm trying to get the capabilities set up properly on a post type to duplicate this ... any chance you can paste me your register_post_type capabilities that you're using?

@nacin
12 years ago

#8 follow-up: @nacin
12 years ago

  • Milestone changed from 3.5.1 to 3.6

22895.diff is a hack that allows user_can_access_admin_page() to respect edit.php?post_type= pages. It still does not respect edit-tags.php?taxonomy= pages, though, and neither does get_admin_page_parent(). Overall, the whole thing is a mess and needs to be rewritten. Fun. Even #12718 only touches the API, rather than the inner guts...

Since this is a new feature, and I'm not really sure of the possible breakage that could be caused by 22895.diff (one problem with the admin menu code is it is completely unmaintainable), I'm going to move this ticket to 3.6. So, 3.5 has create_posts with a caveat: It only works if the user has edit_posts, such as when the post type leverages edit_posts (like attachments do). Not the best, but also not something to go messing with a point release. Think of it as version 0.1.

#9 @DrewAPicture
11 years ago

  • Keywords has-patch needs-testing added

#10 @nacin
11 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

#11 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#12 in reply to: ↑ 8 ; follow-up: @johnbillion
11 years ago

Replying to nacin:

So, 3.5 has create_posts with a caveat: It only works if the user has edit_posts, such as when the post type leverages edit_posts (like attachments do).

I've just run into this problem. A user who doesn't have the create_posts capability for a custom post type can't see the edit menu for that post type unless they also have the general edit_posts capability. This is caused by $parent in user_can_access_admin_page() being empty when it shouldn't.

#13 in reply to: ↑ 12 ; follow-up: @nacin
11 years ago

Replying to johnbillion:

I've just run into this problem. A user who doesn't have the create_posts capability for a custom post type can't see the edit menu for that post type unless they also have the general edit_posts capability. This is caused by $parent in user_can_access_admin_page() being empty when it shouldn't.

So 22895.diff would be the wrong fix? Patch welcome for sure.

#14 @johnbillion
11 years ago

At the moment, I'm not sure. I'll find some time to look at it.

#15 @nacin
11 years ago

  • Milestone changed from 3.7 to 3.8

#16 @nacin
11 years ago

  • Milestone changed from 3.8 to Future Release

Still unsure what the right fix is.

#17 in reply to: ↑ 13 @Solinx
10 years ago

Replying to nacin:

Replying to johnbillion:

I've just run into this problem. A user who doesn't have the create_posts capability for a custom post type can't see the edit menu for that post type unless they also have the general edit_posts capability. This is caused by $parent in user_can_access_admin_page() being empty when it shouldn't.

So 22895.diff would be the wrong fix? Patch welcome for sure.

I've also run into this exact issue as described by johnbillion.

To be honest I found the code dealing with menu rights to be quite a mess. A rewrite or at least some documentation wouldn’t hurt, but obviously both take quite a bit of time. In the meantime there are several possible solutions to this problem.

  1. Address $parent being empty - quick fix
  2. Add $typenow to $pagenow - quick fix
  3. Consider changes that are (potentially) more impactful and time consuming

  1. Address $parent being empty

To address the problem by setting a $parent value the following code could be added to get_admin_page_parent() at line 1525:

	if ( !empty($typenow) ) {
		foreach ( $menu as $menuitem ) {
			if ( $menuitem[2] === "$pagenow?post_type=$typenow" ) {
				return $parent_file = "$pagenow?post_type=$typenow";
			}
		}
	}

Besides in user_can_access_admin_page(), get_admin_page_parent() is also used to determine page title and page hook.

The above code does not influence the former. I have not tested whether it influences the latter, but if it does, it would be for the better. Plugin and theme authors would not expect the hook of the post_type edit page to change (which I think it does upon becoming a parent menu item after removing the create_posts capability).

The con to this solution is that the page being visited in reality has no parent page. There is essentially nothing wrong with $parent being empty in user_can_access_admin_page(), because there is no parent. For this reason I prefer solution 2 and did not look into the above points any further.


  1. Add $typenow to $pagenow

The initial solution I came up with closely resembles what you did and is also located at the same place in user_can_access_admin_page():

if ($pagenow === 'edit.php' && isset($_REQUEST['post_type']))
		$curpage .= '?post_type=' . $_REQUEST['post_type'];	
	else
		$curpage = $pagenow;

Our solutions effectively differ at only one point. I also check whether the current page is an 'edit.php' page. My reason for adding this check was to limit any possible side effects - this issue is limited to edit.php pages.

The con to this solution is that it feels like a hack and does not help improve the quality of the code structure. That said, I strongly recommend implementing this solution as a short term fix to this particular issue.


  1. Consider changes that are (potentially) more impactful and time consuming

As noted, the $parent value being empty in user_can_access_admin_page() is actually as it should be. If you consider that, there are a few problems with user_can_access_admin_page(), where the second solution is a fix targeted to this specific case.

a.) Admin pages with an empty $parent value are passed by blacklists targeted at submenu items.

Unless submenu items can be created without a parent menu item there is no sense to this. The specific issue of this ticket is resolved if the checks for submenu items are removed. It is the Posts submenu item that causes the permission denied message, because that is the only ‘edit.php’ submenu without GET strings attached in the keys.

I do not know what the full impact of removing the submenu blacklists will be. This will have to be studied.

b.) The permission blacklists used in case $parent is empty include GET strings in the array keys, while the checks are made without GET strings

Fixing this inconsistency for this specific case is what solution 2 is aimed at. The code is not quite future proof, but that may not be a concern if a rewrite is planned anyhow.

c.) A blacklist system is used to check whether a user has access to a certain page.

Blacklisting is generally a bad idea. Any rewrite attempt should use whitelists instead.

The use of blacklists is not limited to the situation where $parent is empty. The user_can_access_admin_page() function defaults to true if no matches can be found - which effectively means the whole function is designed as a blacklist system.

Each of these options need further attention if they are to be pursued, and could then just as well be the start of a rewrite.


In sum, I would for now recommend implementing solution 2, or your earlier code, in the upcoming update. It may not be perfect, but it does solve a production issue and can be implemented now without any apparent negative effects.

I hope there is still time to make this part of 4.0.

Cheers,

  1. van Dam

#18 @bobbingwide
10 years ago

Just been talking to johnbillion at #wpldn contributor day.
Apparently #29714 is a duplicate of this defect.
Interestingly the workaround that I documented doesn't work
but I have applied my patch to 4.1-beta2 and it works for me.

He also suggested that I should point out that Nacin's patch would lead to the same side effects that I was getting with the first version of the workaround. Rather than change $pagenow it should be copied to another variable and then the tests be performed using the copy.

So, the challenge that now remains is to create some form of unit test from my source code.

#19 @bobbingwide
10 years ago

#29714 was marked as a duplicate.

@bobbingwide
10 years ago

Don't update the global $pagenow. Use a copy in $pagetest.

#20 @DrewAPicture
10 years ago

#29714 was marked as a duplicate.

#21 @greenshady
10 years ago

I believe I ran into a similar issue, but I'm not certain if it's the same yet. I wanted to make sure I didn't need to open another ticket first.

I have two post types. The first has a normal, top-level menu. The second is a sub-menu of the first.

This is how it's set up:

- Type A
--- Type A (all items)
--- Type B (all items)

Users with the cap to view Type B get the access denied message because they don't have the cap to view Type A.

My workaround has been to check if the user has the cap to view Type A. If not, make Type B the top-level menu.

Last edited 10 years ago by greenshady (previous) (diff)

#22 @jfarthing84
10 years ago

Just encountered this myself.

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


10 years ago

#25 @johnbillion
10 years ago

  • Keywords needs-unit-tests added; 3.7-early removed
  • Milestone changed from Future Release to 4.2
  • Owner set to johnbillion
  • Status changed from new to accepted

#26 @gotVirtual
10 years ago

so basically this is never going to get fixed?
thats about what i have taken away from this.

#27 @valendesigns
10 years ago

@gotVirtual It's likely to not get fixed unless someone writes the unit tests to prove its viability and effectiveness. I would be happy to write unit tests if there was some sample code that went along with this ticket to demonstrate the issue.

#28 @DrewAPicture
10 years ago

  • Keywords needs-refresh added
  • Milestone changed from 4.2 to Future Release

No meaningful activity in 6 weeks, we still need unit tests, and the patch needs a refresh. Punting to future release.

@joshlevinson
9 years ago

Patch refresh

#29 @joshlevinson
9 years ago

  • Keywords needs-refresh removed

@joshlevinson
9 years ago

Unit test

#30 @joshlevinson
9 years ago

  • Keywords dev-feedback added; needs-testing needs-unit-tests removed

I've added a test to cover this specific ticket. A couple notes:

  • The test doesn't provide full coverage for user_can_access_admin_page; it just verifies that 22895.3.diff works as intended in a straightforward way.
  • Since the admin menu functionality involves the use of globals and modifying those globals directly in various files (vs. function bodies), a lot of code from the menu "API" had to be brought within this test. I'm sure that once progress is made on #12718, this will change. Disclaimer: I'm a novice unit tester, so there may be a better way than how I managed to do this. I couldn't think of one.

#31 follow-up: @johnbillion
8 years ago

Related: (maybe dupe of) #16808

#32 in reply to: ↑ 31 @tripflex
8 years ago

Replying to johnbillion:

Related: (maybe dupe of) #16808

Yes, there's about 3 duplicates of this same issue spanning over almost 4 years.

I can confirm this is still (as of 4.6), and has been an issues for years now, can we not get a patch pulled in on this? Very frustrating having to allow capabilities on users they should not have (on regular posts) just to not have this problem

#33 @ocean90
7 years ago

#39745 was marked as a duplicate.

#34 @sebastian.pisula
6 years ago

4.9.5 - Bug still exist ;/

Last edited 6 years ago by sebastian.pisula (previous) (diff)

#35 @pikamander2
3 years ago

@DrewAPicture @SergeyBiryukov - This appears to still be an issue as of 5.8.

When disabling the "Add New" button, users are shown a "Sorry, you are not allowed to access this page." page when they try to access the list of posts unless they have access to at least one submenu.

#36 @pikamander2
3 years ago

Here's a relevant Stack Exchange thread that provides a workaround for developers until this issue is fixed in the core: https://wordpress.stackexchange.com/a/178059/

#37 @dougwollison
3 years ago

So this is still present in 5.9

What if we slapped a filter on the result of user_can_admin_menu()? Certainly a band-aid fix but it would allow people who need to patch it to do so in a less hacky way than the add/remove menu trick Kraftner came up with. I bet there may be some niche use cases where having this filter would be beneficial anyway.

I can submit a patch that renames the current function to a private one, and wrap it in a new functions that runs the result through the filter.

function user_can_access_admin_page() {
    /**
     * Filters the result of _user_can_access_admin_page().
     */
    return apply_filters( 'user_can_access_admin_page', _user_can_access_admin_page() );
}

#38 @SergeyBiryukov
2 years ago

#32088 was marked as a duplicate.

#39 follow-up: @SergeyBiryukov
2 years ago

#56280 was marked as a duplicate.

This ticket was mentioned in PR #3024 on WordPress/wordpress-develop by luistar15.


2 years ago
#40

Trac ticket: https://core.trac.wordpress.org/ticket/22895

I have experience something similar and reported in #56280 and proposed a patch. After some feedback I realized that this is a bigger issue.

So I read the other related bug reports ( #16808 #29714 #39745 #48511 #56280 ) and work on this PR patch.

I collect the cases from these bug reports and I put it in a plugin to test the patch.
https://gist.github.com/luistar15/333a25888e8804fd17490815a74ecc21

So far is working from my perspective.

#41 in reply to: ↑ 39 @luistar15
2 years ago

Replying to SergeyBiryukov:

Hi @SergeyBiryukov, I am not familiar with the process merging patches in wordpress.

Is there any chance that some core developer will review the patch before the next wordpress release?
How long usually takes to review/merge new pull request patchs?

I also suggested an improvement in #56252.

#42 @manfcarlo
21 months ago

#57062 was marked as a duplicate. (Update: no it's not)

Last edited 21 months ago by manfcarlo (previous) (diff)

#43 @manfcarlo
21 months ago

#48511 was marked as a duplicate.

vilu85 commented on PR #3024:


20 months ago
#44

I tested the suggested code in this patch on WP 6.1.1 where this problem still exists and it solved the problem. I encountered the same problem as others have already reported, i.e. custom post types with custom capabilities appear in the admin menu, but users who don't have the "edit_posts" role can't create new custom posts, instead they get a "Sorry, you are not allowed to access this page." error message.

@manfcarlo commented on PR #3024:


20 months ago
#45

@vilu85 (emphasis added)

I tested the suggested code in this patch on WP 6.1.1 where this problem still exists and it solved the problem. I encountered the same problem as others have already reported, i.e. custom post types with custom capabilities appear in the admin menu, but users who don't have the "edit_posts" capability can't create new custom posts, instead they get a "Sorry, you are not allowed to access this page." error message.

The subject of ticket 22895 is the denial of the listing page, not the denial of the "create new" page. You may be encountering ticket number 16808?

When I tested this patch, I also found that it solved 16808 but did not solve 22895.

Note: See TracTickets for help on using tickets.