Simplify parameter sanitization proposal

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

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

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

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)

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

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

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