InvenTree icon indicating copy to clipboard operation
InvenTree copied to clipboard

Django permission group naming can be misleading

Open LavissaWoW opened this issue 10 months ago • 8 comments

Within my work here, I'm noticing the Django permission groups tend to be a bit misleading with regards to what a general user would think they imply.

What a user might think: View: I can see the elements in this category Add: I can create new entries in this category Change: I can edit exisiting entries in this category Delete: I can remove/delete entries in this category

What actually is the case: View: I can perform GET requests Add: I can perform POST requests Change: I Can perform PATCH/PUT requests Delete: I can perform DELETE requests

Here's where theory collides with practise. We have a lot of what one would consider "Change"-defined actions performed via POST requests. State transitions and receiving line items to name two. This inevitably leads to confusion. And for some it might be a problem as they want to granularly limit creating things and managing exisiting things to different groups of employees.

I've created the issue without a template, as it's not a bug per se, but more a call for discussion on the topic.

LavissaWoW avatar Apr 11 '24 19:04 LavissaWoW

Case in point, actually: CUI defines PO state change buttons to be visible if roles.purchase_order.change is true. But clicking any button throws:

Action Prohibited

Create operation not allowed

LavissaWoW avatar Apr 11 '24 19:04 LavissaWoW

@LavissaWoW I would agree, that a POST request is not always the same thing as a create operation - perhaps you can compile a list of API actions where you think the permission is incorrectly specified? We may need to either manually adjust the permissions on those endpoints, or reassess how we implement the roles

SchrodingersGat avatar Apr 11 '24 22:04 SchrodingersGat

@SchrodingersGat Sure thing. I'm aware of the different apps in the system. I only need to run through all the API endpoint definitions, and I'll be able to compile a list of possible discrepancies.

LavissaWoW avatar Apr 11 '24 22:04 LavissaWoW

  • General:
    • Uploading attachments
    • Adding Price breaks
    • Adding parameters
  • Barcode
    • The barcode endpoints ALL require ADD permissions
  • BOM
    • bom_substitute_create (Is this considered an ADD feature, or a CHANGE feature? You're changing the behaviour of a BOM line, after all.)
  • Build
    • build_allocate_create
    • build_auto_allocate_create
    • build_cancel_create
    • build_complete_create
    • build_create_output_create (maybe a corner case, as it's CHANGE for the BO, but ADD for the Stock Item)
    • build_delete_outputs_create (Somehow this is a POST request??)
    • build_finish_create (Should a CHANGE user not be able to mark his work as finished?)
    • build_item_create (Not sure of the usage of this one)
  • Labels
    • I'm not sure if these actually matter
  • Machine
    • machine_restart_create (Requiring ADD for restarting machines necessary?)
  • Order
    • order_'any'_extra_line_create
    • order_'any'_line_create
    • order_'any'_cancel_create
    • order_'any'_complete_create
    • order_'any'_issue_create
    • order_'po/ro'_receive_create
    • order_so_allocate_create
    • order_so_allocate_serials_create
    • order_so_shipment_create
    • order_so_shipment_ship_create
  • Part
    • part_change_category_create
    • part_internal_price_create
    • part_parameter_create
    • part_related_create
    • part_test_template_create
  • Search
    • search_create (why?)
  • Stock
    • stock_install_create (Installing a trackable item as a part of a BO should not be limited to ADD roles)
    • stock_return_create
    • stock_serialize_create
    • stock_uninstall_create
    • stock_add_create
    • stock_assign_create
    • stock_change_status_create
    • stock_count_create
    • stock_merge_create
    • stock_remove_create
    • stock_test_create
    • stock_transfer_create

LavissaWoW avatar Apr 16 '24 21:04 LavissaWoW

@SchrodingersGat @matmair See the above list of endpoints that I feel should not require ADD permissions, but do currently, because they're POST requests.

LavissaWoW avatar Apr 16 '24 21:04 LavissaWoW

Any change to the permission system is a security risk and a highly breaking so it ought to be well thought out. There are a few open issues with the permission system, those should be addressed too if we require every user to reevaluate all their permissions.

FYI there are discussions on GitHub.

matmair avatar Apr 16 '24 21:04 matmair

The big "issue" here is that we have associated our "add" role with a "POST" style request - which makes sense for simple CRUD operations but our API is much more complex than that. I think there is space to re-evalate the permissions (in line with more "logical" operations) - but this would be a change which breaks workflow for existing users at a bare minimum.

SchrodingersGat avatar Apr 18 '24 10:04 SchrodingersGat

@matmair there are security concerns to be argued over requiring any meaningful actions require full POST access too. Such a system leads to a lot of users having a lot of power they might not need or "deserve"

LavissaWoW avatar Apr 18 '24 17:04 LavissaWoW

This issue seems stale. Please react to show this is still important.

github-actions[bot] avatar Jun 18 '24 11:06 github-actions[bot]