From d20fdf87b6cc618306e92c66e11ef54e7eb1edce Mon Sep 17 00:00:00 2001 From: Drew Ulmer Date: Wed, 10 Apr 2013 10:33:50 -0500 Subject: [PATCH] Introduce BaseSanitizer null sanitizer and controller-specific callbacks This updates Devise's StrongParameter support to feature: - A Null base sanitizer to support existing Rails 3.x installations that don't want to use StrongParameters yet - A new, simpler API for ParameterSanitizer: #permit, #permit!, and #forbid - Overrideable callbacks on a controller-basis, e.g. #create_sessions_params for passing the current scope's parameters through StrongParameters and a helper method, whitelisted_params, for rolling your own implementations of #create_x_params in your own controllers. - Lots of tests! --- .../devise/confirmations_controller.rb | 6 +- .../devise/registrations_controller.rb | 18 ++- app/controllers/devise/unlocks_controller.rb | 6 +- app/controllers/devise_controller.rb | 23 ++- lib/devise.rb | 1 + lib/devise/parameter_sanitizer.rb | 74 ++++++--- test/parameter_sanitizer_test.rb | 148 ++++++++++++------ 7 files changed, 190 insertions(+), 86 deletions(-) diff --git a/app/controllers/devise/confirmations_controller.rb b/app/controllers/devise/confirmations_controller.rb index 58802882..4da37d94 100644 --- a/app/controllers/devise/confirmations_controller.rb +++ b/app/controllers/devise/confirmations_controller.rb @@ -6,7 +6,7 @@ class Devise::ConfirmationsController < DeviseController # POST /resource/confirmation def create - self.resource = resource_class.send_confirmation_instructions(resource_params) + self.resource = resource_class.send_confirmation_instructions(create_confirmation_params) if successfully_sent?(resource) respond_with({}, :location => after_resending_confirmation_instructions_path_for(resource_name)) @@ -39,4 +39,8 @@ class Devise::ConfirmationsController < DeviseController def after_confirmation_path_for(resource_name, resource) after_sign_in_path_for(resource) end + + def create_confirmation_params + whitelisted_params(:confirmations) + end end diff --git a/app/controllers/devise/registrations_controller.rb b/app/controllers/devise/registrations_controller.rb index c7cee32c..4558e402 100644 --- a/app/controllers/devise/registrations_controller.rb +++ b/app/controllers/devise/registrations_controller.rb @@ -10,7 +10,7 @@ class Devise::RegistrationsController < DeviseController # POST /resource def create - build_resource + build_resource(create_registration_params) if resource.save if resource.active_for_authentication? @@ -40,7 +40,7 @@ class Devise::RegistrationsController < DeviseController self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key) prev_unconfirmed_email = resource.unconfirmed_email if resource.respond_to?(:unconfirmed_email) - if resource.update_with_password(resource_params) + if resource.update_with_password(update_resource_params) if is_navigational_format? flash_key = update_needs_confirmation?(resource, prev_unconfirmed_email) ? :update_needs_confirmation : :updated @@ -83,11 +83,7 @@ class Devise::RegistrationsController < DeviseController # Build a devise resource passing in the session. Useful to move # temporary session data to the newly created user. def build_resource(hash=nil) - if request.get? - hash ||= {} - else - hash ||= resource_params || {} - end + hash ||= {} self.resource = resource_class.new_with_session(hash, session) end @@ -120,4 +116,12 @@ class Devise::RegistrationsController < DeviseController send(:"authenticate_#{resource_name}!", :force => true) self.resource = send(:"current_#{resource_name}") end + + def create_registration_params + whitelisted_params(:registrations) + end + + def update_resource_params + whitelisted_params(:registrations) + end end diff --git a/app/controllers/devise/unlocks_controller.rb b/app/controllers/devise/unlocks_controller.rb index 3b0d9f7f..6566fc09 100644 --- a/app/controllers/devise/unlocks_controller.rb +++ b/app/controllers/devise/unlocks_controller.rb @@ -8,7 +8,7 @@ class Devise::UnlocksController < DeviseController # POST /resource/unlock def create - self.resource = resource_class.send_unlock_instructions(resource_params) + self.resource = resource_class.send_unlock_instructions(create_unlock_params) if successfully_sent?(resource) respond_with({}, :location => after_sending_unlock_instructions_path_for(resource)) @@ -40,4 +40,8 @@ class Devise::UnlocksController < DeviseController def after_unlock_path_for(resource) new_session_path(resource) end + + def create_unlock_params + whitelisted_params(:unlocks) + end end diff --git a/app/controllers/devise_controller.rb b/app/controllers/devise_controller.rb index 199bc48d..9099d6ae 100644 --- a/app/controllers/devise_controller.rb +++ b/app/controllers/devise_controller.rb @@ -95,7 +95,7 @@ MESSAGE # When building a resource, invoke strong_parameters require/permit # steps if the params hash includes the resource name. if params[resource_name] - hash ||= resource_params || {} + hash ||= whitelisted_params(controller_name) || {} else hash ||= {} end @@ -187,17 +187,26 @@ MESSAGE # Setup a param sanitizer to filter parameters using strong_parameters. See # lib/devise/controllers/parameter_sanitizer.rb for more info. Override this # method in your application controller to use your own parameter sanitizer. - def devise_parameters_sanitizer - @devise_parameters_sanitizer ||= Devise::ParameterSanitizer.new + def devise_parameter_sanitizer + return super if defined?(super) + @devise_parameter_sanitizer ||= if defined?(ActionController::StrongParameters) + Devise::ParameterSanitizer.new(resource_name, params) + else + Devise::BaseSanitizer.new(resource_name, params) + end end # Return the params to be used for mass assignment passed through the # strong_parameters require/permit step. To customize the parameters # permitted for a specific controller, simply prepend a before_filter and - # call #permit_devise_param or #remove_permitted_devise_param on - # parameters_sanitizer to update the default allowed lists of permitted - # parameters. + # call #permit, #permit! or #forbid on devise_parameters_sanitizer to update + # the default allowed lists of permitted parameters for a specific + # controller/action combination. + def whitelisted_params(contr_name) + devise_parameter_sanitizer.sanitize_for(contr_name) + end + def resource_params - params.require(resource_name).permit(devise_parameters_sanitizer.permitted_params_for(controller_name)) + params.fetch(resource_name, {}) end end diff --git a/lib/devise.rb b/lib/devise.rb index 75001585..4e103436 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -10,6 +10,7 @@ module Devise autoload :FailureApp, 'devise/failure_app' autoload :OmniAuth, 'devise/omniauth' autoload :ParamFilter, 'devise/param_filter' + autoload :BaseSanitizer, 'devise/parameter_sanitizer' autoload :ParameterSanitizer, 'devise/parameter_sanitizer' autoload :TestHelpers, 'devise/test_helpers' autoload :TimeInflector, 'devise/time_inflector' diff --git a/lib/devise/parameter_sanitizer.rb b/lib/devise/parameter_sanitizer.rb index ce800e2d..e4638bc8 100644 --- a/lib/devise/parameter_sanitizer.rb +++ b/lib/devise/parameter_sanitizer.rb @@ -1,23 +1,42 @@ module Devise - class ParameterSanitizer - attr_reader :allowed_params + class BaseSanitizer + attr_reader :params, :resource_name, :allowed_params - # Return a list of parameter names permitted to be mass-assigned for the - # passed controller. - def permitted_params_for(controller_name) - allowed_params.fetch(key_for_controller_name(controller_name), []) + def initialize(resource_name, params) + @resource_name, @params = resource_name, params + @allowed_params = {} + end + + def default_params + params.fetch(resource_name, {}) + end + + def sanitize_for(controller) + default_params + end + end + + class ParameterSanitizer < BaseSanitizer + # Return the allowed parameters passed through the StrongParametesr + # require/permit step according to the allowed_params setup via + # #permit, #permit!, #forbid, and any defaults. + def sanitize_for(controller) + permitted_params = allowed_params.fetch(param_key(controller), []).to_a + + params.require(resource_name).permit(permitted_params) end # Set up a new parameter sanitizer with a set of allowed parameters. This # gets initialized on each request so that parameters may be augmented or # changed as needed via before_filter. - def initialize + def initialize(resource_name, params) + super @allowed_params = { - :confirmations_controller => [:email], - :passwords_controller => authentication_keys + [:password, :password_confirmation, :reset_password_token], - :registrations_controller => authentication_keys + [:password, :password_confirmation, :current_password], - :sessions_controller => authentication_keys + [:password], - :unlocks_controller => [:email] + :confirmations => [:email], + :passwords => auth_keys | [:password, :password_confirmation, :reset_password_token], + :registrations => auth_keys | [:password, :password_confirmation, :current_password], + :sessions => auth_keys | [:password], + :unlocks => [:email] } end @@ -27,38 +46,41 @@ module Devise # that when adding a new controller, use the full controller name # (:confirmations_controller) and not the short names # (:confirmation/:confirmations). - def permit_devise_param(controller_name, param_name) - @allowed_params[key_for_controller_name(controller_name)] << param_name + def permit(controller_name, *param_names) + @allowed_params[param_key(controller_name)] |= param_names + true + end + + def permit!(controller_name, *param_names) + @allowed_params[param_key(controller_name)] = param_names true end # Remove specific allowed parameter for a Devise controller. If the # controller_name doesn't exist in allowed_params, it will be added to it # as an empty array. - def remove_permitted_devise_param(controller_name, param_name) - @allowed_params[key_for_controller_name(controller_name)].delete(param_name) + def forbid(controller_name, *param_names) + @allowed_params[param_key(controller_name)] -= param_names true end protected - def authentication_keys + def auth_keys Array(::Devise.authentication_keys) end # Flexibly allow access to permitting/denying/checking parameters by # controller name in the following key formats: :confirmations_controller, # :confirmations, :confirmation - def key_for_controller_name(name) - if allowed_params.has_key?(name.to_sym) - name.to_sym - elsif allowed_params.has_key?(:"#{name}s_controller") - :"#{name}s_controller" - elsif allowed_params.has_key?(:"#{name}_controller") - :"#{name}_controller" + def param_key(controller_name) + k = controller_name.to_sym + + if allowed_params.has_key?(k) + k else - @allowed_params[name.to_sym] = [] - name.to_sym + @allowed_params[k] = [] + k end end end diff --git a/test/parameter_sanitizer_test.rb b/test/parameter_sanitizer_test.rb index 56c86193..e8f9fc84 100644 --- a/test/parameter_sanitizer_test.rb +++ b/test/parameter_sanitizer_test.rb @@ -1,52 +1,112 @@ require 'test_helper' +require 'devise/parameter_sanitizer' -class ParameterSanitizerTest < ActiveSupport::TestCase +class BaseSanitizerTest < ActiveSupport::TestCase def sanitizer - Devise::ParameterSanitizer.new + @sanitizer ||= Devise::BaseSanitizer.new(:user, {}) end - test '#permitted_params_for allows querying of allowed parameters by controller' do - assert_equal [:email], sanitizer.permitted_params_for(:confirmations_controller) - assert_equal [:email, :password, :password_confirmation, :reset_password_token], sanitizer.permitted_params_for(:password) - assert_equal [:email], sanitizer.permitted_params_for(:unlocks) - end - - test '#permitted_params_for returns an empty array for a bad key' do - assert_equal [], sanitizer.permitted_params_for(:bad_key) - end - - test '#permit_devise_param allows adding an allowed param for a specific controller' do - subject = sanitizer - - subject.permit_devise_param(:confirmations_controller, :other) - - assert_equal [:email, :other], subject.permitted_params_for(:confirmations_controller) - end - - test '#remove_permitted_devise_param allows disallowing a param for a specific controller' do - subject = sanitizer - - subject.remove_permitted_devise_param(:confirmations_controller, :email) - - assert_equal [], subject.permitted_params_for(:confirmations_controller) - end - - test '#permit_devise_param allows adding additional devise controllers' do - subject = sanitizer - - subject.permit_devise_param(:invitations_controller, :email) - - assert_equal [:email], subject.permitted_params_for(:invitations) - end - - test '#remove_permitted_devise_param fails gracefully when removing a missing param' do - subject = sanitizer - - # perform twice, just to be sure it handles it gracefully - subject.remove_permitted_devise_param(:invitations_controller, :email) - subject.remove_permitted_devise_param(:invitations_controller, :email) - - assert_equal [], subject.permitted_params_for(:invitations) + test '#default_params returns the params passed in' do + assert_equal({}, sanitizer.default_params) + end +end + +if defined?(ActionController::StrongParameters) + + require 'active_model/forbidden_attributes_protection' + + class ParameterSanitizerTest < ActiveSupport::TestCase + def sanitizer(p={}) + @sanitizer ||= Devise::ParameterSanitizer.new(:user, p) + end + + test '#permit allows adding an allowed param for a specific controller' do + sanitizer.permit(:confirmations, :other) + + assert_equal [:email, :other], sanitizer.allowed_params[:confirmations] + end + + test '#permit allows adding multiple allowed params for a specific controller' do + sanitizer.permit(:confirmations, :other, :testing) + + assert_equal [:email, :other, :testing], sanitizer.allowed_params[:confirmations] + end + + test '#permit! overrides allowed params for a specific controller' do + sanitizer.permit!(:confirmations, :other, :testing) + + assert_equal [:other, :testing], sanitizer.allowed_params[:confirmations] + end + + test '#forbid allows disallowing a param for a specific controller' do + sanitizer.forbid(:confirmations, :email) + + assert_equal [], sanitizer.allowed_params[:confirmations] + end + + test '#forbid allows disallowing multiple params for a specific controller' do + sanitizer.forbid(:sessions, :email, :password) + + assert_equal [], sanitizer.allowed_params[:sessions] + end + + test '#permit allows adding additional devise controllers' do + sanitizer.permit(:invitations, :email) + + assert_equal [:email], sanitizer.allowed_params[:invitations] + end + + test '#permit allows adding additional devise controllers with multiple params' do + sanitizer.permit(:invitations, :email, :pin) + + assert_includes sanitizer.allowed_params[:invitations], :pin + assert_includes sanitizer.allowed_params[:invitations], :email + end + + test '#forbid fails gracefully when removing a missing param' do + # perform twice, just to be sure it handles it gracefully + sanitizer.forbid(:invitations, :email) + sanitizer.forbid(:invitations, :email) + + assert_equal [], sanitizer.allowed_params[:invitations] + end + + test '#forbid fails gracefully when removing multiple missing params' do + # perform twice, just to be sure it handles it gracefully + sanitizer.forbid(:invitations, :email, :badkey) + sanitizer.forbid(:invitations, :email, :badkey) + + assert_equal [], sanitizer.allowed_params[:invitations] + end + + test '#sanitize_for tries to require the resource name on params' do + params = ActionController::Parameters.new({:admin => {}}) + + assert_raises ActionController::ParameterMissing do + sanitizer(params).sanitize_for(:sessions) + end + end + + test '#sanitize_for performs the permit step of strong_parameters, restricting passed attributes' do + params = ActionController::Parameters.new({:user => {:admin => true}}) + + # removes the admin flag + assert_equal({}, sanitizer(params).sanitize_for(:sessions)) + end + + test '#sanitize_for respects any updates to allowed_params' do + params = ActionController::Parameters.new({:user => {:admin => true}}) + sanitizer(params).permit(:sessions, :admin) + + assert_equal({'admin' => true}, sanitizer(params).sanitize_for(:sessions)) + end + + test '#sanitize_for works with newly added controllers' do + params = ActionController::Parameters.new({:user => {:email => 'abc@example.com', :pin => '1234'}}) + sanitizer(params).permit(:invitations, :email, :pin) + + assert_equal({'email' => 'abc@example.com', 'pin' => '1234'}, sanitizer(params).sanitize_for(:invitations)) + end end end