Rails 4 strong parameters support #2358

Merged
merged 7 commits into from Apr 14, 2013

Conversation

Projects
None yet
6 participants
Contributor

latortuga commented Apr 1, 2013

This is a revision of my PR #2354 rebased against the rails4 branch.

This branch is a result of a discussion with @lucasmazza and @carlosantoniodasilva a few weeks ago about the best way to allow flexible support for strong_parameters. This version of the PR is based on the rails4 branch. The result is a slight deviation from the originally discussed solution but I think it represents a decent compromise - sane default configuration that is easy to override via before_filter.

This sets up a new class Devise::ParameterSanitizer to handle applying strong parameter filtering for Devise controllers in a configurable and flexible way. I added tests for the intended usage of it.

On to the code: need to use a pin field instead of a password for logging in? Simply override the session params:

class ApplicationController
  prepend_before_filter :add_allowed_devise_session_params

  private

  def add_allowed_devise_session_params
    parameters_sanitizer.permit_devise_param(:sessions, :pin)
    parameters_sanitizer.remove_permitted_devise_param(:sessions, :password)
  end
end

Notice - no need to override any devise controllers just to configure your strong parameter filtering and the configuration can still happen at the controller level as intended by the strong parameters change.

Everything is green!

latortuga added some commits Mar 13, 2013

@latortuga latortuga Remove protected_attributes gem and all whitelisting 2f88f7c
@latortuga latortuga Remove mass-assignment role-based tests, no longer supported in Rails 4
Mass-assignment security roles are removed in Rails 4 so there's no need
to test :as => :role behavior.
af4a582
@latortuga latortuga Add support for Rails 4 strong_parameters
This brings support for Rails 4 StrongParameters changes.

- Parameter sanitizing is setup for Devise controllers via
  resource_params except Omniauth Callbacks which doesn't use
  resource_params.

- Change #build_resource to not call resource_params for get requests.
  Parameter sanitizing is only needed when params are posted to the
  server so there's no need to try to construct resource params on get
  requests (new, edit).
78f1373
@latortuga latortuga Fix internal helper test referencing resource_params e0ffe8f
Owner

lucasmazza commented Apr 1, 2013

Nicely done @latortuga ❤️ 💚 💙 💛 💜

Owner

lucasmazza commented Apr 1, 2013

Checking the Travis build I think we can safely remove the include ::ActiveModel::MassAssignmentSecurity call inside the test/rails_app/app/mongoid/shim.rb file.

@lucasmazza lucasmazza and 1 other commented on an outdated diff Apr 1, 2013

lib/devise/parameter_sanitizer.rb
+ @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]
+ }
+ end
+
+ # Allow additional parameters for a Devise controller. If the
+ # controller_name doesn't exist in allowed_params, it will be added to it
+ # as an empty array and param_name will be appended to that array. Note
+ # 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)
@lucasmazza

lucasmazza Apr 1, 2013

Owner

Maybe we can smooth this public API a bit. I'm thinking of using just permit and remove (or delete), but I need a second opinion on this.

Also, we could use *param_name so anyone can permit several parameters with a single method call.

@rafaelfranca

rafaelfranca Apr 1, 2013

Collaborator

@lucasmazza This proposed API is a little bit weird.

parameters_sanitizer.permit(:sessions, :pin, :foo, :bar)

Is :sessions a parameter or the name of controller?

Maybe

parameters_sanitizer_for(:sessions).permit(:pin, :foo, :bar)

latortuga added some commits Apr 1, 2013

@latortuga latortuga Remove MassAssignment security from Mongoid test shim b151d2c
@latortuga latortuga Change parameter sanitizer instance method to scope to devise
This way it's very explicit that this method is for devise and it won't
run into any naming collisions with user code.
77203e3

@rafaelfranca rafaelfranca commented on the diff Apr 1, 2013

test/controllers/internal_helpers_test.rb
- assert_equal user_params, @controller.resource_params
+ assert_equal user_params, @controller.send(:resource_params)
@rafaelfranca

rafaelfranca Apr 1, 2013

Collaborator

Still make sense to test this private method?

@josevalim josevalim commented on an outdated diff Apr 1, 2013

