From 3c324bdc42d43a8ee4177bbd8cbaa31951cf52fa Mon Sep 17 00:00:00 2001 From: Michael Frister Date: Thu, 3 Apr 2014 11:31:54 +0200 Subject: [PATCH] Use strong_parameters for User, remove safe_attribtues --- app/controllers/account_controller.rb | 2 +- app/controllers/api/v1/users_controller.rb | 4 +- app/controllers/my_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- app/models/permitted_params.rb | 61 ++++++++---- app/models/user.rb | 14 +-- spec/models/permitted_params_spec.rb | 110 ++++++++++++++++----- 7 files changed, 137 insertions(+), 58 deletions(-) diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index da5e585bbc..cd6feee8a7 100644 --- a/app/controllers/account_controller.rb +++ b/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] diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index d8d6885423..3ccc2f3828 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/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 diff --git a/app/controllers/my_controller.rb b/app/controllers/my_controller.rb index 8ba529d415..45696f6db7 100644 --- a/app/controllers/my_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 69a90b619e..ea35706e32 100644 --- a/app/controllers/users_controller.rb +++ b/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? diff --git a/app/models/permitted_params.rb b/app/models/permitted_params.rb index 40e2aeb391..77e3aaf7d0 100644 --- a/app/models/permitted_params.rb +++ b/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, diff --git a/app/models/user.rb b/app/models/user.rb index 15341c173e..b0fddb924a 100644 --- a/app/models/user.rb +++ b/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) diff --git a/spec/models/permitted_params_spec.rb b/spec/models/permitted_params_spec.rb index efebf26fdd..ebe6340bea 100644 --- a/spec/models/permitted_params_spec.rb +++ b/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