WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#14024 closed defect (bug) (fixed)

Insufficient permissions error after activating plugins

Reported by: SergeyBiryukov Owned by:
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.0
Component: I18N Keywords: has-patch
Focuses: Cc:

Description

If a plugin like WP-DB-Backup or BuddyPress calls wp_get_current_user() on a localized install before wp-content/languages/$locale.php is included (for example, on plugins_loaded action) and default secret keys are not changed, it results in “You do not have sufficient permissions to manage plugins for this site” error, because $wp_default_secret_key is not redefined yet (see #12081 for details).

What is the best way to fix it? For now I have posted the instructions on the support forums to change the default keys and I'm going to translate the string in default-constants.php.

Attachments (3)

14024.patch (552 bytes) - added by SergeyBiryukov 3 years ago.
doing-it-wrong.php (1.0 KB) - added by SergeyBiryukov 3 years ago.
Sample plugin to test the issue
14024.2.patch (526 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 nacin4 years ago

Sounds like plugins should wait until we run this code, which is well after plugins_loaded and immediately before init:

// Set up current user.
$wp->init();

comment:2 filosofo4 years ago

Or at 'set_current_user' perhaps, for users that actually exist.

Thanks for tracing this, SergeyBiryukov. I've been holding off minor updates on WP-DB-Backup while working on a major rewrite, but this is probably good enough reason for a point release.

comment:3 SergeyBiryukov4 years ago

Thanks for your feedback. When users receive this error message, they blame the localized version author because en_US version works fine in this case, and it's really uncomfortable. At first I thought this only looks like a solution in an ideal world, but Austin's reply gave me some hope :) Should the ticket for BuddyPress also be created?

I'd like to add that both WP-DB-Backup and BuddyPress call wp_get_current_user() indirectly by using wp_create_nonce(). It's not mentioned in Codex that we should not use wp_create_nonce() or wp_get_current_user() before init.

comment:4 nacin4 years ago

It's dangerously a cart-before-the-horse situation. I'm not sure why either of them need to create a nonce so early. I imagine it's a legitimate use case that they can get away with pushing off until init.

I wish it didn't work in en_US.

Let me contact the BP guys.

comment:5 hakre4 years ago

It would be good to know what certain kind of functionality is needed before init.

Otherwise I think it's wise that the function in question should return an error if it's not in a useable state.

Related: #14047

comment:6 nacin3 years ago

  • Milestone changed from Awaiting Review to Future Release

What's needed here? Better docs?

comment:7 SergeyBiryukov3 years ago

  1. I guess at least WP-DB-Backup and BuddyPress as highly popular plugins need to be fixed. Has this been reported to the BuddyPress team?
  2. Better docs are needed on this specific problem so that plugin authors could be directed there.
  3. Localization authors should probably translate the string in default-constants.php anyway to prevent the admin area from breaking after installing some untested plugin. This is especially important for site owners whose web host doesn't provide FTP access.

comment:8 SergeyBiryukov3 years ago

I've added a note in Codex for wp_create_nonce() и wp_get_current_user() about this.

Perhaps we should show a warning in a future release instead of wp_die()?

comment:9 filosofo3 years ago

I added SergeyBiryukov's fix to WP-DB-Backup 2.2.3.

comment:10 pavelevap3 years ago

  • Cc pavelevap@… added

comment:11 SergeyBiryukov3 years ago

  • Cc SergeyBiryukov removed
  • Keywords needs-patch added

comment:12 nacin3 years ago

  • Milestone changed from Future Release to 3.3

Let's issue a _doing_it_wrong() for the en_US case.

SergeyBiryukov3 years ago

comment:13 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed

Looks like we can't check if did_action( 'init' ) here, since $wp->init() is called before init.

after_setup_theme is the closest appropriate action.

SergeyBiryukov3 years ago

Sample plugin to test the issue

comment:14 ryan2 years ago

  • Milestone changed from 3.3 to Future Release

comment:15 follow-up: nacin2 years ago

  • Milestone changed from Future Release to 3.4

did_action('init') is fine, as it returns true the moment init fires (rather than only after it completes).

I don't think return false is acceptable. We might just need to fire the _doing_it_wrong() and let it continue.

#19599 should fix the issue. All we need is _doing_it_wrong().

SergeyBiryukov2 years ago

comment:16 in reply to: ↑ 15 SergeyBiryukov2 years ago

Replying to nacin:

did_action('init') is fine, as it returns true the moment init fires

I've tried to use did_action('init'), but in that case the message is displayed even without any plugins, because $wp->init() calls wp_get_current_user() exactly before init fires:
http://core.trac.wordpress.org/browser/tags/3.3/wp-settings.php#L294
http://core.trac.wordpress.org/browser/tags/3.3/wp-includes/class-wp.php#L442

Part of $wp_actions stack:

[after_setup_theme] => 1
[auth_cookie_valid] => 1
[set_current_user] => 1
[init] => 1

Refreshed the patch to continue after showing _doing_it_wrong().

comment:17 SergeyBiryukov2 years ago

Would "Current user information is only available on set_current_user action or later" be the more exact message? Or is it better to recommend to use init for that?

comment:18 follow-up: nacin2 years ago

See #19599, [19771]. This will no longer break in localized builds. Is a _doing_it_wrong() worth it at this point?

comment:19 nacin2 years ago

  • Component changed from I18N to Plugins

Changing component as this is no longer an i18n issue, but a "best practice" issue. With #19599 fixed, I am not sure if anything else can break if you calculate the current user prior to init.

comment:20 in reply to: ↑ 18 SergeyBiryukov2 years ago

Replying to nacin:

See #19599, [19771]. This will no longer break in localized builds.

Confirmed.

Is a _doing_it_wrong() worth it at this point?

It would be consistent with the messages added in #11526. However, if there are no more issues with calling wp_get_current_user() too early, maybe it's not really necessary.

comment:21 SergeyBiryukov2 years ago

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

comment:22 nacin2 years ago

  • Component changed from Plugins to I18N
Note: See TracTickets for help on using tickets.