Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate strong_parameters in Rails 4 #7251

Merged
merged 23 commits into from Sep 18, 2012
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8850054
Integrate ActionController::Parameters from StrongParameters gem
guilleiguaran Jul 12, 2012
a8f6d5c
Integrate ActiveModel::ForbiddenAttributesProtection from StrongParam…
guilleiguaran Jul 13, 2012
f8c9a4d
Remove MassAssignmentSecurity from ActiveModel
guilleiguaran Jul 17, 2012
8020f71
Remove mass assignment security from ActiveRecord
guilleiguaran Jul 17, 2012
0168c7a
Add tests for ForbiddenAttributesProtection in ActiveRecord
guilleiguaran Jul 18, 2012
52aa534
Change AMo::ForbiddenAttributesProtection tests to use a subclass of …
guilleiguaran Jul 18, 2012
d695fdb
Remove all references to attr_accessible/protected and old mass_assig…
guilleiguaran Jul 19, 2012
8042835
Change scaffold_controller to generate and use private method to enca…
guilleiguaran Jul 19, 2012
8c4de0e
Remove integration between attr_accessible/protected and AC::Metal::P…
guilleiguaran Jul 19, 2012
b4d9a58
require abstract_unit in parameters tests
guilleiguaran Jul 20, 2012
9881dbb
Remove attributes whitelist tests from AppGenerator tests
guilleiguaran Jul 22, 2012
7d3be7e
Remove configuration test for config.activerecord.whitelist_attributes
guilleiguaran Jul 22, 2012
4b608c0
config.activerecord.whitelist_attributes isn't used anymore, remove r…
guilleiguaran Jul 22, 2012
1fa4f92
Rename ForbiddenAttributes exception to ForbiddenAttributesError
guilleiguaran Aug 13, 2012
978c568
Change scaffold_generator: Don't use #require or #permit in scaffold …
guilleiguaran Aug 25, 2012
8cfe95d
Don't use assert_nothing_raised when assert_equal is used
guilleiguaran Aug 29, 2012
1e1bee3
Change tainted/untainted wording to permitted/forbidden
guilleiguaran Aug 29, 2012
1aaf449
Add config.action_controller.permit_all_attributes to bypass StrongPa…
guilleiguaran Aug 30, 2012
91bcebb
Support fields_for attributes, which may have numeric symbols as hash…
guilleiguaran Sep 1, 2012
9bfa13b
attr_accessible and attr_protected raise an exception pointing to use…
guilleiguaran Sep 2, 2012
6ae93a0
AC::ParameterMissing inherits from KeyError since it's more appropiat…
guilleiguaran Sep 10, 2012
2d7ae1b
Remove mass_assignment_options from ActiveRecord
guilleiguaran Sep 12, 2012
3919fcd
Set primary key with id= only if primary key exists
guilleiguaran Sep 14, 2012
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+662 −2,232
Diff settings

Always

Just for now

@@ -2,6 +2,7 @@
require 'abstract_controller'
require 'action_dispatch'
require 'action_controller/metal/live'
require 'action_controller/metal/strong_parameters'

module ActionController
extend ActiveSupport::Autoload
@@ -34,6 +35,7 @@ module ActionController
autoload :Rescue
autoload :Responder
autoload :Streaming
autoload :StrongParameters
autoload :Testing
autoload :UrlFor
end
@@ -196,6 +196,7 @@ def self.without_modules(*modules)
Caching,
MimeResponds,
ImplicitRender,
StrongParameters,

Cookies,
Flash,
@@ -42,9 +42,7 @@ module ActionController
# end
#
# On ActiveRecord models with no +:include+ or +:exclude+ option set,
# if attr_accessible is set on that model, it will only wrap the accessible
# parameters, else it will only wrap the parameters returned by the class
# method attribute_names.
# it will only wrap the parameters returned by the class method attribute_names.
#
# If you're going to pass the parameters to an +ActiveModel+ object (such as
# <tt>User.new(params[:user])</tt>), you might consider passing the model class to
@@ -165,10 +163,7 @@ def _set_wrapper_defaults(options, model=nil)

unless options[:include] || options[:exclude]
model ||= _default_wrap_model
role = options.fetch(:as, :default)
if model.respond_to?(:accessible_attributes) && model.accessible_attributes(role).present?
options[:include] = model.accessible_attributes(role).to_a
elsif model.respond_to?(:attribute_names) && model.attribute_names.present?
if model.respond_to?(:attribute_names) && model.attribute_names.present?
options[:include] = model.attribute_names
end
end
@@ -0,0 +1,125 @@
require 'active_support/concern'
require 'active_support/core_ext/hash/indifferent_access'
require 'active_support/rescuable'

module ActionController
class ParameterMissing < KeyError
attr_reader :param

def initialize(param)
@param = param
super("key not found: #{param}")
end
end

class Parameters < ActiveSupport::HashWithIndifferentAccess

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Aug 25, 2012

Member

Would it be better if ActionController::Parameters wrapped a Hash rather than inherited from ActiveSupport::HashWithIndifferentAccess? For example CookieJar used to inherit from Hash but was changed in 0ca69ca. @tenderlove, what do you think?

This comment has been minimized.

Copy link
@rafaelfranca

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Sep 9, 2012

Author Member

@rafaelfranca can you work on this, I'm no having much luck with this and @spastorino won't have time to check it on the next weeks

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Sep 9, 2012

Member

Even though my opinion is to do so, I think the final decision was to not change.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Sep 9, 2012

Member

I remember some discussion about this one but I didn't read yet. I'll try to read now.

This comment has been minimized.

Copy link
@kuraga

kuraga Sep 9, 2012

@rafaelfranca it'll be great you post a link :)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Sep 10, 2012

Member

@kuraga I can't. It is a private discussion.

cattr_accessor :permit_all_parameters, instance_accessor: false
attr_accessor :permitted
alias :permitted? :permitted

def initialize(attributes = nil)
super(attributes)
@permitted = self.class.permit_all_parameters
end

def permit!
@permitted = true
self
end

def require(key)
self[key].presence || raise(ParameterMissing.new(key))
end

alias :required :require

def permit(*filters)
params = self.class.new

filters.each do |filter|
case filter
when Symbol, String then
params[filter] = self[filter] if has_key?(filter)
when Hash then
self.slice(*filter.keys).each do |key, value|
return unless value

key = key.to_sym

params[key] = each_element(value) do |value|
# filters are a Hash, so we expect value to be a Hash too
next if filter.is_a?(Hash) && !value.is_a?(Hash)

value = self.class.new(value) if !value.respond_to?(:permit)

value.permit(*Array.wrap(filter[key]))

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Member

There's probably no need for Array.wrap.

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Sep 10, 2012

Author Member

got a failure removing it:

1) Failure:
test_0004_nested array with strings that should be hashes(NestedParametersTest) [/Users/guille/code/rails/actionpack/test/controller/parameters/nested_parameters_test.rb:68]:
Expected nil (NilClass) to respond to #empty?.

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Sep 10, 2012

Member

Just removing or changing to Array()?

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Sep 10, 2012

Author Member

I had same results removing and changing to Array()

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Sep 10, 2012

Member

Weird I didn't see any case where Kernel#Array can return nil.

