WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#25162 closed defect (bug) (fixed)

Users with no role can see inaccessible dashboard links

Reported by: johnbillion Owned by: johnbillion
Milestone: 4.4 Priority: normal
Severity: minor Version: 3.0
Component: Users Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

This only affects single site installs. Multisite works as expected thanks to the Global Dashboard.

Steps to reproduce:

  1. Edit a user on a single site install and change their role to "No role for this site"
  2. Log in as that user
  3. Note that you're immediately presented with a "You do not have sufficient permissions..." message due to the attempt to access the admin dashboard
  4. Visit the site home page and note that there are links to the admin dashboard and user profile screen in the admin toolbar that also result in a "You do not have sufficient permissions..." message

When a user with no role logs in we should:

  1. Redirect to the home page rather than the admin dashboard
  2. Not show the inaccessible links in the admin toolbar

Attachments (5)

ticket-25162.diff (1.4 KB) - added by pauldewouters 5 years ago.
25162.diff (2.6 KB) - added by johnbillion 5 years ago.
25162.2.diff (2.4 KB) - added by johnbillion 4 years ago.
25162.3.diff (9.9 KB) - added by johnbillion 4 years ago.
25162.4.diff (10.7 KB) - added by johnbillion 4 years ago.

Download all attachments as: .zip

Change History (27)

#1 @kpdesign
6 years ago

Would you still want this user to be able to see/edit their profile (no dashboard), or only access the public side of the website?

#2 @johnbillion
6 years ago

I don't think we should change the dashboard behaviour (no access for users with no role), just correct the display of inaccessible links.

Last edited 6 years ago by johnbillion (previous) (diff)

#3 follow-up: @nacin
6 years ago

No dashboard access means no ability to edit their profile, including their password.

I have never fully enjoyed that "No role for this site" and "Subscriber" are both exposed on single-site. "No role for this site" should really only be shown when a custom user table is in play. Not having a role in multisite is akin to removing the user.

#4 @kpdesign
6 years ago

On single site, the only way an admin can change a user's role to "No role for this site" is after they are already registered. New User Default Role on Settings > General starts at Subscriber.

Do we just want to work on the site redirect and fix the admin bar links/Meta widget links with this ticket? I can work on creating a patch, just want to verify the scope of this before I start.

#5 @johnbillion
6 years ago

kpdesign: Yes, I think that's what we should aim for. The redirect and the toolbar and meta widget links.

#6 in reply to: ↑ 3 @knutsp
6 years ago

Replying to nacin:

I have never fully enjoyed that "No role for this site" and "Subscriber" are both exposed on single-site. "No role for this site" should really only be shown when a custom user table is in play.

We need "No role for this site" on single site installs even if this site doesn't use custom user table. The default table of this site may be used by another site that has custom user table (and usermeta).

#7 @pauldewouters
5 years ago

Made an attempt at a patch if anyone feels like reviewing.

@johnbillion
5 years ago

#8 @johnbillion
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.3

Updated and more comprehensive patch.

#9 @johnbillion
5 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner set to johnbillion
  • Status changed from new to assigned

This needs a bit more work. Patch coming up soon.

#10 follow-up: @obenland
4 years ago

@johnbillion, what's the status on that patch? Is this still going to make it into 4.3?

#11 @helen
4 years ago

  • Milestone changed from 4.3 to Future Release

No word on a patch in months, not a new problem, let's punt.

#12 in reply to: ↑ 10 @knutsp
4 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

@johnbillion, what's the status on that patch? Is this going to make it into 4.4?

#13 @johnbillion
4 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Future Release to 4.4

#14 @johnbillion
4 years ago

In 33924:

When a user with no role logs in, redirect them to the home page rather than their profile screen which they do not have access to.

See #25162

#15 @johnbillion
4 years ago

In 33929:

Remove the 'Site Admin' link from the Meta widget if the user doesn't have access to the admin area.

See #25162

@johnbillion
4 years ago

#16 @johnbillion
4 years ago

  • Keywords needs-unit-tests added

25162.2.diff covers users with no role both on single site and multisite. I'll get some unit tests done.

@johnbillion
4 years ago

#17 @johnbillion
4 years ago

  • Keywords has-patch commit added; needs-patch needs-unit-tests removed

25162.3.diff adds tests which cover users with and without roles on single site and multisite.

#18 @johnbillion
4 years ago

  • Keywords needs-patch added; has-patch commit removed

Ooh, turns out menu items don't need a link, which means the # links don't need to be used. Needs a CSS tweak though.

@johnbillion
4 years ago

#19 @johnbillion
4 years ago

  • Keywords has-patch commit added; needs-patch removed

#20 @johnbillion
4 years ago

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

In 34122:

Update links to the user profile editing screen in the admin toolbar when the current logged in user has no role on the current site. Covers single site and Multisite and introduces tests.

Fixes #25162

#21 @jorbin
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@johnbillion - Looks like this change broke some unit tests for multisite. Can you take a look? https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/80279617

#22 @johnbillion
4 years ago

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

In 34172:

Initialise $_SERVER[ 'REMOTE_ADDR' ] during the test bootstrap so individual tests need not.

Fixes #33877
Fixes #25162

Note: See TracTickets for help on using tickets.