Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#16297 closed defect (bug) (fixed)

User admin shouldn't kick in if not multisite

Reported by: nacin's profile nacin Owned by: ryan's profile ryan
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: Network Admin Keywords: dev-feedback
Focuses: multisite Cc:

Description

If a user has no role on a single-site installation (no role for the blog, shared user tables, etc.), they're redirected to the global dashboard, which then breaks as it assumes multisite.

It looks like a logic issue in [15746/trunk/wp-login.php], an !is_multisite() that instead should be is_multisite(). Changing that restores 3.0/2.9 behavior, which would be to redirect to profile.php and then show an error due to insufficient permissions.

It should also be noted that there is no way for such a user to log out, unless the theme contains a link. This will be solved in part with the admin bar, but I think these logins should instead be rejected. "No role for this site" indicates, to me, that the account should be invalid on that site. This part is future release.

Attachments (5)

16297.diff (733 bytes) - added by nacin 13 years ago.
16297.2.diff (1.1 KB) - added by nacin 13 years ago.
16297.3.diff (854 bytes) - added by nacin 13 years ago.
16297.4.diff (1.2 KB) - added by ryan 13 years ago.
16297.5.diff (521 bytes) - added by ryan 13 years ago.

Download all attachments as: .zip

Change History (25)

#1 @nacin
13 years ago

Actually, the lines in question are:

// If the user doesn't belong to a blog, send them to user admin. If the user can't edit posts, send them to their profile.
if ( is_multisite() && !get_active_blog_for_user($user->id) )
	$redirect_to = user_admin_url();
elseif ( !is_multisite() && !$user->has_cap('read') )
	$redirect_to = user_admin_url();
elseif ( !$user->has_cap('edit_posts') && ( empty( $redirect_to ) || $redirect_to == 'wp-admin/' || $redirect_to == admin_url() ) )
	$redirect_to = admin_url('profile.php');

Lines three and four seem functionally equivalent when the multisite check is reversed. So maybe both lines need to be removed, as the global dashboard was never designed to work outside of multisite.

Will want feedback from ryan on this.

#2 @nacin
13 years ago

Never mind on that last bit -- I believe it's valid for a user to have a blog assigned to them without having the 'read' cap.

@nacin
13 years ago

#3 @nacin
13 years ago

  • Owner set to ryan
  • Status changed from new to reviewing

#4 @nacin
13 years ago

The user admin should probably not be accessible through a direct URL if not multisite, either.

@nacin
13 years ago

#5 @ryan
13 years ago

Looks good.

#6 @nacin
13 years ago

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

(In [17332]) Correct user admin redirection checks, and deny access to the user admin when not running multisite. fixes #16297.

#7 @mdawaffe
13 years ago

I have the following scenario.

A multisite user who is not a member of http://blog.multisite.com/ is sent to http://blog.multisite.com/wp-login.php?redirect_to=http://blog.multisite.com/foo/.

Since the user is not a member of http://blog.multisite.com/, though, the user has no read cap. My redirect_to parameter is ignored and the user is sent to user_admin_url().

A sort of strange scenario, I know. Be that as it may, I believe these wp-login.php redirect_to conditionals are meant to prevent an unauthorized user from being redirected to an *admin* url that he/she doesn't have access to. They shouldn't prevent the user from going to a blog URL.

#8 @mdawaffe
13 years ago

  • Keywords has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Possibilities:

  1. Check to see if $redirect_to is empty before testing these new in 3.1 conditionals.
  2. Check if the already set $redirect_to is an admin URL for the current blog (for which we are checking caps). If so, go through with the new conditionals. If not, bail and let it through.

Reopening for comments.

#9 @nacin
13 years ago

Fine with adding empty( $redirect_url ) checks. See suggested patch in a second.

@nacin
13 years ago

#10 @nacin
13 years ago

After testing this for a bit, the logic in that area looks very cryptic. Even after being redirected to wp-admin/profile.php (case 3) I end up in the user admin anyway. Can't tell if this does no harm, does some harm, or reveals dead code.

@ryan
13 years ago

#11 @ryan
13 years ago

Implements possibility 2, which I've been meaning to do.

#12 @nacin
13 years ago

Looks good. Is there a way for redirect_to to remain empty after all of that? (Comment applied both pre and post patch.)

#13 @nacin
13 years ago

Thus, elseif ( empty( $redirect_to ) ) $redirect_to = admin_url();, or would that be dead code?

#14 @ryan
13 years ago

I think the only way we would have an empty redirect_to is if a plugin hooked to login_redirect made it empty. The empty() check in the existing if probably isn't needed.

#15 @ryan
13 years ago

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

(In [17351]) Don't try to redirect to user admin for unpriv users unless a site admin redirect was requested. fixes #16297

#16 @helenyhou
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In multisite, when I have a user who is an editor on one site (not the main one) and they log into the main site, they are sent to a global dashboard instead of the dashboard for the site they are assigned to. This seems unexpected to me and said users, and is definitely a change from recent versions.

@ryan
13 years ago

#17 @ryan
13 years ago

Patch uses get_dashboard_url() to redirect to the active blog if the user doesn't have access to the current blog.

#18 @helenyhou
13 years ago

Patch works for me.

#19 @ryan
13 years ago

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

(In [17367]) Use get_dashboard_url() to redirect to the active blog if the user doesn't have access to the current blog. fixes #16297

#20 @cgrymala
13 years ago

I have found one issue with this fix that may confuse users. If a user is a "Super Admin" with no explicit role on the "site", they are still redirected to the User Admin instead of the Site Admin area, even though a Super Admin should (and does) have full permissions on every site within the network. For some Super Admin users, this could get really confusing; as they click on the "Site Admin" link while in the Network Admin area, and get redirected to a page with no options except to update their own profiles.

I think:

if ( is_multisite() && !get_active_blog_for_user($user->id) )

should be changed to:

if ( is_multisite() && !get_active_blog_for_user($user->id) && !is_super_admin() )

Note: See TracTickets for help on using tickets.