The user formatter has a setting "Link to the user". The link is only displayed if the account (user that the output is being generated for) has access to view the target user (the one whose name is being displayed).

Expected behaviour: Drupal check access accurately, calling all custom hooks such as hook_entity_access.
Actual behaviour: Drupal checks permission 'access user profiles'

This isn't a security issue - when the link is clicked to visit the page the correct access check is performed, so there will be access denied/403 if necessary.

CommentFileSizeAuthor
#28 user.username.formatter.2921114-24.patch5.8 KBadamps
#24 user.username.formatter.2921114-interdiff-22-24.txt1.1 KBadamps
#24 user.mail_.requried-2992848-24.patch16.91 KBadamps
#22 user.username.formatter.2921114-22.patch5.79 KBadamps
#18 user.username.formatter.2921114-interdiff-16-18.txt1.9 KBadamps
#18 user.username.formatter.2921114-18.patch5.78 KBadamps
#16 user.username.formatter.2921114-interdiff-12-16.txt2.48 KBadamps
#16 user.username.formatter.2921114-16.patch5.92 KBadamps
#12 user.username.formatter.2921114-interdiff-6-12.txt3.7 KBadamps
#12 user.username.formatter.2921114-12.patch3.27 KBadamps
#6 user.username.formatter.2921114-6.patch3.1 KBadamps
#5 user.username.formatter-only.tests-2921114-5.patch2.07 KBadamps
#3 user.username.formatter.2921114-3.patch561 bytesadamps

Comments

AdamPS created an issue. See original summary.

adamps’s picture

adamps commented
Title: Username template ignores entity access » Username formatter ignores entity access
adamps’s picture

adamps commented
Status: Active » Needs review
Issue tags: +Needs tests, +Contributed project blocker
StatusFileSize
new user.username.formatter.2921114-3.patch561 bytes

Blocker for Administer Users by Role.

Status: Needs review » Needs work

The last submitted patch, 3: user.username.formatter.2921114-3.patch, failed testing. View results

adamps’s picture

adamps commented
Issue tags: -Needs tests
StatusFileSize
new user.username.formatter-only.tests-2921114-5.patch2.07 KB

Some of the existing tests cover this. At the moment they assert the presence of a span, which is incorrect for a user with admin permission - it should be a. First patch is test-only, should fail.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new user.username.formatter.2921114-6.patch3.1 KB

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented

The patch looks good and makes sense +1 for RTBC.

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community

Status is RTBC based on #9

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: user.username.formatter.2921114-6.patch, failed testing. View results

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new user.username.formatter.2921114-12.patch3.27 KB
new user.username.formatter.2921114-interdiff-6-12.txt3.7 KB

Reroll

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community

This was just a re-roll so assume it doesn't need another review - putting status back to RTBC

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented

The re-roll looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/user/user.module
@@ -501,7 +502,12 @@ function template_preprocess_username(&$variables) {
-  $variables['profile_access'] = \Drupal::currentUser()->hasPermission('access user profiles');
+  if ($account instanceof AccessibleInterface) {
+    $variables['profile_access'] = $account->access('view');
+  }
+  else {
+    $variables['profile_access'] = \Drupal::currentUser()->hasPermission('access user profiles');
+  }

For a more BC friendly change should this be something like (there might be neater logic)

  $variables['profile_access'] = FALSE;
  if ($account instanceof AccessibleInterface) {
    $variables['profile_access'] = $account->access('view');
  }
  $variables['profile_access'] = $variables['profile_access'] || \Drupal::currentUser()->hasPermission('access user profiles');

Also the test coverage seems quite indirect. I think we could do with some more explicit test coverage. Especially with the extra conditionality being introduced by this patch.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new user.username.formatter.2921114-16.patch5.92 KB
new user.username.formatter.2921114-interdiff-12-16.txt2.48 KB

Here is a new patch with an explicit test.

Regarding BC, I propose that in this case the existing behavior is a bug so we do want to change it. The specific case that is relevant to BC is if the user has 'access user profiles' permission but does not have access. That can happen if an access hook returns forbidden.

  • Existing behavior: show a link but if the user clicks it they get 403 because they don't have access = bug
  • New behavior: don't show a link

Please can someone review the test change then set back to RTBC?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/user/tests/src/Functional/Views/UserFieldsAccessChangeTest.php
@@ -24,11 +24,15 @@ class UserFieldsAccessChangeTest extends UserTestBase {
 
   /**
+   * Path to the test_user_fields_access view.
+   */
+  const VIEW_PATH = 'test_user_fields_access';
+

Why is this added?

Usually we just duplicate paths in test, this just adds indirection which makes it IMHO harder to understand.

adamps’s picture

adamps commented
StatusFileSize
new user.username.formatter.2921114-18.patch5.78 KB
new user.username.formatter.2921114-interdiff-16-18.txt1.9 KB

@Berdir thanks for the review - that's now fixed in the new patch.

adamps’s picture

adamps commented

@a.milkovsky, @Berdir or anyone else please can you set to RTBC so we can unblock this and hopefully get it fixed in time for D8.7?

It just needs a quick review of the tests, which maybe Berdir has done already? No need to actually test because the code hasn't changed since last time, only tests have changed.

a.milkovsky’s picture

a.milkovsky
he/him
Location UTC +2
commented
Status: Needs review » Reviewed & tested by the community

Looks good -> RTBC

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll 
error: patch failed: core/modules/user/tests/src/Functional/UserAdminTest.php:71
error: core/modules/user/tests/src/Functional/UserAdminTest.php: patch does not apply
make: *** [patch87] Error 1
adamps’s picture

adamps commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new user.username.formatter.2921114-22.patch5.79 KB

@larowlan Thanks here is the reroll

Status: Needs review » Needs work

The last submitted patch, 22: user.username.formatter.2921114-22.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new user.mail_.requried-2992848-24.patch16.91 KB
new user.username.formatter.2921114-interdiff-22-24.txt1.1 KB

Fix more deprecations and coding standards

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community

This is just a reroll, the patch is the same as before, so it doesn't need reviewing again

adamps’s picture

adamps commented

I tend to think this issue does not need a change notice because it is just fixing a bug. In case people think it does I don't want to delay commit of the issue so I have written one just in case - feel free to delete/ignore it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

#24 includes another patch.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new user.username.formatter.2921114-24.patch5.8 KB

Sorry that was the wrong patch file

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Adding issue credit

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed bd6a7df and pushed to 8.7.x. Thanks!

  • larowlan committed bd6a7df on 8.7.x 
    Issue #2921114 by AdamPS, Berdir: Username formatter ignores entity...
adamps’s picture

adamps commented
Status: Reviewed & tested by the community » Fixed

Great, many thanks @larowlan

@a.milkovsky helped by reviewing 3 patches so I wonder if we could add credit for him too?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Sure

adamps’s picture

adamps commented

Thanks.

Just one blocker in this area left now! #2992848: Allow customisation of permission to create user with blank email

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.