UserThemeHooks::preprocessUsername() doesn't handle cacheability correctly

A label-specific permissions seems like a good idea.

Linking an old issue.

https://www.drupal.org/node/2726125 if we used the entity reference label formatter, or equivalent logic, then we'd not be doing these permissions checks at all.

I'm not able to reproduce this issue on HEAD with standard profile install:

Steps taken:

• Install Drupal core with standard profile
• Uninstall page_cache module
• Set system.performance cache.page.max to 86400 (Browser and proxy cache maximum age 1 day)
• Confirm no roles besides administrator have "View user information" (access user profiles) permission
• Log in as admin and create an Article node and save
• View the node as anonymous
• Observe Cache-Control header is "max-age=86400, public" and X-Drupal-Dynamic-Cache is "MISS"
• Reload and observe X-Drupal-Dynamic-Cache is "HIT"
• As admin, Add a comment to the node
• Reload node page as anon and X-Drupal-Dynamic-Cache is "MISS"
• Reload node page as anon and X-Drupal-Dynamic-Cache is "HIT"

Additionally, in this line in template_preprocess_node():
$variables['author_name'] = \Drupal::service('renderer')->render($variables['elements']['uid']);
$variables['elements']['uid'] contains a render array with the uid field using the "author" formatter (Drupal\user\Plugin\Field\FieldFormatter\AuthorFormatter), which uses the same "view label" entity access check as the label formatter.

Didn't try to reproduce yet, but I can't imagine that we don't have test coverage for this, between dynamic page cache tests, performance tests and so on.

I think this is needs more info. The umami node page performance tests should include dynamic page cache hits on a node, not sure if they have the right permissions, and because they're real browser tests they can't check headers so it would be more checking the dynamic page cache hit.

I do think we should try to gut node preprocess though either way.

Had a quick look at user_preprocess_user() and was horrified. Opened #3506680: Remove anonymous commenter support from user module for a start.

On the view profile stuff, I think we might need a 'view linked label' entity access operation, which does everything except the own account check, and then 'view' also includes the own account check or something like that. We should not be linking to people's own profiles in node submitted information when they otherwise wouldn't get a link for anyone else, that's just weird.

But what would we render in that case when you do not have access to view profiles? A label that links to an account but gives a 403 as you click on it?

If you don't have access to view profiles, you get a label that doesn't link, just the display name for the user. Uploading a screenshot from my 11.x sandbox logged out.

I think that the current logic in HEAD means that you would see this for every user, except for yourself - and to me that is a user facing bug as well as a caching issue because there is just no reason to render this differently for the current user - you can get to your own profile from the user menu etc.

If we're eating the cache contexts, then what probably happens is that most of the time, the username doesn't get linked, because someone else views the article first before it gets in render cache, but very occasionally, it does get linked for the current user, and then it would be incorrectly cached as a link, leading to a 403. Didn't try to reproduce this though.

I didn't check if we're throwing away the cache tags too or not, but yes if we're not that would happen. And if we're throwing away the cache tags then unless something else adds the user as a cacheable dependency then content won't reflect changes to the username.

By the way, doesn't the needless rendering of usernames mean that we tag said pages with the 'user:ID' cache tag?

Just checked, and when I comment out this line in template_preprocess_node():

$variables['author_name'] = \Drupal::service('renderer')->render($variables['elements']['uid'])

The user:ID tag is not in the X-Drupal-Cache-Tags header.

When I restore the line, user:ID is in the header.

I can confirm that on a vanilla install it does get called, but the result is discarded. From user.module template_preprocess_username():

  if ($account instanceof AccessibleInterface) {
    $variables['profile_access'] = $account->access('view');
  }

I just looked at template_preprocess_username() and as mentioned there is no capturing of cache metadata at all. In the part mentioned above:

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

In either condition, the value returned and set to $variables['profile_access'] is a boolean, and not an access object, so there is no cache metadata.

As an experiment, I tested this change:

