Merge branch '255-correct-object-for-instrumentation'

README.md

@@ -252,7 +252,8 @@ Rack::Attack.track("special_agent", limit: 6, period: 60) do |req|
 end
 
 # Track it using ActiveSupport::Notification
-ActiveSupport::Notifications.subscribe("rack.attack") do |name, start, finish, request_id, req|
+ActiveSupport::Notifications.subscribe("rack.attack") do |name, start, finish, request_id, payload|
+  req = payload[:request]
   if req.env['rack.attack.matched'] == "special_agent" && req.env['rack.attack.match_type'] == :track
     Rails.logger.info "special_agent: #{req.path}"
     STATSD.increment("special_agent")
@@ -330,8 +331,8 @@ Rack::Attack uses the [ActiveSupport::Notifications](http://api.rubyonrails.org/
 You can subscribe to 'rack.attack' events and log it, graph it, etc:
 
 ```ruby
-ActiveSupport::Notifications.subscribe('rack.attack') do |name, start, finish, request_id, req|
-  puts req.inspect
+ActiveSupport::Notifications.subscribe('rack.attack') do |name, start, finish, request_id, payload|
+  puts payload[:request].inspect
 end
 ```
 

lib/rack/attack.rb

@@ -114,7 +114,7 @@ class Rack::Attack
     end
 
     def instrument(request)
-      notifier.instrument('rack.attack', request) if notifier
+      notifier.instrument('rack.attack', request: request) if notifier
     end
 
     def cache

spec/acceptance/blocking_ip_spec.rb

@@ -21,9 +21,9 @@ describe "Blocking an IP" do
     notified = false
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
       notified = true
-      notification_type = request.env["rack.attack.match_type"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"

spec/acceptance/blocking_spec.rb

@@ -23,9 +23,9 @@ describe "#blocklist" do
     notification_matched = nil
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_matched = request.env["rack.attack.matched"]
-      notification_type = request.env["rack.attack.match_type"]
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_matched = payload[:request].env["rack.attack.matched"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"

spec/acceptance/blocking_subnet_spec.rb

@@ -27,9 +27,9 @@ describe "Blocking an IP subnet" do
     notified = false
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
       notified = true
-      notification_type = request.env["rack.attack.match_type"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"

spec/acceptance/safelisting_ip_spec.rb

@@ -36,8 +36,8 @@ describe "Safelist an IP" do
   it "notifies when the request is safe" do
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_type = request.env["rack.attack.match_type"]
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/admin", {}, "REMOTE_ADDR" => "5.6.7.8"

spec/acceptance/safelisting_spec.rb

@@ -39,9 +39,9 @@ describe "#safelist" do
     notification_matched = nil
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_matched = request.env["rack.attack.matched"]
-      notification_type = request.env["rack.attack.match_type"]
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_matched = payload[:request].env["rack.attack.matched"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/safe_space", {}, "REMOTE_ADDR" => "1.2.3.4"

spec/acceptance/safelisting_subnet_spec.rb

@@ -36,8 +36,8 @@ describe "Safelisting an IP subnet" do
   it "notifies when the request is safe" do
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_type = request.env["rack.attack.match_type"]
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/admin", {}, "REMOTE_ADDR" => "5.6.0.0"

spec/acceptance/throttling_spec.rb

@@ -123,11 +123,11 @@ describe "#throttle" do
     notification_data = nil
     notification_discriminator = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_matched = request.env["rack.attack.matched"]
-      notification_type = request.env["rack.attack.match_type"]
-      notification_data = request.env['rack.attack.match_data']
-      notification_discriminator = request.env['rack.attack.match_discriminator']
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_matched = payload[:request].env["rack.attack.matched"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
+      notification_data = payload[:request].env['rack.attack.match_data']
+      notification_discriminator = payload[:request].env['rack.attack.match_discriminator']
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"

spec/acceptance/track_spec.rb

@@ -9,9 +9,9 @@ describe "#track" do
     notification_matched = nil
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_matched = request.env["rack.attack.matched"]
-      notification_type = request.env["rack.attack.match_type"]
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_matched = payload[:request].env["rack.attack.matched"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"

spec/acceptance/track_throttle_spec.rb

@@ -12,9 +12,9 @@ describe "#track with throttle-ish options" do
     notification_matched = nil
     notification_type = nil
 
-    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, request|
-      notification_matched = request.env["rack.attack.matched"]
-      notification_type = request.env["rack.attack.match_type"]
+    ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
+      notification_matched = payload[:request].env["rack.attack.matched"]
+      notification_type = payload[:request].env["rack.attack.match_type"]
     end
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"

spec/rack_attack_instrumentation_spec.rb

@@ -0,0 +1,31 @@
+# ActiveSupport::Subscribers added in ~> 4.0.2.0
+if ActiveSupport::VERSION::MAJOR > 3
+  require_relative 'spec_helper'
+  require 'active_support/subscriber'
+  class CustomSubscriber < ActiveSupport::Subscriber
+    def rack(event)
+      # Do virtually (but not) nothing.
+      event.inspect
+    end
+  end
+
+  describe 'Rack::Attack.instrument' do
+    before do
+      @period = 60 # Use a long period; failures due to cache key rotation less likely
+      Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
+      Rack::Attack.throttle('ip/sec', limit: 1, period: @period) { |req| req.ip }
+    end
+
+    describe "with throttling" do
+      before do
+        ActiveSupport::Notifications.stub(:notifier, ActiveSupport::Notifications::Fanout.new) do
+          CustomSubscriber.attach_to("attack")
+          2.times { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' }
+        end
+      end
+      it 'should instrument without error' do
+        last_response.status.must_equal 429
+      end
+    end
+  end
+end