Merge pull request #3732 from igorbp/respect-current-ability-in-users…

…-controllers

Respect current ability in users controllers

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])
@@ -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
@@ -161,24 +165,27 @@ 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
       end
 
       def load_stock_locations
-        @stock_locations = Spree::StockLocation.all
+        @stock_locations = Spree::StockLocation.accessible_by(current_ability)
       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
 
       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

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
+        create(:user, email: 'not_accessible_user@example.com')
+
+        get :index, params: { id: user.id }
+        expect(assigns(:collection)).to eq [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
@@ -69,6 +112,68 @@
     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
+
+    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
     let(:dummy_role) { Spree::Role.create(name: "dummyrole") }
 
@@ -99,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([])
@@ -110,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')
@@ -120,11 +245,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
 
@@ -179,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
@@ -236,11 +410,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