Use strong_parameters for User, remove safe_attribtues

app/controllers/account_controller.rb

@@ -94,7 +94,7 @@ class AccountController < ApplicationController
       @user = User.new(:language => Setting.default_language)
     else
       @user = User.new
-      @user.safe_attributes = params[:user]
+      @user.attributes = permitted_params.user
       @user.admin = false
       @user.register
       if session[:auth_source_registration]

app/controllers/api/v1/users_controller.rb

@@ -80,7 +80,7 @@ module Api
 
       def create
         @user = User.new(:language => Setting.default_language, :mail_notification => Setting.default_notification_option)
-        @user.safe_attributes = params[:user]
+        @user.attributes = permitted_params.user_create_as_admin
         @user.admin = params[:user][:admin] || false
         @user.login = params[:user][:login]
         @user.password, @user.password_confirmation = params[:user][:password], params[:user][:password_confirmation] if @user.change_password_allowed?
@@ -112,7 +112,7 @@ module Api
       def update
         @user.admin = params[:user][:admin] if params[:user][:admin]
         @user.login = params[:user][:login] if params[:user][:login]
-        @user.safe_attributes = params[:user].except(:login) # :login is protected
+        @user.attributes = permitted_params.user_update_as_admin
         if params[:user][:password].present? && @user.change_password_allowed?
           @user.password, @user.password_confirmation = params[:user][:password], params[:user][:password_confirmation]
         end

app/controllers/my_controller.rb

@@ -69,7 +69,7 @@ class MyController < ApplicationController
     @user = User.current
     @pref = @user.pref
     if request.put?
-      @user.safe_attributes = params[:user]
+      @user.attributes = permitted_params.user
       @user.pref.attributes = params[:pref]
       @user.pref[:no_self_notified] = (params[:no_self_notified] == '1')
       if @user.save

app/controllers/users_controller.rb

@@ -115,7 +115,7 @@ class UsersController < ApplicationController
   verify :method => :post, :only => :create, :render => {:nothing => true, :status => :method_not_allowed }
   def create
     @user = User.new(:language => Setting.default_language, :mail_notification => Setting.default_notification_option)
-    @user.safe_attributes = params[:user]
+    @user.attributes = permitted_params.user_create_as_admin
     @user.admin = params[:user][:admin] || false
     @user.login = params[:user][:login]
     if @user.change_password_allowed?

app/models/permitted_params.rb

@@ -27,7 +27,7 @@
 # See doc/COPYRIGHT.rdoc for more details.
 #++
 
-class PermittedParams < Struct.new(:params, :user)
+class PermittedParams < Struct.new(:params, :current_user)
 
   # This class intends to provide a method for all params hashes coming from the
   # client and that are used for mass assignment.
@@ -184,18 +184,37 @@ class PermittedParams < Struct.new(:params, :user)
 
   alias :update_work_package :new_work_package
 
+  def user
+    permitted_params = params.require(:user).permit(*self.class.permitted_attributes[:user])
+    permitted_params.merge!(custom_field_values(:user))
+  end
+
   def user_update_as_admin
-    if user.admin?
-      permitted_params = params.require(:user).permit(:firstname,
-                                                      :lastname,
-                                                      :mail,
-                                                      :mail_notification,
-                                                      :language,
-                                                      :custom_fields,
-                                                      :identity_url,
-                                                      :auth_source_id,
-                                                      :force_password_change,
-                                                      :group_ids => [])
+    if current_user.admin?
+      allowed_params = self.class.permitted_attributes[:user] + \
+                       [ :auth_source_id,
+                         :force_password_change,
+                         # Found these in safe_attributes and added them here as I
+                         # didn't know the consequences of removing these.
+                         # They were not allowed on update.
+                         :group_ids => []]
+
+      permitted_params = params.require(:user).permit(*allowed_params)
+      permitted_params.merge!(custom_field_values(:user))
+
+      permitted_params
+    else
+      params.require(:user).permit()
+    end
+  end
+
+  def user_create_as_admin
+    if current_user.admin?
+      allowed_params = self.class.permitted_attributes[:user] + \
+                       [ :auth_source_id,
+                         :force_password_change]
+
+      permitted_params = params.require(:user).permit(*allowed_params)
       permitted_params.merge!(custom_field_values(:user))
 
       permitted_params
@@ -255,7 +274,7 @@ class PermittedParams < Struct.new(:params, :user)
   end
 
   def permitted_attributes(key, additions = {})
-    merged_args = { :params => params, :user => user }.merge(additions)
+    merged_args = { :params => params, :current_user => current_user }.merge(additions)
 
     self.class.permitted_attributes[key].map do |permission|
       if permission.respond_to?(:call)
@@ -346,7 +365,7 @@ class PermittedParams < Struct.new(:params, :user)
           # avoid costly allowed_to? if the param is not there at all
           if args[:params]["work_package"] &&
              args[:params]["work_package"].has_key?("watcher_user_ids") &&
-             args[:user].allowed_to?(:add_work_package_watchers, args[:project])
+             args[:current_user].allowed_to?(:add_work_package_watchers, args[:project])
 
             { :watcher_user_ids => [] }
           end
@@ -355,7 +374,7 @@ class PermittedParams < Struct.new(:params, :user)
           # avoid costly allowed_to? if the param is not there at all
           if args[:params]["work_package"] &&
              args[:params]["work_package"].has_key?("time_entry") &&
-             args[:user].allowed_to?(:log_time, args[:project])
+             args[:current_user].allowed_to?(:log_time, args[:project])
 
             { time_entry: [:hours, :activity_id, :comments] }
           end
@@ -384,7 +403,7 @@ class PermittedParams < Struct.new(:params, :user)
           # avoid costly allowed_to? if the param is not there at all
           if args[:params]["planning_element"] &&
              args[:params]["planning_element"].has_key?("watcher_user_ids") &&
-             args[:user].allowed_to?(:add_work_package_watchers, args[:project])
+             args[:current_user].allowed_to?(:add_work_package_watchers, args[:project])
 
             { :watcher_user_ids => [] }
           end
@@ -393,7 +412,7 @@ class PermittedParams < Struct.new(:params, :user)
           # avoid costly allowed_to? if the param is not there at all
           if args[:params]["planning_element"] &&
              args[:params]["planning_element"].has_key?("time_entry") &&
-             args[:user].allowed_to?(:log_time, args[:project])
+             args[:current_user].allowed_to?(:log_time, args[:project])
 
             { time_entry: [:hours, :activity_id, :comments] }
           end
@@ -443,6 +462,14 @@ class PermittedParams < Struct.new(:params, :user)
         :color_id,
         :project_ids => [],
         :custom_field_ids => [] ],
+      :user => [
+        :firstname,
+        :lastname,
+        :mail,
+        :mail_notification,
+        :language,
+        :custom_fields,
+        :identity_url ],
       :wiki_page => [
         :title,
         :parent_id,

app/models/user.rb

@@ -30,7 +30,7 @@
 require "digest/sha1"
 
 class User < Principal
-  include Redmine::SafeAttributes
+  include ActiveModel::ForbiddenAttributesProtection
 
   # Account statuses
   # Code accessing the keys assumes they are ordered, which they are since Ruby 1.9
@@ -655,18 +655,6 @@ class User < Principal
     end
   end
 
-  # These are also implemented as strong_parameters, so also see
-  # app/models/permitted_params.rb
-  # Delete these if everything in the UsersController uses strong_parameters.
-  safe_attributes 'firstname', 'lastname', 'mail', 'mail_notification', 'language',
-                  'custom_field_values', 'custom_fields', 'identity_url'
-
-  safe_attributes 'auth_source_id', 'force_password_change',
-    :if => lambda {|user, current_user| current_user.admin?}
-
-  safe_attributes 'group_ids',
-    :if => lambda {|user, current_user| current_user.admin? && !user.new_record?}
-
   # Utility method to help check if a user should be notified about an
   # event.
   def notify_about?(object)

spec/models/permitted_params_spec.rb

@@ -525,45 +525,109 @@ describe PermittedParams do
                          'auth_source_id',
                          'force_password_change']
 
-    it 'should permit nothing for a non-admin user' do
-      # Hash with {'key' => 'key'} for all admin_permissions
-      field_sample = { :user => Hash[admin_permissions.zip(admin_permissions)] }
+    [:user_update_as_admin, :user_create_as_admin].each do |method|
+      describe method do
 
-      params = ActionController::Parameters.new(field_sample)
-      PermittedParams.new(params, user).user_update_as_admin.should == {}
+        it 'should permit nothing for a non-admin user' do
+          # Hash with {'key' => 'key'} for all admin_permissions
+          field_sample = { :user => Hash[admin_permissions.zip(admin_permissions)] }
+
+          params = ActionController::Parameters.new(field_sample)
+          PermittedParams.new(params, user).method(method).call.should == {}
+        end
+
+        admin_permissions.each do |field|
+          it "should permit #{field}" do
+            hash = { field => 'test' }
+            params = ActionController::Parameters.new(:user => hash)
+
+            PermittedParams.new(params, admin).method(method).call.should ==
+              { field => 'test' }
+          end
+        end
+
+        it "should permit custom field values" do
+          hash = { "custom_field_values" => { "1" => "5" } }
+
+          params = ActionController::Parameters.new(:user => hash)
+
+          PermittedParams.new(params, admin).method(method).call.should == hash
+        end
+
+        it "should remove custom field values that do not follow the schema 'id as string' => 'value as string'" do
+          hash = { "custom_field_values" => { "blubs" => "5", "5" => {"1" => "2"} } }
+
+          params = ActionController::Parameters.new(:user => hash)
+
+          PermittedParams.new(params, admin).method(method).call.should == {}
+        end
+
+      end
     end
 
-    admin_permissions.each do |field|
-      it "should permit #{field}" do
-        hash = { field => 'test' }
+    describe :user_update_as_admin do
+      it 'should permit a group_ids list' do
+        hash = { 'group_ids' => ['1', '2'] }
         params = ActionController::Parameters.new(:user => hash)
 
-        PermittedParams.new(params, admin).user_update_as_admin.should ==
-          { field => 'test' }
+        PermittedParams.new(params, admin).user_update_as_admin.should == hash
       end
     end
 
-    it 'should permit a group_ids list' do
-      hash = { 'group_ids' => ['1', '2'] }
-      params = ActionController::Parameters.new(:user => hash)
+    describe :user_create_as_admin do
+      it 'should not permit a group_ids list' do
+        hash = { 'group_ids' => ['1', '2'] }
+        params = ActionController::Parameters.new(:user => hash)
 
-      PermittedParams.new(params, admin).user_update_as_admin.should == hash
+        PermittedParams.new(params, admin).user_create_as_admin.should == {}
+      end
     end
 
-    it "should permit custom field values" do
-      hash = { "custom_field_values" => { "1" => "5" } }
+    user_permissions = [
+      'firstname',
+      'lastname',
+      'mail',
+      'mail_notification',
+      'language',
+      'custom_fields',
+      'identity_url'
+    ]
+
+    describe :user do
+      user_permissions.each do |field|
+        it "should permit #{field}" do
+          hash = { field => 'test' }
+          params = ActionController::Parameters.new(:user => hash)
+
+          PermittedParams.new(params, admin).user.should ==
+            { field => 'test' }
+        end
+      end
 
-      params = ActionController::Parameters.new(:user => hash)
+      (admin_permissions - user_permissions).each do |field|
+          it "should not permit #{field} (admin-only)" do
+            hash = { field => 'test' }
+            params = ActionController::Parameters.new(:user => hash)
 
-      PermittedParams.new(params, admin).user_update_as_admin.should == hash
-    end
+            PermittedParams.new(params, admin).user.should == {}
+          end
+        end
 
-    it "should remove custom field values that do not follow the schema 'id as string' => 'value as string'" do
-      hash = { "custom_field_values" => { "blubs" => "5", "5" => {"1" => "2"} } }
+      it "should permit custom field values" do
+        hash = { "custom_field_values" => { "1" => "5" } }
 
-      params = ActionController::Parameters.new(:user => hash)
+        params = ActionController::Parameters.new(:user => hash)
 
-      PermittedParams.new(params, admin).user_update_as_admin.should == {}
+        PermittedParams.new(params, admin).user.should == hash
+      end
+
+      it "should remove custom field values that do not follow the schema 'id as string' => 'value as string'" do
+        hash = { "custom_field_values" => { "blubs" => "5", "5" => {"1" => "2"} } }
+
+        params = ActionController::Parameters.new(:user => hash)
+
+        PermittedParams.new(params, admin).user.should == {}
+      end
     end
   end