From 6000cf0581c7f991991c1170f3154006611f151f Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Thu, 18 Jan 2018 15:56:00 +0100 Subject: [PATCH] [proxy] split proxy:call() into rewrite and access phase so policies can change the context to add credentials/usage before the authorize call to 3scale backend --- CHANGELOG.md | 1 + gateway/src/apicast/policy/apicast/policy.lua | 28 +++++------ gateway/src/apicast/proxy.lua | 46 ++++++++++--------- spec/policy/apicast/policy_spec.lua | 45 ------------------ spec/proxy_spec.lua | 36 +-------------- 5 files changed, 42 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd10920d8..d3f1dd844 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `post_action` phase now shares `ngx.ctx` with the main request [PR #539](https://github.com/3scale/apicast/pull/539) - Decrease nginx timer resolution to improve performance and enable PCRE JIT [PR #543](https://github.com/3scale/apicast/pull/543) - Moved `proxy_pass` into new internal location `@upstream` [PR #535](https://github.com/3scale/apicast/pull/535) +- Split 3scale authorization to rewrite and access phase [PR #556](https://github.com/3scale/apicast/pull/556) ## [3.2.0-alpha2] - 2017-11-30 diff --git a/gateway/src/apicast/policy/apicast/policy.lua b/gateway/src/apicast/policy/apicast/policy.lua index cdc3fe91c..3e4b51669 100644 --- a/gateway/src/apicast/policy/apicast/policy.lua +++ b/gateway/src/apicast/policy/apicast/policy.lua @@ -50,7 +50,17 @@ function _M:rewrite(context) p.cache_handler = context.cache_handler end - p.set_upstream(context.service) + local service = context.service + + if service then + ngx.ctx.service = service + + -- it is possible that proxy:rewrite will terminate the request + p:rewrite(service) + end + + p.set_upstream(service) + ngx.ctx.proxy = p end @@ -66,20 +76,12 @@ function _M:post_action(context) end function _M:access(context) - local p = context and context.proxy or ngx.ctx.proxy or self.proxy - ngx.ctx.service = context.service - - local access, handler = p:call(context.service) -- proxy:access() or oauth handler - - local ok, err + local ctx = ngx.ctx + local p = context and context.proxy or ctx.proxy or self.proxy - if access then - ok, err = access() - elseif handler then - ok, err = handler() + if p then + return p:access(context.service, context.usage, context.credentials, context.ttl) end - - return ok, err end _M.content = function() diff --git a/gateway/src/apicast/proxy.lua b/gateway/src/apicast/proxy.lua index 06683792c..a3789900b 100644 --- a/gateway/src/apicast/proxy.lua +++ b/gateway/src/apicast/proxy.lua @@ -126,6 +126,8 @@ local function output_debug_headers(service, usage, credentials) end function _M:authorize(service, usage, credentials, ttl) + if not usage or not credentials then return nil, 'missing usage or credentials' end + local encoded_usage = encode_args(usage) if encoded_usage == '' then return error_no_match(service) @@ -206,36 +208,31 @@ function _M.set_upstream(service) ngx.req.set_header('Host', upstream.host or ngx.var.host) end ------ --- call the proxy and return a handler function --- that will perform an action based on the path and backend version --- @tparam service service service object --- @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(service) - service = _M.set_service(service or ngx.ctx.service) - - self.oauth = service:oauth() +local function handle_oauth(service) + local oauth = service:oauth() - ngx.log(ngx.DEBUG, 'using OAuth: ', self.oauth) + if oauth then + ngx.log(ngx.DEBUG, 'using OAuth: ', oauth) + end - -- means that OAuth integration has own router - if self.oauth and self.oauth.call then - local f, params = self.oauth:call(service) + if oauth and oauth.call then + local f, params = oauth:call(service) if f then - ngx.log(ngx.DEBUG, 'apicast oauth flow') - return nil, function() return f(params) end + ngx.log(ngx.DEBUG, 'OAuth matched route') + return f(params) -- not really about the return value but showing something will call ngx.exit end end - return function() - -- call access phase - return self:access(service) - end + return oauth end -function _M:access(service) +function _M:rewrite(service) + service = _M.set_service(service or ngx.ctx.service) + + -- handle_oauth can terminate the request + self.oauth = handle_oauth(service) + ngx.var.secret_token = service.secret_token local credentials, err = service:extract_credentials() @@ -285,9 +282,14 @@ function _M:access(service) return error_authorization_failed(service) end ctx.credentials = credentials + ctx.ttl = ttl end +end + +function _M:access(service, usage, credentials, ttl) + local ctx = ngx.ctx - return self:authorize(service, usage_params, credentials, ttl) + return self:authorize(service, usage or ctx.usage, credentials or ctx.credentials, ttl or ctx.ttl) end local function response_codes_data() diff --git a/spec/policy/apicast/policy_spec.lua b/spec/policy/apicast/policy_spec.lua index affd6ff25..f81669719 100644 --- a/spec/policy/apicast/policy_spec.lua +++ b/spec/policy/apicast/policy_spec.lua @@ -9,49 +9,4 @@ describe('APIcast module', function() it('has a version', function() 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 17c04b6ea..53b847409 100644 --- a/spec/proxy_spec.lua +++ b/spec/proxy_spec.lua @@ -17,39 +17,7 @@ describe('Proxy', function() assert.same('function', type(proxy.access)) end) - describe(':call', function() - local service - before_each(function() - ngx.var = { backend_endpoint = 'http://localhost:1853' } - service = Service.new({ id = 42, hosts = { 'localhost' }}) - end) - - it('has authorize function after call', function() - proxy:call(service) - - assert.truthy(proxy.authorize) - assert.same('function', type(proxy.authorize)) - end) - - it('returns access function', function() - local access = proxy:call(service) - - assert.same('function', type(access)) - end) - - it('returns oauth handler when matches oauth route', function() - service.backend_version = 'oauth' - stub(ngx.req, 'get_method', function() return 'GET' end) - ngx.var.uri = '/authorize' - - local access, handler = proxy:call(service) - - assert.equal(nil, access) - assert.same('function', type(handler)) - end) - end) - - describe(':access', function() + describe(':rewrite', function() local service before_each(function() ngx.var = { backend_endpoint = 'http://localhost:1853', uri = '/a/uri' } @@ -61,7 +29,7 @@ describe('Proxy', function() service.credentials = { location = 'headers' } service.backend_version = 2 ngx.var.http_app_key = 'key' - assert.falsy(proxy:access(service)) + assert.falsy(proxy:rewrite(service)) end) end)