diff --git a/NEWS.md b/NEWS.md index 798ee6731..23da3e148 100644 --- a/NEWS.md +++ b/NEWS.md @@ -52,6 +52,16 @@ vein, passing a pluralized version of the attribute name to `define_enum_for` would erroneously pass, and now it fails. ([#641]) +* Fix `permit` so that it does not break the functionality of + ActionController::Parameters#require. ([#648], [#675]) + +### Features + +* Add `on` qualifier to `permit`. This allows you to make an assertion that + a restriction was placed on a slice of the `params` hash and not the entire + `params` hash. Although we don't require you to use this qualifier, we do + recommend it, as it's a more precise check. ([#675]) + [#402]: https://github.com/thoughtbot/shoulda-matchers/pull/402 [#587]: https://github.com/thoughtbot/shoulda-matchers/pull/587 [#662]: https://github.com/thoughtbot/shoulda-matchers/pull/662 @@ -59,6 +69,8 @@ [#641]: https://github.com/thoughtbot/shoulda-matchers/pull/641 [#521]: https://github.com/thoughtbot/shoulda-matchers/pull/521 [#607]: https://github.com/thoughtbot/shoulda-matchers/pull/607 +[#648]: https://github.com/thoughtbot/shoulda-matchers/pull/648 +[#675]: https://github.com/thoughtbot/shoulda-matchers/pull/675 # 2.8.0 diff --git a/lib/shoulda/matchers/action_controller/permit_matcher.rb b/lib/shoulda/matchers/action_controller/permit_matcher.rb index 6fc5c8dc3..899c75c35 100644 --- a/lib/shoulda/matchers/action_controller/permit_matcher.rb +++ b/lib/shoulda/matchers/action_controller/permit_matcher.rb @@ -38,14 +38,16 @@ module ActionController # describe UsersController do # it do # should permit(:first_name, :last_name, :email, :password). - # for(:create) + # for(:create). + # on(:user) # end # end # # # Test::Unit # class UsersControllerTest < ActionController::TestCase # should permit(:first_name, :last_name, :email, :password). - # for(:create) + # for(:create). + # on(:user) # end # # If your action requires query parameters in order to work, then you'll @@ -82,7 +84,8 @@ module ActionController # # it do # should permit(:first_name, :last_name, :email, :password). - # for(:update, params: { id: 1 }) + # for(:update, params: { id: 1 }). + # on(:user) # end # end # @@ -93,7 +96,8 @@ module ActionController # end # # should permit(:first_name, :last_name, :email, :password). - # for(:update, params: { id: 1 }) + # for(:update, params: { id: 1 }). + # on(:user) # end # # Finally, if you have an action that isn't one of the seven resourceful @@ -132,10 +136,9 @@ module ActionController # end # # it do - # should permit(:activated).for(:toggle, - # params: { id: 1 }, - # verb: :put - # ) + # should permit(:activated). + # for(:toggle, params: { id: 1 }, verb: :put). + # on(:user) # end # end # @@ -145,10 +148,9 @@ module ActionController # create(:user, id: 1) # end # - # should permit(:activated).for(:toggle, - # params: { id: 1 }, - # verb: :put - # ) + # should permit(:activated). + # for(:toggle, params: { id: 1 }, verb: :put). + # on(:user) # end # # @return [PermitMatcher] @@ -162,11 +164,12 @@ class PermitMatcher attr_writer :stubbed_params def initialize(expected_permitted_params) + @expected_permitted_params = expected_permitted_params @action = nil @verb = nil @request_params = {} - @expected_permitted_params = expected_permitted_params - set_double_collection + @subparameter = nil + @parameters_doubles = ParametersDoubles.new end def for(action, options = {}) @@ -176,19 +179,32 @@ def for(action, options = {}) self end + def add_params(params) + request_params.merge!(params) + self + end + + def on(subparameter) + @subparameter = subparameter + @parameters_doubles = SliceOfParametersDoubles.new(subparameter) + self + end + def in_context(context) @context = context self end def description - "permit #{verb.upcase} ##{action} to receive parameters #{param_names_as_sentence}" + "(on #{verb.upcase} ##{action}) " + expectation end def matches?(controller) @controller = controller ensure_action_and_verb_present! + parameters_doubles.register + Doublespeak.with_doubles_activated do context.__send__(verb, action, request_params) end @@ -197,32 +213,49 @@ def matches?(controller) end def failure_message - "Expected controller to permit #{unpermitted_params.to_sentence}, but it did not." + "Expected #{verb.upcase} ##{action} to #{expectation},\nbut #{reality}." end alias failure_message_for_should failure_message def failure_message_when_negated - "Expected controller not to permit #{verified_permitted_params.to_sentence}, but it did." + "Expected #{verb.upcase} ##{action} not to #{expectation},\nbut it did." end alias failure_message_for_should_not failure_message_when_negated protected - attr_reader :controller, :double_collection, :action, :verb, - :request_params, :expected_permitted_params, :context + attr_reader :controller, :double_collections_by_param, :action, :verb, + :request_params, :expected_permitted_params, :context, :subparameter, + :parameters_doubles + + def expectation + message = 'restrict parameters ' + + if subparameter + message << " for #{subparameter.inspect}" + end + + message << 'to ' + format_param_names(expected_permitted_params) - def set_double_collection - @double_collection = - Doublespeak.double_collection_for(::ActionController::Parameters) + message + end - @double_collection.register_stub(:require).to_return(&:object) - @double_collection.register_proxy(:permit) + def reality + if actual_permitted_params.empty? + 'it did not restrict any parameters' + else + 'the restricted parameters were ' + + format_param_names(actual_permitted_params) + + ' instead' + end + end + + def format_param_names(param_names) + param_names.map(&:inspect).to_sentence end def actual_permitted_params - double_collection.calls_to(:permit).inject([]) do |all_param_names, call| - all_param_names + call.args - end.flatten + parameters_doubles.permitted_params end def permit_called? @@ -258,6 +291,90 @@ def param_names_as_sentence expected_permitted_params.map(&:inspect).to_sentence end + # @private + class ParametersDoubles + def self.permitted_params_within(double_collection) + double_collection.calls_to(:permit).map(&:args).flatten + end + + def initialize + klass = ::ActionController::Parameters + @double_collection = Doublespeak.double_collection_for(klass) + end + + def register + double_collection.register_proxy(:permit) + end + + def permitted_params + ParametersDoubles.permitted_params_within(double_collection) + end + + protected + + attr_reader :double_collection + end + + # @private + class SliceOfParametersDoubles + TOP_LEVEL = Object.new + + def initialize(subparameter) + klass = ::ActionController::Parameters + + @subparameter = subparameter + @double_collections_by_param = { + TOP_LEVEL => Doublespeak.double_collection_for(klass) + } + end + + def register + top_level_collection = double_collections_by_param[TOP_LEVEL] + double_permit_on(top_level_collection) + double_require_on(top_level_collection) + end + + def permitted_params + if double_collections_by_param.key?(subparameter) + ParametersDoubles.permitted_params_within( + double_collections_by_param[subparameter] + ) + else + [] + end + end + + protected + + attr_reader :subparameter, :double_collections_by_param + + private + + def double_permit_on(double_collection) + double_collection.register_proxy(:permit) + end + + def double_require_on(double_collection) + double_collections_by_param = @double_collections_by_param + require_double = double_collection.register_proxy(:require) + + require_double.to_return do |call| + param_name = call.args.first + params = call.return_value + double_collections_by_param[param_name] ||= + double_permit_against(params) + end + end + + def double_permit_against(params) + klass = params.singleton_class + + Doublespeak.double_collection_for(klass).tap do |double_collection| + double_permit_on(double_collection) + end + end + end + # @private class ActionNotDefinedError < StandardError def message diff --git a/lib/shoulda/matchers/doublespeak.rb b/lib/shoulda/matchers/doublespeak.rb index 441c7bf12..e09b0082a 100644 --- a/lib/shoulda/matchers/doublespeak.rb +++ b/lib/shoulda/matchers/doublespeak.rb @@ -13,6 +13,16 @@ class << self def world @_world ||= World.new end + + def debugging_enabled? + ENV['DEBUG_DOUBLESPEAK'] == '1' + end + + def debug(&block) + if debugging_enabled? + puts block.call + end + end end end end diff --git a/spec/unit/shoulda/matchers/action_controller/permit_matcher_spec.rb b/spec/unit/shoulda/matchers/action_controller/permit_matcher_spec.rb index 7e790e420..49f346131 100644 --- a/spec/unit/shoulda/matchers/action_controller/permit_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/action_controller/permit_matcher_spec.rb @@ -1,16 +1,86 @@ require 'unit_spec_helper' describe Shoulda::Matchers::ActionController::PermitMatcher, type: :controller do + shared_examples 'basic tests' do + it 'accepts a subset of the permitted attributes' do + define_controller_with_strong_parameters(action: :create) do |ctrl| + params_with_conditional_require(ctrl.params).permit(:name, :age) + end + + expect(controller).to permit_with_conditional_params( + permit(:name).for(:create) + ) + end + + it 'accepts all of the permitted attributes' do + define_controller_with_strong_parameters(action: :create) do |ctrl| + params_with_conditional_require(ctrl.params).permit(:name, :age) + end + + expect(controller).to permit_with_conditional_params( + permit(:name, :age).for(:create) + ) + end + + it 'rejects attributes that have not been permitted' do + define_controller_with_strong_parameters(action: :create) do |ctrl| + params_with_conditional_require(ctrl.params).permit(:name) + end + + expect(controller).not_to permit_with_conditional_params( + permit(:name, :admin).for(:create) + ) + end + + it 'rejects when #permit has not been called' do + define_controller_with_strong_parameters(action: :create) + + expect(controller).not_to permit_with_conditional_params( + permit(:name).for(:create) + ) + end + + it 'tracks multiple calls to #permit' do + sets_of_attributes = [ + [:eta, :diner_id], + [:phone_number, :address_1, :address_2, :city, :state, :zip] + ] + + params = { + order: { some: 'value' }, + diner: { some: 'value' } + } + + define_controller_with_strong_parameters(action: :create) do |ctrl| + params_with_conditional_require(ctrl.params, :order). + permit(sets_of_attributes[0]) + + params_with_conditional_require(ctrl.params, :diner). + permit(sets_of_attributes[1]) + end + + expect(controller).to permit_with_conditional_params( + permit(*sets_of_attributes[0]).for(:create), + params + ) + + expect(controller).to permit_with_conditional_params( + permit(*sets_of_attributes[1]).for(:create), + params + ) + end + end + it 'requires an action' do assertion = -> { expect(controller).to permit(:name) } - define_controller_for_resource_with_strong_parameters + define_controller_with_strong_parameters expect(&assertion).to raise_error(described_class::ActionNotDefinedError) end it 'requires a verb for a non-restful action' do - define_controller_for_resource_with_strong_parameters + define_controller_with_strong_parameters assertion = lambda do expect(controller).to permit(:name).for(:authorize) @@ -19,54 +89,64 @@ expect(&assertion).to raise_error(described_class::VerbNotDefinedError) end - it 'accepts a subset of the permitted attributes' do - define_controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name, :age) - end + context 'when operating on the entire params hash' do + include_context 'basic tests' do + def permit_with_conditional_params(permit, _params = {}) + permit + end - expect(controller).to permit(:name).for(:create) + def params_with_conditional_require(params, *filters) + params + end + end end - it 'accepts all of the permitted attributes' do - define_controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name, :age) - end + context 'when operating on a slice of the params hash' do + include_context 'basic tests' do + def permit_with_conditional_params(permit, params = nil) + params ||= { user: { some: 'value' } } + permit.add_params(params) + end - expect(controller).to permit(:name, :age).for(:create) - end + def params_with_conditional_require(params, *filters) + if filters.none? + filters = [:user] + end - it 'rejects attributes that have not been permitted' do - define_controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name) + params.require(*filters) + end end - expect(controller).not_to permit(:name, :admin).for(:create) - end - - it 'rejects when #permit has not been called' do - define_controller_for_resource_with_strong_parameters(action: :create) + it 'rejects if asserting that parameters were not permitted, but on the wrong slice' do + define_controller_with_strong_parameters(action: :create) do + params.require(:order).permit(:eta, :diner_id) + end - expect(controller).not_to permit(:name).for(:create) + expect(controller). + not_to permit(:eta, :diner_id). + for(:create, params: { order: { some: 'value' } }). + on(:something_else) + end end it 'can be used more than once in the same test' do - define_controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name) + define_controller_with_strong_parameters(action: :create) do + params.permit(:name) end expect(controller).to permit(:name).for(:create) expect(controller).not_to permit(:admin).for(:create) end - it 'works with routes that require extra params' do + it 'allows extra parameters to be passed to the action if it requires them' do options = { controller_name: 'Posts', action: :show, routes: -> { get '/posts/:slug', to: 'posts#show' } } - define_controller_for_resource_with_strong_parameters(options) do - params.require(:user).permit(:name) + define_controller_with_strong_parameters(options) do + params.permit(:name) end expect(controller). @@ -75,8 +155,8 @@ end it 'works with #update specifically' do - define_controller_for_resource_with_strong_parameters(action: :update) do - params.require(:user).permit(:name) + define_controller_with_strong_parameters(action: :update) do + params.permit(:name) end expect(controller). @@ -84,27 +164,12 @@ for(:update, params: { id: 1 }) end - it 'tracks multiple calls to #permit' do - sets_of_attributes = [ - [:eta, :diner_id], - [:phone_number, :address_1, :address_2, :city, :state, :zip] - ] - - define_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 - - expect(controller).to permit(*sets_of_attributes[0]).for(:create) - expect(controller).to permit(*sets_of_attributes[1]).for(:create) - end - describe '#matches?' do it 'does not raise an error when #fetch was used instead of #require (issue #495)' do matcher = permit(:eta, :diner_id).for(:create) matching = -> { matcher.matches?(controller) } - define_controller_for_resource_with_strong_parameters(action: :create) do + define_controller_with_strong_parameters(action: :create) do params.fetch(:order, {}).permit(:eta, :diner_id) end @@ -120,12 +185,12 @@ params: { user: { some: 'params' } } ) - define_controller_for_resource_with_strong_parameters(action: :create) do + define_controller_with_strong_parameters(action: :create) do params[:foo] = 'bar' actual_foo_param = params[:foo] actual_user_params = params[:user] - params.require(:user).permit(:name) + params.permit(:name) end matcher.matches?(controller) @@ -134,11 +199,32 @@ expect(actual_foo_param).to eq 'bar' end + it 'still allows #require to return a slice of the params' do + expected_user_params = { 'foo' => 'bar' } + actual_user_params = nil + matcher = permit(:name).for( + :update, + params: { id: 1, user: expected_user_params } + ) + + define_controller_with_strong_parameters(action: :update) do + actual_user_params = params.require(:user) + begin + actual_user_params.permit(:name) + rescue + end + end + + matcher.matches?(controller) + + expect(actual_user_params).to eq expected_user_params + end + it 'does not permanently stub the params hash' do matcher = permit(:name).for(:create) params_access = -> { controller.params.require(:user) } - define_controller_for_resource_with_strong_parameters(action: :create) + define_controller_with_strong_parameters(action: :create) matcher.matches?(controller) @@ -167,36 +253,38 @@ it 'returns the correct string' do options = { action: :create, method: :post } - define_controller_for_resource_with_strong_parameters(options) do + define_controller_with_strong_parameters(options) do params.permit(:name, :age) end matcher = described_class.new([:name, :age, :height]).for(:create) - expect(matcher.description). - to eq 'permit POST #create to receive parameters :name, :age, and :height' + expect(matcher.description).to eq( + '(on POST #create) restrict parameters to :name, :age, and :height' + ) end context 'when a verb is specified' do it 'returns the correct string' do options = { action: :some_action } - define_controller_for_resource_with_strong_parameters(options) do + define_controller_with_strong_parameters(options) do params.permit(:name, :age) end matcher = described_class. new([:name]). for(:some_action, verb: :put) - expect(matcher.description). - to eq 'permit PUT #some_action to receive parameters :name' + expect(matcher.description).to eq( + '(on PUT #some_action) restrict parameters to :name' + ) end end end describe 'positive failure message' do it 'includes all missing attributes' do - define_controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name, :age) + define_controller_with_strong_parameters(action: :create) do + params.permit(:name, :age) end assertion = lambda do @@ -205,25 +293,31 @@ for(:create) end - expect(&assertion).to fail_with_message( - 'Expected controller to permit city and country, but it did not.' - ) + message = + "Expected POST #create to restrict parameters to " + + ":name, :age, :city, and :country,\n" + + "but the restricted parameters were :name and :age instead." + + expect(&assertion).to fail_with_message(message) end end describe 'negative failure message' do it 'includes all attributes that should not have been permitted but were' do - define_controller_for_resource_with_strong_parameters(action: :create) do - params.require(:user).permit(:name, :age) + define_controller_with_strong_parameters(action: :create) do + params.permit(:name, :age) end assertion = lambda do expect(controller).not_to permit(:name, :age).for(:create) end - expect(&assertion).to fail_with_message( - 'Expected controller not to permit name and age, but it did.' - ) + message = + "Expected POST #create not to restrict parameters to " + + ":name and :age,\n" + + "but it did." + + expect(&assertion).to fail_with_message(message) end end @@ -283,10 +377,7 @@ Class.new(StandardError) end - def define_controller_for_resource_with_strong_parameters( - options = {}, - &action_body - ) + def define_controller_with_strong_parameters(options = {}, &action_body) model_name = options.fetch(:model_name, 'User') controller_name = options.fetch(:controller_name, 'UsersController') collection_name = controller_name. @@ -300,7 +391,11 @@ def define_controller_for_resource_with_strong_parameters( controller_class = define_controller(controller_name) do define_method action_name do if action_body - instance_eval(&action_body) + if action_body.arity == 0 + instance_eval(&action_body) + else + action_body.call(self) + end end render nothing: true