Unpublished variation access

Created on 27 August 2020, about 4 years ago
Updated 12 September 2024, 2 months ago

Problem/Motivation

Product variation access is checked against the product it belongs to with a bypass for users with the admin permission administer commerce_product, yet the test wether the variation is published or not is not covered by the ProductVariationAccessControlHandler and rely on classes loading variations (Product or ProductVariationStorage for example).

This rises multiple concerns:
- Third-party integration may give access to unpublished variations if they do not test the variation status
- Checking at multiple levels if the variation is published may create duplicated lines of code
- Administrator may not access unpublished variation depending on the context

Proposed resolution

The access handler ProductVariationAccessControlHandler could check the product variation status to centralize the mechanism and allow users with the admin or manage permission to access the variation.

🐛 Bug report
Status

Needs review

Version

2.0

Component

Product

Created by

🇨🇭Switzerland aerzas

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇹🇭Thailand AlfTheCat

    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.

  • First commit to issue fork.
  • Merge request !327Issue #3167716: Unpublished variation access → (Open) created by tgauges
  • Pipeline finished with Success
    2 months ago
    Total: 479s
    #272591
  • Status changed to Needs review 2 months ago
  • 🇩🇪Germany tgauges

    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.

  • Pipeline finished with Failed
    2 months ago
    Total: 475s
    #273235
  • 🇩🇪Germany tgauges

    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.

  • Status changed to Needs work 2 months ago
  • 🇩🇪Germany Anybody Porta Westfalica

    @tgauges thank you, great work!

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

  • Pipeline finished with Failed
    2 months ago
    Total: 497s
    #281270
  • Status changed to Needs review 2 months ago
  • 🇩🇪Germany tgauges

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

  • Pipeline finished with Failed
    about 2 months ago
    #297045
  • Pipeline finished with Success
    about 2 months ago
    #297067
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 329s
    #298739
  • Pipeline finished with Success
    about 1 month ago
    Total: 867s
    #298741
  • Pipeline finished with Skipped
    about 1 month ago
    #298985
  • 🇩🇪Germany Anybody Porta Westfalica

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

  • 🇩🇪Germany Anybody Porta Westfalica

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

  • 🇩🇪Germany Anybody Porta Westfalica

    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

  • 🇩🇪Germany Anybody Porta Westfalica

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

  • 🇩🇪Germany Anybody Porta Westfalica

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

  • 🇩🇪Germany Anybody Porta Westfalica

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

  • 🇩🇪Germany Anybody Porta Westfalica

    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!

  • Pipeline finished with Failed
    15 days ago
    Total: 718s
    #326123
  • 🇩🇪Germany Anybody Porta Westfalica

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

  • Pipeline finished with Success
    11 days ago
    Total: 1562s
    #328900
  • 🇩🇪Germany tgauges

    The test is fixed.

  • 🇩🇪Germany Anybody Porta Westfalica

    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.

  • 🇩🇪Germany Anybody Porta Westfalica

    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).

  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇩🇪Germany Anybody Porta Westfalica

    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!

Production build 0.71.5 2024