Hey Ryan and all,

A quick Drupal Commerce permission question just came up: What are the security implications of granting all users (both anonymous and authenticated) the "View any Product product" permission? There is a security warning on that permission, but I can't seem to find anything particularly bad if all users are granted this permission.

The use case I've got is that if I want fields attached to a product to display in a view--and if that view is placed in a panel that is overriding a node--I can't get the product fields to show up for the user unless I grant the user the "View any Product product" permission.

Thoughts?

Thanks,
Ben

CommentFileSizeAuthor
#78 commerce-commerce_product_permission_security_implications-1303194-78.patch1.68 KBgngn
#75 commerce-commerce_product_permission_security_implications-1303194-75.patch1.68 KBsidharthap
#73 commerce-commerce_product_permission_security_implications-1303194-73.patch1.4 KBsidharthap
#70 commerce-commerce_product_permission_security_implications-1303194-70.patch1.86 KBsidharthap
#45 commerce-commerce_product_permission_security_implications-1303194-45.patch4.07 KBJaesin
#43 commerce-commerce_product_permission_security_implications-1303194-43.patch4.07 KBJaesin
#41 commerce-commerce_product_permission_security_implications-1303194-41.patch4.03 KBJaesin
#34 1303194-34.patch3.35 KBroderik
#32 Screenshot from 2013-04-04 20:22:01.png66.82 KBkaidjohnson
#21 commerce-commerce_product_permission_security_implications-1303194-21.patch2.56 KBcvangysel
#19 commerce-commerce_product_permission_security_implications-1303194-19.patch2.52 KBcvangysel
#12 remove-security-1303194-12.patch1.35 KBvasike
#9 remove-security-1303194-09.patch503 bytesjoshmiller
#8 remove-security-1303194-08.patch461 bytesjoshmiller
#5 1 commerce rules link.png59.43 KBAdam S
#5 2 commerce rules link.png21.55 KBAdam S
#5 3 commerce rules link.png67.85 KBAdam S
#5 4 commerce rule link.png43.29 KBAdam S
#5 5 commerce rules link.png35.92 KBAdam S
#5 6 commerce rules link.png58.2 KBAdam S
#4 commerce credits bug.png22.6 KBAdam S
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Curious... the only possible problem I could see with it would be if disabled products still showed when you didn't want them to. However, I think the real problem with disabled products is that they shouldn't be purchasable - their visibility should really be determined by the visibility of the entities (nodes) they're referenced from. You might test what happens with disabled products, but it could be there isn't a security problem and we should just remove that warning.

keithm’s picture

Title: Security implications of granting "View any Product product" permission » Clarify security implications of granting "View any Product product" permission

I have an app where I don't need display nodes and I'm just exposing product fields with views. Views will refuse to access the products unless the 'View any product of any type' permission is granted to anonymous.

There are two other issues where the proposed solution is to grant access permission to anonymous: #1276450: Views results empty for unprivileged user when using Relationship: Content: Referenced Product and #1276900: Top Sales Views for Anonymous users.

If this grant is a problem, it would be nice to know specifically why, and to have some sort of workaround to expose product fields in views to anonymous users.

TahitiPetey’s picture

Subscribe.

Adam S’s picture

FileSize
22.6 KB

I want a simple Views table with title and a form button to allow adding a product to the shopping cart. This permission prevents me from doing that. I included a picture of what I'm hoping for but can't get for any body who doesn't have the View any product permission.

Adam S’s picture

I'm adding screen captions of my solution to this problem using the Rules Link module. I would write everything out but that would take too long and this way you can see exactly how each page is configured. I am starting the image series with the View configuration and how the table looks when rendered. Make sure to set the user permissions for the Rule Link on the permissions page.

I'm a little confused about the selector for the node field reference being a list containing the number of products. I couldn't find a way around that. I after fiddling with it tried the first on node:field-job-listing-credits:0:sku to get at the sku and it works in every case I tried. So I'm fine with it. It just doesn't seem perfectly clean.

EDIT: And then..... I realize I forgot to account for the price of the item which can't be done in a field in a View without adding a reference to a product.

