Merge pull request #768 from opf/fix/mass_assignment_members_controller_3335

app/controllers/members_controller.rb

@@ -147,33 +147,33 @@ JS
   private
 
   def new_members_from_params
-    members = []
+    user_ids = possibly_seperated_ids_for_entity(params[:member], :user)
+    roles = Role.find_all_by_id(possibly_seperated_ids_for_entity(params[:member], :role))
 
-    attrs = params[:member].dup
-    user_ids = possibly_seperated_ids_for_entity(attrs, :user)
-    roles = Role.find_all_by_id(possibly_seperated_ids_for_entity(attrs, :role))
+    new_member = lambda do |user_id|
+      Member.new(permitted_params.member).tap do |member|
+        member.user_id = user_id if user_id
+      end
+    end
 
-    user_ids.each do |user_id|
-      member = Member.new attrs
-      # workaround due to mass-assignment protected member_roles.role_id
-      member.member_roles << roles.collect {|r| MemberRole.new :role => r }
-      member.user_id = user_id
-      members << member
+    members = user_ids.map do |user_id|
+      new_member.call(user_id)
     end
     # most likely wrong user input, use a dummy member for error handling
     if !members.present? && roles.present?
-      members = [Member.new(attrs.merge({ :member_roles => roles.collect {|r| MemberRole.new :role => r } }))]
+      members << new_member.call(nil)
     end
     members
   end
 
   def each_comma_seperated(array, &block)
-    array.each do |elem|
-      if elem.to_s.match /\d(,\d)*/
-        array += block.call(array.delete(elem))
-      end
+    array.map do |e|
+      if e.to_s.match /\d(,\d)*/
+        block.call(e)
+      else
+        e
       end
-    return array
+    end.flatten
   end
 
   def transform_array_of_comma_seperated_ids(array)
@@ -185,26 +185,24 @@ JS
 
   def possibly_seperated_ids_for_entity(array, entity = :user)
     if !array[:"#{entity}_ids"].nil?
-      transform_array_of_comma_seperated_ids(array.delete(:"#{entity}_ids"))
-    elsif (!array[:"#{entity}_id"].nil?) && ((id = array.delete(:"#{entity}_id")).present?)
+      transform_array_of_comma_seperated_ids(array[:"#{entity}_ids"])
+    elsif !array[:"#{entity}_id"].nil? && (id = array[:"#{entity}_id"]).present?
       [id]
     else
       []
     end
   end
 
-
   def update_member_from_params
     # this way, mass assignment is considered and all updates happen in one transaction (autosave)
-    attrs = params[:member].except(:user_id)
-    attrs.merge! params[:membership].dup if params[:membership].present?
-    attrs.delete(:project_id)
+    attrs = permitted_params.member.dup
+    attrs.merge! permitted_params.membership.dup if params[:membership].present?
 
+    if attrs.include? :role_ids
       role_ids = attrs.delete(:role_ids).map(&:to_i).select{ |i| i > 0 }
-
       @member.assign_roles(role_ids)
-
-    @member.attributes = attrs
+    end
+    @member.update_attributes(attrs)
     @member
   end
 end

app/models/member.rb

@@ -28,14 +28,14 @@
 #++
 
 class Member < ActiveRecord::Base
+  include ActiveModel::ForbiddenAttributesProtection
+
   belongs_to :user
   belongs_to :principal, :foreign_key => 'user_id'
   has_many :member_roles, :dependent => :destroy, :autosave => true
   has_many :roles, :through => :member_roles
   belongs_to :project
 
-  attr_protected :project_id, :user_id, :role_ids
-
   validates_presence_of :project
   validates_uniqueness_of :user_id, :scope => :project_id
 

app/models/permitted_params.rb

@@ -120,6 +120,10 @@ class PermittedParams < Struct.new(:params, :user)
     permitted_params
   end
 
+  def member
+    params.require(:member).permit(*self.class.permitted_attributes[:member])
+  end
+
   def planning_element_type
     params.require(:planning_element_type).permit(*self.class.permitted_attributes[:planning_element_type])
   end
@@ -299,6 +303,8 @@ class PermittedParams < Struct.new(:params, :user)
         :membership => [
           :project_id,
           :role_ids => []]],
