Problem/Motivation

when a custom URL under "Link display" is used for a given display, there is not anything in views to check user access to these links.

@dawehner suggested using the access system provided by the routing system.

this was discovered in #2020393-11: Convert "Recent content" block to a View, when a "More" link was being added.

Steps to reproduce

Instructions to reproduce the issue:

  1. Install fresh Drupal (standard profile)
  2. Go to /admin/structure/views/add, to add new view
    1. Name the view as "Admin only"
    2. Select "Create a page"
    3. Click "Save and edit"
    4. Under "Page Settings" choose a link after "Access: Permissions". By default it's "View published content".
    5. Choose "Administer blocks"
    6. Save the view
  3. Go to /admin/structure/views/add, to add new view.
    1. Name the view as "Public"
    2. Select "Create a block"
    3. Click "Save and edit"
    4. Under "Pager" choose link after "More link:". By default it's no.
    5. Select "Create more link" and "Always display the more link"
    6. Click "Apply"
    7. Under "Pager" choose link after "Link display:". By default it's "none".
    8. Select "Custom URL" and enter "admin-only" to the field
    9. Click "Apply"
    10. Click "No Results Behavior" -> Add
    11. Choose "Global: Text area"
    12. Click "Apply"
    13. Write "This is a public view" to the textfield
    14. Click "Apply"
    15. Save the view
  4. Go to /admin/structure/block/add/views_block%3Apublic-block_1/bartik
    1. Select "Sidebar first" from the "Region" dropdown menu
    2. Click "Save Block"
  5. Log out
  6. Go to the front page
  7. Link "more" is showing on a public block.

Proposed resolution

Check if user has access to the read more link.

Remaining tasks

Code review

User interface changes

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

This is just a rough outline, what to do.

By general idea here is to use Drupal::service('router.route_provider')->getRoutesByPattern in DisplayPluginBase::renderMoreLink().

Once you have the route you can use menu_item_route_access (or its successor, once its exists) to check the actual access.

jiv_e’s picture

Assigned: Unassigned » jiv_e

Hi!

I'll try to take this one. dawehner introduced this on IRC. This is my first D8 issue so I hope you can bare with me.

dawehner said on IRC:
- have a look at core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php::renderMoreLink()
- there is simply no access check going on at all

He also gave me instructions to reproducing the issue:
- for exampe create two views, of which one is restricted to admins
- the second one uses a more link with a custom URL
- which has the URL of the first view

I tried to write a more detailed description:

Instructions to reproduce the issue:

  1. Install fresh D8 (standard profile)
  2. Go to /admin/structure/views/add, to add new view
    1. Name the view as "Admin only"
    2. Select "Create a page"
    3. Click "Save and edit"
    4. Wait for the form to reload
    5. Under "Page Settings" choose the link after "Access: Permissions". By default it's "View published content".
    6. Choose "Administer blocks" and save the view.
  3. Go to /admin/structure/views/add, to add new view.
    1. Name the view as "Public"
    2. Select "Create a page"
    3. Click "Save and edit"
    4. Wait for the form to reload
    5. ...

I realize that now I have to provide the custom more link to the "Public" view. I don't know how. I couldn't find a way to do that in the UI. I found the code you were talking about here. It's not about the view, but the old code your are trying to get rid of, right? I found it from the node.module's function theme_node_recent_block. It's about the more link though.

I also checked the views config file (views.view.public.yml). I found no pointers there.

dawehner’s picture

Thanks for writing this down, that is quite helpful!
If you do have a block display for example you can configure under Link display which display to link to or specify a custom path.

jiv_e’s picture

I had time to look at the code in DisplayPluginBase::renderMoreLink().

I'm probably able to solve the issue on this level. But I'm thinking is there already some general solution to this problem? I would presume this kind of checking for links is needed in many occasions. So it seems that this checking should be centralized. Do you agree?

I'm also wondering why views is rendering the link tag in a template file.

view-more.html.twig:

<div class="more-link">
  <a href="{{ more_url }}">
    {{ link_text }}
  </a>
</div>

Shouldn't we use LinkGenerator::generate to create the link tag?

Could we add the access check to LinkGenerator?

EDIT: I discussed this on IRC. Timplunkett and webchick thought it would be slow to implement this in LinkGenerator. I suggested adding a parameter to generate function if the check is needed.

webchick: jiv_e: Hm. Mmmmaybe. But that just makes the function harder to parse, since it'd have this whole branch of code that'd only execute for some links. It's probably still better to handle in the caller.

I suggested a separate helper function for doing this kind of general access check. I haven't seen any counter arguments yet. I would say this would be good because this is a general task and may be needed elsewhere. So it would reduce duplicate code.

I'm also thinking:

jiv_e:...it's also strange that a site builder decides to make a view on some content as a block and let's somebody see that even if that user doesn't have access to the more page. So in this case we could blame the site builder, right?

If we accept this then this issue shouldn't be fixed.

jiv_e’s picture

