unify request throttle setting

Change-Id: If879c029b32ef8274052c5fbfa3ce216cc7b69ed
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/236598
Reviewed-by: Cody Cutrer <cody@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>

app/middleware/request_throttle.rb

@@ -97,24 +97,21 @@ class RequestThrottle
   end
 
   def allowed?(request, bucket)
-    unless self.class.enabled?
-      return true
-    end
-
     if whitelisted?(request)
       return true
     elsif blacklisted?(request)
+      # blacklisting is useful even if throttling is disabled, this is left in intentionally
       Rails.logger.info("blocking request due to blacklist, client id: #{client_identifiers(request).inspect} ip: #{request.remote_ip}")
       InstStatsd::Statsd.increment("request_throttling.blacklisted")
       return false
     else
       if bucket.full?
-        InstStatsd::Statsd.increment("request_throttling.throttled")
-        if Setting.get("request_throttle.enabled", "true") == "true"
+        if RequestThrottle.enabled?
+          InstStatsd::Statsd.increment("request_throttling.throttled")
           Rails.logger.info("blocking request due to throttling, client id: #{client_identifier(request)} bucket: #{bucket.to_json}")
           return false
         else
-          Rails.logger.info("would block request due to throttling, client id: #{client_identifier(request)} bucket: #{bucket.to_json}")
+          Rails.logger.info("WOULD HAVE blocked request due to throttling, client id: #{client_identifier(request)} bucket: #{bucket.to_json}")
         end
       end
       return true
@@ -179,7 +176,7 @@ class RequestThrottle
   end
 
   def self.enabled?
-    Setting.get("request_throttle.skip", "false") != 'true'
+    Setting.get("request_throttle.enabled", "true") == 'true'
   end
 
   def self.list_from_setting(key)

spec/lib/canvas/request_throttle_spec.rb

@@ -92,6 +92,10 @@ describe 'RequestThrottle' do
   end
 
   describe "#call" do
+    after(:each) do
+      Setting.remove("request_throttle.enabled")
+    end
+
     def set_blacklist(val)
       Setting.set('request_throttle.blacklist', val)
       RequestThrottle.reload!
@@ -128,6 +132,14 @@ describe 'RequestThrottle' do
       expect(throttler.call(request_user_2)).to eq rate_limit_exceeded
     end
 
+    it "still gets blacklisted if throttling disabled" do
+      Setting.set("request_throttle.enabled", "false")
+      expect(RequestThrottle.enabled?).to eq(false)
+      set_blacklist('user:2')
+      expect(strip_variable_headers(throttler.call(request_user_1))).to eq response
+      expect(throttler.call(request_user_2)).to eq rate_limit_exceeded
+    end
+
     it "should blacklist based on access token" do
       set_blacklist("token:#{AccessToken.hashed_token(token2.full_token)}")
       expect(strip_variable_headers(throttler.call(request_query_token))).to eq response