lib/devise/parameter_sanitizer.rb
@@ -0,0 +1,65 @@
+module Devise
+ class ParameterSanitizer
+ attr_reader :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), [])
+ 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
+ @allowed_params = {
@josevalim

josevalim Apr 1, 2013

Owner

Can't we keep this per controller instead? I am fine with people inheriting from controllers in order to customize their parameters.

@josevalim

josevalim Apr 1, 2013

Owner

In particular, I like the approach in this pull request: #2344. We just need to work on detailing and documenting those callbacks (see #2351). However, I was not present when you discussed this implementation, so I would love to hear your thoughts.

@josevalim

josevalim Apr 1, 2013

Owner

In the worst scenario, it could be part of the model configuration option.

Contributor

latortuga commented Apr 2, 2013

I'll expand a bit on the design decision to introduce a new object. The primary intention of the strong_parameters change in Rails 4 is to move authorization of which parameters can be changed out of the model - thus we do not want to make this a model-level configuration, even if it only applies to Devise controllers, because it breaks the spirit of the change. Each controller should have its own set of whitelisted parameters.

I proposed that changing the whitelisted parameters will be a very common need and it would be nice to be able to change one place when a developer needs to do something simple like that. If we put these configuration methods on each controller, the developer is forced to:

  1. Modify routes to override each devise controller they want to change.
  2. Create a new file for each controller they'd like to change.
  3. Find the method that needs to be overridden and provide a new implementation.

In favor of this type of change is that, for existing Devise users, this is a configuration method that they're used to. It isn't a whole lot of overhead but it's 3 steps instead of 1 and when all you want to do is add or remove one parameter, it strikes me as overkill.

@lucasmazza and I worked together on a solution that introduces a new object that handles the parameters for each controller. The original design looked like this. In the spirit of DRY, I moved all of the params.require(x).permit(y) calls into one place in the controller. Then I iterated on being able to customize the parameters with two method calls - #permit_devise_param and #remove_permitted_devise_param.

One thing that I forgot to include in this commit was something else we had discussed: return super if defined?(super) inside DeviseController#parameters_sanitizer. This would allow easy overriding in ApplicationController. After making that change I also realized that we should include the word 'devise' in that method name if it will be overridden by a super class.

I agree with @rafaelfranca that the proposed API change is a bit awkward when taking multiple parameters and I'm coming around to his idea. As another idea, how about this:

devise_parameters_sanitizer.permit(:pin, controller: :sessions)

We could also simplify the API of the param sanitizer and allow the params passed to replace any default params. This would remove the need for delete or remove.

jrgifford referenced this pull request in ClevelandOnRails/rubyonrailsbook Apr 3, 2013

Open

Devise, Omniauth, Sorcery or write-your-own? #15

tagrudev commented Apr 4, 2013

any progress on this ?

Contributor

rymai commented Apr 8, 2013

Great work @latortuga!

@latortuga @rafaelfranca Regarding making the API clearer and adding a way to replace the default params entirely, what about:

class ApplicationController
  prepend_before_filter :add_allowed_devise_session_params
  prepend_before_filter :replace_allowed_devise_password_params

  private

  def add_allowed_devise_session_params
    parameters_sanitizer_for(:sessions).permit(:pin)
    parameters_sanitizer_for(:sessions).forbid(:password)
  end

  def replace_allowed_devise_password_params
    # permit! replaces the default permitted params
    parameters_sanitizer_for(:passwords).permit!(:username, :pin, :pin_confirmation, :reset_pin_token)
  end
end

Regarding @josevalim's comment, I like the callbacks approach and the important thing is that it would be consistent with the current controller callbacks approach (such as here).

@latortuga latortuga 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!
d20fdf8
Contributor

latortuga commented Apr 10, 2013

@rymai I went with your naming and conventions: #permit for adding parameters, #permit! for overwriting an entire controller's parameter list, and #forbid for removing a single parameter. A corresponding #forbid! seemed like overkill given the other 3.

My latest commit introduces changes based on lots of feedback, thanks everyone for pitching in on this discussion. Devise is popular and very widely used so it's important for this to be flexible and useful.

  • DeviseController#parameter_sanitizer has been renamed #devise_parameter_sanitizer
  • #devise_parameter_sanitizer features a new return super if defined?(super) to allow a superclass to add alternative desired implementations. The only requirement for alternative implementations to work with Devise is that the object returned from that method implements #sanitize_for(controller_name) which returns a plain hash of attributes for the current Devise scope.
  • Each Devise controller now features callbacks for create/update actions that can be overridden as necessary, e.g. #create_registration_params.

I have not tested it yet but my supposition is that this patch will work with Rails 3.x with or without the strong_parameters gem, due to the feature checking performed in DeviseController#devise_parameter_sanitizer.

@rymai rymai commented on the diff Apr 10, 2013

test/parameter_sanitizer_test.rb
+ 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)
@rymai

rymai Apr 10, 2013

Contributor

As said by @rafaelfranca earlier, I also think that the API would be clearer like this:

sanitizer_for(:confirmations).permit(:other)

instead of

sanitizer.permit(:confirmations, :other)

@rymai rymai commented on the diff Apr 10, 2013

app/controllers/devise_controller.rb
+ 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, #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)
@rymai

rymai Apr 10, 2013

Contributor

You should replace contr_name by controller_name here (and on the following line). :)

@latortuga

latortuga Apr 11, 2013

Contributor

I originally did this because I was using controller_name and I wanted to avoid shadowing it.

Contributor

rymai commented Apr 10, 2013

Thanks for the update, it looks very good!

Contributor

latortuga commented Apr 11, 2013

Regarding the proposed API change to use sanitizer_for, I didn't do it because it ultimately didn't seem like a good design to me. To make it happen, we would implement method chaining for the sanitizer and that ends up being a Law of Demeter violation. Further, there's only one use case for this object - permit/forbid params for a specific controller. IMHO that's exactly what arguments are for. If it needs to be changed, I'm more in favor of a method signature that takes the params list followed by an options hash with a controller key:

devise_parameter_sanitizer.permit(:pin, controller: :sessions) 

Alternatively, explicit keys for both parameters:

devise_parameters_sanitizer.permit(controller: :sessions, params: [:pin])
Contributor

rymai commented Apr 11, 2013

Ok I see your point. Then what about:

devise_parameter_sanitizer.permit(sessions: :pin)
# or to permit multiple keys at once
devise_parameter_sanitizer.permit(sessions: [:pin, :pass])

This could even allow to permit keys for multiple controllers at the same time:

devise_parameter_sanitizer.permit(sessions: [:pin, :pass], confirmations: :other)

Does it sound like a good idea to you?

@josevalim josevalim merged commit d20fdf8 into plataformatec:rails4 Apr 14, 2013

1 check passed

default The Travis build passed
Details
Owner

josevalim commented Apr 14, 2013

merged! thanks @latortuga !

maran referenced this pull request Apr 14, 2013

Closed

Strong parameters in Rails4 #2372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment