Abstracthelper accept method fix

[tyrsson]

Q	A
Documentation	no
Bugfix	yes
BC Break	no
New Feature	no
RFC	no
QA	no

Description

This change prevents the accept method calling isAllowed unless an ACL instance has been set for the Helper
In addition to #202 Please reference the following:
AbstractHelper:
line 112:

protected $useAcl = true;

AbstractHelperTest
line# 40

public function testHasACLChecksDefaultACL(): void

line#50

public function testHasACLChecksMemberVariable(): void

MenuTest:
line#125

public function testFilterOutPagesBasedOnAcl(): void

line#137

public function testDisablingAcl(): void

[Ocramius]

This kind of test requires test additions .

[tyrsson]

Every method that is being used is covered. Every class member that is being used is covered. I referenced those test in the information above. The only reliable way I see to test this change is to verify that isAllowed() is not called from accept(). I will gladly write the test if someone would just point me in the correct direction of how to accomplish that with PhpUnit. I read the PhpUnit docs for about 2 hours last night trying to figure out a way to test this.

[Ocramius]

If you have found a bug, you have observed it somewhere: we should reproduce that, at first without mocking.

[tyrsson]

Do you have a suggestion as to the best way to Unit test if one method calls another? Is there a way to Unit test that?

[Ocramius]

This change prevents the accept method calling isAllowed unless an ACL instance has been set for the Helper

I'm trying to understand when isAllowed() would be called without an ACL instance: that's what we'd need to reproduce (before this change)

[tyrsson]

This is the original code
line 325

} elseif ($this->getUseAcl()) {

WIthout first calling $helper->setUseAcl(false) in user land then AbstractHelper->isAllowed() would always be called because the AbtractHelper->useAcl flag is defined as true by the AbstractHelper in the prop definition. I referenced that above. If a user installs both laminas view and laminas navigation but is not using the Acl component they should not have to explicitly call AbstractHelper->setUseAcl(false) to prevent AbstractHelper->isAllowed() from being ran, or at least I would not think so. I covered this in issue #202 when I reported it. The issue boils down to this. Why call AbstractHelper->isAllowed() if an Acl component is not even installed? It should not. If there is not one installed, and set on the helper then it could not possibly, or should not possibly, have any influence on whether a page is accepted or not. By default, and in the current code it is called because AbstractHelper->useAcl is defined as true. In the change I use $this->hasAcl() && $this->getUseAcl to first make sure a Acl instance has been set, then to allow overriding that if the user has called AbstractHelper->setUseAcl(false) to bypass the Acl, since that is the current usage the check should respect that.

Now, how to reproduce that in a Unittest.... That's the problem I am struggling with... I suppose maybe a listener could be attached to see if AclListener::accept() is triggered?? AbstractHelper->isAllowed() triggers an accept event, but really, the code execution should never get to there. Why kick off that workflow if the governing component is not available to check against? The accept method in the AclListener simply checks if(! $acl) returns a another flag that is defined as true.

[tyrsson]

@Ocramius
I should also make mention. I am speaking about the AbstractHelper isAllowed method. Not the Acl isAllowed method.

[froschdesign]

@tyrsson
The problem I see here is the change in behaviour. The default ACL listener should handle it if an ACL instance is present or not:

laminas-view/src/Helper/Navigation/Listener/AclListener.php

Lines 15 to 17 in 198f594

	* Determines whether a page should be accepted by ACL when iterating
	*
	* - If helper has no ACL, page is accepted

This also allows a custom listener to handle this situation itself.
With the current change, the handling shifts and a listener can no longer react if there is an ACL instance or not.

---

Therefore, this change cannot simply be adopted, even though I would prefer to remove the entire permission check from the view layer.
To handle the ACL check in your application, I suggest to create a delegator that sets the ACL usage to false if no ACL is present. An example of a delegator and its usage in an application can be found in the laminas-navigation repository: laminas/laminas-navigation#19

[tyrsson]

@froschdesign
In the project I am working on where I discovered this I am already delegating a majority of the helpers anyway. So replacing this class will not be a problem.