From 0fca8ddf31bf64287b94200405028d0d520fc6d2 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Thu, 19 Dec 2013 15:32:45 +0000
Subject: [PATCH 01/19] use strong params instead of attr_protected in member

---
 app/models/member.rb           |  2 --
 app/models/permitted_params.rb | 10 ++++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/app/models/member.rb b/app/models/member.rb
index 24c319e0af..0e6774c78e 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -34,8 +34,6 @@ class Member < ActiveRecord::Base
   has_many :roles, :through => :member_roles
   belongs_to :project
 
-  attr_protected :project_id, :user_id, :role_ids
-
   validates_presence_of :principal, :project
   validates_uniqueness_of :user_id, :scope => :project_id
 
diff --git a/app/models/permitted_params.rb b/app/models/permitted_params.rb
index 7a407a3152..d9ac6b0e8e 100644
--- a/app/models/permitted_params.rb
+++ b/app/models/permitted_params.rb
@@ -85,7 +85,7 @@ class PermittedParams < Struct.new(:params, :user)
   def custom_field_type
     params.require(:type)
   end
-  
+
   def enumeration_type
     params.require(:type)
   end
@@ -116,6 +116,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
@@ -274,6 +278,8 @@ class PermittedParams < Struct.new(:params, :user)
         :membership => [
           :project_id,
           :role_ids => []]],
+      :member => [
+        :role_ids],
       :new_work_package => [
         :subject,
         :description,
@@ -363,7 +369,7 @@ class PermittedParams < Struct.new(:params, :user)
         :redirect_existing_links ],
       :wiki_content => [
         :comments,
-        :text, 
+        :text,
         :lock_version ],
       :move_to => [ :move_to ]
     }

From ba3b747f1e09ce147e79a2b9cead72e385f3dbf9 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Thu, 19 Dec 2013 16:51:25 +0000
Subject: [PATCH 02/19] spec confirming mass assignment weakness

---
 spec/controllers/members_controller_spec.rb | 57 +++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb
index 0c62d610ed..d095e17a93 100644
--- a/spec/controllers/members_controller_spec.rb
+++ b/spec/controllers/members_controller_spec.rb
@@ -40,6 +40,63 @@ describe MembersController do
     User.stub(:current).and_return(user)
   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])
+    }
+
+    def dont_update(field, value)
+      put :update,
+        :project_id => project.identifier,
+        :id => member_2.id,
+        :member => {
+          :role_ids => [role_1.id],
+          field => value
+        }
+
+      response.should_not be_success
+      Member.find(member_2.id).attributes[field.to_s].should_not == value
+    end
+
+    before do
+      User.stub(:current).and_return(admin)
+    end
+
+    it "should specifically not allow 'user_id' to be mass assigned" do
+      dont_update(:user_id, user.id)
+    end
+
+    it "should specifically not allow 'project_id' to be mass assigned" do
+      dont_update(:project_id, project.id)
+    end
+
+    it "should specifically not allow 'created_on' to be mass assigned" do
+      dont_update(:created_on, Time.zone.at(1111111111))
+    end
+
+    it "should specifically not allow 'mail_notification' to be mass assigned" do
+      dont_update(:mail_notification, !member_2.mail_notification)
+    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]
+        }
+      response.should be_success
+      member.roles.should include(role_1, role_2)
+    end
+  end
+
   describe :autocomplete_for_member do
     let(:params) { ActionController::Parameters.new({ "id" => project.identifier.to_s }) }
 

From c84afbdce385e2f2c4ce8dc6d6c1c8ea4a0519d9 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Thu, 19 Dec 2013 19:10:59 +0000
Subject: [PATCH 03/19] use permitted params for member update

---
 app/controllers/members_controller.rb       | 19 +++++++++----------
 app/models/permitted_params.rb              |  2 +-
 spec/controllers/members_controller_spec.rb |  5 +++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index be6638d413..a61a408057 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -28,6 +28,8 @@
 #++
 
 class MembersController < ApplicationController
+  include ActiveModel::ForbiddenAttributesProtection
+
   model_object Member
   before_filter :find_model_object_and_project, :except => [:autocomplete_for_member, :paginate_users]
   before_filter :find_project, :only => [:autocomplete_for_member, :paginate_users]
@@ -148,7 +150,6 @@ JS
 
   def new_members_from_params
     members = []
-
     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))
@@ -193,18 +194,16 @@ JS
     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?
 
-    role_ids = attrs.delete(:role_ids).map(&:to_i).select{ |i| i > 0 }
-
-    @member.assign_roles(role_ids)
-
-    @member.attributes = attrs
+    if attrs.include? :role_ids
+      role_ids = attrs.delete(:role_ids).map(&:to_i).select{ |i| i > 0 }
+      @member.assign_roles(role_ids)
+    end
+    @member.update_attributes(attrs)
     @member
   end
 end
diff --git a/app/models/permitted_params.rb b/app/models/permitted_params.rb
index d9ac6b0e8e..5b084c8f25 100644
--- a/app/models/permitted_params.rb
+++ b/app/models/permitted_params.rb
@@ -279,7 +279,7 @@ class PermittedParams < Struct.new(:params, :user)
           :project_id,
           :role_ids => []]],
       :member => [
-        :role_ids],
+        :role_ids => []],
       :new_work_package => [
         :subject,
         :description,
diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb
index d095e17a93..3d43c3c7ef 100644
--- a/spec/controllers/members_controller_spec.rb
+++ b/spec/controllers/members_controller_spec.rb
@@ -92,8 +92,9 @@ describe MembersController do
         :member => {
           :role_ids => [role_1.id, role_2.id]
         }
-      response.should be_success
-      member.roles.should include(role_1, role_2)
+
+      Member.find(member_2.id).roles.should include(role_1, role_2)
+      response.response_code.should < 400
     end
   end
 

From 19efeed228cb3f8079f3bfca721db50687a18e9a Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Thu, 19 Dec 2013 19:18:30 +0000
Subject: [PATCH 04/19] removed obsolete safety statement from update spec

---
 spec/controllers/members_controller_spec.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb
index 3d43c3c7ef..1b80db4ca8 100644
--- a/spec/controllers/members_controller_spec.rb
+++ b/spec/controllers/members_controller_spec.rb
@@ -57,7 +57,6 @@ describe MembersController do
         :project_id => project.identifier,
         :id => member_2.id,
         :member => {
-          :role_ids => [role_1.id],
           field => value
         }
 

From b4fdd3de0606e79368e199d66747f9762a9457d2 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Thu, 19 Dec 2013 19:30:16 +0000
Subject: [PATCH 05/19] test member creation

---
 spec/controllers/members_controller_spec.rb | 29 +++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb
index 1b80db4ca8..91b722238b 100644
--- a/spec/controllers/members_controller_spec.rb
+++ b/spec/controllers/members_controller_spec.rb
@@ -40,6 +40,35 @@ describe MembersController do
     User.stub(:current).and_return(user)
   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) }

From 01ac1315f9d0a49fd98f8aa2c9157356e4fb1c65 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Thu, 19 Dec 2013 19:37:13 +0000
Subject: [PATCH 06/19] safe mass assignment in member#create

---
 app/controllers/members_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index a61a408057..b5e2d4bafe 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -155,7 +155,7 @@ JS
     roles = Role.find_all_by_id(possibly_seperated_ids_for_entity(attrs, :role))
 
     user_ids.each do |user_id|
-      member = Member.new attrs
+      member = Member.new permitted_params.member.reject { |k, v| k == :role_ids }
       # 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

From f7355c1820636750b855b1d7187bf8b61f7635df Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Fri, 20 Dec 2013 00:39:22 +0000
Subject: [PATCH 07/19] include ForbiddenAttributeProtection in correct file
 (model instead of controller)

---
 app/controllers/members_controller.rb | 2 --
 app/models/member.rb                  | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index b5e2d4bafe..34fdc1688a 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -28,8 +28,6 @@
 #++
 
 class MembersController < ApplicationController
-  include ActiveModel::ForbiddenAttributesProtection
-
   model_object Member
   before_filter :find_model_object_and_project, :except => [:autocomplete_for_member, :paginate_users]
   before_filter :find_project, :only => [:autocomplete_for_member, :paginate_users]
diff --git a/app/models/member.rb b/app/models/member.rb
index 0e6774c78e..b30dd8b592 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -28,6 +28,8 @@
 #++
 
 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

From 0024e137e19568bf8558290b9ec897d2cbe1211e Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Fri, 20 Dec 2013 10:19:40 +0000
Subject: [PATCH 08/19] covered second case of mass assignment in #create

---
 app/controllers/members_controller.rb | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index 34fdc1688a..143b0c9864 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -148,20 +148,23 @@ JS
 
   def new_members_from_params
     members = []
-    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))
+    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 = permitted_params.member.reject { |k, v| k == :role_ids }
 
-    user_ids.each do |user_id|
-      member = Member.new permitted_params.member.reject { |k, v| k == :role_ids }
+    new_member = lambda 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
+      member.user_id = user_id if user_id
+    end
+
+    user_ids.each do |user_id|
+      members << 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
@@ -184,8 +187,8 @@ 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
       []

From fcbcccd801976da5fd61f548084d288457083e72 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Fri, 20 Dec 2013 12:32:54 +0000
Subject: [PATCH 09/19] actually return member

---
 app/controllers/members_controller.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index 143b0c9864..71fff26ef2 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -157,6 +157,7 @@ JS
       # 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 if user_id
+      member
     end
 
     user_ids.each do |user_id|

From 3e028beaba4788782098f7366e0d4c8aae338eb7 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Mon, 6 Jan 2014 15:16:15 +0100
Subject: [PATCH 10/19] fixed tests (use project id)

---
 test/functional/members_controller_test.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb
index fe9befcc2f..6d50857e4e 100644
--- a/test/functional/members_controller_test.rb
+++ b/test/functional/members_controller_test.rb
@@ -47,7 +47,7 @@ class MembersControllerTest < ActionController::TestCase
 
   def test_create
     assert_difference 'Member.count' do
-      post :create, :id => 1, :member => {:role_ids => [1], :user_id => 7}
+      post :create, :project_id => 1, :member => {:role_ids => [1], :user_id => 7}
    end
     assert_redirected_to '/projects/ecookbook/settings/members'
     assert User.find(7).member_of?(Project.find(1))
@@ -55,7 +55,7 @@ class MembersControllerTest < ActionController::TestCase
 
   def test_create_multiple
     assert_difference 'Member.count', 3 do
-      post :create, :id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
+      post :create, :project_id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
     end
     assert_redirected_to '/projects/ecookbook/settings/members'
     assert User.find(7).member_of?(Project.find(1))
@@ -64,7 +64,7 @@ class MembersControllerTest < ActionController::TestCase
   context "post :create in JS format" do
     context "with successful saves" do
       should "add membership for each user" do
-        post :create, :format => "js", :id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
 
         assert User.find(7).member_of?(Project.find(1))
         assert User.find(8).member_of?(Project.find(1))
@@ -72,7 +72,7 @@ class MembersControllerTest < ActionController::TestCase
       end
 
       should "replace the tab with RJS" do
-        post :create, :format => "js", :id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
 
         assert_select_rjs :replace_html, 'tab-content-members'
       end
@@ -81,13 +81,13 @@ class MembersControllerTest < ActionController::TestCase
 
     context "with a failed save" do
       should "not replace the tab with RJS" do
-        post :create, :format => "js", :id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
 
         assert_select '#tab-content-members', 0
       end
 
       should "show an error message" do
-        post :create, :format => "js", :id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
 
         assert_select_rjs :insert_html, :top do
           assert_select '#errorExplanation'

From 47da73d92b726cb5cd7e63dbeec8f8a501b42b74 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Mon, 6 Jan 2014 19:16:11 +0100
Subject: [PATCH 11/19] Revert "fixed tests (use project id)"

This reverts commit 3e028beaba4788782098f7366e0d4c8aae338eb7.
---
 test/functional/members_controller_test.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb
index 6d50857e4e..fe9befcc2f 100644
--- a/test/functional/members_controller_test.rb
+++ b/test/functional/members_controller_test.rb
@@ -47,7 +47,7 @@ class MembersControllerTest < ActionController::TestCase
 
   def test_create
     assert_difference 'Member.count' do
-      post :create, :project_id => 1, :member => {:role_ids => [1], :user_id => 7}
+      post :create, :id => 1, :member => {:role_ids => [1], :user_id => 7}
    end
     assert_redirected_to '/projects/ecookbook/settings/members'
     assert User.find(7).member_of?(Project.find(1))
@@ -55,7 +55,7 @@ class MembersControllerTest < ActionController::TestCase
 
   def test_create_multiple
     assert_difference 'Member.count', 3 do
-      post :create, :project_id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
+      post :create, :id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
     end
     assert_redirected_to '/projects/ecookbook/settings/members'
     assert User.find(7).member_of?(Project.find(1))
@@ -64,7 +64,7 @@ class MembersControllerTest < ActionController::TestCase
   context "post :create in JS format" do
     context "with successful saves" do
       should "add membership for each user" do
-        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
 
         assert User.find(7).member_of?(Project.find(1))
         assert User.find(8).member_of?(Project.find(1))
@@ -72,7 +72,7 @@ class MembersControllerTest < ActionController::TestCase
       end
 
       should "replace the tab with RJS" do
-        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :id => 1, :member => {:role_ids => [1], :user_ids => [7, 8, 9]}
 
         assert_select_rjs :replace_html, 'tab-content-members'
       end
@@ -81,13 +81,13 @@ class MembersControllerTest < ActionController::TestCase
 
     context "with a failed save" do
       should "not replace the tab with RJS" do
-        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
 
         assert_select '#tab-content-members', 0
       end
 
       should "show an error message" do
-        post :create, :format => "js", :project_id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
+        post :create, :format => "js", :id => 1, :member => {:role_ids => [], :user_ids => [7, 8, 9]}
 
         assert_select_rjs :insert_html, :top do
           assert_select '#errorExplanation'

From d69994de8fbf9a3fd92e53efd95b67637ef366f5 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Tue, 7 Jan 2014 13:02:44 +0100
Subject: [PATCH 12/19] more concise member creation using whitelisted member
 role IDs

---
 app/controllers/members_controller.rb | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index 71fff26ef2..16fb15729d 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -150,14 +150,11 @@ JS
     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 = permitted_params.member.reject { |k, v| k == :role_ids }
 
     new_member = lambda 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 if user_id
-      member
+      Member.new(permitted_params.member).tap do |member|
+        member.user_id = user_id if user_id
+      end
     end
 
     user_ids.each do |user_id|

From e3b892d9710df89aafff5526085ce719bc3d98a6 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Tue, 7 Jan 2014 13:03:29 +0100
Subject: [PATCH 13/19] fixed each_comma_seperated [sic]

---
 app/controllers/members_controller.rb | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index 16fb15729d..4f3347f2d1 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -168,12 +168,13 @@ JS
   end
 
   def each_comma_seperated(array, &block)
-    array.each do |elem|
-      if elem.to_s.match /\d(,\d)*/
-        array += block.call(array.delete(elem))
+    array.map do |e|
+      if e.to_s.match /\d(,\d)*/
+        block.call(e)
+      else
+        e
       end
-    end
-    return array
+    end.flatten
   end
 
   def transform_array_of_comma_seperated_ids(array)

From 0a7531710fff0e9aaf0fb16ca333f67c204f3d38 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Tue, 7 Jan 2014 13:03:45 +0100
Subject: [PATCH 14/19] fixed parens

---
 app/controllers/members_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index 4f3347f2d1..eb202aa393 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -187,7 +187,7 @@ JS
   def possibly_seperated_ids_for_entity(array, entity = :user)
     if !array[:"#{entity}_ids"].nil?
       transform_array_of_comma_seperated_ids(array[:"#{entity}_ids"])
-    elsif !array[:"#{entity}_id"].nil? && (id = array[:"#{entity}_id"].present?)
+    elsif !array[:"#{entity}_id"].nil? && (id = array[:"#{entity}_id"]).present?
       [id]
     else
       []

From b0643c339261e4e35cf6e5f5b3b24abd0e32502e Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Tue, 7 Jan 2014 13:46:56 +0100
Subject: [PATCH 15/19] added changelog for fix #3335

---
 doc/CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/CHANGELOG.md b/doc/CHANGELOG.md
index bf9a329c25..42f60b2cbc 100644
--- a/doc/CHANGELOG.md
+++ b/doc/CHANGELOG.md
@@ -32,6 +32,7 @@ See doc/COPYRIGHT.rdoc for more details.
 * `#1951` Layout for ## and ### textile link help is broken
 * `#2161` [Accessibility] Link form elements to their label - new color
 * `#2500` Change default configuration in new OpenProject application so new projects are not public by default
+* `#3335` Fix: Mass assignment in members controller
 * `#3528` [Data Migration] Type 'none' is not migrated properly in Timelines
 * `#3539` [Work package tracking] Modul view of work packages is too broad
 * Fixed workflow copy view

From f6b4291b46c1da27f15055bb512b83b4da49c374 Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Wed, 12 Feb 2014 16:23:25 +0000
Subject: [PATCH 16/19] use map

---
 app/controllers/members_controller.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index eb202aa393..6fbfe280f7 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -147,7 +147,6 @@ 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))
 
@@ -157,8 +156,8 @@ JS
       end
     end
 
-    user_ids.each do |user_id|
-      members << new_member.call(user_id)
+    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?

From 545714f183d30c737ad361c6e10fa6bf39698eca Mon Sep 17 00:00:00 2001
From: Markus Kahl <machisuji@googlemail.com>
Date: Wed, 12 Feb 2014 16:58:08 +0000
Subject: [PATCH 17/19] spec for user/edit_membership

---
 spec/controllers/users_controller_spec.rb | 25 +++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index 2432ba311c..1125a82cfd 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/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

From ab2ab8ab3ba72844638dac169f1f0bfd250dd12d Mon Sep 17 00:00:00 2001
From: Hagen Schink <h.schink@finn.de>
Date: Thu, 13 Feb 2014 14:06:14 +0100
Subject: [PATCH 18/19] Moves permitted params specs

---
 spec/controllers/members_controller_spec.rb | 28 -----------------
 spec/models/permitted_params_spec.rb        | 34 +++++++++++++++++++++
 2 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb
index 7f8ad9e038..74b60d1147 100644
--- a/spec/controllers/members_controller_spec.rb
+++ b/spec/controllers/members_controller_spec.rb
@@ -82,38 +82,10 @@ describe MembersController do
       :roles => [role_1])
     }
 
-    def dont_update(field, value)
-      put :update,
-        :project_id => project.identifier,
-        :id => member_2.id,
-        :member => {
-          field => value
-        }
-
-      response.should_not be_success
-      Member.find(member_2.id).attributes[field.to_s].should_not == value
-    end
-
     before do
       User.stub(:current).and_return(admin)
     end
 
-    it "should specifically not allow 'user_id' to be mass assigned" do
-      dont_update(:user_id, user.id)
-    end
-
-    it "should specifically not allow 'project_id' to be mass assigned" do
-      dont_update(:project_id, project.id)
-    end
-
-    it "should specifically not allow 'created_on' to be mass assigned" do
-      dont_update(:created_on, Time.zone.at(1111111111))
-    end
-
-    it "should specifically not allow 'mail_notification' to be mass assigned" do
-      dont_update(:mail_notification, !member_2.mail_notification)
-    end
-
     it "should, however, allow roles to be updated through mass assignment" do
       put 'update',
         :project_id => project.identifier,
diff --git a/spec/models/permitted_params_spec.rb b/spec/models/permitted_params_spec.rb
index 8cd6d4d7be..de49e0ec48 100644
--- a/spec/models/permitted_params_spec.rb
+++ b/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

From 5ae7541ec1081201462285996569266db4068f85 Mon Sep 17 00:00:00 2001
From: Hagen Schink <h.schink@finn.de>
Date: Thu, 13 Feb 2014 14:06:35 +0100
Subject: [PATCH 19/19] Uses expect syntax

---
 spec/controllers/members_controller_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/controllers/members_controller_spec.rb b/spec/controllers/members_controller_spec.rb
index 74b60d1147..f8aa9def9d 100644
--- a/spec/controllers/members_controller_spec.rb
+++ b/spec/controllers/members_controller_spec.rb
@@ -95,7 +95,7 @@ describe MembersController do
         }
 
       Member.find(member_2.id).roles.should include(role_1, role_2)
-      response.response_code.should < 400
+      expect(response.response_code).to be < 400
     end
   end