Merge pull request #343 from 3scale/proxy-post_action-oauth

don't call post action for oauth routes

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
 

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,26 +67,45 @@ 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
 
   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
 
-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
 
-  local ok, err = fun()
+  if not post_action_proxy then
+    return nil, 'not initialized'
+  end
+
+  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

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
 

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',

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)

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()