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 | 93 +++++++++---------- test/parameter_sanitizer_test.rb | 61 +++++------- 7 files changed, 75 insertions(+), 108 deletions(-) diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index 37dd1c2e..f455a90a 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 52135f22..f51cee4f 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 9db7e726..7d1e7347 100644 --- a/app/controllers/devise/registrations_controller.rb +++ b/app/controllers/devise/registrations_controller.rb @@ -117,10 +117,10 @@ class Devise::RegistrationsController < DeviseController 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 9c355f38..a9b3922e 100644 --- a/app/controllers/devise/sessions_controller.rb +++ b/app/controllers/devise/sessions_controller.rb @@ -35,7 +35,7 @@ class Devise::SessionsController < DeviseController 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 a187fddf..a53469a5 100644 --- a/lib/devise/controllers/helpers.rb +++ b/lib/devise/controllers/helpers.rb @@ -91,10 +91,6 @@ module Devise 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 69f6c9c8..939b2c74 100644 --- a/lib/devise/parameter_sanitizer.rb +++ b/lib/devise/parameter_sanitizer.rb @@ -13,14 +13,23 @@ module Devise 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 @@ module Devise end class ParameterSanitizer < BaseSanitizer - - 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 initialize(*) + super + @permitted = Hash.new { |h,k| h[k] = attributes_for(k) } end - def permitted_parameters - @permitted_parameters ||= PermittedParameters.new(@resource_class) + def sign_in + default_params.permit self.for(:sign_in) + end + + def sign_up + default_params.permit self.for(:sign_up) + end + + 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 061f8f7b..a2dc9a25 100644 --- a/test/parameter_sanitizer_test.rb +++ b/test/parameter_sanitizer_test.rb @@ -2,13 +2,21 @@ require 'test_helper' 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 @@ if defined?(ActionController::StrongParameters) 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