From 94785d415745b2b2f896cbadfdeac0b393cc5ecf Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Thu, 29 Oct 2015 11:46:56 +1100 Subject: [PATCH] Fixing authorization for VariantOverridesController#bulk_reset --- .../admin/variant_overrides_controller.rb | 4 +- app/models/spree/ability_decorator.rb | 2 +- .../admin/variant_overrides_spec.rb | 83 +++++++++++-------- spec/models/spree/ability_spec.rb | 2 +- 4 files changed, 53 insertions(+), 38 deletions(-) diff --git a/app/controllers/admin/variant_overrides_controller.rb b/app/controllers/admin/variant_overrides_controller.rb index fd8b8c7198..8e6029c08b 100644 --- a/app/controllers/admin/variant_overrides_controller.rb +++ b/app/controllers/admin/variant_overrides_controller.rb @@ -34,7 +34,7 @@ module Admin vo_set = VariantOverrideSet.new @variant_overrides, collection_attributes: collection_hash # Ensure we're authorised to update all variant overrides. - vo_set.collection.each { |vo| authorize! :update, vo } + vo_set.collection.each { |vo| authorize! :bulk_reset, vo } # Changed this to use class method instead, to ensure the value in the database is used to reset and not a dirty passed-in value #vo_set.collection.map! { |vo| vo = vo.reset_stock! } @@ -65,7 +65,7 @@ module Admin end def collection_actions - [:index, :bulk_update] + [:index, :bulk_update, :bulk_reset] end end end diff --git a/app/models/spree/ability_decorator.rb b/app/models/spree/ability_decorator.rb index 2ad5910c5e..9bc303116b 100644 --- a/app/models/spree/ability_decorator.rb +++ b/app/models/spree/ability_decorator.rb @@ -111,7 +111,7 @@ class AbilityDecorator OpenFoodNetwork::Permissions.new(user).managed_product_enterprises.include? variant.product.supplier end - can [:admin, :index, :read, :update, :bulk_update], VariantOverride do |vo| + can [:admin, :index, :read, :update, :bulk_update, :bulk_reset], VariantOverride do |vo| next false unless vo.hub.present? && vo.variant.andand.product.andand.supplier.present? hub_auth = OpenFoodNetwork::Permissions.new(user). diff --git a/spec/controllers/admin/variant_overrides_spec.rb b/spec/controllers/admin/variant_overrides_spec.rb index aa2596ab34..882b1759ca 100644 --- a/spec/controllers/admin/variant_overrides_spec.rb +++ b/spec/controllers/admin/variant_overrides_spec.rb @@ -3,57 +3,72 @@ require 'spec_helper' module Admin describe VariantOverridesController, type: :controller do include AuthenticationWorkflow - let!(:hub_owner) { create :admin_user, enterprise_limit: 2 } + let!(:hub_owner) { create :user, enterprise_limit: 2 } + let!(:v1) { create(:variant) } + let!(:v2) { create(:variant) } + let!(:vo1) { create(:variant_override, hub: hub, variant: v1, price: "6.0", count_on_hand: 5, default_stock: 7, enable_reset: true) } + let!(:vo2) { create(:variant_override, hub: hub, variant: v2, price: "6.0", count_on_hand: 2, default_stock: 1, enable_reset: false) } before do controller.stub spree_current_user: hub_owner end describe "bulk_update" do - context "as an enterprise user I update the variant overrides" do - let!(:hub) { create(:distributor_enterprise, owner: hub_owner) } + let!(:hub) { create(:distributor_enterprise, owner: hub_owner) } + let(:params) { { variant_overrides: [{id: vo1.id, price: "10.0"}, {id: vo2.id, default_stock: 12 }] } } + + context "where the producer has not granted create_variant_overrides permission to the hub" do + it "restricts access" do + spree_put :bulk_update, params + expect(response).to redirect_to spree.unauthorized_path + end + end + + context "where the producer has granted create_variant_overrides permission to the hub" do + let!(:er1) { create(:enterprise_relationship, parent: v1.product.supplier, child: hub, permissions_list: [:create_variant_overrides]) } + it "updates the overrides correctly" do - v1 = create(:variant) - v2 = create(:variant) - vo1 = create(:variant_override, hub: hub, variant: v1, price: "6.0", count_on_hand: 5, default_stock: 7) - vo2 = create(:variant_override, hub: hub, variant: v2, price: "6.0", count_on_hand: 5, default_stock: 7) - vo1.price = "10.0" - vo2.default_stock = 12 - # Have to use .attributes as otherwise passes just the ID - spree_put :bulk_update, {variant_overrides: [vo1.attributes, vo2.attributes]} - # Retrieve from database - VariantOverride.find(vo1.id).price.should eq 10 - VariantOverride.find(vo2.id).default_stock.should eq 12 + spree_put :bulk_update, params + vo1.reload.price.should eq 10 + vo2.reload.default_stock.should eq 12 end end end + describe "bulk_reset" do let!(:hub) { create(:distributor_enterprise, owner: hub_owner) } + before do controller.stub spree_current_user: hub.owner end - context "when a reset request is received" do - it "updates stock to default values" do - v1 = create(:variant) - v2 = create(:variant) - vo1 = create(:variant_override, hub: hub, variant: v1, price: "6.0", count_on_hand: 5, default_stock: 7, enable_reset: true) - vo2 = create(:variant_override, hub: hub, variant: v2, price: "6.0", count_on_hand: 2, default_stock: 1, enable_reset: false) - params = {"variant_overrides" => [vo1.attributes, vo2.attributes]} - spree_put :bulk_reset, params - vo1.reload - expect(vo1.count_on_hand).to eq 7 - end - it "doesn't update where reset is disabled" do - v1 = create(:variant) - v2 = create(:variant) - vo1 = create(:variant_override, hub: hub, variant: v1, price: "6.0", count_on_hand: 5, default_stock: 7, enable_reset: true) - vo2 = create(:variant_override, hub: hub, variant: v2, price: "6.0", count_on_hand: 2, default_stock: 1, enable_reset: false) - params = {"variant_overrides" => [vo1.attributes, vo2.attributes]} + context "where the producer has not granted create_variant_overrides permission to the hub" do + let(:params) { { variant_overrides: [ { id: vo1 } ] } } + + it "restricts access" do spree_put :bulk_reset, params - - vo2.reload - expect(vo2.count_on_hand).to eq 2 + expect(response).to redirect_to spree.unauthorized_path + end + end + + context "where the producer has granted create_variant_overrides permission to the hub" do + let!(:er1) { create(:enterprise_relationship, parent: v1.product.supplier, child: hub, permissions_list: [:create_variant_overrides]) } + + context "where reset is enabled" do + let(:params) { { variant_overrides: [ { id: vo1 } ] } } + + it "updates stock to default values" do + spree_put :bulk_reset, params + expect(vo1.reload.count_on_hand).to eq 7 + end + end + + context "where reset is disabled" do + let(:params) { { variant_overrides: [ { id: vo2 } ] } } + it "doesn't update on_hand" do + spree_put :bulk_reset, params + expect(vo2.reload.count_on_hand).to eq 2 + end end end end diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 6b60707775..4af3d233b1 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -323,7 +323,7 @@ module Spree let!(:er1) { create(:enterprise_relationship, parent: s1, child: d1, permissions_list: [:create_variant_overrides]) } it "should be able to access variant overrides page" do - should have_ability([:admin, :index, :bulk_update], for: VariantOverride) + should have_ability([:admin, :index, :bulk_update, :bulk_reset], for: VariantOverride) end it "should be able to read/write their own variant overrides" do