search

laminas / laminas-view

Flexible view layer supporting and providing multiple view layers, helpers, and more
https://docs.laminas.dev/laminas-view/
BSD 3-Clause "New" or "Revised" License
74 stars 46 forks source link

Navigation AbstractHelper::accept should not call isAllowed unless Acl is set #202

Closed tyrsson closed 1 year ago

tyrsson commented 1 year ago

Bug Report

Q A
Version(s) 2.27.0

Summary

Upon installing Laminas Navigation and using the menu helper due to the check in AbstractHelper::accept being made to $this->getUseAcl() (which as far as I can tell is always true) you get a call to isAllowed for every menu entry that is currently visible even when there has not been an Acl instance set.

Current behavior

The helper calls isAllowed with no need to call isAllowed(). If there has not been an instance set then the page can not be filtered by the call to isAllowed since in this condition isAllowed should return true. A user should not have to notify the helper to not use an Acl explicitly if an instance has not been set. At least I wouldn't think that would be the desired behavior.

How to reproduce

Simply install Laminas Navigation, create a container and use the menu helper to render it without setting an Acl instance.

Expected behavior

Without setting an instance of the Acl or without delegating one for the helper to use, which in turn sets an instance, I would expect that isAllowed would not be called at all. I would also think that the check would use $this->hasAcl(). The line of code in question is AbstractHelper line #325

} elseif ($this->getUseAcl()) {
froschdesign commented 1 year ago

$this->getUseAcl() (which as far as I can tell is always true)

Which can be modified via:

$this->navigation()->setUseAcl(false);

I would also think that the check would use $this->hasAcl().

It would be great if you could create a pull request for this. Thanks in advance! 👍🏻

tyrsson commented 1 year ago

@froschdesign Yes, I was aware that it can be modified via setUseAcl :)

I would have already opened a pull request for it I just can not see a way to test the effect of this particular change to accept().

tyrsson commented 1 year ago

I closed the pull request but I am leaving this because it still needs addressed at some point.

froschdesign commented 1 year ago

@Tyrsson The entire permission check must be removed from the view layer. But at the moment we can not check if an ACL instance is set or not because the current behaviour also allows the usage of laminas-permissions-rbac via a listener. And if you use RBAC, there is no ACL instance.

tyrsson commented 1 year ago

Gotcha. I have often wondered why those two components do not share a common interface...

In lieu of this discussion I would say that the documentation for the getUseAcl() method needs to be changed in the online manual.

https://docs.laminas.dev/laminas-navigation/helpers/intro/

Whether or not to use ACLs; both the flag must be enabled and an ACL instance present.

froschdesign commented 1 year ago

I created two reports to track the documentation issues: