Opened 12 years ago
Last modified 22 months ago
#22895 accepted defect (bug)
user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages
Reported by: | kevinB | Owned by: | 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:
- For any post type other the "post", the user has $type->cap->edit_posts but not $type->cap->create_posts
- 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:
- wp-admin/includes/menu.php removes solitary submenus that have the same destination as the parent
- get_admin_page_parent() returns nullstring if there is no $submenu item
- 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)
Change History (49)
#1
@
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
#6
@
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?
#8
follow-up:
↓ 12
@
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.
#12
in reply to:
↑ 8
;
follow-up:
↓ 13
@
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:
↓ 17
@
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 generaledit_posts
capability. This is caused by$parent
inuser_can_access_admin_page()
being empty when it shouldn't.
So 22895.diff would be the wrong fix? Patch welcome for sure.
#17
in reply to:
↑ 13
@
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 generaledit_posts
capability. This is caused by$parent
inuser_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.
- Address
$parent
being empty - quick fix - Add
$typenow
to$pagenow
- quick fix - Consider changes that are (potentially) more impactful and time consuming
- 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.
- 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.
- 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,
- van Dam
#18
@
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.
#21
@
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.
#23
@
10 years ago
Cross link: Thomas Kräftner answered a question about this on WordPress.StackExchange.
This ticket was mentioned in Slack in #core by kaiser. View the logs.
10 years ago
#25
@
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
@
10 years ago
so basically this is never going to get fixed?
thats about what i have taken away from this.
#27
@
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
@
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.
#30
@
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 that22895.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.
#32
in reply to:
↑ 31
@
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
#35
@
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
@
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
@
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() );
}
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
@
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.
22 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:
22 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.
This will be obvious to some, but note that this issue only occurs when: