Don't remove non-accessible roles when assigning new accessible roles

When the current ability logged in assigned new roles to which they have
access to a user, other non-accessible roles were removed. We're now
ensuring that non-accessible roles are not touched.

It also fixes the roles being saved twice: once on the generic
`user#update` and a second on `#set_roles`. Now, that only happens in
the latter situation.

Fixes #4528

backend/app/controllers/spree/admin/users_controller.rb

@@ -103,10 +103,6 @@ def user_params
           attributes |= [:email]
         end
 
-        if can? :manage, Spree::Role
-          attributes += [{ spree_role_ids: [] }]
-        end
-
         if can? :manage, Spree::StockLocation
           attributes += [{ stock_location_ids: [] }]
         end
@@ -142,9 +138,13 @@ def load_stock_locations
       end
 
       def set_roles
-        if user_params[:spree_role_ids]
-          @user.spree_roles = Spree::Role.accessible_by(current_ability).where(id: user_params[:spree_role_ids])
-        end
+        roles_ids = params[:user][:spree_role_ids]
+        return unless roles_ids
+
+        @user.update_spree_roles(
+          Spree::Role.where(id: roles_ids),
+          ability: current_ability
+        )
       end
 
       def set_stock_locations

backend/spec/controllers/spree/admin/users_controller_spec.rb

@@ -361,6 +361,14 @@ def user
         put :update, params: { id: user.id, user: { spree_role_ids: [role1.id, role2.id] } }
         expect(user.reload.spree_roles).to eq([role1])
       end
+
+      it "can't remove non accessible roles when assigning accessible ones" do
+        role1 = Spree::Role.create(name: "accessible_role")
+        role2 = Spree::Role.create(name: "not_accessible_role")
+        user.spree_roles << role2
+        put :update, params: { id: user.id, user: { spree_role_ids: [role1.id] } }
+        expect(user.reload.spree_roles).to match_array([role1, role2])
+      end
     end
 
     context "allowed to update email" do

core/app/models/concerns/spree/user_methods.rb

@@ -90,6 +90,21 @@ def can_be_deleted?
       orders.none?
     end
 
+    # Updates the roles in keeping with the given ability's permissions
+    #
+    # Roles that are not accessible to the given ability will be ignored. It
+    # also ensure not to remove non accessible roles when assigning new
+    # accessible ones.
+    #
+    # @param given_roles [Spree::Role]
+    # @param ability [Spree::Ability]
+    def update_spree_roles(given_roles, ability:)
+      accessible_roles = Spree::Role.accessible_by(ability)
+      non_accessible_roles = Spree::Role.all - accessible_roles
+      new_accessible_roles = given_roles - non_accessible_roles
+      self.spree_roles = spree_roles - accessible_roles + new_accessible_roles
+    end
+
     private
 
     def check_for_deletion

core/spec/models/spree/concerns/user_methods_spec.rb

@@ -21,6 +21,52 @@
     end
   end
 
+  describe '#update_spree_roles' do
+    let(:ability) do
+      Class.new do
+        include CanCan::Ability
+
+        def initialize(_user)
+          can :manage, ::Spree::Role, name: 'accessible_role'
+        end
+      end.new(:user)
+    end
+    let!(:accessible_role) { create(:role, name: 'accessible_role') }
+    let!(:non_accessible_role) { create(:role, name: 'non_accessible_role') }
+
+    it 'can add accessible roles' do
+      user = create(:user, spree_roles: [])
+
+      user.update_spree_roles([accessible_role], ability: ability)
+
+      expect(user.reload.spree_roles).to eq([accessible_role])
+    end
+
+    it 'can remove accessible roles' do
+      user = create(:user, spree_roles: [accessible_role])
+
+      user.update_spree_roles([], ability: ability)
+
+      expect(user.reload.spree_roles).to eq([])
+    end
+
+    it "can't add non accessible roles" do
+      user = create(:user, spree_roles: [])
+
+      user.update_spree_roles([non_accessible_role], ability: ability)
+
+      expect(user.reload.spree_roles).to eq([])
+    end
+
+    it "can't remove non accessible roles" do
+      user = create(:user, spree_roles: [non_accessible_role])
+
+      user.update_spree_roles([], ability: ability)
+
+      expect(user.reload.spree_roles).to eq([non_accessible_role])
+    end
+  end
+
   describe '#last_incomplete_spree_order' do
     subject { test_user.last_incomplete_spree_order }