diff --git a/app/middleware/request_throttle.rb b/app/middleware/request_throttle.rb index 03439e98b53..caeae244ac4 100644 --- a/app/middleware/request_throttle.rb +++ b/app/middleware/request_throttle.rb @@ -111,7 +111,7 @@ class RequestThrottle Rails.logger.info("blocking request due to throttling, client id: #{client_identifier(request)} bucket: #{bucket.to_json}") return false else - Rails.logger.info("WOULD HAVE blocked request due to throttling, client id: #{client_identifier(request)} bucket: #{bucket.to_json}") + Rails.logger.info("WOULD HAVE throttled request (config disabled), client id: #{client_identifier(request)} bucket: #{bucket.to_json}") end end return true @@ -308,12 +308,10 @@ class RequestThrottle # expecting the block to return the final cost. It then increments again, # subtracting the initial up_front_cost from the final cost to erase it. def reserve_capacity(up_front_cost = self.up_front_cost) - return (self.count = yield) unless RequestThrottle.enabled? - increment(0, up_front_cost) cost = yield ensure - increment(cost || 0, -up_front_cost) if RequestThrottle.enabled? + increment(cost || 0, -up_front_cost) end def full? diff --git a/spec/lib/canvas/request_throttle_spec.rb b/spec/lib/canvas/request_throttle_spec.rb index 23d99b5030b..d4e90420dc1 100644 --- a/spec/lib/canvas/request_throttle_spec.rb +++ b/spec/lib/canvas/request_throttle_spec.rb @@ -107,7 +107,7 @@ describe 'RequestThrottle' do expect(strip_variable_headers(throttler.call(request_user_1))).to eq response end - it "should have headers even when disabled" do + it "should have headers even when disabled including cost tracking" do allow(RequestThrottle).to receive(:enabled?).and_return(false) allow(throttler).to receive(:calculate_cost).and_return(30) @@ -115,7 +115,11 @@ describe 'RequestThrottle' do expected[1]['X-Request-Cost'] = '30' # hwm of 600 - cost of the request expected[1]['X-Rate-Limit-Remaining'] = '570.0' - expect(throttler.call(request_user_1)).to eq expected + output = throttler.call(request_user_1) + expect(output[1]['X-Request-Cost']).to eq('30') + remaining = output[1]['X-Rate-Limit-Remaining'].to_f + expect(remaining > 570.0).to be_truthy + expect(remaining < 580.0).to be_truthy end it "should blacklist based on ip" do @@ -414,12 +418,21 @@ describe 'RequestThrottle' do expect(@bucket.get_up_front_cost_for_path("/somethingelse")).to eq @bucket.up_front_cost end - it "does nothing if disabled" do - expect(RequestThrottle).to receive(:enabled?).twice.and_return(false) - expect(@bucket).to receive(:increment).never + it "still tracks cost when disabled (for debugging)" do + allow(RequestThrottle).to receive(:enabled?).and_return(false) + expect(@bucket).to receive(:increment).twice @bucket.reserve_capacity {} end + it "will always be allowed when disabled, even with full bucket" do + allow(RequestThrottle).to receive(:enabled?).and_return(false) + req = request_logged_out + allow(req).to receive(:fullpath).and_return("/") + allow(req).to receive(:env).and_return({'canvas.request_throttle.user_id' => ['123']}) + allow(@bucket).to receive(:full?).and_return(true) + expect(throttler.allowed?(request_logged_out, @bucket)).to be_truthy + end + after do Timecop.safe_mode = true end