diff options
16 files changed, 222 insertions, 108 deletions
diff --git a/actionmailer/lib/rails/generators/mailer/templates/mailer.rb b/actionmailer/lib/rails/generators/mailer/templates/mailer.rb index b9be70a2f0..348d314758 100644 --- a/actionmailer/lib/rails/generators/mailer/templates/mailer.rb +++ b/actionmailer/lib/rails/generators/mailer/templates/mailer.rb @@ -5,7 +5,7 @@ class <%= class_name %>Mailer < ApplicationMailer # Subject can be set in your I18n file at config/locales/en.yml # with the following lookup: # - # en.<%= file_path.tr("/",".") %>.<%= action %>.subject + # en.<%= file_path.tr("/",".") %>_mailer.<%= action %>.subject # def <%= action %> @greeting = "Hi" diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 6c4ce6195e..313ac63fab 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,39 @@ +* Default headers, removed in controller actions, are no longer reapplied on + the test response. + + *Jonas Baumann* + +* Deprecate all *_filter callbacks in favor of *_action callbacks. + + *Rafael Mendonça França* + +* Allow you to pass `prepend: false` to protect_from_forgery to have the + verification callback appended instead of prepended to the chain. + This allows you to let the verification step depend on prior callbacks. + Example: + + class ApplicationController < ActionController::Base + before_action :authenticate + protect_from_forgery prepend: false, unless: -> { @authenticated_by.oauth? } + + private + def authenticate + if oauth_request? + # authenticate with oauth + @authenticated_by = 'oauth'.inquiry + else + # authenticate with cookies + @authenticated_by = 'cookie'.inquiry + end + end + end + + *Josef Šimánek* + +* Remove `ActionController::HideActions` + + *Ravil Bayramgalin* + * Remove `respond_to`/`respond_with` placeholder methods, this functionality has been extracted to the `responders` gem. diff --git a/actionpack/lib/abstract_controller/base.rb b/actionpack/lib/abstract_controller/base.rb index 8c7cec3561..c95b9a4097 100644 --- a/actionpack/lib/abstract_controller/base.rb +++ b/actionpack/lib/abstract_controller/base.rb @@ -57,21 +57,11 @@ module AbstractController controller.public_instance_methods(true) end - # The list of hidden actions. Defaults to an empty array. - # This can be modified by other modules or subclasses - # to specify particular actions as hidden. - # - # ==== Returns - # * <tt>Array</tt> - An array of method names that should not be considered actions. - def hidden_actions - [] - end - # A list of method names that should be considered actions. This # includes all public instance methods on a controller, less # any internal methods (see internal_methods), adding back in # any methods that are internal, but still exist on the class - # itself. Finally, hidden_actions are removed. + # itself. # # ==== Returns # * <tt>Set</tt> - A set of all methods that should be considered actions. @@ -82,9 +72,7 @@ module AbstractController # Except for public instance methods of Base and its ancestors internal_methods + # Be sure to include shadowed public instance methods of this class - public_instance_methods(false)).uniq.map(&:to_s) - - # And always exclude explicitly hidden actions - hidden_actions.to_a + public_instance_methods(false)).uniq.map(&:to_s) methods.to_set end diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb index 32de82780f..db999c2765 100644 --- a/actionpack/lib/abstract_controller/callbacks.rb +++ b/actionpack/lib/abstract_controller/callbacks.rb @@ -1,3 +1,5 @@ +require 'active_support/deprecation' + module AbstractController module Callbacks extend ActiveSupport::Concern @@ -28,6 +30,16 @@ module AbstractController # The basic idea is that <tt>:only => :index</tt> gets converted to # <tt>:if => proc {|c| c.action_name == "index" }</tt>. # + # Note that <tt>:only</tt> has priority over <tt>:if</tt> in case they + # are used together. + # + # only: :index, if: -> { true } # the :if option will be ignored. + # + # Note that <tt>:if</tt> has priority over <tt>:except</tt> in case they + # are used together. + # + # except: :index, if: -> { true } # the :except option will be ignored. + # # ==== Options # * <tt>only</tt> - The callback should be run only for this action # * <tt>except</tt> - The callback should be run for all actions except this action @@ -55,7 +67,11 @@ module AbstractController skip_after_action(*names) skip_around_action(*names) end - alias_method :skip_filter, :skip_action_callback + + def skip_filter(*names) + ActiveSupport::Deprecation.warn("#{callback}_filter is deprecated and will removed in Rails 5.1. Use #{callback}_action instead.") + skip_action_callback(*names) + end # Take callback names and an optional callback proc, normalize them, # then call the block with each callback. This allows us to abstract @@ -170,14 +186,22 @@ module AbstractController set_callback(:process_action, callback, name, options) end end - alias_method :"#{callback}_filter", :"#{callback}_action" + + define_method "#{callback}_filter" do |*names, &blk| + ActiveSupport::Deprecation.warn("#{callback}_filter is deprecated and will removed in Rails 5.1. Use #{callback}_action instead.") + send("#{callback}_action", *names, &blk) + end define_method "prepend_#{callback}_action" do |*names, &blk| _insert_callbacks(names, blk) do |name, options| set_callback(:process_action, callback, name, options.merge(:prepend => true)) end end - alias_method :"prepend_#{callback}_filter", :"prepend_#{callback}_action" + + define_method "prepend_#{callback}_filter" do |*names, &blk| + ActiveSupport::Deprecation.warn("prepend_#{callback}_filter is deprecated and will removed in Rails 5.1. Use prepend_#{callback}_action instead.") + send("prepend_#{callback}_action", *names, &blk) + end # Skip a before, after or around callback. See _insert_callbacks # for details on the allowed parameters. @@ -186,11 +210,19 @@ module AbstractController skip_callback(:process_action, callback, name, options) end end - alias_method :"skip_#{callback}_filter", :"skip_#{callback}_action" + + define_method "skip_#{callback}_filter" do |*names, &blk| + ActiveSupport::Deprecation.warn("skip_#{callback}_filter is deprecated and will removed in Rails 5.1. Use skip_#{callback}_action instead.") + send("skip_#{callback}_action", *names, &blk) + end # *_action is the same as append_*_action alias_method :"append_#{callback}_action", :"#{callback}_action" - alias_method :"append_#{callback}_filter", :"#{callback}_action" + + define_method "append_#{callback}_filter" do |*names, &blk| + ActiveSupport::Deprecation.warn("append_#{callback}_filter is deprecated and will removed in Rails 5.1. Use append_#{callback}_action instead.") + send("append_#{callback}_action", *names, &blk) + end end end end diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 5cb11bc479..e6038396f9 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -206,7 +206,6 @@ module ActionController AbstractController::AssetPaths, Helpers, - HideActions, UrlFor, Redirecting, ActionView::Layouts, diff --git a/actionpack/lib/action_controller/metal/hide_actions.rb b/actionpack/lib/action_controller/metal/hide_actions.rb deleted file mode 100644 index af36ffa240..0000000000 --- a/actionpack/lib/action_controller/metal/hide_actions.rb +++ /dev/null @@ -1,40 +0,0 @@ - -module ActionController - # Adds the ability to prevent public methods on a controller to be called as actions. - module HideActions - extend ActiveSupport::Concern - - included do - class_attribute :hidden_actions - self.hidden_actions = Set.new.freeze - end - - private - - # Overrides AbstractController::Base#action_method? to return false if the - # action name is in the list of hidden actions. - def method_for_action(action_name) - self.class.visible_action?(action_name) && super - end - - module ClassMethods - # Sets all of the actions passed in as hidden actions. - # - # ==== Parameters - # * <tt>args</tt> - A list of actions - def hide_action(*args) - self.hidden_actions = hidden_actions.dup.merge(args.map(&:to_s)).freeze - end - - def visible_action?(action_name) - not hidden_actions.include?(action_name) - end - - # Overrides AbstractController::Base#action_methods to remove any methods - # that are listed as hidden methods. - def action_methods - @action_methods ||= Set.new(super.reject { |name| hidden_actions.include?(name) }).freeze - end - end - end -end diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index b9a1e7d242..7facbe79aa 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -87,6 +87,11 @@ module ActionController #:nodoc: # # * <tt>:only/:except</tt> - Only apply forgery protection to a subset of actions. Like <tt>only: [ :create, :create_all ]</tt>. # * <tt>:if/:unless</tt> - Turn off the forgery protection entirely depending on the passed proc or method reference. + # * <tt>:prepend</tt> - By default, the verification of the authentication token is added to the front of the + # callback chain. If you need to make the verification depend on other callbacks, like authentication methods + # (say cookies vs oauth), this might not work for you. Pass <tt>prepend: false</tt> to just add the + # verification callback in the position of the protect_from_forgery call. This means any callbacks added + # before are run first. # * <tt>:with</tt> - Set the method to handle unverified request. # # Valid unverified request handling methods are: @@ -94,9 +99,11 @@ module ActionController #:nodoc: # * <tt>:reset_session</tt> - Resets the session. # * <tt>:null_session</tt> - Provides an empty session during request but doesn't reset it completely. Used as default if <tt>:with</tt> option is not specified. def protect_from_forgery(options = {}) + options = options.reverse_merge(prepend: true) + self.forgery_protection_strategy = protection_method_class(options[:with] || :null_session) self.request_forgery_protection_token ||= :authenticity_token - prepend_before_action :verify_authenticity_token, options + before_action :verify_authenticity_token, options append_after_action :verify_same_origin_request end diff --git a/actionpack/lib/action_dispatch/testing/test_response.rb b/actionpack/lib/action_dispatch/testing/test_response.rb index 82039e72e7..369ea1467c 100644 --- a/actionpack/lib/action_dispatch/testing/test_response.rb +++ b/actionpack/lib/action_dispatch/testing/test_response.rb @@ -25,5 +25,12 @@ module ActionDispatch # Was there a server-side error? alias_method :error?, :server_error? + + def merge_default_headers(original, *args) + # Default headers are already applied, no need to merge them a second time. + # This makes sure that default headers, removed in controller actions, will + # not be reapplied to the test response. + original + end end end diff --git a/actionpack/test/abstract/callbacks_test.rb b/actionpack/test/abstract/callbacks_test.rb index 8cba049485..07571602e4 100644 --- a/actionpack/test/abstract/callbacks_test.rb +++ b/actionpack/test/abstract/callbacks_test.rb @@ -267,9 +267,11 @@ module AbstractController end class AliasedCallbacks < ControllerWithCallbacks - before_filter :first - after_filter :second - around_filter :aroundz + ActiveSupport::Deprecation.silence do + before_filter :first + after_filter :second + around_filter :aroundz + end def first @text = "Hello world" diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb index 950788743e..001493afc0 100644 --- a/actionpack/test/controller/base_test.rb +++ b/actionpack/test/controller/base_test.rb @@ -1,31 +1,11 @@ require 'abstract_unit' require 'active_support/logger' require 'controller/fake_models' -require 'pp' # require 'pp' early to prevent hidden_methods from not picking up the pretty-print methods until too late # Provide some controller to run the tests on. module Submodule class ContainedEmptyController < ActionController::Base end - - class ContainedNonEmptyController < ActionController::Base - def public_action - render :nothing => true - end - - hide_action :hidden_action - def hidden_action - raise "Noooo!" - end - - def another_hidden_action - end - hide_action :another_hidden_action - end - - class SubclassedController < ContainedNonEmptyController - hide_action :public_action # Hiding it here should not affect the superclass. - end end class EmptyController < ActionController::Base @@ -35,10 +15,6 @@ class NonEmptyController < ActionController::Base def public_action render :nothing => true end - - hide_action :hidden_action - def hidden_action - end end class DefaultUrlOptionsController < ActionController::Base @@ -108,10 +84,7 @@ class ControllerInstanceTests < ActiveSupport::TestCase def setup @empty = EmptyController.new @contained = Submodule::ContainedEmptyController.new - @empty_controllers = [@empty, @contained, Submodule::SubclassedController.new] - - @non_empty_controllers = [NonEmptyController.new, - Submodule::ContainedNonEmptyController.new] + @empty_controllers = [@empty, @contained] end def test_performed? @@ -124,10 +97,6 @@ class ControllerInstanceTests < ActiveSupport::TestCase @empty_controllers.each do |c| assert_equal Set.new, c.class.action_methods, "#{c.controller_path} should be empty!" end - - @non_empty_controllers.each do |c| - assert_equal Set.new(%w(public_action)), c.class.action_methods, "#{c.controller_path} should not be empty!" - end end def test_temporary_anonymous_controllers @@ -161,12 +130,6 @@ class PerformActionTest < ActionController::TestCase assert_equal "The action 'non_existent' could not be found for EmptyController", exception.message end - def test_get_on_hidden_should_fail - use_controller NonEmptyController - assert_raise(AbstractController::ActionNotFound) { get :hidden_action } - assert_raise(AbstractController::ActionNotFound) { get :another_hidden_action } - end - def test_action_missing_should_work use_controller ActionMissingController get :arbitrary_action diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb index 2e08a6af9f..b9fb6be4e3 100644 --- a/actionpack/test/controller/filters_test.rb +++ b/actionpack/test/controller/filters_test.rb @@ -225,6 +225,30 @@ class FilterTest < ActionController::TestCase skip_before_action :clean_up_tmp, if: -> { true } end + class SkipFilterUsingOnlyAndIf < ConditionalFilterController + before_action :clean_up_tmp + before_action :ensure_login + + skip_before_action :ensure_login, only: :login, if: -> { false } + skip_before_action :clean_up_tmp, only: :login, if: -> { true } + + def login + render text: 'ok' + end + end + + class SkipFilterUsingIfAndExcept < ConditionalFilterController + before_action :clean_up_tmp + before_action :ensure_login + + skip_before_action :ensure_login, if: -> { false }, except: :login + skip_before_action :clean_up_tmp, if: -> { true }, except: :login + + def login + render text: 'ok' + end + end + class ClassController < ConditionalFilterController before_action ConditionalClassFilter end @@ -596,6 +620,16 @@ class FilterTest < ActionController::TestCase assert_equal %w( ensure_login ), assigns["ran_filter"] end + def test_if_is_ignored_when_used_with_only + test_process(SkipFilterUsingOnlyAndIf, 'login') + assert_nil assigns['ran_filter'] + end + + def test_except_is_ignored_when_used_with_if + test_process(SkipFilterUsingIfAndExcept, 'login') + assert_equal %w(ensure_login), assigns["ran_filter"] + end + def test_skipping_class_actions test_process(ClassController) assert_equal true, assigns["ran_class_action"] diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 5535c7ae78..5c0651bd73 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -279,6 +279,11 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest def redirect redirect_to action_url('get') end + + def remove_default_header + response.headers.except! 'X-Frame-Options' + head :ok + end end def test_get @@ -506,6 +511,24 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest end end + def test_removed_default_headers_on_test_response_are_not_reapplied + with_test_route_set do + begin + header_to_remove = 'X-Frame-Options' + original_default_headers = ActionDispatch::Response.default_headers + ActionDispatch::Response.default_headers = { + 'X-Content-Type-Options' => 'nosniff', + header_to_remove => 'SAMEORIGIN', + } + get '/remove_default_header' + assert_includes headers, 'X-Content-Type-Options' + assert_not_includes headers, header_to_remove, "Should not contain removed default header" + ensure + ActionDispatch::Response.default_headers = original_default_headers + end + end + end + private def with_test_route_set with_routing do |set| diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb index 3e0bfe8d14..ea2d35c3f8 100644 --- a/actionpack/test/controller/request_forgery_protection_test.rb +++ b/actionpack/test/controller/request_forgery_protection_test.rb @@ -103,6 +103,31 @@ class RequestForgeryProtectionControllerUsingNullSession < ActionController::Bas end end +class PrependProtectForgeryBaseController < ActionController::Base + before_action :custom_action + attr_accessor :called_callbacks + + def index + render inline: 'OK' + end + + protected + + def add_called_callback(name) + @called_callbacks ||= [] + @called_callbacks << name + end + + + def custom_action + add_called_callback("custom_action") + end + + def verify_authenticity_token + add_called_callback("verify_authenticity_token") + end +end + class FreeCookieController < RequestForgeryProtectionControllerUsingResetSession self.allow_forgery_protection = false @@ -431,6 +456,41 @@ class RequestForgeryProtectionControllerUsingExceptionTest < ActionController::T end end +class PrependProtectForgeryBaseControllerTest < ActionController::TestCase + PrependTrueController = Class.new(PrependProtectForgeryBaseController) do + protect_from_forgery prepend: true + end + + PrependFalseController = Class.new(PrependProtectForgeryBaseController) do + protect_from_forgery prepend: false + end + + PrependDefaultController = Class.new(PrependProtectForgeryBaseController) do + protect_from_forgery + end + + def test_verify_authenticity_token_is_prepended + @controller = PrependTrueController.new + get :index + expected_callback_order = ["verify_authenticity_token", "custom_action"] + assert_equal(expected_callback_order, @controller.called_callbacks) + end + + def test_verify_authenticity_token_is_not_prepended + @controller = PrependFalseController.new + get :index + expected_callback_order = ["custom_action", "verify_authenticity_token"] + assert_equal(expected_callback_order, @controller.called_callbacks) + end + + def test_verify_authenticity_token_is_prepended_by_default + @controller = PrependDefaultController.new + get :index + expected_callback_order = ["verify_authenticity_token", "custom_action"] + assert_equal(expected_callback_order, @controller.called_callbacks) + end +end + class FreeCookieControllerTest < ActionController::TestCase def setup @controller = FreeCookieController.new diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 1371317e3c..6b5081b7a9 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -453,6 +453,9 @@ module ActiveRecord c.verify! end c + rescue + disconnect! + raise end end diff --git a/railties/test/generators/mailer_generator_test.rb b/railties/test/generators/mailer_generator_test.rb index 8d2d97f64f..febd2fd12e 100644 --- a/railties/test/generators/mailer_generator_test.rb +++ b/railties/test/generators/mailer_generator_test.rb @@ -26,8 +26,8 @@ class MailerGeneratorTest < Rails::Generators::TestCase def test_mailer_with_i18n_helper run_generator assert_file "app/mailers/notifier_mailer.rb" do |mailer| - assert_match(/en\.notifier\.foo\.subject/, mailer) - assert_match(/en\.notifier\.bar\.subject/, mailer) + assert_match(/en\.notifier_mailer\.foo\.subject/, mailer) + assert_match(/en\.notifier_mailer\.bar\.subject/, mailer) end end @@ -126,7 +126,7 @@ class MailerGeneratorTest < Rails::Generators::TestCase run_generator ["Farm::Animal", "moos"] assert_file "app/mailers/farm/animal_mailer.rb" do |mailer| assert_match(/class Farm::AnimalMailer < ApplicationMailer/, mailer) - assert_match(/en\.farm\.animal\.moos\.subject/, mailer) + assert_match(/en\.farm\.animal_mailer\.moos\.subject/, mailer) end assert_file "test/mailers/previews/farm/animal_mailer_preview.rb" do |preview| assert_match(/\# Preview all emails at http:\/\/localhost\:3000\/rails\/mailers\/farm\/animal/, preview) diff --git a/railties/test/generators/namespaced_generators_test.rb b/railties/test/generators/namespaced_generators_test.rb index a4dad1f2b4..d0ea01dfb0 100644 --- a/railties/test/generators/namespaced_generators_test.rb +++ b/railties/test/generators/namespaced_generators_test.rb @@ -156,8 +156,8 @@ class NamespacedMailerGeneratorTest < NamespacedGeneratorTestCase def test_mailer_with_i18n_helper run_generator assert_file "app/mailers/test_app/notifier_mailer.rb" do |mailer| - assert_match(/en\.notifier\.foo\.subject/, mailer) - assert_match(/en\.notifier\.bar\.subject/, mailer) + assert_match(/en\.notifier_mailer\.foo\.subject/, mailer) + assert_match(/en\.notifier_mailer\.bar\.subject/, mailer) end end |