Problem/Motivation

Currently, when node authlink is being used, the page response is returning the header 'X-Drupal-Dynamic-Cache: UNCACHEABLE'. This is happening because node_authlink is caching the access per user: https://git.drupalcode.org/project/node_authlink/-/blob/8.x-1.x/src/Acce...

The reason why it happens is that Auto-placeholdering, with default configuration, is marking the dynamic cache as uncacheable when there are a user cache context.

Steps to reproduce

1. Install and configure node authlink
2. Ensure renderer.config auto_placeholder_conditions are configured by default:

  renderer.config:
    auto_placeholder_conditions:
      max-age: 0
      contexts: ['session', 'user']
      tags: []

Proposed resolution

Cache access per URL, so when the node authlink URL is set the access is calculated again.

Issue fork node_authlink-3450094

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

omarlopesino created an issue. See original summary.

omarlopesino opened merge request !12

omarlopesino’s picture

omarlopesino
he/him
commented
Status: Active » Needs review

I've just created a MR using URL cache contexts. I've tested manually and node Authlink works as usual:

- The Authlink URL only works on the proper URL.
- When the Authlink is removed, if I try to enter the Authlink URL on a browser without session, I get an access denied.

Please review, thanks!

omarlopesino’s picture

omarlopesino
he/him
commented

I would like to propose adding tests to the module. This issue affects the node access and having tests may prevent regressions or future security issues.

tunic’s picture

tunic
he/him
Primary language Spanish
Location Madrid
commented
Assigned: Unassigned » tunic
Priority: Normal » Major

I'm wondering if the module should enable caching.

I mean, the module decorates the EntityAccessCheck but that class does not add any caching information. And given that NodeAuthlink always adds caching information on any AccessResult it means any URL that calls the entity check access uses the cache info added by the module.

I think this may be too drastic. So I think the caching info should only be added for URLs handled by the module or even remove it completely. Anybody that has the URL should be able to access, so no need to cache by user.

Ill try add some automated tests to be sure which approach is the good one.

tunic’s picture

tunic
he/him
Primary language Spanish
Location Madrid
commented
Related issues: +#3450411: Add automated tests
omarlopesino’s picture

omarlopesino
he/him
commented

I've updated the patch to not use access contexts. In my tests, the module behaves as usual. Pending to review with the tests that will be added at https://www.drupal.org/project/node_authlink/issues/3450411

  • tunic committed 908b0df0 on 2.x authored by omarlopesino 
    Issue #3450094: Cache contexts used are not compatible with dynamic...
tunic’s picture

tunic
he/him
Primary language Spanish
Location Madrid
commented
Status: Needs review » Fixed

Tested, before we had:

x-drupal-dynamic-cache: UNCACHEABLE (poor cacheability)

Now we have:

x-drupal-dynamic-cache: MISS

This is an improvement, and good enough to close this issue.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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