From 4e318b5167fbe16cb4f583a6e4d2bb52e925bc27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sun, 11 Aug 2013 22:18:29 +0200 Subject: [PATCH] Simplify parameter sanitization proposal --- CHANGELOG.rdoc | 5 ++ README.md | 14 +-- .../devise/registrations_controller.rb | 4 +- app/controllers/devise/sessions_controller.rb | 2 +- lib/devise/controllers/helpers.rb | 4 - lib/devise/parameter_sanitizer.rb | 89 +++++++++---------- test/parameter_sanitizer_test.rb | 61 +++++-------- 7 files changed, 73 insertions(+), 106 deletions(-) diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index 37dd1c2e3b..f455a90a9b 100644 --- a/CHANGELOG.rdoc +++ b/CHANGELOG.rdoc @@ -3,10 +3,15 @@ * backwards incompatible changes * Do not store confirmation, unlock and reset password tokens directly in the database. This means tokens previously stored in the database are no longer valid. You can reenable this temporarily by setting `config.allow_insecure_tokens_lookup = true` in your configuration file. It is recommended to keep this configuration set to true just temporarily in your production servers only to aid migration * The Devise mailer and its views were changed to explicitly receive a token as argument. You will need to update your mailers and re-copy the views to your application with `rails g devise:views` + * Sanitization of parameters should be done by calling `devise_parameter_sanitizier.sanitize(:action)` instead of `devise_parameter_sanitizier.for(:action)` * deprecations * Token authentication is deprecated +* enhancements + * Better security defaults + * Allow easier customization of parameter sanitizer + * bug fix * Do not sign in after confirmation * Do not store confirmation, unlock and reset password tokens directly in the database diff --git a/README.md b/README.md index 52135f2200..f51cee4f07 100644 --- a/README.md +++ b/README.md @@ -196,23 +196,15 @@ class ApplicationController < ActionController::Base protected def configure_permitted_parameters - # permit parameters for all actions - devise_permitted_parameters.add(:username, :age) - - # permit a parameter for a single action - devise_permitted_parameters.for(:sign_up) << :hometown + devise_parameter_sanitizer.for(:sign_up) << :username end end ``` -To remove or overwrite the defaults that Devise provides: +To completely change Devise defaults or invoke custom behaviour, you can also pass a block: ```ruby def configure_permitted_parameters - # remove a permitted parameter - devise_permitted_parameters.remove(:email) - - # overwrite the Devise defaults devise_parameter_sanitizer.for(:sign_in) { |u| u.permit(:username, :email) } end ``` @@ -267,7 +259,7 @@ rails generate devise:views users If the customization at the views level is not enough, you can customize each controller by following these steps: -1. Create your custom controller, for example a `Admins::SessionsController`: +1. Create your custom controller, for example a `Admins::SessionsController`: ```ruby class Admins::SessionsController < Devise::SessionsController diff --git a/app/controllers/devise/registrations_controller.rb b/app/controllers/devise/registrations_controller.rb index 9db7e72635..7d1e734796 100644 --- a/app/controllers/devise/registrations_controller.rb +++ b/app/controllers/devise/registrations_controller.rb @@ -117,10 +117,10 @@ def authenticate_scope! end def sign_up_params - devise_parameter_sanitizer.for(:sign_up) + devise_parameter_sanitizer.sanitize(:sign_up) end def account_update_params - devise_parameter_sanitizer.for(:account_update) + devise_parameter_sanitizer.sanitize(:account_update) end end diff --git a/app/controllers/devise/sessions_controller.rb b/app/controllers/devise/sessions_controller.rb index 9c355f389c..a9b3922e83 100644 --- a/app/controllers/devise/sessions_controller.rb +++ b/app/controllers/devise/sessions_controller.rb @@ -35,7 +35,7 @@ def destroy protected def sign_in_params - devise_parameter_sanitizer.for(:sign_in) + devise_parameter_sanitizer.sanitize(:sign_in) end def serialize_options(resource) diff --git a/lib/devise/controllers/helpers.rb b/lib/devise/controllers/helpers.rb index a187fddfa4..a53469a544 100644 --- a/lib/devise/controllers/helpers.rb +++ b/lib/devise/controllers/helpers.rb @@ -91,10 +91,6 @@ def devise_parameter_sanitizer end end - def devise_permitted_parameters - devise_parameter_sanitizer.permitted_parameters - end - # Tell warden that params authentication is allowed for that specific page. def allow_params_authentication! request.env["devise.allow_params_authentication"] = true diff --git a/lib/devise/parameter_sanitizer.rb b/lib/devise/parameter_sanitizer.rb index 69f6c9c83a..939b2c747d 100644 --- a/lib/devise/parameter_sanitizer.rb +++ b/lib/devise/parameter_sanitizer.rb @@ -13,14 +13,23 @@ def for(kind, &block) if block_given? @blocks[kind] = block else - block = @blocks[kind] - block ? block.call(default_params) : fallback_for(kind) + default_for(kind) + end + end + + def sanitize(kind) + if block = @blocks[kind] + block.call(default_params) + elsif respond_to?(kind, true) + send(kind) + else + raise NotImplementedError, "Devise doesn't know how to sanitize parameters for #{kind}" end end private - def fallback_for(kind) + def default_for(kind) default_params end @@ -30,61 +39,45 @@ def default_params end class ParameterSanitizer < BaseSanitizer + def initialize(*) + super + @permitted = Hash.new { |h,k| h[k] = attributes_for(k) } + end - class PermittedParameters - - def initialize(resource_class) - @resource_class = resource_class - @for = { :sign_in => sign_in, :sign_up => sign_up, :account_update => account_update } - end - - def sign_in - auth_keys + [:password, :remember_me] - end - - def sign_up - auth_keys + [:password, :password_confirmation] - end - - def account_update - auth_keys + [:password, :password_confirmation, :current_password] - end - - def auth_keys - @resource_class.authentication_keys.respond_to?(:keys) ? @resource_class.authentication_keys.keys : @resource_class.authentication_keys - end - - def for(kind) - @for[kind] - end - - def add(*params) - @for.each { |action, permitted| permitted.push *params } - end - - def remove(*params) - @for.each do |action, permitted| - permitted.delete_if { |param| params.include? param } - end - end + def sign_in + default_params.permit self.for(:sign_in) + end + def sign_up + default_params.permit self.for(:sign_up) end - def permitted_parameters - @permitted_parameters ||= PermittedParameters.new(@resource_class) + def account_update + default_params.permit self.for(:account_update) end private - def fallback_for(kind) - if respond_to?(kind, true) - send(kind) - elsif (permitted = permitted_parameters.for(kind)) - default_params.permit permitted - else - raise NotImplementedError, "Devise Parameter Sanitizer doesn't know how to sanitize parameters for #{kind}" + # Change for(kind) to return the values in the @permitted + # hash, allowing the developer to customize at runtime. + def default_for(kind) + @permitted[kind] || raise("No sanitizer provided for #{kind}") + end + + def attributes_for(kind) + case kind + when :sign_in + auth_keys + [:password, :remember_me] + when :sign_up + auth_keys + [:password, :password_confirmation] + when :account_update + auth_keys + [:password, :password_confirmation, :current_password] end end + def auth_keys + @auth_keys ||= @resource_class.authentication_keys.respond_to?(:keys) ? + @resource_class.authentication_keys.keys : @resource_class.authentication_keys + end end end diff --git a/test/parameter_sanitizer_test.rb b/test/parameter_sanitizer_test.rb index 061f8f7bc1..a2dc9a25a2 100644 --- a/test/parameter_sanitizer_test.rb +++ b/test/parameter_sanitizer_test.rb @@ -2,13 +2,21 @@ require 'devise/parameter_sanitizer' class BaseSanitizerTest < ActiveSupport::TestCase - def sanitizer - Devise::BaseSanitizer.new(User, :user, { user: { "email" => "jose" } }) + def sanitizer(params) + params = ActionController::Parameters.new(params) + Devise::BaseSanitizer.new(User, :user, params) end test 'returns chosen params' do + sanitizer = sanitizer(user: { "email" => "jose" }) assert_equal({ "email" => "jose" }, sanitizer.for(:sign_in)) end + + test 'allow custom blocks' do + sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) + sanitizer.for(:sign_in) { |user| user.permit(:email) } + assert_equal({ "email" => "jose" }, sanitizer.sanitize(:sign_in)) + end end if defined?(ActionController::StrongParameters) @@ -22,76 +30,49 @@ def sanitizer(params) test 'filters some parameters on sign in by default' do sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid", "remember_me" => "1" }) - assert_equal({ "email" => "jose", "password" => "invalid", "remember_me" => "1" }, sanitizer.for(:sign_in)) + assert_equal({ "email" => "jose", "password" => "invalid", "remember_me" => "1" }, sanitizer.sanitize(:sign_in)) end test 'handles auth keys as a hash' do swap Devise, :authentication_keys => {:email => true} do sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) - assert_equal({ "email" => "jose", "password" => "invalid" }, sanitizer.for(:sign_in)) + assert_equal({ "email" => "jose", "password" => "invalid" }, sanitizer.sanitize(:sign_in)) end end test 'filters some parameters on sign up by default' do sanitizer = sanitizer(user: { "email" => "jose", "role" => "invalid" }) - assert_equal({ "email" => "jose" }, sanitizer.for(:sign_up)) + assert_equal({ "email" => "jose" }, sanitizer.sanitize(:sign_up)) end test 'filters some parameters on account update by default' do sanitizer = sanitizer(user: { "email" => "jose", "role" => "invalid" }) - assert_equal({ "email" => "jose" }, sanitizer.for(:account_update)) + assert_equal({ "email" => "jose" }, sanitizer.sanitize(:account_update)) end test 'allows custom hooks' do sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) sanitizer.for(:sign_in) { |user| user.permit(:email, :password) } - assert_equal({ "email" => "jose", "password" => "invalid" }, sanitizer.for(:sign_in)) - end - - test 'adding permitted parameters for a single action' do - sanitizer = sanitizer(user: { "email" => "jose", "username" => "jose1" }) - sanitizer.permitted_parameters.for(:sign_up).push(:username) - - assert_equal({ "email" => "jose", "username" => "jose1" }, sanitizer.for(:sign_up)) - assert_equal({ "email" => "jose" }, sanitizer.for(:sign_in)) - end - - test 'adding permitted parameters for all actions' do - sanitizer = sanitizer(user: { "email" => "jose", "username" => "jose1" }) - sanitizer.permitted_parameters.add(:username) - - assert_equal({ "email" => "jose", "username" => "jose1" }, sanitizer.for(:sign_in)) - assert_equal({ "email" => "jose", "username" => "jose1" }, sanitizer.for(:sign_up)) - assert_equal({ "email" => "jose", "username" => "jose1" }, sanitizer.for(:account_update)) - end - - test 'removing default parameters' do - sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) - sanitizer.permitted_parameters.remove(:email) - - assert_equal({ "password" => "invalid" }, sanitizer.for(:sign_in)) - assert_equal({ "password" => "invalid" }, sanitizer.for(:sign_up)) - assert_equal({ "password" => "invalid" }, sanitizer.for(:account_update)) + assert_equal({ "email" => "jose", "password" => "invalid" }, sanitizer.sanitize(:sign_in)) end test 'adding multiple permitted parameters' do sanitizer = sanitizer(user: { "email" => "jose", "username" => "jose1", "role" => "valid" }) - - sanitizer.permitted_parameters.add(:username, :role) - assert_equal({ "email" => "jose", "username" => "jose1", "role" => "valid" }, sanitizer.for(:sign_in)) + sanitizer.for(:sign_in).concat([:username, :role]) + assert_equal({ "email" => "jose", "username" => "jose1", "role" => "valid" }, sanitizer.sanitize(:sign_in)) end test 'removing multiple default parameters' do sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid", "remember_me" => "1" }) - sanitizer.permitted_parameters.remove(:email, :password) - - assert_equal({ "remember_me" => "1" }, sanitizer.for(:sign_in)) + sanitizer.for(:sign_in).delete(:email) + sanitizer.for(:sign_in).delete(:password) + assert_equal({ "remember_me" => "1" }, sanitizer.sanitize(:sign_in)) end test 'raises on unknown hooks' do sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) assert_raise NotImplementedError do - sanitizer.for(:unknown) + sanitizer.sanitize(:unknown) end end end