Opened 3 years ago

Closed 16 months ago

Last modified 16 months ago

#14024 closed defect (bug) (fixed)

Insufficient permissions error after activating plugins

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

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 21 months ago.
doing-it-wrong.php (1.0 KB) - added by SergeyBiryukov 21 months ago.
Sample plugin to test the issue
14024.2.patch (526 bytes) - added by SergeyBiryukov 17 months ago.

Download all attachments as: .zip

Change History (25)

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();

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.

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.

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.

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

  • Milestone changed from Awaiting Review to Future Release

What's needed here? Better docs?

  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.

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()?

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

  • Cc pavelevap@… added
  • Cc SergeyBiryukov removed
  • Keywords needs-patch added
  • Milestone changed from Future Release to 3.3

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

  • 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.

Sample plugin to test the issue

  • Milestone changed from 3.3 to Future Release

comment:15 follow-up: ↓ 16   nacin17 months 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().

comment:16 in reply to: ↑ 15   SergeyBiryukov17 months 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().

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: ↓ 20   nacin16 months ago

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

  • 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   SergeyBiryukov16 months 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.

  • Resolution set to fixed
  • Status changed from new to closed
  • Component changed from Plugins to I18N
Note: See TracTickets for help on using tickets.