diff --git a/core/modules/user/user.module b/core/modules/user/user.module
index a22c79094da..2d2e681a81f 100644
--- a/core/modules/user/user.module
+++ b/core/modules/user/user.module
@@ -138,7 +138,11 @@ function user_preprocess_block(&$variables): void {
  *   - account: The user account (\Drupal\Core\Session\AccountInterface).
  */
 function template_preprocess_username(&$variables): void {
+  $metadata = \Drupal\Core\Render\BubbleableMetadata::createFromRenderArray($variables);
   $account = $variables['account'] ?: new AnonymousUserSession();
+  if ($account instanceof \Drupal\Core\Cache\CacheableDependencyInterface) {
+    $metadata->addCacheableDependency($account);
+  }

   $variables['extra'] = '';
   $variables['uid'] = $account->id();
@@ -164,11 +168,13 @@ function template_preprocess_username(&$variables): void {
   }
   $variables['name'] = $name;
   if ($account instanceof AccessibleInterface) {
-    $variables['profile_access'] = $account->access('view');
+    $profile_access = $account->access('view', NULL, TRUE);
   }
   else {
-    $variables['profile_access'] = \Drupal::currentUser()->hasPermission('access user profiles');
+    $profile_access = \Drupal\Core\Access\AccessResultAllowed::allowedIfHasPermission(\Drupal::currentUser(), 'access user profiles');
   }
+  $metadata->addCacheableDependency($profile_access);
+  $variables['profile_access'] = $profile_access->isAllowed();

   $external = FALSE;
   // Populate link path and attributes if appropriate.
@@ -198,6 +204,7 @@ function template_preprocess_username(&$variables): void {
       ])->toString();
     }
   }
