Permissions by field triggers warning

Can confirm this on 3.0.3

Taking a deeper look I think there must be a change in return values somewhere? The code does not look like it could work as the return values that get checked are not keyed with 'value' but target_id.

The code in question is this one

            case 'shared':  //allow if user shares a value is in this field

                $user = User::load(\Drupal::currentUser()->id());
                $user_field = $config->getExtra();
                $user_field_values =  $user->get($user_field)->getValue();

                foreach($node_field_value as $value){
                    foreach($user_field_values as $uvalue){
>>>                    if($value['value'] == $uvalue['value'] ){
                            $control_entity = $user;
                            $grant_access_read = $config_bool_read ? true : false;
                            $grant_access_update = $config_bool_update ? true : false;
                            $grant_access_delete = $config_bool_delete ? true : false;
                            break;
                        }
                    }
                }
                break;

The arrays $node_field_value and $user_field_values look like this

$node_field_value
Array
(
    [0] => Array
        (
            [target_id] => 26
        )

)

$user_field_values
Array
(
    [0] => Array
        (
            [target_id] => 25
        )

    [1] => Array
        (
            [target_id] => 26
        )

)

Also the module is basically unusable with this error so setting this to critical.

There is now a MR to fix this.

It's only changing the index value to target_id if you want to quick fix this in your install. See the code block marked with >>> above.

Can you provide how you are triggering this without another contrib moudle?

The fix looks good, but it would be nice to have some insight as to when the array key changed. Can we be sure that the key will be "target_id" in all cases?

This seems like something that the module should have test coverage for, so that if the array key changes again in future, it will be flagged early.

I've carried out some testing of this module without PBF or any other module.

The steps were:

1. Install ABR.
2. Grant "Apply Access by Reference" permission to authenticated user.
3. Add new ABR configuration, of type "Profile Value", using "reference" on User, and on node, with "Edit" permission.
4. Add text field called "Reference" to user.
5. Create content type "Yacht".
6. Add text field called "Reference" to Yacht.
7. Create a Yacht and populate the reference field.
8. Create a new user. Leave reference empty.
9. Log in as new user (in incognito window).
10. Verify user cannot edit Yacht node.
11. Update user, and set reference to same value as yacht, then clear cache.
12. Verify user can now edit Yacht node.

I added a breakpoint at the if ($value['value'] == $uvalue['value']) line, and in this situation the value key is populated for both sides, and no sign of target_id.

I think if the current change were implemented, it would break the module for standalone use.

Some more investigation is needed to understand what key value should be used in different situations.

The MR should also be retargeted against 4.x branch to allow pipelines to run. I think it would be useful to add PBF to the require-dev section of composer.json, and a test added to explicitly check ABR and PBF behave correctly together.

I can also confirm than with I run the tests locally, one of the tests fails:

1) Drupal\Tests\access_by_ref\Functional\ProfileValueTest::testUserReference
Exception: Warning: Undefined array key "target_id"
access_by_ref_node_access()() (Line: 167)

Thanks! And agree about the test coverage

A problem with creating a Dev dependency on PBF is it doesn't have a D11 version yet.

However the PBF field type is derived from EntityReferenceItem, so a test based on any entity reference field should be sufficient.

The current test assumes a text field is used for the "reference".

In the "user" type it will always be the user, and in the "user mail" type it will always be a text (email) field. However, for "shared" it could be either.

I think the check needs to allow for either, so maybe something like this could work:

            if (array_key_exists('value', $value) && array_key_exists('value', $uvalue)) {
              $key = 'value';
            }
            elseif (array_key_exists('target_id', $value) && array_key_exists('target_id', $uvalue)) {
              $key = 'target_id';
            }
            else {
              break;
            }
            if ($value[$key] == $uvalue[$key]) {

For testing, I think it would be fairly easy to clone the ProfileValueTest class, which assigns users to "red" and "blue" teams using text fields. It should be fairly easy to make them taxonomy terms instead of text values, which would cover the entity reference case.

lostcarpark → changed the visibility of the branch 9.x-3.x to hidden.

I have created MR !15 against the 4.x branch.

I created a new test case that tests a profile value using a taxonomy term.

I ran the test with the current access check, and verified it failed.

I implemented the access check suggested and verified that both text profile value and reference profile value checks pass.

I haven't explicitly tested for the PBF field type, but I believe it will behave the same as any other reference field, so this change should fix the issue. However a manual test of PBF would be helpful.

Excellent test coverage, ran the test-only feature https://git.drupalcode.org/project/access_by_ref/-/jobs/5232057 and got it perfectly.

Fixed some small little stuff on the pipeline but rest was perfect.

Will release soon

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