+      :member => [
+        :role_ids => []],
       :new_work_package => [
         :subject,
         :description,

doc/CHANGELOG.md

@@ -29,6 +29,7 @@ See doc/COPYRIGHT.rdoc for more details.
 
 # Changelog
 
+* `#3335` Fix: Mass assignment in members controller
 * `#3371` [Work Package Tracking] Wrong 404 when custom query not exists
 * `#3440` New workpackage form layout
 * `#4087` Accessible form errors

spec/controllers/members_controller_spec.rb

@@ -41,6 +41,64 @@ describe MembersController do
     User.stub(:current).and_return(admin)
   end
 
+  describe "create" do
+    let(:admin) { FactoryGirl.create(:admin) }
+    let(:project_2) { FactoryGirl.create(:project) }
+
+    before do
+      User.stub(:current).and_return(admin)
+    end
+
+    it "should work for multiple users" do
+      post :create,
+        :project_id => project_2.identifier,
+        :member => {
+          :user_ids => [admin.id, user.id],
+          :role_ids => [role.id]
+        }
+
+      response.response_code.should < 400
+
+      [admin, user].each do |u|
+        u.reload
+        u.memberships.should have_at_least(1).item
+
+        u.memberships.find do |m|
+          m.roles.should include(role)
+        end.should_not be_nil
+      end
+    end
+  end
+
+  describe "update" do
+    let(:admin) { FactoryGirl.create(:admin) }
+    let(:project_2) { FactoryGirl.create(:project) }
+    let(:role_1) { FactoryGirl.create(:role) }
+    let(:role_2) { FactoryGirl.create(:role) }
+    let(:member_2) { FactoryGirl.create(
+      :member,
+      :project => project_2,
+      :user => admin,
+      :roles => [role_1])
+    }
+
+    before do
+      User.stub(:current).and_return(admin)
+    end
+
+    it "should, however, allow roles to be updated through mass assignment" do
+      put 'update',
+        :project_id => project.identifier,
+        :id => member_2.id,
+        :member => {
+          :role_ids => [role_1.id, role_2.id]
+        }
+
+      Member.find(member_2.id).roles.should include(role_1, role_2)
+      expect(response.response_code).to be < 400
+    end
+  end
+
   describe :autocomplete_for_member do
     let(:params) { ActionController::Parameters.new({ "id" => project.identifier.to_s }) }
 

spec/controllers/users_controller_spec.rb

@@ -360,6 +360,31 @@ describe UsersController do
     end
   end
 
+  describe "update memberships" do
+    let(:project) { FactoryGirl.create(:project) }
+    let(:role) { FactoryGirl.create(:role) }
+
+    it "works" do
+      # i.e. it should successfully add a user to a project's members
+      as_logged_in_user admin do
+        post :edit_membership,
+          :id => user.id,
+          :membership => {
+            :project_id => project.id,
+            :role_ids => [role.id]
+          },
+          :format => "js"
+      end
+
+      expect(response.status).to eql(200)
+
+      is_member = user.reload.memberships.any? do |m|
+        m.project_id == project.id && m.role_ids.include?(role.id)
+      end
+      expect(is_member).to eql(true)
+    end
+  end
+
   describe "Anonymous should not be able to create a user" do
 
     it "should redirect to the login page" do

spec/models/permitted_params_spec.rb

@@ -711,4 +711,38 @@ describe PermittedParams do
       it_behaves_like 'allows params'
     end
   end
+
+  describe 'member' do
+    let (:attribute) { :member }
+
+    describe 'role_ids' do
+      let(:hash) { { "role_ids" => [] } }
+
+      it_behaves_like 'allows params'
+    end
+
+    describe 'user_id' do
+      let(:hash) { { "user_id" => "blubs" } }
+
+      it_behaves_like 'forbids params'
+    end
+
+    describe 'project_id' do
+      let(:hash) { { "user_id" => "blubs" } }
+
+      it_behaves_like 'forbids params'
+    end
+
+    describe 'created_on' do
+      let(:hash) { { "created_on" => "blubs" } }
+
+      it_behaves_like 'forbids params'
+    end
+
+    describe 'mail_notification' do
+      let(:hash) { { "mail_notification" => "blubs" } }
+
+      it_behaves_like 'forbids params'
+    end
+  end
 end