Fix several issues with strong parameters matcher
* Instead of decorating controller params, use our "Doublespeak" mini-library to stub ActionController::Parameters. This prevents from interfering with the params object if it is used in various ways, i.e. if `params.fetch(...).permit(...)` is used instead of `params.require(...).permit(...)`. * Fix compat with Rails 4.1, where the verb for #update is PATCH not PUT * Track multiple calls to #permit within a given controller action * Fix so that if the route for your action requires params (such as :id) then you can specify those params
This commit is contained in:
parent
dfebd81af0
commit
218293ad07
19
NEWS.md
19
NEWS.md
|
@ -1,4 +1,4 @@
|
|||
# 2.6.1
|
||||
# HEAD
|
||||
|
||||
* Fix `ComparisonMatcher` so that `validate_numericality_of` comparison matchers
|
||||
work with large numbers.
|
||||
|
@ -13,8 +13,21 @@
|
|||
|
||||
* Fix callback matchers and correct test coverage.
|
||||
|
||||
* Fix `permit` so that it does not interfere with the existing `params` hash of
|
||||
the controller you're testing.
|
||||
* Fix `permit` so that it does not interfere with different usages of `params`
|
||||
in your controller action. Specifically, this will not raise an error:
|
||||
`params.fetch(:foo, {}).permit(:bar, :baz)` (the `permit` will have no
|
||||
problems recognizing that :bar and :baz are permitted params).
|
||||
|
||||
* Fix `permit` on Rails 4.1 to use PATCH by default for #update instead of PUT.
|
||||
Previously you had to specify this manually.
|
||||
|
||||
* Fix `permit` so that it track multiple calls to #permit in your controller
|
||||
action. Previously only the last usage of #permit would be considered in
|
||||
determining whether the matcher matched.
|
||||
|
||||
* Fix `permit` so that if the route for your action requires params (such as id)
|
||||
then you can now specify those params:
|
||||
`permit(:first_name, :last_name).for(:update, params: { id: 42 })`.
|
||||
|
||||
# 2.6.0
|
||||
|
||||
|
|
|
@ -10,22 +10,25 @@ require 'active_support/hash_with_indifferent_access'
|
|||
module Shoulda
|
||||
module Matchers
|
||||
module ActionController
|
||||
def permit(*attributes)
|
||||
StrongParametersMatcher.new(self, attributes)
|
||||
def permit(*params)
|
||||
StrongParametersMatcher.new(params).in_context(self)
|
||||
end
|
||||
|
||||
class StrongParametersMatcher
|
||||
attr_writer :stubbed_params
|
||||
|
||||
def initialize(context = nil, attributes)
|
||||
@attributes = attributes
|
||||
@context = context
|
||||
@stubbed_params = NullStubbedParameters.new
|
||||
def initialize(expected_permitted_params)
|
||||
@action = nil
|
||||
@verb = nil
|
||||
@request_params = {}
|
||||
@expected_permitted_params = expected_permitted_params
|
||||
set_double_collection
|
||||
end
|
||||
|
||||
def for(action, options = {})
|
||||
@action = action
|
||||
@verb = options[:verb] || verb_for_action
|
||||
@verb = options.fetch(:verb, default_verb)
|
||||
@request_params = options.fetch(:params, {})
|
||||
self
|
||||
end
|
||||
|
||||
|
@ -35,118 +38,80 @@ module Shoulda
|
|||
end
|
||||
|
||||
def description
|
||||
"permit #{verb.upcase} ##{action} to receive parameters #{attributes_as_sentence}"
|
||||
"permit #{verb.upcase} ##{action} to receive parameters #{param_names_as_sentence}"
|
||||
end
|
||||
|
||||
def matches?(controller)
|
||||
@controller = controller
|
||||
simulate_controller_action && parameters_difference.empty?
|
||||
ensure_action_and_verb_present!
|
||||
|
||||
Doublespeak.with_doubles_activated do
|
||||
context.__send__(verb, action, request_params)
|
||||
end
|
||||
|
||||
def does_not_match?(controller)
|
||||
@controller = controller
|
||||
simulate_controller_action && parameters_intersection.empty?
|
||||
unpermitted_params.empty?
|
||||
end
|
||||
|
||||
def failure_message
|
||||
"Expected controller to permit #{parameters_difference.to_sentence}, but it did not."
|
||||
"Expected controller to permit #{unpermitted_params.to_sentence}, but it did not."
|
||||
end
|
||||
alias failure_message_for_should failure_message
|
||||
|
||||
def failure_message_when_negated
|
||||
"Expected controller not to permit #{parameters_intersection.to_sentence}, but it did."
|
||||
"Expected controller not to permit #{verified_permitted_params.to_sentence}, but it did."
|
||||
end
|
||||
alias failure_message_for_should_not failure_message_when_negated
|
||||
|
||||
private
|
||||
|
||||
attr_reader :controller, :verb, :action, :attributes, :context
|
||||
attr_reader :controller, :double_collection, :action, :verb,
|
||||
:request_params, :expected_permitted_params, :context
|
||||
|
||||
def simulate_controller_action
|
||||
ensure_action_and_verb_present!
|
||||
stub_params
|
||||
def set_double_collection
|
||||
@double_collection =
|
||||
Doublespeak.register_double_collection(::ActionController::Parameters)
|
||||
|
||||
begin
|
||||
context.send(verb, action)
|
||||
ensure
|
||||
unstub_params
|
||||
@double_collection.register_stub(:require).to_return { |params| params }
|
||||
@double_collection.register_proxy(:permit)
|
||||
end
|
||||
|
||||
verify_permit_call
|
||||
def actual_permitted_params
|
||||
double_collection.calls_to(:permit).inject([]) do |all_param_names, call|
|
||||
all_param_names + call.args
|
||||
end.flatten
|
||||
end
|
||||
|
||||
def verify_permit_call
|
||||
@stubbed_params.permit_was_called
|
||||
def permit_called?
|
||||
actual_permitted_params.any?
|
||||
end
|
||||
|
||||
def parameters_difference
|
||||
attributes - @stubbed_params.shoulda_permitted_params
|
||||
def unpermitted_params
|
||||
expected_permitted_params - actual_permitted_params
|
||||
end
|
||||
|
||||
def parameters_intersection
|
||||
attributes & @stubbed_params.shoulda_permitted_params
|
||||
end
|
||||
|
||||
def stub_params
|
||||
matcher = self
|
||||
|
||||
controller.singleton_class.class_eval do
|
||||
alias_method :__shoulda_original_params__, :params
|
||||
|
||||
define_method :params do
|
||||
matcher.stubbed_params = StubbedParameters.new(__shoulda_original_params__)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def unstub_params
|
||||
controller.singleton_class.class_eval do
|
||||
alias_method :params, :__shoulda_original_params__
|
||||
end
|
||||
def verified_permitted_params
|
||||
expected_permitted_params & actual_permitted_params
|
||||
end
|
||||
|
||||
def ensure_action_and_verb_present!
|
||||
if action.blank?
|
||||
raise ActionNotDefinedError
|
||||
end
|
||||
|
||||
if verb.blank?
|
||||
raise VerbNotDefinedError
|
||||
end
|
||||
end
|
||||
|
||||
def verb_for_action
|
||||
verb_lookup = { create: :post, update: :put }
|
||||
verb_lookup[action]
|
||||
end
|
||||
|
||||
def attributes_as_sentence
|
||||
attributes.map(&:inspect).to_sentence
|
||||
end
|
||||
|
||||
class StubbedParameters < SimpleDelegator
|
||||
attr_reader :permit_was_called, :shoulda_permitted_params
|
||||
|
||||
def initialize(original_params)
|
||||
super(original_params)
|
||||
@permit_was_called = false
|
||||
end
|
||||
|
||||
def require(*args)
|
||||
self
|
||||
end
|
||||
|
||||
def permit(*args)
|
||||
@shoulda_permitted_params = args
|
||||
@permit_was_called = true
|
||||
super(*args)
|
||||
def default_verb
|
||||
case action
|
||||
when :create then :post
|
||||
when :update then RailsShim.verb_for_update
|
||||
end
|
||||
end
|
||||
|
||||
class NullStubbedParameters < ActiveSupport::HashWithIndifferentAccess
|
||||
def permit_was_called; false; end
|
||||
def shoulda_permitted_params; self; end
|
||||
def require(*); self; end
|
||||
def permit(*); self; end
|
||||
def param_names_as_sentence
|
||||
expected_permitted_params.map(&:inspect).to_sentence
|
||||
end
|
||||
|
||||
class ActionNotDefinedError < StandardError
|
||||
|
@ -157,8 +122,7 @@ module Shoulda
|
|||
|
||||
class VerbNotDefinedError < StandardError
|
||||
def message
|
||||
'You must specify an HTTP verb when using a non-RESTful action.' +
|
||||
' e.g. for(:authorize, verb: :post)'
|
||||
'You must specify an HTTP verb when using a non-RESTful action. For example: for(:authorize, verb: :post)'
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -33,6 +33,14 @@ module Shoulda # :nodoc:
|
|||
end
|
||||
end
|
||||
|
||||
def self.verb_for_update
|
||||
if action_pack_gte_4_1?
|
||||
:patch
|
||||
else
|
||||
:put
|
||||
end
|
||||
end
|
||||
|
||||
def self.active_record_major_version
|
||||
::ActiveRecord::VERSION::MAJOR
|
||||
end
|
||||
|
@ -44,6 +52,14 @@ module Shoulda # :nodoc:
|
|||
def self.action_pack_major_version
|
||||
::ActionPack::VERSION::MAJOR
|
||||
end
|
||||
|
||||
def self.action_pack_gte_4_1?
|
||||
Gem::Requirement.new('>= 4.1').satisfied_by?(action_pack_version)
|
||||
end
|
||||
|
||||
def self.action_pack_version
|
||||
Gem::Version.new(::ActionPack::VERSION::STRING)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Shoulda::Matchers::ActionController do
|
||||
describe "#permit" do
|
||||
describe '#permit' do
|
||||
it 'matches when the sent parameter is allowed' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name)
|
||||
|
@ -56,8 +56,8 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
|
|||
end
|
||||
end
|
||||
|
||||
describe "#matches?" do
|
||||
it "is true for a subset of the allowable attributes" do
|
||||
describe '#matches?' do
|
||||
it 'is true for a subset of the allowable attributes' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name)
|
||||
end
|
||||
|
@ -66,7 +66,7 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
|
|||
expect(matcher.matches?(@controller)).to be_true
|
||||
end
|
||||
|
||||
it "is true for all the allowable attributes" do
|
||||
it 'is true for all the allowable attributes' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name, :age)
|
||||
end
|
||||
|
@ -75,7 +75,7 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
|
|||
expect(matcher.matches?(@controller)).to be_true
|
||||
end
|
||||
|
||||
it "is false when any attributes are not allowed" do
|
||||
it 'is false when any attributes are not allowed' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name)
|
||||
end
|
||||
|
@ -84,67 +84,130 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
|
|||
expect(matcher.matches?(@controller)).to be_false
|
||||
end
|
||||
|
||||
it "is false when permit is not called" do
|
||||
it 'is false when permit is not called' do
|
||||
controller_for_resource_with_strong_parameters(action: :create)
|
||||
|
||||
matcher = described_class.new([:name]).in_context(self).for(:create)
|
||||
expect(matcher.matches?(@controller)).to be_false
|
||||
end
|
||||
|
||||
it "requires an action" do
|
||||
it 'requires an action' do
|
||||
controller_for_resource_with_strong_parameters
|
||||
matcher = described_class.new([:name])
|
||||
expect { matcher.matches?(@controller) }
|
||||
.to raise_error(Shoulda::Matchers::ActionController::StrongParametersMatcher::ActionNotDefinedError)
|
||||
expect { matcher.matches?(@controller) }.
|
||||
to raise_error(described_class::ActionNotDefinedError)
|
||||
end
|
||||
|
||||
it "requires a verb for non-restful action" do
|
||||
it 'requires a verb for non-restful action' do
|
||||
controller_for_resource_with_strong_parameters
|
||||
matcher = described_class.new([:name]).for(:authorize)
|
||||
expect { matcher.matches?(@controller) }
|
||||
.to raise_error(Shoulda::Matchers::ActionController::StrongParametersMatcher::VerbNotDefinedError)
|
||||
expect { matcher.matches?(@controller) }.
|
||||
to raise_error(described_class::VerbNotDefinedError)
|
||||
end
|
||||
|
||||
it 'works with routes that require extra params' do
|
||||
options = {
|
||||
controller_name: 'Posts',
|
||||
action: :show,
|
||||
routes: -> {
|
||||
get '/posts/:slug', to: 'posts#show'
|
||||
}
|
||||
}
|
||||
controller_for_resource_with_strong_parameters(options) do
|
||||
params.require(:user).permit(:name)
|
||||
end
|
||||
|
||||
matcher = described_class.new([:name]).
|
||||
in_context(self).
|
||||
for(:show, verb: :get, params: { slug: 'foo' })
|
||||
expect(matcher.matches?(@controller)).to be_true
|
||||
end
|
||||
|
||||
it 'works with #update specifically' do
|
||||
controller_for_resource_with_strong_parameters(action: :update) do
|
||||
params.require(:user).permit(:name)
|
||||
end
|
||||
|
||||
matcher = described_class.new([:name]).
|
||||
in_context(self).
|
||||
for(:update, params: { id: 1 })
|
||||
expect(matcher.matches?(@controller)).to be_true
|
||||
end
|
||||
|
||||
it 'does not raise an error when #fetch was used instead of #require (issue #495)' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.fetch(:order, {}).permit(:eta, :diner_id)
|
||||
end
|
||||
|
||||
matcher = described_class.new([:eta, :diner_id]).
|
||||
in_context(self).
|
||||
for(:create)
|
||||
expect(matcher.matches?(@controller)).to be_true
|
||||
end
|
||||
|
||||
it 'tracks multiple calls to #permit' do
|
||||
sets_of_attributes = [
|
||||
[:eta, :diner_id],
|
||||
[:phone_number, :address_1, :address_2, :city, :state, :zip]
|
||||
]
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:order).permit(sets_of_attributes[0])
|
||||
params.require(:diner).permit(sets_of_attributes[1])
|
||||
end
|
||||
|
||||
matcher = described_class.new(sets_of_attributes[0]).
|
||||
in_context(self).
|
||||
for(:create)
|
||||
expect(matcher.matches?(@controller)).to be_true
|
||||
|
||||
matcher = described_class.new(sets_of_attributes[1]).
|
||||
in_context(self).
|
||||
for(:create)
|
||||
expect(matcher.matches?(@controller)).to be_true
|
||||
end
|
||||
|
||||
context 'stubbing params on the controller' do
|
||||
it 'still allows the original params to be set and accessed' do
|
||||
actual_value = nil
|
||||
actual_user_params = nil
|
||||
actual_foo_param = nil
|
||||
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params[:foo] = 'bar'
|
||||
actual_value = params[:foo]
|
||||
actual_foo_param = params[:foo]
|
||||
|
||||
actual_user_params = params[:user]
|
||||
|
||||
params.require(:user).permit(:name)
|
||||
end
|
||||
|
||||
matcher = described_class.new([:name]).in_context(self).for(:create)
|
||||
matcher = described_class.new([:name]).
|
||||
in_context(self).
|
||||
for(:create, params: { user: { some: 'params' } })
|
||||
matcher.matches?(@controller)
|
||||
|
||||
expect(actual_value).to eq 'bar'
|
||||
expect(actual_user_params).to eq('some' => 'params')
|
||||
expect(actual_foo_param).to eq 'bar'
|
||||
end
|
||||
|
||||
it 'stubs the params while the controller action is being run' do
|
||||
params_class = nil
|
||||
|
||||
it 'stubs the params during the controller action' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params_class = params.class
|
||||
params.require(:user).permit(:name)
|
||||
params.require(:user)
|
||||
end
|
||||
|
||||
matcher = described_class.new([:name]).in_context(self).for(:create)
|
||||
matcher.matches?(@controller)
|
||||
|
||||
expect(params_class).to be described_class::StubbedParameters
|
||||
expect { matcher.matches?(@controller) }.not_to raise_error
|
||||
end
|
||||
|
||||
it 'does not permanently stub params' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name)
|
||||
end
|
||||
controller_for_resource_with_strong_parameters(action: :create)
|
||||
|
||||
matcher = described_class.new([:name]).in_context(self).for(:create)
|
||||
matcher.matches?(@controller)
|
||||
|
||||
expect(@controller.params).to be_a(ActionController::Parameters)
|
||||
expect {
|
||||
@controller.params.require(:user)
|
||||
}.to raise_error(::ActionController::ParameterMissing)
|
||||
end
|
||||
|
||||
it 'prevents permanently stubbing params on error' do
|
||||
|
@ -156,64 +219,79 @@ describe Shoulda::Matchers::ActionController::StrongParametersMatcher do
|
|||
rescue SimulatedError
|
||||
end
|
||||
|
||||
expect(@controller.params).to be_a(ActionController::Parameters)
|
||||
expect {
|
||||
@controller.params.require(:user)
|
||||
}.to raise_error(::ActionController::ParameterMissing)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "failure message" do
|
||||
it "includes all missing attributes" do
|
||||
describe 'failure message' do
|
||||
it 'includes all missing attributes' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name, :age)
|
||||
end
|
||||
|
||||
expect {
|
||||
expect(@controller).to permit(:name, :age, :city, :country).for(:create)
|
||||
}.to fail_with_message("Expected controller to permit city and country, but it did not.")
|
||||
}.to fail_with_message('Expected controller to permit city and country, but it did not.')
|
||||
end
|
||||
|
||||
it "includes all attributes that should not have been allowed but were" do
|
||||
it 'includes all attributes that should not have been allowed but were' do
|
||||
controller_for_resource_with_strong_parameters(action: :create) do
|
||||
params.require(:user).permit(:name, :age)
|
||||
end
|
||||
|
||||
expect {
|
||||
expect(@controller).not_to permit(:name, :age).for(:create)
|
||||
}.to fail_with_message("Expected controller not to permit name and age, but it did.")
|
||||
}.to fail_with_message('Expected controller not to permit name and age, but it did.')
|
||||
end
|
||||
end
|
||||
|
||||
describe "#for" do
|
||||
context "when given :create" do
|
||||
it "posts to the controller" do
|
||||
describe '#for' do
|
||||
context 'when given :create' do
|
||||
it 'POSTs to the controller' do
|
||||
controller = ActionController::Base.new
|
||||
context = stub('context', post: nil)
|
||||
context = mock()
|
||||
context.expects(:post).with(:create, {})
|
||||
matcher = described_class.new([:name]).in_context(context).for(:create)
|
||||
|
||||
matcher.matches?(controller)
|
||||
expect(context).to have_received(:post).with(:create)
|
||||
end
|
||||
end
|
||||
|
||||
context "when given :update" do
|
||||
it "puts to the controller" do
|
||||
context 'when given :update' do
|
||||
if rails_gte_41?
|
||||
it 'PATCHes to the controller' do
|
||||
controller = ActionController::Base.new
|
||||
context = stub('context', put: nil)
|
||||
context = mock()
|
||||
context.expects(:patch).with(:update, {})
|
||||
matcher = described_class.new([:name]).in_context(context).for(:update)
|
||||
|
||||
matcher.matches?(controller)
|
||||
expect(context).to have_received(:put).with(:update)
|
||||
end
|
||||
end
|
||||
|
||||
context "when given a custom action and verb" do
|
||||
it "deletes to the controller" do
|
||||
else
|
||||
it 'PUTs to the controller' do
|
||||
controller = ActionController::Base.new
|
||||
context = stub('context', delete: nil)
|
||||
matcher = described_class.new([:name]).in_context(context).for(:hide, verb: :delete)
|
||||
context = mock()
|
||||
context.expects(:put).with(:update, {})
|
||||
matcher = described_class.new([:name]).in_context(context).for(:update)
|
||||
|
||||
matcher.matches?(controller)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when given a custom action and verb' do
|
||||
it 'calls the action with the verb' do
|
||||
controller = ActionController::Base.new
|
||||
context = mock()
|
||||
context.expects(:delete).with(:hide, {})
|
||||
matcher = described_class.new([:name]).
|
||||
in_context(context).
|
||||
for(:hide, verb: :delete)
|
||||
|
||||
matcher.matches?(controller)
|
||||
expect(context).to have_received(:delete).with(:hide)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -58,27 +58,30 @@ module ControllerBuilder
|
|||
$test_app.create_temp_view(path, contents)
|
||||
end
|
||||
|
||||
def controller_for_resource_with_strong_parameters(options = {}, &block)
|
||||
controller_name = 'Users'
|
||||
block ||= -> { {} }
|
||||
def controller_for_resource_with_strong_parameters(options = {}, &action_body)
|
||||
model_name = options.fetch(:model_name, 'User')
|
||||
controller_name = options.fetch(:controller_name, 'UsersController')
|
||||
collection_name = controller_name.
|
||||
to_s.sub(/Controller$/, '').underscore.
|
||||
to_sym
|
||||
action_name = options.fetch(:action, :some_action)
|
||||
routes ||= options.fetch(:routes, -> { resources collection_name })
|
||||
|
||||
define_model "User"
|
||||
controller_class = define_controller controller_name do
|
||||
define_method options.fetch(:action, 'some_action') do
|
||||
@user = User.create(user_params)
|
||||
render nothing: true
|
||||
define_model(model_name)
|
||||
|
||||
controller_class = define_controller(controller_name) do
|
||||
define_method action_name do
|
||||
if action_body
|
||||
instance_eval(&action_body)
|
||||
end
|
||||
|
||||
private
|
||||
define_method :user_params, &block
|
||||
render nothing: true
|
||||
end
|
||||
end
|
||||
|
||||
setup_rails_controller_test(controller_class)
|
||||
|
||||
collection_name = controller_name.
|
||||
to_s.sub(/Controller$/, '').underscore.
|
||||
to_sym
|
||||
define_routes { resources(collection_name) }
|
||||
define_routes(&routes)
|
||||
|
||||
controller_class
|
||||
end
|
||||
|
|
|
@ -10,6 +10,10 @@ module RailsVersions
|
|||
def rails_4_x?
|
||||
Gem::Requirement.new('~> 4.0').satisfied_by?(rails_version)
|
||||
end
|
||||
|
||||
def rails_gte_41?
|
||||
Gem::Requirement.new('>= 4.1').satisfied_by?(rails_version)
|
||||
end
|
||||
end
|
||||
|
||||
RSpec.configure do |config|
|
||||
|
|
Loading…
Reference in New Issue