add a wrapper around buildLink() that checks access [#2488856]

Currently, outputting a flag link requires the caller to check for access first:

    if (!$flag->hasActionAccess($action)){
      continue;
    }

    $link = $link_type_plugin->renderLink($flag, $entity);

(The Views field code doesn't do this, which is a bug... but we're going to fix it here.)

Rendering a flag link should be public API, because themers or contrib modules may want to render the link in other places. Requiring an access check first isn't very good DX, and adds to the risk of bugs. Furthermore, there is no way you would want to render a link the user doesn't have access to, because they're our links, and we check access on those paths: so you'd be outputting a link to a URL that would give the user a 403.

Adding access checking would mean operation the $flag entity from inside the link type plugin. But that's ok, as the link already does this sort of thing:

      $render['#title'] = $flag->getUnflagShortText();
      $render['#alt'] = $flag->getUnflagLongText();

(Arguably, we could move renderLink() to the flag, which would pass the link plugin parameters, but that's for another issue.)

Comment	File	Size	Author
#13	interdiff-11.13.txt	904 bytes	martin107
#13	checkAccess-2488856-13.patch	4.42 KB	martin107
#13	8.x-4.x: PHP 5.5 & MySQL 5.5 32 pass, 3 fail PHP 5.6 & MySQL 5.5 32 pass, 3 fail
#11	checkAccess-2488856-11.patch	4.42 KB	martin107
#11
#4	checkAccess-2488856-4.patch	2.92 KB	martin107
#4
#4	interdiff-2-4.txt	1.76 KB	martin107
#2	checkAccess-2488856-2.patch	2.42 KB	martin107
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

socketwench CreditAttribution: socketwench as a volunteer commented 18 June 2015 at 14:35

In other issues I've stated a preference for keeping this out of the render logic. Given how it affects tokens, I'm reconsidering my stance. We should at least make a patch and consider if we should do this.

Comment #2

martin107 CreditAttribution: martin107 commented 19 September 2015 at 12:43

Status:

Active

» Needs review

File	Size
checkAccess-2488856-2.patch	2.42 KB

This is a initial version ... just so we can see how it fits .. and see if it is what we want to do... I am still in two minds.

There are already lots of tightly focused issues in this area... maybe methods are going to be moved and renamed in subsequent issues.
So I am going to tread a careful line here..

ActionLinkTypePluginInterface::buildLink()
ActionLinkTypePluginInterface::renderLink()

are both public.

renderLink() calls buildLink() and buidLink() is not called anywhere else - So I think buildLink() should be private/protected and removed from the interface?

Otherwise access checking should be in buildLink() not as I have done, renderLink() - that way all attempt to render or build the link pass through an access check.

Anyway this patch is just to answer the question what does the code look like if we add an access check in the renderLink() method.

In short the fallout from the change is that a conditional check can be removed from FlagLinkBuilder:build() but one has be to added to AjaxActionLink:renderLink().
The gain is that the view field bug is corrected.

In the interest of full disclosure, removal of the if statement in FlagLinkBuilder:build() changes the returned render array inconsequentially - when the access check fails. The return type changes from

[]

['link' => []]

Comment #3

joachim CreditAttribution: joachim commented 19 September 2015 at 12:52

Status:

Needs review

» Needs work

Looking good. Thanks!

ActionLinkTypePluginInterface::buildLink()
ActionLinkTypePluginInterface::renderLink()

Those names are not ideal :/
buildLink() doesn't build a link, it creates a URL. renderLink() doesn't render, as it returns a render array -- Drupal terminology here is 'build'.

I'll file an issue to rename these to getLinkURL() and buildLink() later on.

> So I think buildLink() should be private/protected and removed from the interface?

I'd say to leave it. Experience on D7 has shown that some people want to build the flag/unflag themselves for weird theming purposes.

+++ b/src/ActionLinkTypeBase.php
@@ -81,6 +81,12 @@ abstract class ActionLinkTypeBase extends PluginBase implements ActionLinkTypePl
   public function renderLink($action, FlagInterface $flag, EntityInterface $entity) {
+
+    if (!$flag->hasActionAccess($action) ) {
+      // Must always return a render array.
+      return [];
+    }

The docs should be updated to say access is checked, and an empty render array is returned if access is denied.

```
+++ b/src/FlagLinkBuilder.php
@@ -56,18 +56,13 @@ class FlagLinkBuilder implements FlagLinkBuilderInterface {
+    return ['link' => $link];
```
> In the interest of full disclosure, removal of the if statement in FlagLinkBuilder:build() changes the returned render array inconsequentially - when the access check fails.

Why not check !empty($link) and then only return the nested array if it's not empty?

+++ b/src/Plugin/ActionLink/AJAXactionLink.php
@@ -40,8 +40,11 @@ class AJAXactionLink extends ActionLinkTypeBase {
+    if(!empty($render)) {

Missing space after the if ()

Comment #4

martin107 CreditAttribution: martin107 commented 23 September 2015 at 11:27

Status:

Needs work

» Needs review

File	Size
interdiff-2-4.txt	1.76 KB
checkAccess-2488856-4.patch	2.92 KB

1, 2 and 3 fixed.

Thanks

So there is one outstanding issue on my mind....

I'd say to leave it. Experience on D7 has shown that some people want to build the flag/unflag themselves for weird theming purposes.

A dual track approach is fine....but there is a BUT

The current return object from

ActionLinkTypePluginInterface::buildLink() is a UrlObject

As we need a uniform/system wide approach to preventing unauthorised access to renderLink() ... well the same goes for buildLink()

Returning a blank URL or a URL to the home page... seems inappropriate. Throwing an exception, rejecting the whole page seems extreme

I am not sure what the best way forward on that front is.

Comment #5

socketwench CreditAttribution: socketwench as a volunteer commented 23 September 2015 at 13:59

iirc, buildLink() was left public so that anyone that wanted to do something crazier than just return a route name, they could do so. Then they would only need to override buildLink() and they would return any URL they wanted. If we do need to change the access specifier, protected would be best.

Comment #6

joachim CreditAttribution: joachim commented 23 September 2015 at 14:20

> As we need a uniform/system wide approach to preventing unauthorised access to renderLink() ... well the same goes for buildLink()

I don't see this as preventing access, but rather improving DX so that the code that requests a flag link doesn't require a load of boilerplate code upfront.

Nothing *bad* happens if you use renderLink() without access - you just might get a link that's not valid, in the sense that a user following that link would see a 404/403/whatever status we used for stale links.

Also -- sorry, I was sure I'd updated this issue -- I think we should add the new checks in a wrapper method, so renderLink() is still the pure render logic, and getLink()* is the 'get me a link and do all the thinking about it too' method.

* subject to bikeshed.

Comment #7

socketwench CreditAttribution: socketwench as a volunteer commented 24 September 2015 at 00:07

Also -- sorry, I was sure I'd updated this issue -- I think we should add the new checks in a wrapper method, so renderLink() is still the pure render logic, and getLink()* is the 'get me a link and do all the thinking about it too' method.

Sure, as long as we still have routeName().

Comment #8

joachim CreditAttribution: joachim commented 24 September 2015 at 10:05

Status:

Needs review

» Postponed

I took a look at the code this morning, thinking to make a patch, and I think this is actually blocked by #2571325: rename *Link() methods. I want to make a new method that wraps around renderLink(), and I'm wedged into a corner by the lack of method names I can use!

Comment #9

martin107 CreditAttribution: martin107 commented 26 September 2015 at 14:23

Status:

Postponed

» Active

Comment #10

joachim CreditAttribution: joachim commented 26 September 2015 at 16:14

Title:

renderLink() should check access

» add a wrapper around buildLink() that checks access

Ok so the new plan is to add a wrapper called getLink() that does the sanity checks and then gets a render array from buildLink().

So buildLink is kept pure, and remains public for people who want to do bizarre and crazy things, while getLink() is the nice DX that does the legwork for you. buildLink() should get a '@see getLink()' and suitable warnings in its docs.

Comment #11

martin107 CreditAttribution: martin107 commented 28 September 2015 at 11:54

Status:

Active

» Needs review

File	Size
checkAccess-2488856-11.patch	4.42 KB

I have added the wrapper function.

After method name change, the old patch no longer applied .... instead of rerolling, I started the patch over.

What gets dropped on the floor...

In the old patch AJAXactionLink::buildLink() included an if statement whose effect was to only include ajax library if the link was populated
We can put that in a new issue if needed.

Comment #12

joachim CreditAttribution: joachim commented 1 October 2015 at 08:13

Status:

Needs review

» Needs work

Just a typo:

+++ b/src/ActionLinkTypePluginInterface.php
@@ -17,8 +17,30 @@ use Drupal\Component\Plugin\ConfigurablePluginInterface;
+   * This method in not recommended for general use.

@@ -34,6 +56,10 @@ interface ActionLinkTypePluginInterface extends PluginFormInterface, Configurabl
+   * This method in not recommended for general use.

s/in/is/

Other than that, everything looks great :)

Comment #13

martin107 CreditAttribution: martin107 commented 1 October 2015 at 10:17

Status:

Needs work

» Needs review

File	Size
checkAccess-2488856-13.patch	4.42 KB
8.x-4.x: PHP 5.5 & MySQL 5.5 32 pass, 3 fail PHP 5.6 & MySQL 5.5 32 pass, 3 fail
interdiff-11.13.txt	904 bytes

Ahh crap ....Ground just swallow me up.

@joachim thanks

Comment #14

1 October 2015 at 10:19

Status:

Needs review

» Needs work

The last submitted patch, 13: checkAccess-2488856-13.patch, failed testing.

Opened up a https://simplytest.me/ sandbox as a golden reference

First sand box :-
FlagFieldEntryTest and FlagResetTest ( Flag unpatched ) both tests pass.
Flag 8.x-4.x latest Sun Sep 27 16:22:43 2015 -0500 git commit 88b25b25ee3d70fe2d0524fe39e6ccaa303ad3d5

second sandbox :-
configured to accept patches from d.o.
( https://www.drupal.org/files/issues/checkAccess-2488856-13.patch )

Both tests fail in the same way as testbot fails.... rats.

I will have sometime to look at this over the weekend.

simplytest++

Comment #19

joachim CreditAttribution: joachim commented 2 October 2015 at 12:15

But #11 passed, and the difference between #11 and #13 is just comments!

Comment #20

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 2 October 2015 at 12:50

The difference is that #13 is 3 days later. There was a recent core change that requires annoying test fixes, see #2578159: Tests failing due to recent core change with Title Block for an example.

Comment #21

joachim CreditAttribution: joachim commented 2 October 2015 at 13:12

Status:

Needs work

» Postponed

Thanks!
I've filed #2579067: Tests failing due to recent core change with Title Block.

Comment #22

2 October 2015 at 20:58

joachim queued 13: checkAccess-2488856-13.patch for re-testing.

Comment #23

joachim CreditAttribution: joachim commented 2 October 2015 at 20:58

Status:

Postponed

» Needs review

Comment #24

joachim CreditAttribution: joachim commented 3 October 2015 at 11:47

Status:

Needs review

» Fixed

Tests pass now, so committing.

Thanks!

Comment #25

3 October 2015 at 11:47

joachim committed 0eb4082 on 8.x-4.x authored by martin107

Issue #2488856 by martin107: Added a wrapper method around buildLink()...

Comment #26

17 October 2015 at 11:54

Status:

Fixed

» Closed (fixed)

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

add a wrapper around buildLink() that checks access

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Comment #25

Comment #26

Child issues

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community