Status: Active » Closed (won't fix)

After discussing with dawehner in IRC we decided that we should not fix this.

Reason is that it's seems to be an exotic edge case if someone needs to build a views block without restricted access and then restrict the access to the "more page". This functionality needs not to be in core.

Also... this is not a security issue because after clicking the link user gets "Access denied" page. It would just be a small annoyance for the user and the site builder could fix it in different ways if it's really needed.

jiv_e’s picture

Issue summary: View changes

Add instructions to reproduce the issue

xjm’s picture

Assigned: jiv_e » Unassigned
Category: Task » Bug report
Status: Closed (won't fix) » Active

So, there's no way we can call this Views bug "esoteric", as it exists in a view that is in core. :)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hgoto’s picture

Status: Active » Needs review
FileSize
707 bytes

Now the more link is generated in DisplayPluginBase::renderMoreLink() and no template is used. So we can achieve this just by changing one line, maybe?

Drupal\views\Plugin\views\display\DisplayPluginBase::renderMoreLink():

before:

      if ($url) {

after:

      if ($url && $url->access()) {

Maybe, do we need a test?

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Ideally we somehow also apply the cacheability metadata. Note: We need some additional tests.

hgoto’s picture

@dawehner thank you for your review. I see! Surely we should. I'm searching how to apply cacheability metadata to them...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hgoto’s picture

Status: Needs work » Needs review
FileSize
1.21 KB

Here is a revised patch with cacheability metadata following the feedback #10. Tests are not yet added.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Hydra’s picture

Just rerolled the patch agains 8.9.13

Hydra’s picture

FileSize
1.27 KB

Removed the code style fixes

Hydra’s picture

Version: 8.9.x-dev » 9.1.x-dev
Category: Bug report » Feature request
FileSize
1.25 KB

Updated the patch for 9.1.x

ranjith_kumar_k_u’s picture

sulfikar_s’s picture

FileSize
30.49 KB
29.76 KB

Hi, I've tested the above patch on #22. It worked as per the requirements!

Attaching the before and after screenshots below,

Before,

before.png

After,

after.png

You can see there is no "more" link for the anonymous user. So RTBC+1

Version: 9.1.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gaurav-mathur’s picture

Assigned: Unassigned » gaurav-mathur
gaurav-mathur’s picture

Assigned: gaurav-mathur » Unassigned
FileSize
7.24 KB
7.02 KB

Applied patch #22 successfully in Drupal 9.4 and working fine.Adding screenshots to refer after and before patch.

smustgrave’s picture

Version: 9.4.x-dev » 10.1.x-dev

The patch in #22 still applies to 10.1 but is missing the tests. Moving back to NW for those

smustgrave’s picture

Status: Needs review » Needs work

The last submitted patch, 30: 2053015-30-tests-only.patch, failed testing. View results

Bushra Shaikh’s picture

FileSize
8.45 KB
7.03 KB

Verified patch #30 on Drupal 10.0.x and 10.1.x. Patch applied successfully and looks good to me.
Screenshots are attached for reference:
Can be moved to RTBC
RTBC+1

prasanth_kp’s picture

FileSize
39.96 KB
39.22 KB

Applied patch #30 and it fixes the issue.

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

The Issue Summary has steps to reproduce for Drupal 8. Is this still valid? Let's get an issue summary update, including a proposed resolution, and a set of the latest screenshots. And let's put the steps to reproduce under the correct heading. Adding tag for an issue summary update.

I took a very brief view of the patch.

  1. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
    @@ -294,6 +294,13 @@ public function testReadMoreCustomURL() {
    +    // Test if user doesn't have permission to link read more doesn't display.
    

    This sentence does not make sense to me. For one there are two negatives. And there should be quotes around 'read more'.

  2. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
    @@ -294,6 +294,13 @@ public function testReadMoreCustomURL() {
    +    $this->assertStringNotContainsString('/admin/content', $output, 'The read more link with href "/admin/content" was not found.');
    

    This is testing the absence of something. Do we have a test for the presence of 'read more' link when it should be visible?

  3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2115,12 +2115,26 @@ public function renderMoreLink() {
    +      // Add cache metadata with the link target page.
    

    I keep reading reading this sentence and expect a final prepositional phrase, as in, "Add cache metadata with the link target page to ....". Can this be clearer?

smustgrave’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
1.39 KB
2.24 KB

#34.1 updated
#34.2 a few lines above it's testing for the link
#34.3 updated

Updated issue summary.

Ranjit1032002’s picture

Status: Needs review » Reviewed & tested by the community

Patch #35 applied successfully and working as expected.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2115,12 +2115,26 @@ public function renderMoreLink() {
    +        $more_link['#cache']['contexts'][] = $url->toUriString();
    

    This certainly can't be right – \Drupal\Core\Url::toUriString returns the URI string representing the data in the object which doesn't necessarily have anything to do with the cache contexts 🤔

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2115,12 +2115,26 @@ public function renderMoreLink() {
    +        $this->applyDisplayCacheabilityMetadata($more_link);
    

    Why do we need to apply the cache contexts from the view display here? Aren't those already applied in \Drupal\views\Plugin\views\display\DisplayPluginBase::render?

  3. +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
    @@ -294,6 +294,13 @@ public function testReadMoreCustomURL() {
    +    $this->assertStringNotContainsString('/admin/content', $output, 'The read more link with href "/admin/content" was not found.');
    

    I do believe this works given that there was a failing test patch. Regardless of that, I'm wondering if we could add some additional assertion here to make sure that we are not just asserting for the wrong thing to not exist in the output?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.