[wp-trac] [WordPress Trac] #22895: user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages

[WordPress Trac]

#22895: user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages
-----------------------------------------------+---------------------------
 Reporter:  kevinB                             |       Owner:
     Type:  defect (bug)                       |      Status:  new
 Priority:  normal                             |   Milestone:  Future
Component:  Role/Capability                    |  Release
 Severity:  normal                             |     Version:  3.5
 Keywords:  has-patch needs-testing 3.7-early  |  Resolution:
                                               |     Focuses:
-----------------------------------------------+---------------------------

Comment (by Solinx):

 Replying to [comment:13 nacin]:
 > Replying to [comment:12 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 [attachment: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,
 W. van Dam

--
Ticket URL: <https://core.trac.wordpress.org/ticket/22895#comment:17>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform