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

[kevinB]

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

2. 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

2. get_admin_page_parent() returns nullstring if there is no $submenu item

3. 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.

[kevinB]

This will be obvious to some, but note that this issue only occurs when:

• $type_obj->cap->create_posts is defined to != $type_obj->cap->edit_posts

[georgestephanis]

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

[georgestephanis]

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?

[georgestephanis]

[22900] [22908]

[nacin]

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.

[wonderboymusic]

these are all marked 3.7-early

[johnbillion]

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.

[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.

[johnbillion]

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

[nacin]

Still unsure what the right fix is.

[Solinx]

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.

---

2. 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.

---

3. 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,

23. van Dam

[bobbingwide]

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.

[bobbingwide]

#29714 was marked as a duplicate.

[bobbingwide]

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

[DrewAPicture]

#29714 was marked as a duplicate.

[greenshady]

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.

[jfarthing84]

Just encountered this myself.

[F J Kaiser]

Cross link: ​Thomas Kräftner answered a question about this on WordPress.StackExchange.

[gotVirtual]

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

[valendesigns]

@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.

[DrewAPicture]

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

[joshlevinson]

Patch refresh

[joshlevinson]

Unit test

[joshlevinson]

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.

[johnbillion]

Related: (maybe dupe of) #16808

[tripflex]

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

[ocean90]

#39745 was marked as a duplicate.

[sebastian.pisula]

4.9.5 - Bug still exist ;/

[pikamander2]

@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.

[pikamander2]

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/

[dougwollison]

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() );
}

[SergeyBiryukov]

#32088 was marked as a duplicate.

[SergeyBiryukov]

#56280 was marked as a duplicate.

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.

[luistar15]

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.

[manfcarlo]

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

[manfcarlo]

#48511 was marked as a duplicate.

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.

@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.