Permissions broken when multiple terms are selected

I'm seeing the same issue on nodes -- will post an update if we come up with a solution

Updating the title to better reflect issue description and account for the fact that I'm seeing this on node entities.

To further clarify my scenario in case it's useful, I also have "Require all terms granted" unchecked. Each of my terms grants access to a single role -- roleA -- users with roleA can access the node. When I update the term field to grant access to roleA and roleB, my roleA users lose access to the node.

I'm not sure whether or not this is relevant, but this scenario results in the user seeing the 403 page set at /admin/config/system/site-information. If I instead enable "Require all terms granted" and select multiple terms then the users receive an "access denied" message instead of the 403 page. I suppose this could indicate an issue with my setup and not the module but I'm not entirely sure.

• Have you checked your PHP logs?
• Which PHP version are you using?
• Do you have other access restriction modules running?

If one term is granting access and an other isn't, then access is granted. Without "Require all terms granted" option.

-

I was able to reproduce this behavior if Permissions by Entity is enabled. Looking at the code in PbE, I think there are two issues:

1) PbE isn't honoring the 'Require all terms granted' and 'Permission mode' settings.
2) PbE is filtering on all entities, including nodes. I think that's why OP saw it happen on nodes, because PbE was overriding the standard PbT behavior.

The attached patch addresses these in the following ways:
For #1, I ported the logic being used in the 'canUserAccessByNodeId' function located in AccessCheck into the 'isAccessAllowed' function located in AccessChecker.
For #2, I simply added a checked in the entity_access hook used by PbE to exclude nodes from being processed.

Re-rolling for the latest version.

Fixing a few bugs with permission mode support

Fixing php warnings with last patch

I have tried to reproduce your issue, because it sounded like a major one: no access by selecting more than 1 term. I could not reproduce it.

What did I?

1., Created node
2., Assigned two terms, to which an editor had access
3., Tried to access as guest role user: no access
4., Tried to access as editor: access granted

So I could not reproduce it.

Your patch does not contain any tests which are pointing to a scenario, where a bug can be reproduced. It just changes the processing, but does not show why.

Hi @Peter Majmesku,
It's not clear from your response, but did you have Permissions by Entity when you were testing this? That's the root of the issue and that's also why your tests as written aren't catching the problem (because permissions by term and permissions by entity tests are run independently). The problem manifests because permissions by entity doesn't honor the 'Require all terms granted ' and it's also processing on any entity passed to the hook_entity_access function, including nodes.

Your patch does not contain any tests which are pointing to a scenario, where a bug can be reproduced.

I've attached two patches to this ticket that setup a basic test to illustrate the issue. I don't know if the new tests makes sense to be a permanent test since it's adding dependencies on the main permissions by term modules to the permissions by entity module, however it does show the problem and I'm sure we could come up with a better test case once we agree the problem exists. The modified testDisabledRequireAllTermsGranted test now first tests the node access like normal (which works) and then mimics the result being processed by permissions_by_entity_entity_access, which causes the test to now fail.

Updating the version of this thread to 2.28, as the issue is still occurring on most recent version.

FWIW, we've also got PBE enabled on the environment where we were seeing this issue.

I´m having the same Issue with PBE enabled. If I remove PBE the PBT Module works perfect with respecting two terms without the single Term option enabled.

let me sum up with activated PBE:
1., Created node
2., Assigned two terms, one term with approved roles everything except guest, the other term only guest (so everyone should have access)
3., Tried to access with a member of authenticated user: no access (should have access)
4., Admin access granted

If I´m disabling PBE:
1., Created node
2., Assigned two terms, one term with approved roles everything except guest, the other term only guest (so everyone should have access)
3., Tried to access as authenticated user: access granted
4., Admin access granted

I have the same issue and added patch #11. It resolved my issues for node access but broke the access caching for menu items - displaying all menu items however term permissions were set. After some debugging it looks like it was down to this part of the patch:

+ if ($operation === 'view' && $entity instanceof FieldableEntityInterface && !$entity->isNew() && $entity->getEntityTypeId() != 'node') {

Removing the && $entity->getEntityTypeId() != 'node') meant the nodes were being checked again and the access result cached. This resolved the issue.

I just can repeat myself.

PbE needs a massive refactoring ...

@ytsurk opened https://www.drupal.org/project/permissions_by_term/issues/3269784 so maybe we could get this effort started.

Rerolling for the latest version

Thanks for sharing the patch. Englisch

The patch can be applied to the new version.

This patch cannot be applied. Because anonymous users will get access to media entities, which have one or more terms related, that do not grant them access. I could even test this simply by attaching one term to a media entity of type document, which grants access to authenticated users.

The patch modifies some deep logic in the module. Therefor it needs tests. Please add PHPUnit tests to the patch, which are demonstrating the bug and will pass after the patch is applied.

Attached tests with and without patch. It demonstrates that node access returns different results when PBE is turned on, which shouldn't be the case.

Same caveats apply from the tests I did on the 2.x branch:

I don't know if the new tests makes sense to be a permanent test since it's adding dependencies on the main permissions by term modules to the permissions by entity module, however it does show the problem and I'm sure we could come up with a better test case once we agree the problem exists. The modified testDisabledRequireAllTermsGranted test now first tests the node access like normal (which works) and then mimics the result being processed by permissions_by_entity_entity_access, which causes the test to now fail.

Edit, no entirely sure how to demonstrate this in the issue queue as unit tests for the 3.x branch don't appear to be enabled.

The patch breaks permissions for guests, like I have described in #24. So it cannot be applied and must be modified.

About your tests edit: please follow the SOLID princile and add an new test for your concrete case with proper naming. Do not just modify the general test in testDisabledRequireAllTermsGranted.

The PbT base module should not know about PbE, since it's an extension. Your test should be placed in PbE.

> no entirely sure how to demonstrate this in the issue queue as unit tests for the 3.x branch don't appear to be enabled.

I've googled a bit about that. But still no idea. Do you know how to enable the testbot for the 3.1.x-dev Git-branch? I just see that the test runs are offered here in the issue queue for the 8.x-1.x Git-branch. The 3.1.x-dev Git-branch follows the naming convention and somehow it's not triggered. Otherwise I would open an issue at Drupal infra.

Hi, there has been no activity for a while, I take the liberty of modifying the original patch to update it. I didn't have time to set up the tests.

I see with the patch that an anonymous user does not have access to the content but that is normal. It totally depends on how you handle authentication.

I have updated the issue fork against 3.1.x-dev and fixed the failing tests.

Could someone with a bit more history on this issue kindly check if everything still works as expected?

Hi, the patch still works but a duplicate foreach slipped in.

MR !26 updated to remove the duplicated foreach.

@bakop Thanks for catching that duplication.

I gave the fork another quick test myself and merged afterwards.

Included in 3.1.27