Unpublished variation access

This looks like a bug to me.

Steps to reproduce:

• Create a view with product variations
• Unpublish a product variation shown in the view
• Access the view as anonymous user and you will see that the unpublished variation shows up.

In my case, the web store displays variations, not products. So this is a big problem.

I created an issue fork with the existing patch applied. The tests seem fine. I'm currently in the process of testing the code in a project.

I tested the changes against version 2.37 of the module and it works as expected. One could argue that this is not really a bug and merging this change would amount to a breaking change. In this case this issue lends itself to version 3 of this module, but that is for the maintainer to decide.

@tgauges thank you, great work!

Tests are failing and it looks related:
Drupal\Tests\commerce_product\Kernel\ProductVariationStorage 0 passes 1 fails

I fixed the relevant test but now another unrelated (?) test is failing. Could you take a look at it?

@tgauges the MR targets the wrong branch! It needs to target 3.0.x-dev! I can't change that!

PS: Maybe you could alternatively create a separate MR against 3.0.x-dev so the other one is kept for 2.x?

Whao, @tgauges you just saved my day. Some weeks ago I came here for a minor impact, but now I found it's major and you MR fixes it!

We have a product with several variations. To switch them, we're using rendered entity display (swatches).
Now the client reported, that even disabled product variations are visible to end-users! And he's correct.

Below some screenshots as logged out user - both after clearing caches.

Without patch:

With patch:

So I think this is at least a major, if not even a critical bug, if you'd rate this as information exposure (while I don't think product variations typically contain highly sensitive information)

But I think this needs to be fixed asap and maybe even in 2.x? At least definitely in 3.0.x

anybody → changed the visibility of the branch 3167716-3.0.x-unpublished-variations-access-fix to hidden.

anybody → changed the visibility of the branch 3167716-3.0.x-unpublished-variations-access-fix to active.

anybody → changed the visibility of the branch 3.0.x to hidden.

Created MR!361 against 3.0.x-dev. Hopefully the tests will pass there :)

I couldn't find any unexpected side-effects yet, but of course, access-related changes always need to be done carefully. Thank you for the tests!

@tgauges test fails, would you have a final look?

The test is fixed.

Great work @tgauges thank you!

I think @jsacksick should now have a look and decide if this should be backported to 2.x or just go into 3.0.x-dev.

As MR!361 applies on 2.0.x and 3.0.x I'm now hiding MR !327.

Sadly I have to put this back to "Needs review", as I found a case, where it doesn't seem to work as expected for me, but I didn't find the root cause yet, why it works in some products and doesn't work in others (unpublished variations are shown).

Uploading MR!361 as static patch for composer until this is resolved. I'd vote to also commit this fix to 2.0.x.

https://www.drupal.org/files/issues/2024-11-04/commerce-unpublished_vari... → works both for 3.0.x and 2.0.x (and 2.40.0) currently!

grevil → changed the visibility of the branch 3167716-unpublished-variation-access to hidden.

Too bad I didn't see this prior to tagging 3.1.0, merged in 3.x, thanks!

Just wondering if we shouldn't have went with neutral() similarly to the EntityAccessControlHandler provided by the entity module.

Like the following:

      if ($entity instanceof EntityPublishedInterface && !$entity->isPublished()) {
        if ($account->id() != $entity->getOwnerId()) {
          // There's no permission for viewing other user's unpublished entity.
          return AccessResult::neutral()->cachePerUser();
        }

        $permissions = [
          "view own unpublished {$entity->getEntityTypeId()}",
        ];
        $result = AccessResult::allowedIfHasPermissions($account, $permissions)->cachePerUser();
      }

I think it's not possible to grant access from a hook_entity_access() when forbidden() is explicitly called

Maybe we should have went with neutral().

Best Practice:
Use neutral() when your handler can't definitively decide or you want to allow flexibility for other modules.

Use forbidden() only when you're certain that access must be denied, no matter what.

Ok, I actually committed a followup change, which calls allowedIfHasPermission() which basically calls allowedIf() which does the following:

return $condition ? static::allowed() : static::neutral();

So under the hood, neutral() is called if the user doesn't have the manage permission.