From 4a9e27f0f0c5d479452ee2bb9c044d9829cdc8da Mon Sep 17 00:00:00 2001 From: Igor Barbosa Date: Thu, 13 Aug 2020 20:51:09 -0300 Subject: [PATCH 1/5] List only accessible users in Spree::Admin::UsersController Admin user's index was displaying a list with all users even if current ability states it shouldn't be accessible Now only users that are allowed to be accessed will be displayed on the list --- .../spree/admin/users_controller.rb | 2 +- .../spree/admin/users_controller_spec.rb | 43 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 0fac0445196..03e53ffdb01 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -124,7 +124,7 @@ def model_class def collection return @collection if @collection - @search = Spree.user_class.ransack(params[:q]) + @search = super.ransack(params[:q]) @collection = @search.result.includes(:spree_roles) @collection = @collection.includes(:spree_orders) @collection = @collection.page(params[:page]).per(Spree::Config[:admin_products_per_page]) diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 6606dc96aba..24f2dda079e 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -18,6 +18,49 @@ } end + context "#index" do + stub_authorization! do |_user| + can :manage, Spree.user_class + end + + context "when the user can manage all users" do + it "assigns a list of all users as @collection" do + get :index, params: { id: user.id } + expect(assigns(:collection)).to eq [user] + end + + it "assigns a ransack search for Spree.user_class" do + get :index, params: { id: user.id } + expect(assigns[:search]).to be_a Ransack::Search + expect(assigns[:search].klass).to eq Spree.user_class + end + end + + context "when user cannot manage some users" do + stub_authorization! do |_user| + can :manage, Spree.user_class + cannot :manage, Spree.user_class, email: 'not_accessible_user@example.com' + end + + it "assigns a list of accessible users as @collection" do + not_accessible_user = create(:user, email: 'not_accessible_user@example.com') + + get :index, params: { id: user.id } + expect(assigns(:collection)).to_not include not_accessible_user + end + end + + context "when Spree.user_class does not respond to #accessible_by" do + it "assigns a list of all users as @collection" do + allow(Spree.user_class).to receive(:respond_to?).and_call_original + allow(Spree.user_class).to receive(:respond_to?).with(:accessible_by).and_return(false) + + get :index, params: { id: user.id } + expect(assigns(:collection)).to eq [user] + end + end + end + context "#show" do stub_authorization! do |_user| can [:admin, :manage], Spree.user_class From f742f5e8bc33c5acb8d96aac61d4c3eeb8e27956 Mon Sep 17 00:00:00 2001 From: Igor Barbosa Date: Thu, 13 Aug 2020 21:38:27 -0300 Subject: [PATCH 2/5] Load only accessible roles in Spree::Admin::UsersController --- .../spree/admin/users_controller.rb | 2 +- .../spree/admin/users_controller_spec.rb | 36 +++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 03e53ffdb01..286e9e4f38b 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -161,7 +161,7 @@ def sign_in_if_change_own_password end def load_roles - @roles = Spree::Role.all + @roles = Spree::Role.accessible_by(current_ability) if @user @user_roles = @user.spree_roles end diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 24f2dda079e..d61b6845b13 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -43,10 +43,10 @@ end it "assigns a list of accessible users as @collection" do - not_accessible_user = create(:user, email: 'not_accessible_user@example.com') + create(:user, email: 'not_accessible_user@example.com') get :index, params: { id: user.id } - expect(assigns(:collection)).to_not include not_accessible_user + expect(assigns(:collection)).to eq [user] end end @@ -112,6 +112,38 @@ end end + context '#new' do + context "when the user can manage all roles" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :index, Spree::Role + end + + it "assigns a list of all roles as @roles" do + role = create(:role) + + get :new, params: { id: user.id } + expect(assigns(:roles)).to eq [role] + end + end + + context "when user cannot list some roles" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :index, Spree::Role + cannot :index, Spree::Role, name: 'not_accessible_role' + end + + it "assigns a list of accessible roles as @roles" do + accessible_role = create(:role, name: 'accessible_role') + create(:role, name: 'not_accessible_role') + + get :new, params: { id: user.id } + expect(assigns(:roles)).to eq [accessible_role] + end + end + end + describe "#create" do let(:dummy_role) { Spree::Role.create(name: "dummyrole") } From f956f09fe26f21a2244a2c518e3bb92083e9b00d Mon Sep 17 00:00:00 2001 From: Igor Barbosa Date: Thu, 13 Aug 2020 21:54:47 -0300 Subject: [PATCH 3/5] Load only accessible stock locations in Spree::Admin::UsersController --- .../spree/admin/users_controller.rb | 2 +- .../spree/admin/users_controller_spec.rb | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 286e9e4f38b..98df35fb1f0 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -168,7 +168,7 @@ def load_roles end def load_stock_locations - @stock_locations = Spree::StockLocation.all + @stock_locations = Spree::StockLocation.accessible_by(current_ability) end def set_roles diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index d61b6845b13..95f4a42cf05 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -142,6 +142,36 @@ expect(assigns(:roles)).to eq [accessible_role] end end + + context "when the user can manage all stock_locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :index, Spree::StockLocation + end + + it "assigns a list of all stock_locations as @stock_locations" do + stock_location = create(:stock_location) + + get :new, params: { id: user.id } + expect(assigns(:stock_locations)).to eq [stock_location] + end + end + + context "when user cannot list some stock_locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :index, Spree::StockLocation + cannot :index, Spree::StockLocation, name: 'not_accessible_stock_location' + end + + it "assigns a list of accessible stock_locations as @stock_locations" do + accessible_stock_location = create(:stock_location, name: 'accessible_stock_location') + create(:stock_location, name: 'not_accessible_stock_location') + + get :new, params: { id: user.id } + expect(assigns(:stock_locations)).to eq [accessible_stock_location] + end + end end describe "#create" do From 39a588884853adc19ef95ddab084ba98077a879e Mon Sep 17 00:00:00 2001 From: Igor Barbosa Date: Fri, 14 Aug 2020 20:21:38 -0300 Subject: [PATCH 4/5] Disallow admins to set not accessible users stock locations Admins users able to create and update a user could inject any stock location id to user params, and the user would have it assigned to it even if the admin don't have the ability to manage it. --- .../spree/admin/users_controller.rb | 9 +- .../spree/admin/users_controller_spec.rb | 92 +++++++++++++++++-- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 98df35fb1f0..a0b022d724b 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -141,6 +141,10 @@ def user_params attributes += [{ spree_role_ids: [] }] end + if can? :manage, Spree::StockLocation + attributes += [{ stock_location_ids: [] }] + end + unless can? :update_password, @user attributes -= [:password, :password_confirmation] end @@ -178,7 +182,10 @@ def set_roles end def set_stock_locations - @user.stock_locations = Spree::StockLocation.where(id: (params[:user][:stock_location_ids] || [])) + if user_params[:stock_location_ids] + @user.stock_locations = + Spree::StockLocation.accessible_by(current_ability).where(id: user_params[:stock_location_ids]) + end end end end diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 95f4a42cf05..4c3c26bcf6b 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -225,11 +225,45 @@ def user expect(user.reload.bill_address.city).to eq('New York') end - it "can set stock locations" do - location = Spree::StockLocation.create(name: "my_location") - location_2 = Spree::StockLocation.create(name: "my_location_2") - post :create, params: { user: { stock_location_ids: [location.id, location_2.id] } } - expect(user.stock_locations).to match_array([location, location_2]) + context "when the user can manage stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :manage, Spree::StockLocation + end + + it "can create user with stock locations" do + location = Spree::StockLocation.create(name: "my_location") + post :create, params: { user: { stock_location_ids: [location.id] } } + expect(user.stock_locations).to eq([location]) + end + end + + context "when the user cannot manage stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + cannot :manage, Spree::StockLocation + end + + it "cannot assign users stock locations" do + location = Spree::StockLocation.create(name: "my_location") + post :create, params: { user: { stock_location_ids: [location.id] } } + expect(user.stock_locations).to eq([]) + end + end + + context "when the user can manage only some stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :manage, Spree::StockLocation + cannot :manage, Spree::StockLocation, name: 'not_accessible_location' + end + + it "can assign accessible stock locations to user" do + location1 = Spree::StockLocation.create(name: "accessible_location") + location2 = Spree::StockLocation.create(name: "not_accessible_location") + post :create, params: { user: { stock_location_ids: [location1.id, location2.id] } } + expect(user.stock_locations).to eq([location1]) + end end end @@ -341,11 +375,49 @@ def user expect(user.reload.bill_address.city).to eq('New York') end - it "can set stock locations" do - location = Spree::StockLocation.create(name: "my_location") - location_2 = Spree::StockLocation.create(name: "my_location_2") - post :update, params: { id: user.id, user: { stock_location_ids: [location.id, location_2.id] } } - expect(user.stock_locations).to match_array([location, location_2]) + context "when the user can manage stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :manage, Spree::StockLocation + end + + it "can update user stock locations" do + location1 = Spree::StockLocation.create(name: "my_location") + location2 = Spree::StockLocation.create(name: "my_location2") + user.stock_locations = [location1] + put :update, params: { id: user.id, user: { stock_location_ids: [location2.id] } } + expect(user.reload.stock_locations).to eq([location2]) + end + end + + context "when the user cannot manage stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + cannot :manage, Spree::StockLocation + end + + it "cannot update users stock locations" do + location1 = Spree::StockLocation.create(name: "my_location") + location2 = Spree::StockLocation.create(name: "my_location2") + user.stock_locations = [location1] + put :update, params: { id: user.id, user: { stock_location_ids: [location2.id] } } + expect(user.reload.stock_locations).to eq([location1]) + end + end + + context "when the user can manage only some stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :manage, Spree::StockLocation + cannot :manage, Spree::StockLocation, name: 'not_accessible_location' + end + + it "can update accessible stock locations to user" do + location1 = Spree::StockLocation.create(name: "accessible_location") + location2 = Spree::StockLocation.create(name: "not_accessible_location") + put :update, params: { id: user.id, user: { stock_location_ids: [location1.id, location2.id] } } + expect(user.reload.stock_locations).to eq([location1]) + end end end From 3037fc972f8041c4377e7b4229da6953fabb3e48 Mon Sep 17 00:00:00 2001 From: Igor Barbosa Date: Fri, 14 Aug 2020 21:01:41 -0300 Subject: [PATCH 5/5] Disallow admins to set not accessible users roles Admins users able to create and update a user could inject any role id to user params, and the user would have it assigned to it even if the admin don't have the ability to manage it. --- .../spree/admin/users_controller.rb | 4 +- .../spree/admin/users_controller_spec.rb | 39 ++++++++++++++++++- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index a0b022d724b..ea43f7b7d8d 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -176,8 +176,8 @@ def load_stock_locations end def set_roles - if user_params[:spree_role_ids] && can?(:manage, Spree::Role) - @user.spree_roles = Spree::Role.where(id: user_params[:spree_role_ids]) + if user_params[:spree_role_ids] + @user.spree_roles = Spree::Role.accessible_by(current_ability).where(id: user_params[:spree_role_ids]) end end diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 4c3c26bcf6b..181dacfd02a 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -204,6 +204,11 @@ def user end context "when the user cannot manage roles" do + stub_authorization! do |_user| + can :manage, Spree.user_class + cannot :manage, Spree::Role + end + it "cannot assign users roles" do post :create, params: { user: { name: "Bob Bloggs", spree_role_ids: [dummy_role.id] } } expect(user.spree_roles).to eq([]) @@ -215,6 +220,21 @@ def user end end + context "when the user can manage only some roles" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :manage, Spree::Role + cannot :manage, Spree::Role, name: "not_accessible_role" + end + + it "can assign accessible roles to user" do + role1 = Spree::Role.create(name: "accessible_role") + role2 = Spree::Role.create(name: "not_accessible_role") + post :create, params: { user: { spree_role_ids: [role1.id, role2.id] } } + expect(user.spree_roles).to eq([role1]) + end + end + it "can create a shipping_address" do post :create, params: { user: { ship_address_attributes: valid_address_attributes } } expect(user.reload.ship_address.city).to eq('New York') @@ -255,7 +275,7 @@ def user stub_authorization! do |_user| can :manage, Spree.user_class can :manage, Spree::StockLocation - cannot :manage, Spree::StockLocation, name: 'not_accessible_location' + cannot :manage, Spree::StockLocation, name: "not_accessible_location" end it "can assign accessible stock locations to user" do @@ -318,6 +338,21 @@ def user end end + context "when the user can manage only some stock locations" do + stub_authorization! do |_user| + can :manage, Spree.user_class + can :manage, Spree::Role + cannot :manage, Spree::Role, name: "not_accessible_role" + end + + it "can update accessible roles to user" do + role1 = Spree::Role.create(name: "accessible_role") + role2 = Spree::Role.create(name: "not_accessible_role") + put :update, params: { id: user.id, user: { spree_role_ids: [role1.id, role2.id] } } + expect(user.reload.spree_roles).to eq([role1]) + end + end + context "allowed to update email" do stub_authorization! do |_user| can [:admin, :update, :update_email], Spree.user_class @@ -409,7 +444,7 @@ def user stub_authorization! do |_user| can :manage, Spree.user_class can :manage, Spree::StockLocation - cannot :manage, Spree::StockLocation, name: 'not_accessible_location' + cannot :manage, Spree::StockLocation, name: "not_accessible_location" end it "can update accessible stock locations to user" do