GitLab

hence bypassing restriction on portal_simulation, which is not accessible by regular user.

I am a bit surprised by this, there are lots of portal_simulation access, why the rest of the application is working as this user.

One thing I know is that many years ago, it was considered that "it's not good that users can see simulation", so we applied in Nexedi ERP5 a different configuration of security, see portal_simulation/manage_access :

vs the standard

I think this was never really finished and I don't know if it actually solves anything ( no user has "View" permission on simulation, so I don't think there's a way to view simulation ). What I am thinking is that maybe it's time to give up this unfinished patch instead ?

This arises from the Resource_viewInventoryDialog view, which tries to access getVariationCategoryItemList.

BTW a small tip, before copy/pasting a gitlab link, press y to get a permanent link, replacing "master" branch by the current commit hash ( so the link becomes https://lab.nexedi.com/nexedi/erp5/-/blob/69817b19f90e07553cefd4008706ef20d2ecd724/bt5/erp5_pdm/SkinTemplateItem/portal_skins/erp5_pdm/Resource_viewInventoryDialog/listbox.xml#L334 )

I am a bit surprised by this, there are lots of portal_simulation access, why the rest of the application is working as this user.

Well, extension.erp5.InventoryBrain.py seems to proxy many of the accesses, if I understand correctly what it does. My first guess had been that all users accesses to portal_simulation were made through this component, hence why everything works.

maybe it's time to give up this unfinished patch instead

Do you know where I can find this patch? I have tested to restore the security manually on my dev instance and it seems to work, but I do not know how the values were set or if there are other parts to this patch.

When I wrote "lots of portal_simulation access", I had in mind scripts calling methods on portal_simulation, for example Resource_viewInventoryDialog uses Resource_getFutureInventoryList which contains this:

return [
    makeResultLine(brain)
    for brain in portal.portal_simulation.getFutureInventoryList(
        **inventory_list_kw)
]

So if this works, it means it's allowed to access portal_simulation itself. This may just mean that I misunderstood the initial problem.

Still, there is a "different configuration" on Nexedi ERP5, "different configuration" is more technically correct than patch. Maybe it comes from portal.portal_simulation.Base_setDefaultSecurity() in a script named ERP5Site_postInstall. It can be good to know, but the problem was apparently different.

ah I think I understand the problem, what's user can not access is not portal_simulation itself, it's that this list method here uses brain.getObject().getVariationCategoryItemList() and this brain.getObject() might return a simulation movement which (in Nexedi custom configuration) the user can not access.

So this still seems a problem related to Nexedi ERP5 configuration (but I am guessing a lot here, I might be completely wrong)

If you don't know Inventory API well yet, this might help: getInventoryList returns a list of InventoryListBrain. getInventoryList returns "aggregated inventory", for example it answers "give me the stock, grouped by node (warehouse) and by resource (product)" then it will return something like: [ InventoryListBrain(node=Lille warehouse, resource=apple, quantity=2), InventoryListBrain(node=Lille warehouse, resource=orange, quantity=4) ], this means there are 2 apples in Lille, which might be the consequence of a purchase packing list of 10 apples and a sales packing list of 8 apples, but this API does not return the detail ( getMovementHistoryList does and it has another MovementHistoryListBrain ). On an InventoryListBrain, there are some properties and methods to access resource, node, etc. You can also use getObject() and it will return a random movement from the group. In the case of future inventory, this also includes simulation movement: for example there is a planned sales order of 1 apple that made a planned simulation movement to reduce the future stock by 1 - this simulation movement will become a sales packing list when the sales order is confirmed.

This .getObject() API is sometimes useful, because not everything is exposed in InventoryListBrain, so sometimes you know that because of the group_by* parameters of getInventoryList, all movements from the groups are supposed to share the same property (for example after using getInventoryList(group_by_resource=True), you can assume that getResource is same for all movements of the group), but sometimes resorting to getObject() seems a missing API. Properties exposed on InventoryListBrain are easier to use because they give you something independent on the "side", in the apple example above, in the purchase packing list, Lille warehouse was destination and in the sales packing list it was the source and at InventoryListBrain it is the node in a side independent way that is hard to know with getObject(), so it's especially missing API for properties depending on the "side", variation do not depend on side, so it does not seem the best fix to add API for variation here.

I don't know what's the proper fix here, if Nexedi ERP5 custom configuration does not solve anything and creates more problems like this, one way could be to reconsider this configuration. I don't remember this configuration was causing problems in the past, I don't know why I remembered this configuration (maybe just because I don't like that we have a non standard configuration here) and I'm not really pushing to drop it either.

I added getVariationCategoryValueListDict because variation_text is not useful from application developer point of view, it is too primitive. Thanks to getVariationCategoryValueListDict, I do not need to write the same code in many places. It is useful for customer projects which use variations seriously. It is not added for bypassing restriction.

Adding such a method for bypassing restriction is a bad idea.

ERP5 user who uses inventory must have all required permissions on all movements that have an impact on inventory.

Can I ask what the rest of the component (extension.erp5.InventoryBrain) is intended for then, especially the InventoryListBrain class? It seemed to me like a way to proxy security on simulations, but seems like this is a wrong guess.

InventoryListBrain is a set of helper methods that are useful to make inventory view with listbox.

But, for instance, getResourceValue does bypass security, and seems to do nothing else: even if the user does not have access to the linked resource, it will be accessible to him in the simulation.

getResourceValue itself is not a problem because if a resource is not accessible by user then unauthorized error will occur when user uses that resource. But getResourceTitle is a problem, someone who should not be able to get title of the resource can get it. I did not think about bypass security, because all those methods on InventoryListBrain are useful, I can reduce code, but it is a bug. If brain does not have getResourceValue then I have to retrieve an object by using brain.resource_uid everytime, it is painful.

Thanks Yusei and Jérôme for all your feedbacks and for pointing aspects I didn't even know. I am closing this MR since it seems:

even if some methods (getResourceTitle was cited among others) do bypass security, this is not an intended behavior, and should not get merged into ERP5 master;
the security of simulations on Nexedi ERP5 is different than elsewhere, so the reason why fix was created in the first place is meaningless here.

Regarding Nexedi ERP5, I will keep this fix for now, as a temporary measure, and will eventually fully rework on our use of the Inventory API, and have a proper look at the security on the module, which goes far behind this issue.

closed

GitLab

erp5_core: add `getVariationCategoryItemList` to `InventoryListBrain`

Description of the problem

Proposed solution