Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#2526 closed defect (bug) (fixed)

WP DB Backup plugin is using $user_level instead of capabilities

Reported by: markjaquith Owned by: robmiller
Milestone: 2.1 Priority: normal
Severity: normal Version: 2.0.1
Component: Administration Keywords: has-patch tested dev-feedback
Focuses: Cc:


Like the title says...

Attachments (5)

wp-db-backup.php.diff (591 bytes) - added by robmiller 12 years ago.
2526.diff (695 bytes) - added by robmiller 12 years ago.
2526.2.diff (591 bytes) - added by robmiller 12 years ago.
2526.3.diff (631 bytes) - added by robmiller 12 years ago.
2526.4.diff (1.3 KB) - added by robmiller 12 years ago.
Switched admin page creation to use caps

Download all attachments as: .zip

Change History (26)

#1 @markjaquith
12 years ago

  • Milestone set to 2.1
  • Version changed from 1.2 to 2.0.1

#2 @robmiller
12 years ago

  • Keywords has-patch needs-testing added
  • Owner changed from anonymous to robmiller
  • Status changed from new to assigned

Done, but would appreciate some testing as I've only really delved into the Roles/Caps system relatively recently so I've no doubt made a silly error :)

#3 @MichaelH
12 years ago

With wp-db-backup.php.diff installed on svn 3656, no user, with any role, can perform a backup.

#4 @MichaelH
12 years ago

Oops, to that last comment I should say, only users with the administrator role can see the Manage->Backup option. But, when that user clicks on Backup!, it does display the screen with the progress bar, but that's it. It just sits there.

12 years ago

#5 @robmiller
12 years ago

  • Keywords tested added; needs-testing removed

Fixed and tested.

12 years ago

#6 @robmiller
12 years ago

Oops, spoke rather too soon. This one should be fine :)

#7 @MichaelH
12 years ago

Thanks robmiller--with latest patch it works--only users with the administrator role can access the Manage->Backup option.

#8 @markjaquith
12 years ago

The only problem with that is what if you don't have a role called "administrator"? Then no one gets the capability.

Remember that "administrator" isn't some magical role... it's just like any other role. You can't count on every WP setup to have all the roles set up how they're set up when WP is first installed. Because of that, I think that a good strategy for granting new capabilities is to piggyback onto an existing capability. The one I usually use is "manage_options" because if you can manage options, you pretty much have supreme control of the blog. So instead of giving it to the "administrator" role, give it to any user/role who has the "manage_options" capability. Essentially you're just saying that because they've already been given so much trust, they're the best candidate to get this additional ability.

#9 @robmiller
12 years ago

Hmm, I see. It's certainly a consideration I hadn't thought of.

I'll modify it to latch on to manage_options (with appropriate comments so it doesn't seem hackish).

12 years ago

#10 @robmiller
12 years ago


#11 @MichaelH
12 years ago

With svn at 3658, 2626.3.diff installed, Role Manager plugin http://redalt.com/wiki/Role+Manager installed:

Assign Subscriber, Contributer, Author, or Editor the Manage Option capability and they don't see the Manage->Backup menu choice.

Create a user, give them the Administrator role, then delete the Manage Options capability from that user. That user can see the Manage->Backup menu, they can select the backup options and click on Backup! button, it displays the screen with the empty progress bar and just sits there--no error or insufficient permissions message shows.

Noticed that at lines 325 and 329 in wp-db-backup.php, user level 9 gets passed to the add_management_page function. Not sure what that means for this ticket--meaning can you can pass a "capability" to functions such as add_management_page, or do those functions need to be revised to handle capabilities?

#12 @westi
12 years ago

Yes add_management_page should get called with the capability rather than then numerical 9 as well so that the menu is only shown for the capable users.

#13 @robmiller
12 years ago

Hmm, yeah. How does one pass a cap to add_*_page(), then?

I guess you could do

if(current_user_can('manage_options')) {
    add_management_page(__('Backup'), __('Backup'), 0, basename(__FILE__), array(&$this, 'backup_menu'));

But that seems pretty hackish. It seems like it would be better to modify the add_*_page functions to take caps.

#14 @westi
12 years ago

rob you don't need to do that. add_*_page() already support being called with caps.

The menus are stripped down in wp-admin/menu.php which calls current_user_can itself - this calls has_cap on an instance of WP_User which checks to see if you passed a number and if so converts it to a "level" cap.

#15 @robmiller
12 years ago

Sweet. Guess it would pay to actually read the functions themselves instead of guessing.

12 years ago

Switched admin page creation to use caps

#16 @MichaelH
12 years ago

This works with 2526.4.diff installed.

Note: giving a user the manage_options capability is not quite enough to let a user see the Manage->Backup choice. It appears a user also needs something like edit_posts or manage_categories capability to expose the Manage choice in the admin menus. Question is, should this ticket resolve that?

#17 @abhay
12 years ago

  • Keywords dev-feedback added

I don't believe that we should fix the capability limitation with this ticket but with another one since that's a separate issue.

I propose adding a new capability "use_plugins" which should be in the core rather than a capability that is plugged in.

#18 @ryan
12 years ago

How about a new "export" cap that can be used by the backup plugin and by the eventual exporter?

#19 @westi
12 years ago

+1 to an "export" cap

Much better than hooking onto "manage_options"

#20 @markjaquith
12 years ago

Yes, that'd be good, but we need a way to register capabilities... otherwise you're just going to be marrying it to an existing capability.

See #2531

#21 @westi
12 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in [3815] and [4049]

Note: See TracTickets for help on using tickets.