From 73e1dcc7b9090c1a4191199e93bc0bd2afa96fc8 Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Wed, 29 Mar 2017 09:11:02 +0200 Subject: [PATCH 1/2] [apicast] use instance property for sharing data with post_action because there is just one instance of apicast module it can be used as global state but still thrown out when new instance is created in tests --- apicast/src/apicast.lua | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/apicast/src/apicast.lua b/apicast/src/apicast.lua index 9f0062a3b..37cc314ab 100644 --- a/apicast/src/apicast.lua +++ b/apicast/src/apicast.lua @@ -18,15 +18,17 @@ local mt = { __index = _M } --- So there is no way to use ngx.ctx between request and post_action. --- We somehow need to share the instance of the proxy between those. --- This table is used to store the proxy object with unique reqeust id key --- and removed in the post_action. -local post_action_proxy = {} - --- This is called when APIcast boots the master process. function _M.new() - return setmetatable({ configuration = configuration_store.new() }, mt) + return setmetatable({ + configuration = configuration_store.new(), + -- So there is no way to use ngx.ctx between request and post_action. + -- We somehow need to share the instance of the proxy between those. + -- This table is used to store the proxy object with unique reqeust id key + -- and removed in the post_action. Because it there is just one instance + -- of this module in each worker. + post_action_proxy = {} + }, mt) end function _M:init() @@ -65,8 +67,14 @@ function _M:rewrite() ngx.ctx.proxy = p end -function _M.post_action() +function _M:post_action() local request_id = ngx.var.original_request_id + local post_action_proxy = self.post_action_proxy + + if not post_action_proxy then + return nil, 'not initialized' + end + local p = ngx.ctx.proxy or post_action_proxy[request_id] post_action_proxy[request_id] = nil @@ -78,10 +86,15 @@ function _M.post_action() end end -function _M.access() +function _M:access() local p = ngx.ctx.proxy - local fun = p:call() -- proxy:access() or oauth handler + local post_action_proxy = self.post_action_proxy + + if not post_action_proxy then + return nil, 'not initialized' + end + local fun = p:call() -- proxy:access() or oauth handler local ok, err = fun() post_action_proxy[ngx.var.original_request_id] = p From 872b598bf11923da8ecdceddd72c1712e11e3930 Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Wed, 29 Mar 2017 09:12:16 +0200 Subject: [PATCH 2/2] [proxy] don't call post_actions for already handled requests for example oauth has own routes that can handle the request so there is no need to call post_action for them it should be called only when the access phase is evaluated --- CHANGELOG.md | 1 + apicast/src/apicast.lua | 16 +++++++++---- apicast/src/proxy.lua | 16 ++++++++++++- doc/config.ld | 1 + spec/apicast_spec.lua | 51 ++++++++++++++++++++++++++++++++++++++--- spec/proxy_spec.lua | 34 ++++++++++++++++++++++----- 6 files changed, 105 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc6d5185f..2a3de531e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Use `RESOLVER` before falling back to `resolv.conf` [PR #324](https://github.com/3scale/apicast/pull/324) - Improve error logging when failing to download configuration [PR #335](https://github.com/3scale/apicast/pull/325) - Service hostnames are normalized to lower case [PR #336](https://github.com/3scale/apicast/pull/326) +- Don't attempt to perform post\_action when request was handled without authentication [PR #343](https://github.com/3scale/apicast/pull/343) ### Fixed diff --git a/apicast/src/apicast.lua b/apicast/src/apicast.lua index 37cc314ab..bcc31fdc5 100644 --- a/apicast/src/apicast.lua +++ b/apicast/src/apicast.lua @@ -80,9 +80,10 @@ function _M:post_action() post_action_proxy[request_id] = nil if p then - p:post_action() + return p:post_action() else ngx.log(ngx.INFO, 'could not find proxy for request id: ', request_id) + return nil, 'no proxy for request' end end @@ -94,10 +95,17 @@ function _M:access() return nil, 'not initialized' end - local fun = p:call() -- proxy:access() or oauth handler - local ok, err = fun() + local access, handler = p:call() -- proxy:access() or oauth handler - post_action_proxy[ngx.var.original_request_id] = p + local ok, err + + if access then + ok, err = access() + post_action_proxy[ngx.var.original_request_id] = p + elseif handler then + ok, err = handler() + -- no proxy because that would trigger post action + end return ok, err end diff --git a/apicast/src/proxy.lua b/apicast/src/proxy.lua index 1a1a8a1b5..e6d03d55e 100644 --- a/apicast/src/proxy.lua +++ b/apicast/src/proxy.lua @@ -1,3 +1,11 @@ +------------ +-- Proxy +-- Module that handles the request authentication and proxying upstream. +-- +-- @module proxy +-- @author mikz +-- @license Apache License Version 2.0 + local env = require 'resty.env' local custom_config = env.get('APICAST_CUSTOM_CONFIG') local configuration_store = require 'configuration_store' @@ -266,6 +274,12 @@ function _M:set_backend_upstream(service) ngx.var.backend_host = backend.host or server or ngx.var.backend_host end +----- +-- call the proxy and return a handler function +-- that will perform an action based on the path and backend version +-- @string host optional hostname, uses `ngx.var.host` otherwise +-- @treturn nil|function access function (when the request needs to be authenticated with this) +-- @treturn nil|function handler function (when the request is not authenticated and has some own action) function _M:call(host) host = host or ngx.var.host local service = ngx.ctx.service or self:set_service(host) @@ -279,7 +293,7 @@ function _M:call(host) if f then ngx.log(ngx.DEBUG, 'apicast oauth flow') - return function() return f(params) end + return nil, function() return f(params) end end end diff --git a/doc/config.ld b/doc/config.ld index 3ca0a4786..e2badb188 100644 --- a/doc/config.ld +++ b/doc/config.ld @@ -1,4 +1,5 @@ file = { + '../apicast/src/proxy.lua', '../apicast/src/configuration/service.lua', '../apicast/src/resty/env.lua', '../apicast/src/resty/http_ng.lua', diff --git a/spec/apicast_spec.lua b/spec/apicast_spec.lua index a2e7c6f85..cd4ab904a 100644 --- a/spec/apicast_spec.lua +++ b/spec/apicast_spec.lua @@ -1,12 +1,57 @@ -local apicast = require 'apicast' +local _M = require 'apicast' describe('APIcast module', function() it('has a name', function() - assert.truthy(apicast._NAME) + assert.truthy(_M._NAME) end) it('has a version', function() - assert.truthy(apicast._VERSION) + assert.truthy(_M._VERSION) + end) + + describe(':access', function() + + local apicast + + before_each(function() + apicast = _M.new() + ngx.ctx.proxy = {} + end) + + it('triggers post action when access phase succeeds', function() + ngx.var = { original_request_id = 'foobar' } + + stub(ngx.ctx.proxy, 'call', function() + return function() return 'ok' end + end) + + stub(ngx.ctx.proxy, 'post_action', function() + return 'post_ok' + end) + + local ok, err = apicast:access() + + assert.same('ok', ok) + assert.falsy(err) + + assert.same('post_ok', apicast:post_action()) + end) + + it('skips post action when access phase not executed', function() + stub(ngx.ctx.proxy, 'call', function() + -- return nothing for example + end) + + local ok, err = apicast:access() + + assert.falsy(ok) + assert.falsy(err) + + ngx.ctx.proxy = nil -- in post_action the ctx is not shared + ngx.var = { original_request_id = 'foobar' } + + assert.equal(nil, apicast:post_action()) + end) end) end) diff --git a/spec/proxy_spec.lua b/spec/proxy_spec.lua index b3185c729..48e5e51b4 100644 --- a/spec/proxy_spec.lua +++ b/spec/proxy_spec.lua @@ -13,14 +13,36 @@ describe('Proxy', function() assert.same('function', type(proxy.access)) end) - it('has authorize function after call', function() - ngx.var = { backend_endpoint = 'http://localhost:1853' } - configuration:add({ id = 42, hosts = { 'localhost' }}) + describe(':call', function() + before_each(function() + ngx.var = { backend_endpoint = 'http://localhost:1853' } + configuration:add({ id = 42, hosts = { 'localhost' }}) + end) + + it('has authorize function after call', function() + proxy:call('localhost') - proxy:call('localhost') + assert.truthy(proxy.authorize) + assert.same('function', type(proxy.authorize)) + end) - assert.truthy(proxy.authorize) - assert.same('function', type(proxy.authorize)) + it('returns access function', function() + local access = proxy:call('localhost') + + assert.same('function', type(access)) + end) + + it('returns oauth handler when matches oauth route', function() + local service = configuration:find_by_id(42) + service.backend_version = 'oauth' + stub(ngx.req, 'get_method', function() return 'GET' end) + ngx.var.uri = '/authorize' + + local access, handler = proxy:call('localhost') + + assert.equal(nil, access) + assert.same('function', type(handler)) + end) end) it('has post_action function', function()