Merge pull request #541 from 3scale/resp-and-reject-reason-in-proxy

Extract rejection reason in Proxy instead of CacheHandler

CHANGELOG.md

@@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Extract Test::APIcast to own package on CPAN [PR #528](https://github.com/3scale/apicast/pull/528)
 - Load policies by the APIcast loader instead of changing load path [PR #532](https://github.com/3scale/apicast/pull/532), [PR #536](https://github.com/3scale/apicast/pull/536)
 - Add `src` directory to the Lua load path when using CLI [PR #533](https://github.com/3scale/apicast/pull/533)
+- Move rejection reason parsing from CacheHandler to Proxy [PR #541](https://github.com/3scale/apicast/pull/541)
 
 ## [3.2.0-alpha2] - 2017-11-30
 

gateway/src/apicast/backend/cache_handler.lua

@@ -18,14 +18,6 @@ local mt = {
   end,
 }
 
--- Returns the rejection reason from the headers of a 3scale backend response.
--- The header is set only when the authrep call to backend enables the option
--- to get the rejection reason. This is specified in the '3scale-options'
--- header of the request.
-local function rejection_reason(response_headers)
-  return response_headers and response_headers['3scale-rejection-reason']
-end
-
 function _M.new(handler)
   local name = handler or _M.handlers.default
   ngx.log(ngx.DEBUG, 'backend cache handler: ', name)
@@ -51,12 +43,9 @@ function _M.handlers.strict(cache, cached_key, response, ttl)
       ngx.log(ngx.INFO, 'apicast cache write key: ', cached_key, ', ttl: ', ttl, ' sub: ')
       cache:set(cached_key, 200, ttl or 0)
     end
-
-    return true
   else
     ngx.log(ngx.NOTICE, 'apicast cache delete key: ', cached_key, ' cause status ', response.status)
     cache:delete(cached_key)
-    return false, rejection_reason(response.headers)
   end
 end
 
@@ -67,9 +56,6 @@ function _M.handlers.resilient(cache, cached_key, response, ttl)
     ngx.log(ngx.INFO, 'apicast cache write key: ', cached_key, ' status: ', status, ', ttl: ', ttl )
 
     cache:set(cached_key, status, ttl or 0)
-
-    local authorized = (status == 200)
-    return authorized, (not authorized and rejection_reason(response.headers))
   end
 end
 

gateway/src/apicast/proxy.lua

@@ -359,10 +359,23 @@ function _M:post_action(force)
   end
 end
 
+-- Returns the rejection reason from the headers of a 3scale backend response.
+-- The header is set only when the authrep call to backend enables the option
+-- to get the rejection reason. This is specified in the '3scale-options'
+-- header of the request.
+local function rejection_reason(response_headers)
+  return response_headers and response_headers['3scale-rejection-reason']
+end
+
 function _M:handle_backend_response(cached_key, response, ttl)
   ngx.log(ngx.DEBUG, '[backend] response status: ', response.status, ' body: ', response.body)
 
-  return self.cache_handler(self.cache, cached_key, response, ttl)
+  self.cache_handler(self.cache, cached_key, response, ttl)
+
+  local authorized = (response.status == 200)
+  local unauthorized_reason = not authorized and rejection_reason(response.headers)
+
+  return authorized, unauthorized_reason
 end
 
 if custom_config then

spec/backend/cache_handler_spec.lua

@@ -1,12 +1,7 @@
-
 local _M = require('apicast.backend.cache_handler')
 local lrucache = require('resty.lrucache')
 
-local response = require('resty.http_ng.response')
-
-
 describe('Cache Handler', function()
-
   describe('.new', function()
     it('has a default handler', function()
       local handler = _M.new()
@@ -37,7 +32,7 @@ describe('Cache Handler', function()
       local cache = lrucache.new(1)
       ngx.var = { cached_key = nil }
 
-      assert.truthy(handler(cache, 'foobar', { status = 200 }))
+      handler(cache, 'foobar', { status = 200 })
 
       assert.equal(200, cache:get('foobar'))
     end)
@@ -46,7 +41,7 @@ describe('Cache Handler', function()
       local cache = lrucache.new(1)
       ngx.var = { cached_key = 'foobar' } -- means it is performed in post_action
 
-      assert.truthy(handler(cache, 'foobar', { status = 200 }))
+      handler(cache, 'foobar', { status = 200 })
 
       assert.falsy(cache:get('foobar'))
     end)
@@ -55,7 +50,7 @@ describe('Cache Handler', function()
       local cache = lrucache.new(1)
       cache:set('foobar', 200)
 
-      assert.falsy(handler(cache, 'foobar', { status = 403 }))
+      handler(cache, 'foobar', { status = 403 })
 
       assert.falsy(cache:get('foobar'))
     end)
@@ -64,47 +59,35 @@ describe('Cache Handler', function()
       local cache = lrucache.new(1)
       cache:set('foobar', 200)
 
-      assert.falsy(handler(cache, 'foobar', { status = 503 }))
+      handler(cache, 'foobar', { status = 503 })
 
       assert.falsy(cache:get('foobar'))
     end)
-
-    it('returns a rejection reason when given', function()
-      local cache = lrucache.new(1)
-
-      local authorized, rejection_reason = handler(
-        cache, 'foobar', response.new(nil, 403, { ['3scale-rejection-reason'] = 'some_reason' }, ''))
-
-      assert.falsy(authorized)
-      assert.equal('some_reason', rejection_reason)
-      assert.falsy(cache:get('foobar'))
-    end)
   end)
 
-
   describe('resilient', function()
     local handler = _M.handlers.resilient
 
     it('caches successful response', function()
       local cache = lrucache.new(1)
 
-      assert.truthy(handler(cache, 'foobar', { status = 200 }))
+      handler(cache, 'foobar', { status = 200 })
 
       assert.equal(200, cache:get('foobar'))
     end)
 
     it('caches forbidden response', function()
       local cache = lrucache.new(1)
 
-      assert.falsy(handler(cache, 'foobar', { status = 403 }))
+      handler(cache, 'foobar', { status = 403 })
 
       assert.equal(403, cache:get('foobar'))
     end)
 
     it('not caches server errors', function()
       local cache = lrucache.new(1)
 
-      assert.falsy(handler(cache, 'foobar', { status = 503 }))
+      handler(cache, 'foobar', { status = 503 })
 
       assert.falsy(cache:get('foobar'))
     end)
@@ -114,21 +97,10 @@ describe('Cache Handler', function()
 
       cache:set('foobar', 200)
 
-      assert.falsy(handler(cache, 'foobar', { status = 503 }))
+      handler(cache, 'foobar', { status = 503 })
 
       assert.equal(200, cache:get('foobar'))
     end)
-
-    it('returns a rejection reason when given', function()
-      local cache = lrucache.new(1)
-
-      local authorized, rejection_reason = handler(
-        cache, 'foobar', response.new(nil, 403, { ['3scale-rejection-reason'] = 'some_reason' }, ''))
-
-      assert.falsy(authorized)
-      assert.equal('some_reason', rejection_reason)
-      assert.equal(403, cache:get('foobar'))
-    end)
   end)
 
 end)

spec/proxy_spec.lua

@@ -1,3 +1,6 @@
+local http_ng_response = require('resty.http_ng.response')
+local lrucache = require('resty.lrucache')
+
 local configuration_store = require 'apicast.configuration_store'
 local Service = require 'apicast.configuration.service'
 
@@ -109,4 +112,16 @@ describe('Proxy', function()
       assert.spy(proxy.cache_handler).was.called_with(proxy.cache, 'client_id=blah:foo=0', response, nil)
     end)
   end)
+
+  describe('.handle_backend_response', function()
+    it('returns a rejection reason when given', function()
+      local authorized, rejection_reason = proxy:handle_backend_response(
+        lrucache.new(1),
+        http_ng_response.new(nil, 403, { ['3scale-rejection-reason'] = 'some_reason' }, ''),
+        nil)
+
+      assert.falsy(authorized)
+      assert.equal('some_reason', rejection_reason)
+    end)
+  end)
 end)