Any page with a node on it is completely uncacheable if you do not have the "access user profiles" permission

Relevant code:

// UserAccessControlHandler::checkAccess() snippet
 case 'view':
        // Only allow view access if the account is active.
        $result = AccessResult::allowedIfHasPermission($account, 'access user profiles');

        if ($result->isAllowed()) {
          // Does not run if you do not have the permission.
        }

        // Users can view own profiles at all times.
        return $result->orIf(AccessResult::allowedIf($account->id() == $entity->id())->addCacheContexts(['user']));

// node.module template_preprocess_node()
  $skip_custom_preprocessing = $node->getEntityType()->get('enable_base_field_custom_preprocess_skipping');
  $submitted_configurable = $node->getFieldDefinition('created')->isDisplayConfigurable('view') || $node->getFieldDefinition('uid')->isDisplayConfigurable('view');

  if (!$skip_custom_preprocessing || !$submitted_configurable) {
    $variables['date'] = \Drupal::service('renderer')->render($variables['elements']['created']);
    unset($variables['elements']['created']);
    $variables['author_name'] = \Drupal::service('renderer')->render($variables['elements']['uid']); // THIS TRIGGERS USER ACCESS CHECK.
    unset($variables['elements']['uid']);
  }

A label-specific permissions seems like a good idea.

Linking an old issue.

Crediting my colleague Anna as she found this bug and managed to narrow it down to node.module. I took over from there.

kristiaanvandeneynde → credited anna d → .

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.

Posted a proof-of-concept that I can confirm on our projects skips the user 'view' access check and trades it in for a 'view label' access check.

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.

Hmm, this is awkward. We have the same symptoms on two different projects and the MR fixes it. We do use a sort of base profile of our own so let me see if that or any of the modules it ships with interferes with $variables['elements']['uid'] in any way. Have a full day scheduled for this tomorrow so will see what I can find.

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');
  }

As you can see, the access check's cacheable netadata is not used at all. Isn't that a bug on its own? You'd think that whatever cacheable metadata determines user view access needs to be added to the page.

I also confirmed that $variables['elements']['uid'] is the same on a vanilla install and our installs.

Still digging why on our sites it does bubble up (or perhaps comes from somewhere else). The rabbit hole goes deeper.

Had a quick look at user_preprocess_user() and was horrified. Opened 📌 Remove anonymous commenter support from user module Active 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.

Okay so I'm going to be more careful with my statements now because I resent that I cried wolf and it wasn't reproducible on a clean install. I do hope you understand I felt compelled to create a critical issue given the impact if my findings were true.

The bug we found on one project was due to the markerio module adding the user cache context to every single page. See 🐛 This module kills Dynamic Page Cache Active

The reason I came across core first, was due to how I was debugging. I put XDebug listeners on the Renderer and RefinableCacheableDependencyTrait to track down where the user cache context was being added. This led to me seeing a lot of user cache contexts being merged in (one for every node on the page) and that never happened before. Only after the UserAccessControlHandler fix mentioned in the IS does this happen.

Now, because we are needlessly rendering user names and user names can link to a profile if you have access to it, we basically run a user 'view' access check on every page with a node on it. This leads to the user cache context being added to something that luckily never gets used. But that still feels really dirty.

The bug is very much alive in core, because by all means should that access check actually bubble up. So the only thing saving us here from an uncacheable website is the fact that template_preprocess_username() does a poor job at consuming the cacheable metadata of an access check it ran.

Damn...

Marking as major because of the access checks not bubbling up, but it's no longer critical as one bug cancels out the other. Just not sure how we should proceed here. Feels like a lot of work to rephrase this issue to be about the access bug in template_preprocess_username() so maybe we should open a follow-up but mark it as "please don't fix yet or all hell will break loose" and postpone it until we no longer needlessly render usernames all over.

Also keep in mind that if we fix the access check bubbling up, any page that has author names with links enabled will start varying by user, killing caching once more. So perhaps we need to rethink whether we want to keep that "can access own profile" logic.

Either way, will debug our 2nd project with the same symptoms now. Could very well also be markerio running havoc there.

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.

This would also be an acceptable fix. 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?

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.

By the way, doesn't the needless rendering of usernames mean that we tag said pages with the 'user:ID' cache tag? So if you have an expensive page with 20 nodes on it from different authors and one of them updates their account, your expensive page gets invalidated? Even if you never showed any of their names to begin with?

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 = new \Drupal\Core\Render\BubbleableMetadata();
   $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.

Thanks for the confirmations @godotislate!

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. So maybe if we were to fix this earlier, we could use the MR here as a current workaround until template_preprocess_node() is properly refactored.

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.

I like where this is headed. Have only reviewed the MR on the surface, but the new entity access operation makes sense. Thanks!

Wow!

Tbh, I am a bit hesitanr about introducing a new entity operation, although I am fully understanding why this is feels a compelling solutions. 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 it.

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

kristiaanvandeneynde → changed the visibility of the branch 3506444-any-page-with to hidden.

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