Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
- Install fresh Drupal (standard profile)
- Go to /admin/structure/views/add, to add new view
- Name the view as "Admin only"
- Select "Create a page"
- Click "Save and edit"
- Under "Page Settings" choose a link after "Access: Permissions". By default it's "View published content".
- Choose "Administer blocks"
- Save the view
- Go to /admin/structure/views/add, to add new view.
- Name the view as "Public"
- Select "Create a block"
- Click "Save and edit"
- Under "Pager" choose link after "More link:". By default it's no.
- Select "Create more link" and "Always display the more link"
- Click "Apply"
- Under "Pager" choose link after "Link display:". By default it's "none".
- Select "Custom URL" and enter "admin-only" to the field
- Click "Apply"
- Click "No Results Behavior" -> Add
- Choose "Global: Text area"
- Click "Apply"
- Write "This is a public view" to the textfield
- Click "Apply"
- Save the view
- Go to /admin/structure/block/add/views_block%3Apublic-block_1/bartik
- Select "Sidebar first" from the "Region" dropdown menu
- Click "Save Block"
- Log out
- Go to the front page
- 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
Comment | File | Size | Author |
---|---|---|---|
#35 | 2053015-35.patch | 2.24 KB | smustgrave |
| |||
#35 | interdiff-30-35.txt | 1.39 KB | smustgrave |
Comments
Comment #1
dawehnerThis 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.
Comment #2
jiv_e CreditAttribution: jiv_e commentedHi!
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:
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.
Comment #3
dawehnerThanks 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.
Comment #4
jiv_e CreditAttribution: jiv_e commentedI 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:
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.
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:
If we accept this then this issue shouldn't be fixed.
Comment #5
jiv_e CreditAttribution: jiv_e commentedAfter 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.
Comment #5.0
jiv_e CreditAttribution: jiv_e commentedAdd instructions to reproduce the issue
Comment #6
xjmSo, there's no way we can call this Views bug "esoteric", as it exists in a view that is in core. :)
Comment #9
hgoto CreditAttribution: hgoto as a volunteer commentedNow 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:
after:
Maybe, do we need a test?
Comment #10
dawehnerIdeally we somehow also apply the cacheability metadata. Note: We need some additional tests.
Comment #11
hgoto CreditAttribution: hgoto as a volunteer commented@dawehner thank you for your review. I see! Surely we should. I'm searching how to apply cacheability metadata to them...
Comment #13
hgoto CreditAttribution: hgoto as a volunteer commentedHere is a revised patch with cacheability metadata following the feedback #10. Tests are not yet added.
Comment #19
Hydra CreditAttribution: Hydra as a volunteer and at erdfisch commentedJust rerolled the patch agains 8.9.13
Comment #20
Hydra CreditAttribution: Hydra as a volunteer and at erdfisch commentedRemoved the code style fixes
Comment #21
Hydra CreditAttribution: Hydra as a volunteer and at erdfisch commentedUpdated the patch for 9.1.x
Comment #22
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedUpdated
Comment #23
sulfikar_s CreditAttribution: sulfikar_s at Zyxware Technologies commentedHi, I've tested the above patch on #22. It worked as per the requirements!
Attaching the before and after screenshots below,
Before,
After,
You can see there is no "more" link for the anonymous user. So RTBC+1
Comment #26
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #27
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedApplied patch #22 successfully in Drupal 9.4 and working fine.Adding screenshots to refer after and before patch.
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commentedThe patch in #22 still applies to 10.1 but is missing the tests. Moving back to NW for those
Comment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #32
Bushra Shaikh CreditAttribution: Bushra Shaikh at QED42 for Drupal India Association commentedVerified 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
Comment #33
prasanth_kp CreditAttribution: prasanth_kp as a volunteer and at Zyxware Technologies commentedApplied patch #30 and it fixes the issue.
Comment #34
quietone CreditAttribution: quietone at PreviousNext commentedThe 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.
This sentence does not make sense to me. For one there are two negatives. And there should be quotes around 'read more'.
This is testing the absence of something. Do we have a test for the presence of 'read more' link when it should be visible?
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?
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commented#34.1 updated
#34.2 a few lines above it's testing for the link
#34.3 updated
Updated issue summary.
Comment #36
Ranjit1032002Patch #35 applied successfully and working as expected.
Comment #37
lauriiiThis 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 🤔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
?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?