From ec44947b379757fa99822657c26f135cee6fe330 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 13 Jul 2025 05:34:55 +0500 Subject: [PATCH 1/2] Add special handling for admin users in order permissions Modifies order and line item permission logic to give admin users full access to all orders and line items, bypassing the regular complex joins queries to get orders editable by producers. These complex joins are needed for regular users but for user admins we need to return all orders. --- app/services/permissions/order.rb | 36 ++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/app/services/permissions/order.rb b/app/services/permissions/order.rb index c9e043a8bd..7c48f069ad 100644 --- a/app/services/permissions/order.rb +++ b/app/services/permissions/order.rb @@ -29,12 +29,17 @@ module Permissions # Any orders that the user can edit def editable_orders - orders = Spree::Order.joins(:distributor).where( - id: produced_orders.select(:id), - distributor: { enable_producers_to_edit_orders: true } - ).or( - managed_or_coordinated_orders_where_clause - ) + orders = if @user.admin? + # It returns all orders if the user is an admin + managed_or_coordinated_orders_where_clause + else + Spree::Order.joins(:distributor).where( + id: produced_orders.select(:id), + distributor: { enable_producers_to_edit_orders: true } + ).or( + managed_or_coordinated_orders_where_clause + ) + end filtered_orders(orders) end @@ -45,13 +50,20 @@ module Permissions # Any line items that I can edit def editable_line_items - Spree::LineItem.editable_by_producers( - @permissions.managed_enterprises.select("enterprises.id") - ).or( - Spree::LineItem.where( - order_id: filtered_orders(managed_or_coordinated_orders_where_clause).select(:id) - ) + managed_or_coordinated_line_items_where_clause = Spree::LineItem.where( + order_id: filtered_orders(managed_or_coordinated_orders_where_clause).select(:id) ) + + if @user.admin? + # It returns all line_items if the user is an admin + managed_or_coordinated_line_items_where_clause + else + Spree::LineItem.editable_by_producers( + @permissions.managed_enterprises.select("enterprises.id") + ).or( + managed_or_coordinated_line_items_where_clause + ) + end end private From e6b9373570f91e503cb02a161a258c700f936fd3 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 13 Jul 2025 18:07:14 +0500 Subject: [PATCH 2/2] Refactor line items search to improve security and maintainability Moves search field configuration from frontend to backend to prevent potential security issues with exposing internal field names. The change also improves maintainability by centralizing search logic in the controller. Adds conditional logic to use name_alias for non-admin users when searching distributor names, enhancing data access control. --- .rubocop_todo.yml | 1 + .../line_items_controller.js.coffee | 18 +++----------- .../admin/bulk_line_items_controller.rb | 24 ++++++++++++++++++- spec/services/permissions/order_spec.rb | 4 +++- 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index eff92d4bc1..2ced80c5ae 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -98,6 +98,7 @@ Metrics/ClassLength: - 'lib/reporting/reports/enterprise_fee_summary/enterprise_fees_with_tax_report_by_producer.rb' - 'lib/reporting/reports/enterprise_fee_summary/scope.rb' - 'lib/reporting/reports/xero_invoices/base.rb' + - 'app/services/permissions/order.rb' # Offense count: 30 # Configuration parameters: AllowedMethods, AllowedPatterns, Max. diff --git a/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee b/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee index 1ebebf2bbb..ee37ebe737 100644 --- a/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee +++ b/app/assets/javascripts/admin/line_items/controllers/line_items_controller.js.coffee @@ -19,18 +19,6 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout, $scope.page = 1 $scope.per_page = $scope.per_page_options[0].id $scope.filterByVariantId = null - searchThrough = ["order_distributor_name_alias", - "order_bill_address_phone", - "order_bill_address_firstname", - "order_bill_address_lastname", - "order_bill_address_full_name", - "order_bill_address_full_name_reversed", - "order_bill_address_full_name_with_comma", - "order_bill_address_full_name_with_comma_reversed", - "variant_supplier_name", - "order_email", - "order_number", - "product_name"].join("_or_") + "_cont" $scope.confirmRefresh = -> LineItems.allSaved() || confirm(t("unsaved_changes_warning")) @@ -75,11 +63,10 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout, [formattedStartDate, formattedEndDate] = $scope.formatDates($scope.startDate, $scope.endDate) RequestMonitor.load LineItems.index( - "q[#{searchThrough}]": $scope.query, - "q[variant_id_eq]": $scope.filterByVariantId if $scope.filterByVariantId, "q[order_state_not_eq]": "canceled", "q[order_shipment_state_not_eq]": "shipped", "q[order_completed_at_not_null]": "true", + "q[variant_id_eq]": $scope.filterByVariantId if $scope.filterByVariantId, "q[order_distributor_id_eq]": $scope.distributorFilter, "q[variant_supplier_id_eq]": $scope.supplierFilter, "q[order_order_cycle_id_eq]": $scope.orderCycleFilter, @@ -87,7 +74,8 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout, "q[order_completed_at_lt]": if formattedEndDate then formattedEndDate else undefined, "q[s]": "order_completed_at desc", "page": $scope.page, - "per_page": $scope.per_page + "per_page": $scope.per_page, + "search_query": $scope.query ) $scope.formatDates = (startDate, endDate) -> diff --git a/app/controllers/admin/bulk_line_items_controller.rb b/app/controllers/admin/bulk_line_items_controller.rb index c1d966e545..cb818862da 100644 --- a/app/controllers/admin/bulk_line_items_controller.rb +++ b/app/controllers/admin/bulk_line_items_controller.rb @@ -12,7 +12,7 @@ module Admin @line_items = order_permissions. editable_line_items.where(order_id: orders). includes(:variant). - ransack(params[:q]).result.order(:id) + ransack(line_items_search_query).result.order(:id) @pagy, @line_items = pagy(@line_items) if pagination_required? @@ -88,5 +88,27 @@ module Admin def page params[:page] || 1 end + + def line_items_search_query + query = params.permit(q: {}).to_h[:q] || {} + + search_fields_string = [ + spree_current_user.admin? ? "order_distributor_name" : "order_distributor_name_alias", + "order_bill_address_phone", + "order_bill_address_firstname", + "order_bill_address_lastname", + "order_bill_address_full_name", + "order_bill_address_full_name_reversed", + "order_bill_address_full_name_with_comma", + "order_bill_address_full_name_with_comma_reversed", + "variant_supplier_name", + "order_email", + "order_number", + "product_name" + ].join("_or_") + search_query = "#{search_fields_string}_cont" + + query.merge({ search_query => params[:search_query] }) + end end end diff --git a/spec/services/permissions/order_spec.rb b/spec/services/permissions/order_spec.rb index 27c7f0930f..4cba893542 100644 --- a/spec/services/permissions/order_spec.rb +++ b/spec/services/permissions/order_spec.rb @@ -27,7 +27,9 @@ RSpec.describe Permissions::Order do before { allow(OpenFoodNetwork::Permissions).to receive(:new) { basic_permissions } } context "with user cannot only manage line_items in orders" do - let(:user) { instance_double('Spree::User', can_manage_line_items_in_orders_only?: false) } + let(:user) do + instance_double('Spree::User', can_manage_line_items_in_orders_only?: false, admin?: false) + end describe "finding orders that are visible in reports" do let(:random_enterprise) { create(:distributor_enterprise) }