hexabinaer’s picture

Could we get back to the question why we call it "security implications"? Obviously we're all good boys and girls and make up phantastic workarounds only to avoid security risks :-)

As far as I understood, the 'implications' rather refer to "users can see displays of disabled products. Right, Ryan? In this case I wouldn't call it 'security implications' but rather clarify this way:
Warning: Disabled products' displays may still be visible.

When I disable one of my products, I can still access the display but the order button is replaced by a disabled button "Product not available". Not much of a security risk, is it? And this can be healed through views' filters.

At least we should offer a (temporary?) documentation for this (already cost me some days to find out that the permission caused my trouble). Such as:

If you are using product display fields in views, make sure to grant 'view any xy product' permissions to all users and use a filter to hide disabled products' displays (using relation 'Product': filter 'Commerce Product: Status > Yes').

DeFr’s picture

Doing a bit of git-history-digging, the permission on product was changed to use commerce_entity_access_permissions to generate the permissions for the different bundles in 11d7b40f. At that point, commerce_entity_access_permissions didn't flag the "view any" as security sentive.

This changed in 32a9bc0e, which started flagging all the "view any XXX" permission as security sensitive as part of #1245158: View any XXX, Edit any XXX, Create any XXX permissions don't work. From that issue though, I'm not sure why "View any $bundle product" is a problem; as fas as I can tell, the "security sensitive" flag was meant for the "View any $type entity" permission only, which makes commerce_entity_access_query_alter completely changes it's behavior and stop altering the query.

In summary, I'm not really sure this permission was meant to be flag security sensitive in the first place.

joshmiller’s picture

Category: support » bug
Status: Active » Needs review
FileSize
461 bytes

Got pinged on this in the forums... http://www.drupalcommerce.org/node/1128

Essentially, this security warning is confusing people and I submit we remove it for the "View any XXX" permission per all the fantastic arguments made in #7.

Josh

joshmiller’s picture

Whoops... I admit this patch is probably over-simplistic to the real problem at hand... So I re-read the comments and noticed that I removed the "View any $type" and not "View any %bundle type" -- which is the real culprit. Ignore #8 patch, this one removes the security notice for the correct permission.

Let the bikeshed begin :)

bojanz’s picture

#9 sounds fine. Just wondering if there are other permissions that should get the same treatment.

HyperGlide’s picture

Glad I found this thread and helps to clear up several issues on this ambiguous warning. Thank you!

vasike’s picture

2 more "view" permissions found in all commerce package with "restrict access" set.
patch attached

vasike’s picture

Status: Needs review » Reviewed & tested by the community

with the last patch, all "View" permissions for all Commerce entities (Products, Order, Customer profiles) will be afected.

clashar’s picture

thank you guys for removing these warnings!

rszrama’s picture

Category: bug » task
Status: Reviewed & tested by the community » Needs work

I'm going to demote this back down, because honestly there are security implications at stake here. It may turn out to be a non-issue or something else to work around, but code like this gives me pause:

  // Do not apply any conditions for users with administrative view permissions.
  if (user_access('administer ' . $entity_type . ' entities', $account)
    || user_access('view any ' . $entity_type . ' entity', $account)) {
    return;
  }

Notice that this code from commerce_entity_access_query_alter() assumes administer * and view any * are functionally equivalent in this regard. That's not to say some other implementation of hook_entity_access_query_alter() couldn't apply conditions to restrict viewing permissions on say a per-product basis (like the Content Access module of yore did for nodes through Access Control Lists), but at the core level we're still setting a pattern that a view anyer would have as much access to viewing entities as an administrator.

robcarr’s picture

I think Ryan's absolutely correct in not removing the security implication warning from the current permissions. However...

Do we not need an additional permission comparable to 'View published content' (in 'Node')?

For example, 'view enabled products' (with options for 'all' products, and each product type). Would obviously need a bit of a review of the entire commerce_product module, but it seems the only safe way for this to move forward.

Otherwise, we are stuck with the cumbersome 'Product Display' nodes as the only way to render Products to all users (in Views or otherwise), or crossing fingers and hoping that there are no exploits with the 'View all products' permission being universally granted.

