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

[tyrsson]

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]

$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]

@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]

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

[froschdesign]

@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]

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]

I created two reports to track the documentation issues:

• Description of the method Laminas\View\Helper\Navigation\AbstractHelper::setUseAcl() is missing laminas-navigation#48
• Add description of usage with laminas-permissions-rbac via a listener laminas-navigation#49