>> Array(nil)
=> []
>> Array([])
=> []
>> Array(1)
=> [1]
>> Array(raise 'foo')
RuntimeError: foo
    from (irb):4
    from /Users/rafaelmfranca/.rbenv/versions/1.9.3/bin/irb:12:in `<main>'
>> Array({})
=> []

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Sep 10, 2012

Author Member

Here we are wrapping a Hash:

>> Array.wrap({key: "value"})
=> [{:key=>"value"}]
>> Array({key: "value"})
=> [[:key, "value"]]
>> Array({})
=> []
>> Array.wrap({})
=> [{}]

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Sep 10, 2012

Member

In this particular case, isn't Array.wrap a defensive thing? What will the difference be over:

value.permit(*Array.wrap(filter[key]))
# vs
value.permit(filter[key])

Do we expect it to be nil or something else?

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Sep 20, 2012

Author Member

@carlosantoniodasilva sorry, just saw your last reply now, you're right this is defensive, feel free to improve it in master branch 😄

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Sep 20, 2012

Member

This is not just defensive. Sometimes filter[key] return just an element and the splat operator would not work.

end
end
end
end

params.permit!
end

def [](key)
convert_hashes_to_parameters(key, super)
end

def fetch(key, *args)
convert_hashes_to_parameters(key, super)
rescue KeyError
raise ActionController::ParameterMissing.new(key)
end

def slice(*keys)
self.class.new(super)
end

def dup
super.tap do |duplicate|
duplicate.instance_variable_set :@permitted, @permitted
end
end

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Member

Shouldn't we override initialize_dup (or copy) instead? More on @jonleighton's post

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Sep 10, 2012

Author Member

HashWithIndifferentAccess is overriding #dup and it doesn't call to super or initialize_dup never, so if we override initialize_dup or initialize_copy here it won't be called never

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Sep 10, 2012

Member

=( Ok, it's something we can fix there and here later.


private
def convert_hashes_to_parameters(key, value)
if value.is_a?(Parameters) || !value.is_a?(Hash)
value
else
# Convert to Parameters on first access
self[key] = self.class.new(value)
end
end

def each_element(object)
if object.is_a?(Array)
object.map { |el| yield el }.compact
elsif object.is_a?(Hash) && object.keys.all? { |k| k =~ /\A-?\d+\z/ }
hash = object.class.new
object.each { |k,v| hash[k] = yield v }
hash
else
yield object
end
end
end

module StrongParameters
extend ActiveSupport::Concern
include ActiveSupport::Rescuable

included do
rescue_from(ActionController::ParameterMissing) do |parameter_missing_exception|
render text: "Required parameter missing: #{parameter_missing_exception.param}", status: :bad_request
end
end

def params
@_params ||= Parameters.new(request.parameters)
end

def params=(val)
@_params = val.is_a?(Hash) ? Parameters.new(val) : val
end
end
end
@@ -19,6 +19,10 @@ class Railtie < Rails::Railtie #:nodoc:
ActionController::Helpers.helpers_path = app.helpers_paths
end

initializer "action_controller.parameters_config" do |app|
ActionController::Parameters.permit_all_parameters = app.config.action_controller.delete(:permit_all_parameters)
end

initializer "action_controller.set_configs" do |app|
paths = app.config.paths
options = app.config.action_controller
@@ -0,0 +1,113 @@
require 'abstract_unit'
require 'action_controller/metal/strong_parameters'

class NestedParametersTest < ActiveSupport::TestCase
test "permitted nested parameters" do
params = ActionController::Parameters.new({
book: {
title: "Romeo and Juliet",
authors: [{
name: "William Shakespeare",
born: "1564-04-26"
}, {
name: "Christopher Marlowe"
}],
details: {
pages: 200,
genre: "Tragedy"
}
},
magazine: "Mjallo!"
})

permitted = params.permit book: [ :title, { authors: [ :name ] }, { details: :pages } ]

assert permitted.permitted?
assert_equal "Romeo and Juliet", permitted[:book][:title]
assert_equal "William Shakespeare", permitted[:book][:authors][0][:name]
assert_equal "Christopher Marlowe", permitted[:book][:authors][1][:name]
assert_equal 200, permitted[:book][:details][:pages]
assert_nil permitted[:book][:details][:genre]
assert_nil permitted[:book][:authors][1][:born]
assert_nil permitted[:magazine]
end

test "nested arrays with strings" do
params = ActionController::Parameters.new({
:book => {
:genres => ["Tragedy"]
}
})

permitted = params.permit :book => :genres
assert_equal ["Tragedy"], permitted[:book][:genres]
end

test "permit may specify symbols or strings" do
params = ActionController::Parameters.new({
:book => {
:title => "Romeo and Juliet",
:author => "William Shakespeare"
},
:magazine => "Shakespeare Today"
})

permitted = params.permit({:book => ["title", :author]}, "magazine")
assert_equal "Romeo and Juliet", permitted[:book][:title]
assert_equal "William Shakespeare", permitted[:book][:author]
assert_equal "Shakespeare Today", permitted[:magazine]
end

test "nested array with strings that should be hashes" do
params = ActionController::Parameters.new({
book: {
genres: ["Tragedy"]
}
})

permitted = params.permit book: { genres: :type }
assert_empty permitted[:book][:genres]
end

test "nested array with strings that should be hashes and additional values" do
params = ActionController::Parameters.new({
book: {
title: "Romeo and Juliet",
genres: ["Tragedy"]
}
})

permitted = params.permit book: [ :title, { genres: :type } ]
assert_equal "Romeo and Juliet", permitted[:book][:title]
assert_empty permitted[:book][:genres]
end

test "nested string that should be a hash" do
params = ActionController::Parameters.new({
book: {
genre: "Tragedy"
}
})

permitted = params.permit book: { genre: :type }
assert_nil permitted[:book][:genre]
end

test "fields_for-style nested params" do
params = ActionController::Parameters.new({
book: {
authors_attributes: {
:'0' => { name: 'William Shakespeare', age_of_death: '52' },
:'-1' => { name: 'Unattributed Assistant' }
}
}
})
permitted = params.permit book: { authors_attributes: [ :name ] }

assert_not_nil permitted[:book][:authors_attributes]['0']
assert_not_nil permitted[:book][:authors_attributes]['-1']
assert_nil permitted[:book][:authors_attributes]['0'][:age_of_death]
assert_equal 'William Shakespeare', permitted[:book][:authors_attributes]['0'][:name]
assert_equal 'Unattributed Assistant', permitted[:book][:authors_attributes]['-1'][:name]
end
end
@@ -0,0 +1,73 @@
require 'abstract_unit'
require 'action_controller/metal/strong_parameters'

class ParametersPermitTest < ActiveSupport::TestCase
setup do
@params = ActionController::Parameters.new({ person: {
age: "32", name: { first: "David", last: "Heinemeier Hansson" }
}})
end

test "fetch raises ParameterMissing exception" do
e = assert_raises(ActionController::ParameterMissing) do
@params.fetch :foo
end
assert_equal :foo, e.param
end

test "fetch doesnt raise ParameterMissing exception if there is a default" do
assert_equal "monkey", @params.fetch(:foo, "monkey")
assert_equal "monkey", @params.fetch(:foo) { "monkey" }
end

test "permitted is sticky on accessors" do
assert !@params.slice(:person).permitted?
assert !@params[:person][:name].permitted?

@params.each { |key, value| assert(value.permitted?) if key == :person }

assert !@params.fetch(:person).permitted?

assert !@params.values_at(:person).first.permitted?
end

test "permitted is sticky on mutators" do
assert !@params.delete_if { |k| k == :person }.permitted?
assert !@params.keep_if { |k,v| k == :person }.permitted?
end

test "permitted is sticky beyond merges" do
assert !@params.merge(a: "b").permitted?
end

test "modifying the parameters" do
@params[:person][:hometown] = "Chicago"
@params[:person][:family] = { brother: "Jonas" }

assert_equal "Chicago", @params[:person][:hometown]
assert_equal "Jonas", @params[:person][:family][:brother]
end

test "permitting parameters that are not there should not include the keys" do
assert !@params.permit(:person, :funky).has_key?(:funky)
end

test "permit state is kept on a dup" do
@params.permit!
assert_equal @params.permitted?, @params.dup.permitted?
end

test "permitted takes a default value when Parameters.permit_all_parameters is set" do
begin
ActionController::Parameters.permit_all_parameters = true
params = ActionController::Parameters.new({ person: {
age: "32", name: { first: "David", last: "Heinemeier Hansson" }
}})

assert params.slice(:person).permitted?
assert params[:person][:name].permitted?
ensure
ActionController::Parameters.permit_all_parameters = false
end
end
end
@@ -0,0 +1,10 @@
require 'abstract_unit'
require 'action_controller/metal/strong_parameters'

class ParametersRequireTest < ActiveSupport::TestCase
test "required parameters must be present not merely not nil" do
assert_raises(ActionController::ParameterMissing) do
ActionController::Parameters.new(person: {}).require(:person)
end
end
end
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.