vegantriathlete’s picture

Pitching in my .02.

For my use case, I do not wish to force the client to create a product display for every product entity. Instead, I have built a view that makes the products visible (directly through the product entities). I ran into the same perplexing situation that the product was not displaying for anonymous users. When I went through the permissions, I balked at granting the "View any product of any type" (or even the more specific "View any Product product" -- e.g. a specific product type) on account of that "Warning: Give to trusted roles only; this permission has security implications." It seems that, perhaps, there aren't security implications at all. And isn't it even more problematic to use the solution of disabling the query rewriting in Views?

What's the status of this now?

For the time being, my plan is to grant the permission for anonymous users and authenticated users. Can somebody provide a use case in which this would be a problem?

w00zle’s picture

I've run into a problem very similar to what vegantriathlete is describing. Basically, I added a couple of Taxonomy term reference fields to one of my Product bundles. I then created a View of Product Displays and used the "Relationships" feature of Views to create exposed filters for the Taxonomy term references on the Product. I did this because the Taxonomy terms were being used to describe various attributes of the Product itself, so it made sense to put them directly on the Product rather than the Product Display. When I tried to use the View as anonymous user, I was able to see my exposed filters, but I could not see any results. The results displayed correctly when logged in as an admin.

I would really appreciate some clarification on this issue. If it's a huge security risk, I am happy to re-implement my View using Product Displays, but from a data modeling standpoint, it seems correct to have the Taxonomy term references on the Products they describe rather than on the Product Displays.

cvangysel’s picture

Removing the restricted access security implications of this permission doesn't seem the way to go

  • You don't want any customer to view any product. Of course disabled products aren't purchasable, but a security implication is present if a store uses Views for displaying their products and they're preparing their store for a the release of new products. I know they should just set a filter to see if the product's state is set to active, but inherently this permission is dangerous because, as noted in #15, it's equivalent to administrator privileges.
  • The implementation of this access restriction is generic, and is used by commerce entities other than commerce_product. Removing the access restriction would create inconsistency in usage and underlying implementation. This is especially important for maintainability reasons. Removing the access restriction for all entities as suggested in #9 and #12 seems like a bad idea, as e.g. 'View any commerce_payment entity' remains a potentially harmfull permission.

Attached is a patch that adds new permissions for commerce_product entities such that only active products are visible.

cvangysel’s picture

Status: Needs work » Needs review
cvangysel’s picture

When investigating the related issue #1845706: When used with Entity Form, the Product reference module does not seem to respect product status I came across a small bug in #19; the problem in that issue clearly shows the need of separate permissions for active products and shows the dangers of the "View any product" permission.

Attached is a new patch that fixes the problem.

megansh’s picture

dpearcefl’s picture

Can we proceed on t his patch? Thanks.

Watergate’s picture

Status: Active » Needs review

subscribing

edit: sorry for subscribing, it was late...

HyperGlide’s picture

@MrWatergate no need to "subscribe" just "Follow" http://drupal.org/node/1306444

akalata’s picture

#21 makes things a bit clearer, thanks!

One suggestion: Would it be possible to move these new 'View active' permissions to near the top of the list, the way Node's 'view all published' comes before the bundle-specific options?

nodecode’s picture

Applied the patch in #21, cleared caches and it works just as advertised. I need this because I use Views to display product variations in a table on product pages. This patch is very necessary IMHO

Any reason it has just been sitting for four months now?

nodecode’s picture

Status: Needs review » Reviewed & tested by the community

bam!

kaidjohnson’s picture

After applying the patch in #21, our product display views (i.e. shopping cart, etc) are not rendering, even though the product(s) in the cart are all active and the user has permission to 'View active products of any type'.

We previously had this issue when the user was not granted 'View any product of any type' and it was resolved by either A) Disabling SQL rewriting on the view (not ideal) or B) Enabling the not-totally-secure 'View any product of any type' permission for the user, but are trying to avoid both of those solutions for the same reason this thread exists.

