Unpublished variations are not properly checked for access

Reopening this one because I don't see another patch mentioned in #2.

I met the same issue recently. Product variations have the Published flag, but it is completely ignored during the access check. The Published flag isn't mentioned in ProductVariationAccessControlHandler at all.

I wonder what would be the proper fix for this? Maybe the Published flag should be deleted from product variations? As I don't see it's used. Or should it be added to the access check as in @Aerzas' patch?

Changing to "Support request" because it's not clear yet what this issue is.

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

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).
All still working as expected, it was a role difference.

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!

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.

These changes combined have had quite a detrimental effect on an admin's ability to view the display products on a site, as a variation being published no longer limits if it will be shown to an admin or not. The results have been unpredictable, and I have not spotted a pattern in the breakages other than on products with unpublished variations.

Normal users do not have the same issues, provided they don't have access to unpublished variations.

This is when an admin views the products as if to buy them - both product lists and full product display pages. The breakages result in either all of the unpublished variations being presented to an admin, or only some of them, or only the 1st unpublished variation. Quite often, the published variations are not presented to the admin. It makes testing and publishing difficult to impossible. I haven't dived into what causes the particular breakage situation, but I did figure out the minimum required change for it to work again for me. Patch attached.

I'm time limited so I haven't created a new issue and added tests to show what is happening sorry, but I can write out the products which went haywire on me:

Product 1; some of the variations published resulted in only some of the unpublished variations included in the display, some published, and not a predictable choice of which, while normally it would only show the published ones:
- Unpublished variation; shown
- Unpublished variation; shown
- Published; shown
- Published; shown
- Published; shown
- Unpublished variation
- Unpublished variation
- Unpublished variation; shown
- Unpublished variation; shown
- Unpublished variation
- Unpublished variation
- Published
- Published
- Published

Product 2; no published variations resulted in all variations being displayed, while normally it would show none:
- Unpublished variation; shown
- Unpublished variation; shown
- Unpublished variation; shown
- Unpublished variation; shown
- Unpublished variation; shown
- Unpublished variation; shown

I left out removing an isPublished check on ProductVariationStorage::loadFromContext as it does allow an admin to view an unpublished variation which I found quite useful. That's never previously worked, and it does still now prevent a normal user from viewing a variation they aren't allowed to see.

I am having the same issue, on the product display page my admin users are expecting to see what an end-user would see, but they are now seeing in some cases dozens of unpublished products in the add to cart widget.

Sorry, but I have to strongly disagree. If you expect higher role to see what your regular users see, then you need to configure permissions or your roles like that or add custom logic. Typically, higher roles are able to see things users don't see - by design, controlled by permissions. That has been the case for nodes and other entities since Drupal 6.

I'm not saying that #38 and #39 are not valid concerns or follow-up issues or bugs, but having a (correct) access check for unpublished entities and having a permission to control that, is important. We should stick to Drupal standards and best-practices here and not introduce our own logic.

So if your administrators should not be able to see unpublished product variations, they probably shouldn't be administrators or you should mark (by css) or filter unpublished product variations. Also check the permissions assigned to them and if they should be "managers" or something like that, without the special "all permissions" administrator role.

And of course we should ensure that we have tests and the needed permissions to ensure it works as expected. Indeed we should have a follow-up for #38 for further tests.

I agree that access controls on the product variations is indeed desired, but this has also changed the code for the DISPLAY NODE meaning that admins cannot view the display node of a product and have any idea what an end user would see.

I would agree this needs tests to demonstrate why this is NOT desired, and to prevent it being broken again in the future. No problem in it being a follow up issue. The commit in #35 did fix an underlying issue, but it has made viewing a display node for an admin completely useless.

The patch in #38 fixes the issue for me while not compromising access control to the entities, it just fixes the display nodes.

I did figure out the seeming randomness in which unpublished variations were included in the display node - only the first of each variation + attribute is included in the display, published or not. An admin just gets the first one regardless of it being available to be purchased or not.

Thanks @elc yes, absolutely. I think having a follow-up with your points is the way to go. To sum up my comment I just wanted to say: We shouldn't revert the important things here, but proceed in the right direction, fixing the real bugs!

@anybody, I think my complaint about this change is also limited to display nodes.

And I think my and @elc's complaint is backed up by the code itself—I'd be very surprised if anyone expected unpublished nodes to appear when calling a method named ProductVariationStorage::loadEnabled(). Enabled strongly suggests a status check, not a permission check.

Happy to open a follow-up about it!

For anyone else with this issue there is actually an easy fix in the meantime: Handle ProductEvents::FILTER_VARIATIONS, and then remove the unpublished variations:

public function onFilterVariations(FilterVariationsEvent $event): void {
  $enabled_variations = $event->getVariations();

  // Filter unpublished variations.
  foreach ($enabled_variations as $variation_id => $enabled_variation) {
    if (!$enabled_variation->isPublished()) {
      unset($enabled_variations[$variation_id]);
    }
  }

  $event->setVariations($enabled_variations);
}

I ran into this issue too when upgrading my site to 3.x. Anyone with the ability to see unpublished products can now see unpublished variations too on product pages and views of products.
This is desirable when editing products and their variations. It is not desirable behavior for us with the variation widget displayed on products.

I think the problem in addressing this is that both types of behavior are potentially desirable, based on the business needs. How it's displayed on the widget probably needs to be configurable.

Here is the followup issue
#3572669: Add an "Add to cart form" formatter setting to filter unpublished variations out of the list