From df8b48646b0d0bc10d07c7757d8a841e85f77a37 Mon Sep 17 00:00:00 2001 From: Lucas Mazza Date: Tue, 15 Dec 2015 14:56:15 -0200 Subject: [PATCH] Remove `ParametersSanitizer` inheritance. We no longer need to support the `BaseSanitizer` implementation for apps without the Strong Parameters API, and this section is lacking a minimal set of docs to document the expected behavior besides the `README` section. --- README.md | 15 +- lib/devise.rb | 1 - lib/devise/controllers/helpers.rb | 6 +- lib/devise/parameter_sanitizer.rb | 239 ++++++++++++++++++++++-------- test/parameter_sanitizer_test.rb | 174 +++++++++++++--------- 5 files changed, 290 insertions(+), 145 deletions(-) diff --git a/README.md b/README.md index 5ecba844..d877a6e8 100644 --- a/README.md +++ b/README.md @@ -201,7 +201,7 @@ class ApplicationController < ActionController::Base protected def configure_permitted_parameters - devise_parameter_sanitizer.for(:sign_up) << :username + devise_parameter_sanitizer.permit(:sign_up, keys: [:username]) end end ``` @@ -212,7 +212,9 @@ To permit simple scalar values for username and email, use this ```ruby def configure_permitted_parameters - devise_parameter_sanitizer.for(:sign_in) { |u| u.permit(:username, :email) } + devise_parameter_sanitizer.permit(:sign_in) do |user_params| + user_params.permit(:username, :email) + end end ``` @@ -220,7 +222,9 @@ If you have some checkboxes that express the roles a user may take on registrati ```ruby def configure_permitted_parameters - devise_parameter_sanitizer.for(:sign_up) { |u| u.permit({ roles: [] }, :email, :password, :password_confirmation) } + devise_parameter_sanitizer.permit(:sign_up) do |user_params| + user_params.permit({ roles: [] }, :email, :password, :password_confirmation) + end end ``` For the list of permitted scalars, and how to declare permitted keys in nested hashes and arrays, see @@ -231,8 +235,9 @@ If you have multiple Devise models, you may want to set up a different parameter ```ruby class User::ParameterSanitizer < Devise::ParameterSanitizer - def sign_in - default_params.permit(:username, :email) + def initialize(*) + super + permit(:sign_up, keys: [:username, :email]) end end ``` diff --git a/lib/devise.rb b/lib/devise.rb index d1869651..2d3540b7 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -12,7 +12,6 @@ module Devise autoload :FailureApp, 'devise/failure_app' autoload :OmniAuth, 'devise/omniauth' autoload :ParameterFilter, 'devise/parameter_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/controllers/helpers.rb b/lib/devise/controllers/helpers.rb index 2516b8cb..702744f0 100644 --- a/lib/devise/controllers/helpers.rb +++ b/lib/devise/controllers/helpers.rb @@ -154,11 +154,7 @@ module Devise # lib/devise/parameter_sanitizer.rb for more info. Override this # method in your application controller to use your own parameter sanitizer. def devise_parameter_sanitizer - @devise_parameter_sanitizer ||= if defined?(ActionController::StrongParameters) - Devise::ParameterSanitizer.new(resource_class, resource_name, params) - else - Devise::BaseSanitizer.new(resource_class, resource_name, params) - end + @devise_parameter_sanitizer ||= Devise::ParameterSanitizer.new(resource_class, resource_name, params) end # Tell warden that params authentication is allowed for that specific page. diff --git a/lib/devise/parameter_sanitizer.rb b/lib/devise/parameter_sanitizer.rb index 7b10cf55..5eba0632 100644 --- a/lib/devise/parameter_sanitizer.rb +++ b/lib/devise/parameter_sanitizer.rb @@ -1,99 +1,208 @@ module Devise - class BaseSanitizer - attr_reader :params, :resource_name, :resource_class + # The +ParameterSanitizer+ deals with permitting specific parameters values + # for each +Devise+ scope in the application. + # + # The sanitizer knows about Devise default parameters (like +password+ and + # +password_confirmation+ for the `RegistrationsController`), and you can + # extend or change the permitted parameters list on your controllers. + # + # === Permitting new parameters + # + # You can add new parameters to the permitted list using the +permit+ method + # in a +before_action+ method, for instance. + # + # class ApplicationController < ActionController::Base + # before_action :configure_permitted_parameters, if: :devise_controller? + # + # protected + # + # def configure_permitted_parameters + # # Permit the `subscribe_newsletter` parameter along with the other + # # sign up parameters. + # devise_parameter_sanitizer.permit(:sign_up, keys: [:subscribe_newsletter]) + # end + # end + # + # Using a block yields an +ActionController::Parameters+ object so you can + # permit nested parameters and have more control over how the parameters are + # permitted in your controller. + # + # def configure_permitted_parameters + # devise_parameter_sanitizer.permit(:sign_up) do |user| + # user.permit(newsletter_preferences: []) + # end + # end + class ParameterSanitizer + DEFAULT_PERMITTED_ATTRIBUTES = { + sign_in: [:password, :remember_me], + sign_up: [:password, :password_confirmation], + account_update: [:password, :password_confirmation, :current_password] + } def initialize(resource_class, resource_name, params) - @resource_class = resource_class - @resource_name = resource_name + @auth_keys = extract_auth_keys(resource_class) @params = params - @blocks = Hash.new - end + @resource_name = resource_name + @permitted = {} - def for(kind, &block) - if block_given? - @blocks[kind] = block - else - default_for(kind) + DEFAULT_PERMITTED_ATTRIBUTES.each_pair do |action, keys| + permit(action, keys: keys) end end - def sanitize(kind) - if block = @blocks[kind] - block.call(default_params) + # Sanitize the parameters for a specific +action+. + # + # === Arguments + # + # * +action+ - A +Symbol+ with the action that the controller is + # performing, like +sign_up+, +sign_in+, etc. + # + # === Examples + # + # # Inside the `RegistrationsController#create` action. + # resource = build_resource(devise_parameter_sanitizer.sanitize(:sign_up)) + # resource.save + # + # Returns an +ActiveSupport::HashWithIndifferentAccess+ with the permitted + # attributes. + def sanitize(action) + permissions = @permitted[action] + + # DEPRECATED: Remove this branch on Devise 4.1. + if respond_to?(action, true) + deprecate_instance_method_sanitization(action) + return send(action) + end + + if permissions.respond_to?(:call) + cast_to_hash permissions.call(default_params) + elsif permissions.present? + cast_to_hash permit_keys(default_params, permissions) else - default_sanitize(kind) + unknown_action!(action) + end + end + + # Add or remove new parameters to the permitted list of an +action+. + # + # === Arguments + # + # * +action+ - A +Symbol+ with the action that the controller is + # performing, like +sign_up+, +sign_in+, etc. + # * +keys:+ - An +Array+ of keys that also should be permitted. + # * +except:+ - An +Array+ of keys that shouldn't be permitted. + # * +block+ - A block that should be used to permit the action + # parameters instead of the +Array+ based approach. The block will be + # called with an +ActionController::Parameters+ instance. + # + # === Examples + # + # # Adding new parameters to be permitted in the `sign_up` action. + # devise_parameter_sanitizer.permit(:sign_up, keys: [:subscribe_newsletter]) + # + # # Removing the `password` parameter from the `account_update` action. + # devise_parameter_sanitizer.permit(:account_update, except: [:password]) + # + # # Using the block form to completely override how we permit the + # # parameters for the `sign_up` action. + # devise_parameter_sanitizer.permit(:sign_up) do |user| + # user.permit(:email, :password, :password_confirmation) + # end + # + # + # Returns nothing. + def permit(action, keys: nil, except: nil, &block) + if block_given? + @permitted[action] = block + end + + if keys.present? + @permitted[action] ||= @auth_keys.dup + @permitted[action].concat(keys) + end + + if except.present? + @permitted[action] ||= @auth_keys.dup + @permitted[action] = @permitted[action] - except + end + end + + # DEPRECATED: Remove this method on Devise 4.1. + def for(action, &block) # :nodoc: + if block_given? + deprecate_for_with_block(action) + permit(action, &block) + else + deprecate_for_without_block(action) + @permitted[action] or unknown_action!(action) end end private - def default_for(kind) - raise ArgumentError, "a block is expected in Devise base sanitizer" - end - - def default_sanitize(kind) - default_params + # Cast a sanitized +ActionController::Parameters+ to a +HashWithIndifferentAccess+ + # that can be used elsewhere. + # + # Returns an +ActiveSupport::HashWithIndifferentAccess+. + def cast_to_hash(params) + # TODO: Remove the `with_indifferent_access` method call when we only support Rails 5+. + params && params.to_h.with_indifferent_access end def default_params - params.fetch(resource_name, {}) - end - end - - class ParameterSanitizer < BaseSanitizer - def initialize(*) - super - @permitted = Hash.new { |h,k| h[k] = attributes_for(k) } + @params.fetch(@resource_name, {}) end - def sign_in - permit self.for(:sign_in) + def permit_keys(parameters, keys) + parameters.permit(*keys) end - def sign_up - permit self.for(:sign_up) + def extract_auth_keys(klass) + auth_keys = klass.authentication_keys + + auth_keys.respond_to?(:keys) ? auth_keys.keys : auth_keys end - def account_update - permit self.for(:account_update) + def unknown_action!(action) + raise NotImplementedError, "Devise doesn't know how to sanitize parameters for #{action}" end - private + def deprecate_for_with_block(action) + ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc) + [Devise] Changing the sanitized parameters through "#{self.class.name}#for(#{action}) is deprecated and it will be removed from Devise 4.1. + Please use the `permit` method: - # TODO: We do need to flatten so it works with strong_parameters - # gem. We should drop it once we move to Rails 4 only support. - def permit(keys) - default_params.permit(*Array(keys)) + devise_parameter_sanitizer.permit(:#{action}) do |user| + # Your block here. + end + MESSAGE end - # 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}") + def deprecate_for_without_block(action) + ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc) + [Devise] Changing the sanitized parameters through "#{self.class.name}#for(#{action}) is deprecated and it will be removed from Devise 4.1. + Please use the `permit` method to add or remove any key: + + To add any new key, use the `keys` keyword argument: + devise_parameter_sanitizer.permit(:#{action}, keys: [:key1, key2, key3]) + + To remove any existing key, use the `except` keyword argument: + devise_parameter_sanitizer.permit(:#{action}, except: [:email]) + MESSAGE end - def default_sanitize(kind) - if respond_to?(kind, true) - send(kind) - else - raise NotImplementedError, "Devise doesn't know how to sanitize parameters for #{kind}" - end - end + def deprecate_instance_method_sanitization(action) + ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc) + [Devise] Parameter sanitization through a "#{self.class.name}##{action}" method is deprecated and it will be removed from Devise 4.1. + Please use the `permit` method on your sanitizer `initialize` method. - 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 + class #{self.class.name} < Devise::ParameterSanitizer + def initialize(*) + super + permit(:#{action}, keys: [:key1, :key2, :key3]) + end + end + MESSAGE end end end diff --git a/test/parameter_sanitizer_test.rb b/test/parameter_sanitizer_test.rb index a291580b..b40bf6a7 100644 --- a/test/parameter_sanitizer_test.rb +++ b/test/parameter_sanitizer_test.rb @@ -1,95 +1,131 @@ require 'test_helper' require 'devise/parameter_sanitizer' -class BaseSanitizerTest < ActiveSupport::TestCase +class ParameterSanitizerTest < ActiveSupport::TestCase def sanitizer(params) - Devise::BaseSanitizer.new(User, :user, params) + params = ActionController::Parameters.new(params) + Devise::ParameterSanitizer.new(User, :user, params) end - test 'returns chosen params' do - sanitizer = sanitizer(user: { "email" => "jose" }) - assert_equal({ "email" => "jose" }, sanitizer.sanitize(:sign_in)) + test 'permits the default parameters for sign in' do + sanitizer = sanitizer('user' => { 'email' => 'jose' }) + sanitized = sanitizer.sanitize(:sign_in) + + assert_equal({ 'email' => 'jose' }, sanitized) + end + + test 'permits the default parameters for sign up' do + sanitizer = sanitizer('user' => { 'email' => 'jose', 'role' => 'invalid' }) + sanitized = sanitizer.sanitize(:sign_up) + + assert_equal({ 'email' => 'jose' }, sanitized) + end + + test 'permits the default parameters for account update' do + sanitizer = sanitizer('user' => { 'email' => 'jose', 'role' => 'invalid' }) + sanitized = sanitizer.sanitize(:account_update) + + assert_equal({ 'email' => 'jose' }, sanitized) + end + + test 'permits news parameters for an existing action' do + sanitizer = sanitizer('user' => { 'username' => 'jose' }) + sanitizer.permit(:sign_in, keys: [:username]) + sanitized = sanitizer.sanitize(:sign_in) + + assert_equal({ 'username' => 'jose' }, sanitized) + end + + test 'permits news parameters for an existing action with a block' do + sanitizer = sanitizer('user' => { 'username' => 'jose' }) + sanitizer.permit(:sign_in) do |user| + user.permit(:username) + end + + sanitized = sanitizer.sanitize(:sign_in) + + assert_equal({ 'username' => 'jose' }, sanitized) + end + + test 'permit parameters for new actions' do + sanitizer = sanitizer('user' => { 'email' => 'jose@omglol', 'name' => 'Jose' }) + sanitizer.permit(:invite_user, keys: [:email, :name]) + + sanitized = sanitizer.sanitize(:invite_user) + + assert_equal({ 'email' => 'jose@omglol', 'name' => 'Jose' }, sanitized) + end + + test 'fails when we do not have any permitted parameters for the action' do + sanitizer = sanitizer('user' => { 'email' => 'jose', 'password' => 'invalid' }) + + assert_raise NotImplementedError do + sanitizer.sanitize(:unknown) + end + end + + test 'removes permitted parameters' do + sanitizer = sanitizer('user' => { 'email' => 'jose@omglol', 'username' => 'jose' }) + + sanitizer.permit(:sign_in, keys: [:username], except: [:email]) + sanitized = sanitizer.sanitize(:sign_in) + + assert_equal({ 'username' => 'jose' }, sanitized) end end -if defined?(ActionController::StrongParameters) - require 'active_model/forbidden_attributes_protection' - - class ParameterSanitizerTest < ActiveSupport::TestCase - def sanitizer(params) - params = ActionController::Parameters.new(params) - Devise::ParameterSanitizer.new(User, :user, params) +class DeprecatedParameterSanitizerAPITest < ActiveSupport::TestCase + class CustomSanitizer < Devise::ParameterSanitizer + def sign_in + default_params.permit(:username) end + end + + def sanitizer(params) + params = ActionController::Parameters.new(params) + Devise::ParameterSanitizer.new(User, :user, params) + end + + test 'overriding instance methods have precedence over the default sanitized attributes' do + assert_deprecated do + params = ActionController::Parameters.new(user: { "username" => "jose", "name" => "Jose" }) + sanitizer = CustomSanitizer.new(User, :user, params) - test 'filters some parameters on sign in by default' do - sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid", "remember_me" => "1" }) sanitized = sanitizer.sanitize(:sign_in) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "email" => "jose", "password" => "invalid", "remember_me" => "1" }, sanitized) - end - test 'handles auth keys as a hash' do - swap Devise, authentication_keys: {email: true} do - sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) - sanitized = sanitizer.sanitize(:sign_in) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "email" => "jose", "password" => "invalid" }, sanitized) - end + assert_equal({ "username" => "jose" }, sanitized) end + end - test 'filters some parameters on sign up by default' do - sanitizer = sanitizer(user: { "email" => "jose", "role" => "invalid" }) - sanitized = sanitizer.sanitize(:sign_up) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "email" => "jose" }, sanitized) - end - - test 'filters some parameters on account update by default' do - sanitizer = sanitizer(user: { "email" => "jose", "role" => "invalid" }) - sanitized = sanitizer.sanitize(:account_update) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "email" => "jose" }, sanitized) - end - - test 'allows custom hooks' do - sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) - sanitizer.for(:sign_in) { |user| user.permit(:email, :password) } + test 'adding new parameters by mutating the Array' do + assert_deprecated do + sanitizer = sanitizer('user' => { 'username' => 'jose' }) + sanitizer.for(:sign_in) << :username sanitized = sanitizer.sanitize(:sign_in) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "email" => "jose", "password" => "invalid" }, sanitized) - end - test 'adding multiple permitted parameters' do - sanitizer = sanitizer(user: { "email" => "jose", "username" => "jose1", "role" => "valid" }) - sanitizer.for(:sign_in).concat([:username, :role]) - sanitized = sanitizer.sanitize(:sign_in) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "email" => "jose", "username" => "jose1", "role" => "valid" }, sanitized) + assert_equal({ 'username' => 'jose' }, sanitized) end + end - test 'removing multiple default parameters' do - sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid", "remember_me" => "1" }) + test 'adding new parameters with a block' do + assert_deprecated do + sanitizer = sanitizer('user' => { 'username' => 'jose' }) + sanitizer.for(:sign_in) { |user| user.permit(:username) } + + sanitized = sanitizer.sanitize(:sign_in) + + assert_equal({ 'username' => 'jose' }, sanitized) + end + end + + test 'removing multiple default parameters' do + assert_deprecated do + sanitizer = sanitizer('user' => { 'email' => 'jose', 'password' => 'invalid', 'remember_me' => '1' }) sanitizer.for(:sign_in).delete(:email) sanitizer.for(:sign_in).delete(:password) sanitized = sanitizer.sanitize(:sign_in) - sanitized = sanitized.to_h if sanitized.respond_to? :to_h - assert_equal({ "remember_me" => "1" }, sanitized) - end - test 'raises on unknown hooks' do - sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) - assert_raise NotImplementedError do - sanitizer.sanitize(:unknown) - end - end - - test 'passes parameters to filter as arguments to sanitizer' do - params = {user: stub} - sanitizer = Devise::ParameterSanitizer.new(User, :user, params) - - params[:user].expects(:permit).with(kind_of(Symbol), kind_of(Symbol), kind_of(Symbol)) - - sanitizer.sanitize(:sign_in) + assert_equal({ 'remember_me' => '1' }, sanitized) end end end