Preview links not rewriting

willguv commented 2 months ago

Flagged by Jamie

@stephen-cox I've created a preview link for a guide that has 11 pages. I clicked 'Add all pages to this guide' and when I view this preview link logged in I can see all the pages. When I'm logged out I can only see the first page. Does it only work for users who are logged in? Most colleagues I use this with will not have an account on the website.

Originally posted by @Dixonj1 in #600

willguv commented 1 month ago

@stephen-cox I think it's urgent we track down this problem and patch/ suggest a fix to maintainers. This feature working properly is more important to content editors than character counts. Can you let me kow how we go about this please? Thanks

andybroomfield commented 1 month ago

Just confirming the issue applies to a fresh copy of localgov drupal with localgov_demo enabled.

Steps to reproduce:

Create an unpublished guide overview page
Create several unpublished guide pages, link them to the guide overview.
On the guide overview page, click preview link
On the preview link page, click and all pages for this guide
Copy the preview link and open in a private browser window (anon user)

Expected result

Page shows with links to all guide pages in the top navigation box.

Actual result

Anon user can only preview the top level page, navigaton box does not show additional links. (Though if they paste the links in it does give them access, so it's not a permission issue).

And pasting in the link to the second page directly.

willguv commented 1 month ago

Thanks for testing this @andybroomfield !

andybroomfield commented 1 month ago

The troublesome code is this line in localgov_guides/src/Plugin/Block/GuidesAbstractBaseBlock.php:

return ($guide_node instanceof NodeInterface) && $guide_node->access('view');

If you remove && $guide_node->access('view') it now works as expected. That will also mean that anyone else can see the link also regardless on if they have a preview link or not!

I suspect it's the same in other places. We check to see if a page can be accessed before showing the link, but we are only concerned with the page in it's normal state, not accounting for the preview link token.

Not sure resolution, maybe a custom access checker that can account for preview links?

mccrodp commented 1 month ago

I have been able to reproduce this - thanks for helpful steps above. Now I'll dig deeper into @andybroomfield's latest comment above to see if I can replicate and investigate possible access solutions before reporting back.

willguv commented 1 month ago

Thanks @mccrodp

Looping in @stephen-cox who's chatting with Paul from Haringey about the issues he's been having

mccrodp commented 1 month ago

I suspect it's the same in other places. We check to see if a page can be accessed before showing the link, but we are only concerned with the page in it's normal state, not accounting for the preview link token.

Not sure resolution, maybe a custom access checker that can account for preview links?

There is a PreviewLinkAccessCheck class in the Drupal\preview_link\Access\ namespace. There's also an interesting class PreviewLinkEntityHooks class in Drupal\preview_link\ namespace that has a preview_link_entity_access hook by the look of it. Both of these seem to have ways to determine access using the token, rather than standard entity access that's currently in use: $guide_node->access('view').

However, I don't see documentation for preview_link module and been out of this space a while. I don't know how to call Preview link's access method.

I presume we want a custom access checker that: when Preview Link module is enabled, uses the preview link access check instead of default entity access check. Please correct if I'm wrong 😄

stephen-cox commented 1 month ago

Looking into this I have found a potential bug with the Preview Link module. The entity access check it adds has a condition to restrict it to the current entity, so when it checks if the user has access to the other preview links it doesn't allow it. As access to these pages isn't allowed, they aren't displayed in the navigation. Removing this check allows access, but there is a danger that this allows too much.

The condition causing the issue is here: https://git.drupalcode.org/project/preview_link/-/blob/2.1.x/src/PreviewLinkEntityHooks.php?ref_type=heads#L69

Removing the $route_entity->id() === $entity->id() condition allows the blocks to work as expected with preview links.

I will create an issue in the Preview Link issue queue with a potential fix and see what the module maintainers think of this.

stephen-cox commented 1 month ago

I have created an issue for this on drupal.org: https://www.drupal.org/project/preview_link/issues/3481523

The outstanding question I have is, is it safe to just drop the check for the current page or should we check whether the entity belongs to a preview link?

The later sounds safer, so I will look at how feasible this is, but any thoughts are welcome.

willguv commented 4 weeks ago

Hi @stephen-cox what's the latest on this please? Thanks

stephen-cox commented 2 weeks ago

This should have been fixed for Guides, Step by steps, Subsites and Publications with https://github.com/localgovdrupal/localgov/pull/785

There's still a problem with Directories, for which we don't have an easy answer. Discussion for this here https://github.com/localgovdrupal/localgov_directories/issues/404

localgovdrupal / localgov