WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 9 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:

Description

Like the title says...

Attachments (5)

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

Download all attachments as: .zip

Change History (26)

comment:1 @markjaquith10 years ago

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

@robmiller9 years ago

comment:2 @robmiller9 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 :)

comment:3 @MichaelH9 years ago

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

comment:4 @MichaelH9 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.

@robmiller9 years ago

comment:5 @robmiller9 years ago

  • Keywords tested added; needs-testing removed

Fixed and tested.

@robmiller9 years ago

comment:6 @robmiller9 years ago

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

comment:7 @MichaelH9 years ago

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

comment:8 @markjaquith9 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.

comment:9 @robmiller9 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).

@robmiller9 years ago

comment:10 @robmiller9 years ago

Sorted.

comment:11 @MichaelH9 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?

comment:12 @westi9 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.

comment:13 @robmiller9 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.

comment:14 @westi9 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.

comment:15 @robmiller9 years ago

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

@robmiller9 years ago

Switched admin page creation to use caps

comment:16 @MichaelH9 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?

comment:17 @abhay9 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.

comment:18 @ryan9 years ago

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

comment:19 @westi9 years ago

+1 to an "export" cap

Much better than hooking onto "manage_options"

comment:20 @markjaquith9 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

comment:21 @westi9 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.