EDIT:
Using:
Drupal 7.21
Views 3.6
Commerce 1.4

nodecode’s picture

@kaidjohnson
did you clear caches? this was necessary in my case.

kaidjohnson’s picture

@nodecode
Yes, of course. The caches have been fully flushed multiple times.

kaidjohnson’s picture

Here's a glimpse at what we're doing for our shopping cart view - it's based on the shopping cart provided by the commerce module, in case it offers any insights into what's going on here.

kaidjohnson’s picture

Status: Reviewed & tested by the community » Active

Here's an update from some more testing:

My anonymous users are given the permission "View Active Products of Any Type".
They CAN view the products to be able to add them to their cart.
The default commerce_cart_form view is fully viewable by them.

We want to modify our cart to display details only found on the Product itself (and NOT on the line item), such as SKU and Product Type. As soon as we modify the default commerce_cart_form view to make a relationship from the Line Items to the Product, the view is no longer viewable by our anonymous users (or anyone with only the "View Active Products of Any Type" permission).

If we grant these users permission to "View any product of any type", then our anonymous users can view our modified commerce_cart_form view without any problems.

Looking at the patch, I believe the issue we're up against exists here:

if (user_access('view active ' . $entity_type . ' entity', $account)) {
    $conditions->condition($base_table . '.status', 1);
}

In this circumstance, $entity_type = 'commerce_product', but $base_table = 'commerce_order'. Perhaps I'm wrong, and admittedly, the Commerce Query structure is not very familiar to me, but It seems that checking for commerce_order.status = 1 doesn't fulfill the need of actually querying the commerce_product.status = 1. The entire query is based on the commerce_order table, but the complex relational structure we have set up (commerce_line_item related to commerce_order; then commerce_product related to commerce_line_item), appears to make the query not have the correct conditional structure.

It seems to be an awkward situation, because the "View any product of any type" completely bypasses the query/conditional structure (see commerce.module, line 1072).

Any insight would be greatly appreciated.

roderik’s picture

Status: Needs review » Active
FileSize
3.35 KB

Thanks to the readability of this issue and comment #33, I could decide to give this patch a whirl (instead of being lazy and just enabling 'view all products' on my site), because my 'base table' happens to be different too.

It looks like commerce_product_query_commerce_product_access_alter() itself does not adhere to the expectations of commerce_entity_access_query_alter(). For reference, these are the comments in commerce_entity_access_query_alter(), where $base_table is a function argument:

  // Read the base table from the query if available or default to the first
  // table in the query's tables array.
  if (!isset($base_table) && !$base_table = $query->getMetaData('base_table')) {
    // Assume that the base table is the first table if not set. It will result
    // in an invalid query if the first table is not the table we expect,
    // forcing the caller to actually properly pass a base table in that case.
    $tables = &$query->getTables();
    reset($tables);
    $base_table = key($tables);
  }

So the below patch adds a code block in commerce_product_query_commerce_product_access_alter() that "actually properly passes a base table".

...I think. I don't imagine that the change in behavior breaks any existing working code, right?
(that is: adding conditions on 'the first commerce_product table' where before 'the first table of the query' was always taken)

Please review the try{}; I don't know the practical behavior of queries & Tags that well; it just seemed a good thing to try{} from the function specification.

roderik’s picture

Status: Active » Needs review

and 1/10th of a second after I pressed "save", I thought: ...

lmeurs’s picture

In the "Shopping cart summary (Commerce Order)" view we wanted to turn Line item titles into links to their corresponding product displays. To do this we had to add two relations in Views:

  1. From Line item to Product variation
  2. From Product variation to Product display

This worked for admins with sufficient rights, but not for customers... Altering "View any Product product" permission did the trick! Then we found this thread and tried both patches from #21 and #34, but only #34 worked in our setup!

Thanks a lot all, we really appreciate the effort!

jlockhart’s picture

We were having this same problem with variants in panel views not visible to anonymous users. I applied the #21 patch and set the permission, flushed cache, and Viola! Thanks!!

vegantriathlete’s picture

I'm working on a client project that IS using product display nodes. A product image has been attached to the product entity.

I am using the Shopping cart block that is provided through commerce order and have modified the view to include the image of the product. So, I'm running into the same problem that unless the user is assigned a role that is granted permission to "View any [Product] Product" the block doesn't display.

I don't think it's a good option to add that image to the Product Display node in this situation, because it will be problematic (or am I missing something?) to get the view to establish the relationship to the Product Display node to be able to pull the image into the view display.

It would be great to get a resolution to this.

drclaw’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I've been using the patch in comment #34 for some time now with no problems. I'm just gonna be bold and mark this as RTBC. Any objections?

ssoulless’s picture

I just used the patch in #34 checked the permission "View active products of any type" for all the roles. After that my product catalogs made with views worked great.

I have not used line item relationships in views, is there another config in the permissions in order to make work a view with that relatioship?

Jaesin’s picture

Despite the try-catch statement, The approach in #34 seems to work fine for me.

I've done a little refactoring and added a bit more comments in hopes of getting some review and commit action going here. Having a permission that distinguishes between active and inactive products seems like an oversight to me so let's get this committed.

Status: Reviewed & tested by the community » Needs work
Jaesin’s picture

Opps. That's why I shouldn't try to submit patches just before going to bed.

Let me try that again.

dnotes’s picture

Status: Needs work » Reviewed & tested by the community

I see no problems with #43 except comment typos, and this patch does solve the rather glaring issue that views using fields to display products can not at present be seen by anyone bereft of a permission which says it has security implications. With this patch applied, I have verified that:

1. A new permission appears to view all active products, and one to view all active products of each type.
2. Anonymous users with either of those permissions can see views with fields from active products, even if they do not have the "view all products" permission.
3. Products that are disabled do not show up in those views, even if the product display node is published and visible to anonymous users.

I'm marking this as RTBC, since it at least solves the major issue. However, while I don't mind having the new permissions, I still don't feel like I have a good understanding of the security implications of "View any Product product". I have read the comments which say that "administer products" and "view any product" are equivalent, but I don't think that's really true, just as "administer nodes" and "bypass node access" are not exactly equivalent. In that case, there is an additional explanation of "bypass node access", and I would like to see a similar clarification on this permission.

Jaesin’s picture

ssoulless’s picture

#45 should be commited ASAP !!!

griz’s picture

Agreed :)

Liam McDermott’s picture

I have read the comments which say that "administer products" and "view any product" are equivalent, but I don't think that's really true, just as "administer nodes" and "bypass node access" are not exactly equivalent.

This seems to be the primary reason this isn't getting fixed and it looks like it's down to a really weird misunderstanding of permissions.

The necessary and sufficient condition for viewing any product is the permission 'View any product', 'administer products' is just an override for any product related permission, this does not mean 'View any product' can do any of the other things 'administer products' can. Therefore they are not equivalent, 'administer products' is always a higher-level permission.

ptmkenny’s picture

I just want to add to the voices for committing this-- I just wasted a couple hours because I thought this was due to i18n errors when actually it was a permissions issue. (The permission hadn't been checked because of the warning on the permissions page.)

luco’s picture

I just realized that the paths
admin/commerce/products/XX
and
admin/commerce/products/XX/edit
both lead to the same page - product edit.

this page is accessible by anyone with the View any product of any type permission.

I found out because I wanted the title from the Node that references the product entity to appear in the Cart for Anonymous users, and that was the only way to get it to work.

bas.bakker queued 34: 1303194-34.patch for re-testing.

The last submitted patch, 34: 1303194-34.patch, failed testing.

zmove’s picture

+1 for patch #45. When you have some extended products listing and you works with views and panels. Users can't see products when this permission is not checked, which is a problem is it give the user the right to go to admin/commerce/products/xx/edit...

rszrama’s picture

Status: Reviewed & tested by the community » Needs review

I'm marking this needs review simply because I do think it deserves a good thinking session from myself or bojanz. Once added, we won't be able to remove or change the new permission, and as I've written here and elsewhere, there are satisfactory ways to tell Views not to apply product access checks when adding relationships to products. Specifically, if you need a relationship from nodes or line items to products via a reference field, you should update the query settings to bypass query rewriting (or whatever the label for that checkbox is now).

Personally, I'd rather see us have the ability to give view access to any product referenced by a field that a user has access to view (i.e. tie product view access to entity / field view access).

DeFr’s picture

Just wanted to add two notes on this issue.

First, with regards to bypassing the query rewriting, that's probably okay-ish for the most often cited use case in this issue of adding product fields to the commerce cart view ; if those items are in your cart, you've probably see them earlier, so bypassing the query rewriting should be more or less ok. That's a lot less true for views of product displays where you also want to show product fields (related blocks, cross sell, stuff like that): if you set the view to bypass query rewriting and you have node access modules enabled, they won't apply either, and you'll start showing stuff like draft nodes (Workflow access), nodes from other domains (Domain access), …

Yes, most of the node access modules also provide a view filter that can be explicitely set, so there's a kind of workaround, but that's one more thing you need to do whenever you untick the bypass query rewriting to get access to commerce product fields, and that's one more risk to get things wrong ; moreover if you're node access requirements change, you need to go over all your views that displays commerce product. I wouldn't really call that satisfactory.

Another thing that recently came up in this issue is the admin/commerce/products/%commerce_product. Yes, that path leads to the product edit form ; giving users the "View any product of any type" permission does not grant them access to that page though, because it's correctly protected by the update permission and not the view one. Revelant snippet from Commerce Product UI:

  $items['admin/commerce/products/%commerce_product'] = array(
    'title callback' => 'commerce_product_ui_product_title',
    'title arguments' => array(3),
    'page callback' => 'commerce_product_ui_product_form_wrapper',
    'page arguments' => array(3),
    'access callback' => 'commerce_product_access',
    'access arguments' => array('update', 3),
    …

(Also just re-tested to make sure)

Finally, I think that conceptually, tying access of commerce products to the access of entities that references them would make sense: it'd match the behavior of files in core (see file_file_download). That would not really work in a hook_query_alter, because you need a concrete product id to find the entities that are referencing it ; the good news though is that it should be completely unneeded at least for Views relationships (if you're referencing the product through a relationship, then it's because you have access to an entity that's referencing the product).

lmeurs’s picture

rszrama states at #54 to reconsider the patch from #44, in the meanwhile we'd love a quick and safe workaround.

Instead of:

  1. Granting "View any Product product" permission to anonymous users,
  2. Disable SQL rewriting at "View > Query settings > Disable SQL rewriting",
  3. Applying the patch from #44

might the following temporary quick fix be a valid one?

Since commerce_entity_access_query_alter() falsifies the query when no conditions are supplied, we can check whether any conditions are supplied ourselves by implementing hook_commerce_entity_access_condition_ENTITY_TYPE_alter() and adding a condition if none supplied. Our condition only checks the product variation's status, just like the patch does.

This way all other logic seems to be left intact, isn't it?

/**
 * Implements hook_commerce_entity_access_condition_ENTITY_TYPE_alter()
 *
 * THIS IS A TEMPORARY FIX TILL THE FOLLOWING ISSUE IS FIXED
 * @link https://www.drupal.org/node/1303194
 *
 * PROBLEM:
 * Views with a relationship to product variations cannot show the product
 * variations directly (opposed to via product displays) to anonymous users due
 * to rights restrictions. See empty "Commerce add to cart confirmation" popup
 * and error at cart details page.
 *
 * WORKAROUND 1:
 * This can be circumvented by granting "View any Product product" rights to
 * anonymous users, but this may lead to security implications since this makes
 * commerce_entity_access_query_alter() quit processing before other permissions
 * are checked and other modules got a chance to add conditions.
 * @link https://www.drupal.org/node/1303194#comment-9541113
 *
 * WORKAROUND 2:
 * Disable SQL rewriting at "View > Query settings > Disable SQL rewriting".
 * Disabling SQL rewriting will disable node_access checks as well as other
 * modules that implement hook_query_alter() like Workflow access and Domain
 * access.
 *
 * PATCH:
 * A patch was supplied and RTBC, but rszrama decided to reconsider this.
 * @link https://www.drupal.org/node/1303194#comment-9202749
 * @link https://www.drupal.org/node/1303194#comment-9518445
 *
 * TEMPORARY FIX:
 * If no access conditions were provided during the processing of
 * commerce_entity_access_query_alter(), the query gets falsified by the end of
 * this function. Our Commerce entity access condition alter implementation
 * checks for any provided conditions. If none available it adds a condition
 * that checks whether the product variation is published. Now at least one
 * condition is provided, so there is no need for
 * commerce_entity_access_query_alter() to falsify the query.
 *
 * @see commerce_entity_access_query_alter()
 */
function MODULENAME_commerce_entity_access_condition_commerce_product_alter($conditions, $context) {
  // If no conditions were added yet.
  if (COUNT($conditions) === 0) {
    // Add condition that checks whether the product variation is published.
    $conditions->condition($context['base_table'] . '.status', 1);
  }
}
rszrama’s picture

Disabling SQL rewriting is a quick and safe workaround; it only looks scary because of the help text. For any default Commerce View, the entity access check is happening prior to the specific IDs being passed in as a contextual filter, so we don't need Views to be duplicating the effort. For any custom View, you just need to ensure it has some manner of filtering enabled, such as only published nodes of a certain type, which almost any View will already have anyways.

rahafrouz’s picture

after all these comments, I don't know what to do to show my products to authenticated users??? I created some products, without product view, and I want them to be able to shop! is there anything else but granting them "view any product" that I can do?

adam1’s picture

I would really appreciate if a permission "View active products of any type" as #45 suggests, could be commited soon. Because disabling SQL rewriting is not an option in this/my situation :
I show my products using a product display: when setting the field formatter for the product variation field to "rendered product", the variations are only rendered for privileged users, anonymous users can not see the variations.

James A’s picture

I experienced this issue some time ago and simply set the view any product permission to true in order to get around it. Since then some products have expired and I therefore tried the patch in #45 as this would help but my issue is that if the first item in the viewable list is disabled my users see nothing. if it's not the first then it appears to work as expected. Has anyone else come across this?

lingochad’s picture

@James A - I have run into this for a long time now also and the only solution that works for me is to enable the permission for anonymous and authenticated users as you did. If I understand correctly, the only trouble with that is that they may see disabled products which is the problem you are bumping into. Does it work to sort your products in some way? Can you filter the view to not get the expired products? Commerce Product: Status is a field that is available. That seem to fit what you need.

I would certainly love to see a fix for this. I keep coming back to this thread periodically to see if it is fixed.

i.bajrai’s picture

I am turning on "View any BUNDLE product" for every user role on few products that I need the user to refer to when building another product.

Otherwise the entity reference module tells me "The referenced entity (commerce_product: [ID]) is invalid.".

Am I likely to have problems people being able to delete these products or some similar of a malicious nature?

This thread didn't really explain if bad things happen or not :P

MatthijsG’s picture

2017 calling .. the message is still there .. granting this permission to (anonymous) users let them see the fields (pictures etc) but they can't edit or make new products.

So, i'm wondering why this isn't solved after 5 years !!??

heddn’s picture

This also effects non-Views things. Like if you want to filter a Panels node variation by product bundle... if you have the view products global permission, then it will make that rule pass. But if you are anonymous and haven't granted the global permission, then your nice Panels rule will fall on its face (ungracefully) and make your life hard.

#1138322: Product integration with Panels?

heddn’s picture

Some notes from discussing with jmiller and mglaman in IRC:

heddn: if there is a panels rule on a node variation that says "show me if bundle = X", it is always failing because the user is anonymous and does not have "view" permission on the product bundle.

jmiller: Depends on the use case, but, in general, most product details are not a security problem. You are further helped out by the fact that there is no way to view products without some other layer or layers of access requirements (Views is the notably exception, but often there are filters based on stock or product status)

mglaman: I would just give view access for all of it. its a permission we're stuck with, and isn't much of a real security matter unless you have B2B type stuff with restricted access.

carrotandme’s picture

Can't "Disable SQL rewriting" could be a risk to show unexpected users the detail of order? In my usage, it can be.

I listed the products via the relationship from line items on my custom cart. The cart's path for users is "/my-cart". On contextual filter, I set "Order Id" = "Current user's cart order ID". But, once you put any order id after "/my-cart"(i.e "/my-cart/100"), you can view the details of any order UNDER "Disable SQL rewriting" on THE QUERY SETTINGS. So, if anybody who try this way, it'd be better to be careful.

rszrama’s picture

In almost every case I can think of there are ways to filter or check access besides relying on SQL rewriting. Those are generally sufficient, but if you are depending on a contextual filter that is usually empty but need to validate it in the event it's given, you'd need to create a new Views plugin argument validator plugin. That'd make a nice feature request in general if you care to contribute it. There's an example in the Commerce Card on File module.

GuyPaddock’s picture

Just ran into this on a customer's site, where product images weren't appearing in the cart for non-admins. We have a custom cart view that just renders the product entity in the cart, but since non-admins don't have the "view any product of any type" permission, the entity is not rendering in the cart.

+1 for clarifying that granting this permission is not a security risk.

rkelbel48’s picture

I also just ran into this issue as well, I have a view that allows users to see completed/pending orders and filter their order by product within the orders, with an exposed filter via products relationship.

I also agree that a new option for granting this permission needs to happen or the security risk message does need removed if it's not actually an issue.

Subscribed to see where this issue goes.

sidharthap’s picture

Removing the restricted access security implications only for product entity type does not potentially dangerous. viewing any products permission is safe even for anonymous users.
My first attempt, This patch only remove restrict access for commerce_product and adds a notice with description.

timmillwood’s picture

Status: Needs review » Needs work
  1. +++ b/commerce.module
    @@ -1130,9 +1130,12 @@ function commerce_entity_access_permissions($entity_type) {
    +      // Only exception for product entity not to display security implications notice.
    

    This comment mentions product entities but is in the main commerce module. I think this comment can be removed.

  2. +++ b/commerce.module
    @@ -1130,9 +1130,12 @@ function commerce_entity_access_permissions($entity_type) {
    +      $description = (!$restrict_access) ? t('Notice: Allows users to view any %bundle @entity_type', array('@entity_type' => $labels['singular'], '%bundle' => $bundle_info['label'])) : '';
    ...
    +        'description' => $description,
    

    I don't think we need to add a description as part of this patch.

timmillwood’s picture

+++ b/commerce.module
@@ -1130,9 +1130,12 @@ function commerce_entity_access_permissions($entity_type) {
       $permissions['view any ' . $entity_type . ' entity of bundle ' . $bundle_name] = array(
...
-        'restrict access' => TRUE,
+        'restrict access' => $restrict_access,
+        'description' => $description,

I like that this only changes the View any %bundle @entity_type permission, I agree the View any @entity_type of any type permission should be kept as restricted.

sidharthap’s picture

@timmillwood Thank you.
Updated patch as per comment #71

timmillwood’s picture

  1. +++ b/commerce.module
    @@ -1074,7 +1074,7 @@ function commerce_entity_access($op, $entity, $account, $entity_type) {
      * Return permission names for a given entity type.
    

    I think it would be good to add @param and @return docs in the docblock.

  2. +++ b/commerce.module
    @@ -1074,7 +1074,7 @@ function commerce_entity_access($op, $entity, $account, $entity_type) {
    +function commerce_entity_access_permissions($entity_type, $restrict_access = TRUE) {
    

    I wonder if naming the variable $restrict_access is clear enough that it only changes it for the one permission? Maybe the docblock update is enough to cover this?

sidharthap’s picture

@timmillwood Thank you.
Updated patch with comment line.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
gngn’s picture

Any chance the patch from #75 could be commited and make it to upcoming releases?

gngn’s picture

I added a version of #75 for current 7.x-1.x-dev (26 February 2010).

No changes to #75 (i.e. just the diff line numbers).