diff --git a/CHANGELOG.md b/CHANGELOG.md index 446e4f77d..2442d80c1 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/gateway/src/apicast/backend/cache_handler.lua b/gateway/src/apicast/backend/cache_handler.lua index d9a9dcb79..475a3b79c 100644 --- a/gateway/src/apicast/backend/cache_handler.lua +++ b/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 diff --git a/gateway/src/apicast/proxy.lua b/gateway/src/apicast/proxy.lua index 8c6a56c41..06683792c 100644 --- a/gateway/src/apicast/proxy.lua +++ b/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 diff --git a/spec/backend/cache_handler_spec.lua b/spec/backend/cache_handler_spec.lua index 97eed4b57..4a752bba1 100644 --- a/spec/backend/cache_handler_spec.lua +++ b/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,31 +59,19 @@ 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) @@ -96,7 +79,7 @@ describe('Cache Handler', function() 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) @@ -104,7 +87,7 @@ describe('Cache Handler', function() 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) diff --git a/spec/proxy_spec.lua b/spec/proxy_spec.lua index af01c93aa..17c04b6ea 100644 --- a/spec/proxy_spec.lua +++ b/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)