fix(plugins): multiple auth fix in an OR scenario (#2222)

* fix(plugins): multiple auth fix in an OR scenario

The last auth plugin running would overritw the results of the previous ones.
So if the first authenticates succesfully, and the second fails, then the
second one would overwrite the consumer details with the details of the
anonymous consumer

CHANGELOG.md

@@ -112,6 +112,9 @@
 - Relax multipart MIME type parsing. A space is allowed in between values
   of the Content-Type header.
   [#2215](https://github.com/Mashape/kong/pull/2215)
+- Multiple auth plugins would overwrite eachothers results, causing 
+  false negatives in an OR scenario.
+  [#2222](https://github.com/Mashape/kong/pull/2222)
 - Admin API:
   - Better handling of non-supported HTTP methods on endpoints of the Admin
     API. In some cases this used to throw an internal error. Calling any

kong/plugins/basic-auth/access.lua

@@ -147,7 +147,15 @@ local function do_authentication(conf)
   return true
 end
 
+
 function _M.execute(conf)
+
+  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
+    -- we're already authenticated, and we're configured for using anonymous, 
+    -- hence we're in a logical OR between auth methods and we're already done.
+    return
+  end
+
   local ok, err = do_authentication(conf)
   if not ok then
     if conf.anonymous ~= "" then
@@ -164,4 +172,5 @@ function _M.execute(conf)
   end
 end
 
+
 return _M

kong/plugins/hmac-auth/access.lua

@@ -216,7 +216,15 @@ local function do_authentication(conf)
   return true
 end
 
+
 function _M.execute(conf)
+
+  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
+    -- we're already authenticated, and we're configured for using anonymous, 
+    -- hence we're in a logical OR between auth methods and we're already done.
+    return
+  end
+
   local ok, err = do_authentication(conf)
   if not ok then
     if conf.anonymous ~= "" then
@@ -233,4 +241,5 @@ function _M.execute(conf)
   end
 end
 
+
 return _M

kong/plugins/jwt/handler.lua

@@ -168,9 +168,16 @@ local function do_authentication(conf)
   return true
 end
 
+
 function JwtHandler:access(conf)
   JwtHandler.super.access(self)
 
+  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
+    -- we're already authenticated, and we're configured for using anonymous, 
+    -- hence we're in a logical OR between auth methods and we're already done.
+    return
+  end
+
   local ok, err = do_authentication(conf)
   if not ok then
     if conf.anonymous ~= "" then
@@ -187,4 +194,5 @@ function JwtHandler:access(conf)
   end
 end
 
+
 return JwtHandler

kong/plugins/key-auth/handler.lua

@@ -125,9 +125,16 @@ local function do_authentication(conf)
   return true
 end
 
+
 function KeyAuthHandler:access(conf)
   KeyAuthHandler.super.access(self)
 
+  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
+    -- we're already authenticated, and we're configured for using anonymous, 
+    -- hence we're in a logical OR between auth methods and we're already done.
+    return
+  end
+
   local ok, err = do_authentication(conf)
   if not ok then
     if conf.anonymous ~= "" then
@@ -144,4 +151,5 @@ function KeyAuthHandler:access(conf)
   end
 end
 
+
 return KeyAuthHandler

kong/plugins/ldap-auth/access.lua

@@ -102,20 +102,28 @@ local function load_consumer(consumer_id, anonymous)
 end
 
 local function set_consumer(consumer, credential)
+
   if consumer then
+    -- this can only be the Anonymous user in this case
     ngx_set_header(constants.HEADERS.CONSUMER_ID, consumer.id)
     ngx_set_header(constants.HEADERS.CONSUMER_CUSTOM_ID, consumer.custom_id)
     ngx_set_header(constants.HEADERS.CONSUMER_USERNAME, consumer.username)
-  end
-  ngx.ctx.authenticated_consumer = consumer
-  if credential then
-    ngx_set_header(constants.HEADERS.CREDENTIAL_USERNAME, credential.username)
-    ngx.ctx.authenticated_credential = credential
-    ngx_set_header(constants.HEADERS.ANONYMOUS, nil) -- in case of auth plugins concatenation
-  else
     ngx_set_header(constants.HEADERS.ANONYMOUS, true)
+    ngx.ctx.authenticated_consumer = consumer
+    return
   end
 
+  -- here we have been authenticated by ldap
+  ngx_set_header(constants.HEADERS.CREDENTIAL_USERNAME, credential.username)
+  ngx.ctx.authenticated_credential = credential
+
+  -- in case of auth plugins concatenation, remove remnants of anonymous
+  ngx.ctx.authenticated_consumer = nil
+  ngx_set_header(constants.HEADERS.ANONYMOUS, nil)
+  ngx_set_header(constants.HEADERS.CONSUMER_ID, nil)
+  ngx_set_header(constants.HEADERS.CONSUMER_CUSTOM_ID, nil)
+  ngx_set_header(constants.HEADERS.CONSUMER_USERNAME, nil)
+
 end
 
 local function do_authentication(conf)
@@ -148,7 +156,15 @@ local function do_authentication(conf)
   return true
 end
 
+
 function _M.execute(conf)
+
+  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
+    -- we're already authenticated, and we're configured for using anonymous, 
+    -- hence we're in a logical OR between auth methods and we're already done.
+    return
+  end
+
   local ok, err = do_authentication(conf)
   if not ok then
     if conf.anonymous ~= "" then
@@ -165,4 +181,5 @@ function _M.execute(conf)
   end
 end
 
+
 return _M

kong/plugins/oauth2/access.lua

@@ -525,8 +525,17 @@ local function do_authentication(conf)
   return true
 end
 
+
 function _M.execute(conf)
+
+  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
+    -- we're already authenticated, and we're configured for using anonymous, 
+    -- hence we're in a logical OR between auth methods and we're already done.
+    return
+  end
+
   if ngx.req.get_method() == "POST" then
+
     local from, _, err = ngx.re.find(ngx.var.uri, [[\/oauth2\/token]], "oj")
     if err then
       ngx.log(ngx.ERR, "could not search for token path segment: ", err)
@@ -559,10 +568,12 @@ function _M.execute(conf)
         return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
       end
       set_consumer(consumer, nil, nil)
+
     else
       return responses.send(err.status, err.message, err.headers)
     end
   end
 end
 
+
 return _M

spec/03-plugins/10-key-auth/02-access_spec.lua

@@ -244,3 +244,198 @@ describe("Plugin: key-auth (access)", function()
     end)
   end)
 end)
+
+
+describe("Plugin: key-auth (access)", function()
+
+  local client, user1, user2, anonymous
+
+  setup(function()
+    local api1 = assert(helpers.dao.apis:insert {
+      name = "api-1",
+      hosts = { "logical-and.com" },
+      upstream_url = "http://mockbin.org/request"
+    })
+    assert(helpers.dao.plugins:insert {
+      name = "basic-auth",
+      api_id = api1.id
+    })
+    assert(helpers.dao.plugins:insert {
+      name = "key-auth",
+      api_id = api1.id
+    })
+
+    anonymous = assert(helpers.dao.consumers:insert {
+      username = "Anonymous"
+    })
+    user1 = assert(helpers.dao.consumers:insert {
+      username = "Mickey"
+    })
+    user2 = assert(helpers.dao.consumers:insert {
+      username = "Aladdin"
+    })
+
+    local api2 = assert(helpers.dao.apis:insert {
+      name = "api-2",
+      hosts = { "logical-or.com" },
+      upstream_url = "http://mockbin.org/request"
+    })
+    assert(helpers.dao.plugins:insert {
+      name = "basic-auth",
+      api_id = api2.id,
+      config = {
+        anonymous = anonymous.id
+      }
+    })
+    assert(helpers.dao.plugins:insert {
+      name = "key-auth",
+      api_id = api2.id,
+      config = {
+        anonymous = anonymous.id
+      }
+    })
+
+    assert(helpers.dao.keyauth_credentials:insert {
+      key = "Mouse",
+      consumer_id = user1.id
+    })
+    assert(helpers.dao.basicauth_credentials:insert {
+      username = "Aladdin",
+      password = "OpenSesame",
+      consumer_id = user2.id
+    })
+
+    assert(helpers.start_kong())
+    client = helpers.proxy_client()
+  end)
+
+
+  teardown(function()
+    if client then client:close() end
+    helpers.stop_kong()
+  end)
+
+  describe("multiple auth without anonymous, logical AND", function()
+
+    it("passes with all credentials provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-and.com",
+          ["apikey"] = "Mouse",
+          ["Authorization"] = "Basic QWxhZGRpbjpPcGVuU2VzYW1l",
+        }
+      })
+      assert.response(res).has.status(200)
+      assert.request(res).has.no.header("x-anonymous-consumer")
+      local id = assert.request(res).has.header("x-consumer-id")
+      assert.not_equal(id, anonymous.id)
+      assert(id == user1.id or id == user2.id)
+    end)
+
+    it("fails 401, with only the first credential provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-and.com",
+          ["apikey"] = "Mouse",
+        }
+      })
+      assert.response(res).has.status(401)
+    end)
+
+    it("fails 401, with only the second credential provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-and.com",
+          ["Authorization"] = "Basic QWxhZGRpbjpPcGVuU2VzYW1l",
+        }
+      })
+      assert.response(res).has.status(401)
+    end)
+
+    it("fails 401, with no credential provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-and.com",
+        }
+      })
+      assert.response(res).has.status(401)
+    end)
+
+  end)
+
+  describe("multiple auth with anonymous, logical OR", function()
+
+    it("passes with all credentials provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-or.com",
+          ["apikey"] = "Mouse",
+          ["Authorization"] = "Basic QWxhZGRpbjpPcGVuU2VzYW1l",
+        }
+      })
+      assert.response(res).has.status(200)
+      assert.request(res).has.no.header("x-anonymous-consumer")
+      local id = assert.request(res).has.header("x-consumer-id")
+      assert.not_equal(id, anonymous.id)
+      assert(id == user1.id or id == user2.id)
+    end)
+
+    it("passes with only the first credential provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-or.com",
+          ["apikey"] = "Mouse",
+        }
+      })
+      assert.response(res).has.status(200)
+      assert.request(res).has.no.header("x-anonymous-consumer")
+      local id = assert.request(res).has.header("x-consumer-id")
+      assert.not_equal(id, anonymous.id)
+      assert.equal(user1.id, id)
+    end)
+
+    it("passes with only the second credential provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-or.com",
+          ["Authorization"] = "Basic QWxhZGRpbjpPcGVuU2VzYW1l",
+        }
+      })
+      assert.response(res).has.status(200)
+      assert.request(res).has.no.header("x-anonymous-consumer")
+      local id = assert.request(res).has.header("x-consumer-id")
+      assert.not_equal(id, anonymous.id)
+      assert.equal(user2.id, id)
+    end)
+
+    it("passes with no credential provided", function()
+      local res = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          ["Host"] = "logical-or.com",
+        }
+      })
+      assert.response(res).has.status(200)
+      assert.request(res).has.header("x-anonymous-consumer")
+      local id = assert.request(res).has.header("x-consumer-id")
+      assert.equal(id, anonymous.id)
+    end)
+
+  end)
+
+end)