+  $metadata->applyTo($variables);
 }

 /**

Now when viewing the node, I do see the x-drupal-dynamic-cache header as "UNCACHEABLE (poor cacheability)" as well as "user" in x-drupal-cache-contexts.

I also updated this issue's title from template_preprocess_user() to template_preprocess_username(), but please correct if this is a mistake.

On the view profile stuff, I think we might need a 'view linked label' entity access operation, which does everything except the own account check,

Keep in mind that if we fix user.module here before we fix node.module in another issue, we actually will introduce the critical issue I reported in the first place.

I think the fix might be easier than expected, and doesn't need a new entity access operation.

Changing this in template_preprocess_username():

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

to

  $variables['profile_access'] = FALSE;
  if ($account instanceof UserInterface) {
    $profile_access = AccessResultAllowed::allowedIfHasPermission(\Drupal::currentUser(), 'access user profiles');
    $metadata->addCacheableDependency($profile_access);
    $variables['profile_access'] = $profile_access->isAllowed();
  }

should hopefully do all that's needed.

I think hard-coding the permission is fine in this case so #31 looks good to me.

I was wrong about hard-coding the permission. For the "view" operation in UserAccessControlHandler::checkAccess(), there are also checks whether the user being viewed is active. Adding the new 'view linked label' operation is relatively low lift anyway, so pushed up MR 11216.

The IS needs an update, which I can get to later.

Wow!

Tbh, I am a bit hesitant about introducing a new entity operation, although I am fully understanding why this is feels a compelling solution. Why am I hesitant? Because I have been involved several private conversations about what *view label" is and what not; and I am still not convinced that the majority of devs aware of that entity operation.

Would "view linked label" become a generic entity operation or username only? Should it be handled properly by all logic in core/contrib?

What I can also add that I have an extensive test coverage for username access in my module with a test controller that could be also useful foe debugging.

https://git.drupalcode.org/project/view_usernames/-/blob/1.x/tests/modul...

I can't think of a use case for the operation other than for user accounts.

Found while triaging Node issues, this definitely seems like a User module thing.

I haven't read the full thread but I'm a bit confused why we'd want to show a linked username if the user doesn't have access to view the profile? We'd be showing a link to a 403?

I haven't read the full thread but I'm a bit confused why we'd want to show a linked username if the user doesn't have access to view the profile? We'd be showing a link to a 403?

No, the username label would not be linked at all. It would just be text.

The effective difference only applies to users who are viewing their own username's label. Access for the "view" operation is allowed for users with the "access user profiles" permission and for users viewing their own profile. Access for the "view linked label" operation is allowed for users with the "access user profiles" permission and ignores whether the user is viewing their own label. This means that the label is displayed as a link to all users, or displayed as text to all users. This removes the need for the user cache context, and it also makes the label display consistent to all users.

fwiw #40 sounds like how it should always have worked - there are plenty of ways for people to get to their own account.

Saw these two issues today that seem to tread similar ground but haven't looked closely:
#3386313: The entity link label formatter should check URL access
#3293287: EntityReferenceLabelFormatter returns a link even if the user only have "view label" access

Wanted to see what this one needs next?

Wanted to see what this one needs next?

I resolved MR threads and rebase to get the tests to run again. I think it's ready.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-based.

Updating title since function was moved.

I rebased this to try and reproduce the issue during my review, but I'm having problems with manual testing. While reading through the issue comments (again) to try and get hints I found this quote from #29:

Keep in mind that if we fix user.module here before we fix node.module in another issue, we actually will introduce the critical issue I reported in the first place.

It sounds like this issue needs to be postponed to deal with the node module bug first. Is that right? Or have things changed? Has another issue been opened to fix the node module bug yet?

I'm also tagging this for an issue summary update because it is next-to-impossible to figure out what this issue is about from it. In particular, the Problem/Motivation section needs to be cleaned up, preferably with a short summary of the problem at hand.

For those reasons I'm setting the issue status to Needs Work. I reviewed the code. It looks good. I couldn't find anything to comment about.

@dcam the node issue is #2528314: template_preprocess_node() does not add cacheability metadata but was marked as duplicate of #3458589: Deprecate $variables['page'] for node.html.twig and node_is_page(), however that issue definitely didn't fix the problem mentioned here.

The relevant section in node preprocessNode() is:

  // Make created, uid and title fields available separately. Skip this custom
    // preprocessing if the field display is configurable and skipping has been
    // enabled.
    // @todo https://www.drupal.org/project/drupal/issues/3015623
    //   Eventually delete this code and matching template lines. Using
    //   $variables['content'] is more flexible and consistent.
    $submitted_configurable = $node->getFieldDefinition('created')->isDisplayConfigurable('view') || $node->getFieldDefinition('uid')->isDisplayConfigurable('view');
    if (!$skip_custom_preprocessing || !$submitted_configurable) {
      $variables['date'] = !empty($variables['elements']['created']) ? $this->renderer->render($variables['elements']['created']) : '';
      $variables['author_name'] = !empty($variables['elements']['uid']) ? $this->renderer->render($variables['elements']['uid']) : '';
      unset($variables['elements']['created'], $variables['elements']['uid']);
    }

That $variables['author_name'] is the problem identified above and it's very much still there.

@catch Thank you for clarifying that. I looked at that other issue, but I wasn't sure that it was about the same node problem since it:
A. Is a much older issue than this one, so it couldn't have been opened in response to this
B. Got closed as a duplicate.

I've updated IS, and it's hopefully more clear.

I rebased this to try and reproduce the issue during my review, but I'm having problems with manual testing. While reading through the issue comments (again) to try and get hints I found this quote from #29:

Keep in mind that if we fix user.module here before we fix node.module in another issue, we actually will introduce the critical issue I reported in the first place.

It sounds like this issue needs to be postponed to deal with the node module bug first. Is that right? Or have things changed? Has another issue been opened to fix the node module bug yet?

The issue brought up in #29 is that bubbling cacheable metadata correctly from the preprocess method would bubble the user cache context, which is very undesirable. We're addressing that here with the introduction of the view linked label operation, which is different from the view operation, in that we don't care if the viewing user and the viewed user are same. The UI experience will be that all users who can't access user profiles see the user name as static text, all user who can access user profiles see the user name as a link.

Just in case, I also added a CR for the 'view linked label' entity operation: https://www.drupal.org/node/3565758

One additional note: While access to the 'view label' operation is granted to all users in UserAccessControlHandler::checkAccess(), I wonder whether it needs to be accounted for here, in cases where user access control handler is overridden or 'view label' access is otherwise modified. Drupal\user\Plugin\Field\FieldFormatter\AuthorFormatter, which uses the username theme element, has its own access check against 'view label', so maybe it makes sense that the expectation in any use case is that access to view the user name at all is checked before the username theme element is added?

@godotislate, thank you for indulging my request for more and better information. I understand it much better now.

I've updated IS, and it's hopefully more clear.

Yes, it is. I don't know where I went wrong with my manual testing previously because I think I did most of the things in the steps to reproduce, but I wasn't getting the cached linked usernames. This time I was able to reproduce the problem accurately with your new instructions.

The issue brought up in #29 is that bubbling cacheable metadata correctly from the preprocess method would bubble the user cache context, which is very undesirable. We're addressing that here with the introduction of the view linked label operation...

Got it. Obviously I didn't put two and two together and understand that the new operation was meant to correct that problem. Thank you for taking the time to explain it to me.

The new change record looks good to me.

With my better understanding of the issue I re-reviewed the code. It still looks good to me overall. I only found one documentation issue to nitpick. I hope you don't mind me setting this to Needs Work for that.

Finally, I noticed #849602: Update 'username' theme template to use 'view label' operation. in the Needs Review queue earlier today. I checked its MR and it is modifying the same hook. These two will conflict with each other.

Ok, we cross-posted. My suggestion was already applied. So I'm going to go ahead and RTBC this.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased and set this issue version to main.

Committed/pushed to main and 11.x, thanks!

Note: This could have some tricky side effects in custom/contrib code if they did something special on the previously used operation. We noticed #3588357: Handle new "view linked label" access operation in our performance tests, which is minor, but there might be cases where extra access checks either allowed or disallowed access, which now no longer works. It's likely just a bug and not a security issue for them, as it just controls whether or not there is a link, but still a bit concerning.

Maybe the CR can more clearly mention that access hooks might need to handle this operation.