Merge pull request Kong#375 from Mashape/fix/auth-401

[fix/auth] reply 401 to missing credentials

Former-commit-id: ae502745158779366dc822e23c346ccd7246def5

kong/plugins/basicauth/access.lua

@@ -41,6 +41,9 @@ local function retrieve_credentials(request, conf)
       username = basic_parts[1]
       password = basic_parts[2]
     end
+  else
+    ngx.ctx.stop_phases = true
+    return responses.send_HTTP_UNAUTHORIZED()
   end
 
   if conf.hide_credentials then

kong/plugins/keyauth/access.lua

@@ -118,14 +118,13 @@ local retrieve_credentials = {
 function _M.execute(conf)
   if skip_authentication(ngx.req.get_headers()) then return end
 
-  local credential
+  local key, key_found, credential
   for _, v in ipairs({ constants.AUTHENTICATION.QUERY, constants.AUTHENTICATION.HEADER }) do
-    local key = retrieve_credentials[v](ngx.req, conf)
-
-    -- Make sure we are not sending an empty table to find_by_keys
+    key = retrieve_credentials[v](ngx.req, conf)
     if key then
+      key_found = true
       credential = cache.get_or_set(cache.keyauth_credential_key(key), function()
-        local credentials, err = dao.keyauth_credentials:find_by_keys { key = key }
+        local credentials, err = dao.keyauth_credentials:find_by_keys {key = key}
         local result
         if err then
           return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
@@ -134,19 +133,25 @@ function _M.execute(conf)
         end
         return result
       end)
+      if credential then break end
     end
+  end
 
-    if credential then break end
+  -- No key found in the request's headers or parameters
+  if not key_found then
+    ngx.ctx.stop_phases = true
+    return responses.send_HTTP_UNAUTHORIZED("No API key found in headers or querystring")
   end
 
+  -- No key found in the DB, this credential is invalid
   if not credential then
     ngx.ctx.stop_phases = true -- interrupt other phases of this request
     return responses.send_HTTP_FORBIDDEN("Invalid authentication credentials")
   end
 
   -- Retrieve consumer
   local consumer = cache.get_or_set(cache.consumer_key(credential.consumer_id), function()
-    local result, err = dao.consumers:find_by_primary_key({ id = credential.consumer_id })
+    local result, err = dao.consumers:find_by_primary_key({id = credential.consumer_id})
     if err then
       return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
     end

kong/tools/responses.lua

@@ -11,6 +11,7 @@ local _M = {
     HTTP_CREATED = 201,
     HTTP_NO_CONTENT = 204,
     HTTP_BAD_REQUEST = 400,
+    HTTP_UNAUTHORIZED = 401,
     HTTP_FORBIDDEN = 403,
     HTTP_NOT_FOUND = 404,
     HTTP_METHOD_NOT_ALLOWED = 405,
@@ -23,12 +24,15 @@ local _M = {
 -- Define some rules that will ALWAYS be applied to some status codes.
 -- Ex: 204 must not have content, but if 404 has no content then "Not found" will be set.
 local response_default_content = {
-  [_M.status_codes.HTTP_NOT_FOUND] = function(content)
-    return content and content or "Not found"
+  [_M.status_codes.HTTP_UNAUTHORIZED] = function(content)
+    return content or "Unauthorized"
   end,
   [_M.status_codes.HTTP_NO_CONTENT] = function(content)
     return nil
   end,
+  [_M.status_codes.HTTP_NOT_FOUND] = function(content)
+    return content or "Not found"
+  end,
   [_M.status_codes.HTTP_INTERNAL_SERVER_ERROR] = function(content)
     return "An unexpected error occurred"
   end,

spec/integration/proxy/database_cache_spec.lua

@@ -43,7 +43,7 @@ describe("Database cache", function()
     os.execute("sleep "..tonumber(5))
 
     local _, status = http_client.get(spec_helper.PROXY_URL.."/get", {}, {host = "cache.test"})
-    assert.are.equal(403, status)
+    assert.are.equal(401, status)
 
     -- Create a consumer and a key will make it work again
     local consumer, err = env.dao_factory.consumers:insert { username = "john" }

spec/integration/proxy/resolver_spec.lua

@@ -165,7 +165,7 @@ describe("Resolver", function()
 
     it("should return the correct Server and no Via header when the request was NOT proxied", function()
       local _, status, headers = http_client.get(STUB_GET_URL, nil, { host = "mockbin-auth.com"})
-      assert.are.equal(403, status)
+      assert.are.equal(401, status)
       assert.are.equal(constants.NAME.."/"..constants.VERSION, headers.server)
       assert.falsy(headers.via)
     end)

spec/plugins/basicauth/access_spec.lua

@@ -11,16 +11,16 @@ describe("Authentication Plugin", function()
     spec_helper.prepare_db()
     spec_helper.insert_fixtures {
       api = {
-        { name = "tests basicauth", public_dns = "basicauth.com", target_url = "http://mockbin.com" }
+        {name = "tests basicauth", public_dns = "basicauth.com", target_url = "http://mockbin.com"}
       },
       consumer = {
-        { username = "basicauth_tests_consuser" }
+        {username = "basicauth_tests_consuser"}
       },
       plugin_configuration = {
-        { name = "basicauth", value = {}, __api = 1 }
+        {name = "basicauth", value = {}, __api = 1}
       },
       basicauth_credential = {
-        { username = "username", password = "password", __consumer = 1 }
+        {username = "username", password = "password", __consumer = 1}
       }
     }
 
@@ -36,50 +36,43 @@ describe("Authentication Plugin", function()
     it("should return invalid credentials when the credential value is wrong", function()
       local response, status = http_client.get(STUB_GET_URL, {}, {host = "basicauth.com", authorization = "asd"})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(403, status)
+      assert.equal("Invalid authentication credentials", body.message)
     end)
 
     it("should not pass when passing only the password", function()
       local response, status = http_client.get(STUB_GET_URL, {}, {host = "basicauth.com", authorization = "Basic OmFwaWtleTEyMw=="})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(403, status)
+      assert.equal("Invalid authentication credentials", body.message)
     end)
 
     it("should not pass when passing only the username", function()
       local response, status = http_client.get(STUB_GET_URL, {}, {host = "basicauth.com", authorization = "Basic dXNlcjEyMzo="})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(403, status)
+      assert.equal("Invalid authentication credentials", body.message)
     end)
 
-    it("should return invalid credentials when the credential parameter name is wrong in GET", function()
+    it("should reply 401 when authorization is missing", function()
       local response, status = http_client.get(STUB_GET_URL, {}, {host = "basicauth.com", authorization123 = "Basic dXNlcm5hbWU6cGFzc3dvcmQ="})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
-    end)
-
-    it("should return invalid credentials when the credential parameter name is wrong in POST", function()
-      local response, status = http_client.post(STUB_POST_URL, {}, {host = "basicauth.com", authorization123 = "Basic dXNlcm5hbWU6cGFzc3dvcmQ="})
-      local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(401, status)
+      assert.equal("Unauthorized", body.message)
     end)
 
     it("should pass with GET", function()
       local response, status = http_client.get(STUB_GET_URL, {}, {host = "basicauth.com", authorization = "Basic dXNlcm5hbWU6cGFzc3dvcmQ="})
-      assert.are.equal(200, status)
+      assert.equal(200, status)
       local parsed_response = cjson.decode(response)
-      assert.are.equal("Basic dXNlcm5hbWU6cGFzc3dvcmQ=", parsed_response.headers.authorization)
+      assert.equal("Basic dXNlcm5hbWU6cGFzc3dvcmQ=", parsed_response.headers.authorization)
     end)
 
     it("should pass with POST", function()
       local response, status = http_client.post(STUB_POST_URL, {}, {host = "basicauth.com", authorization = "Basic dXNlcm5hbWU6cGFzc3dvcmQ="})
-      assert.are.equal(200, status)
+      assert.equal(200, status)
       local parsed_response = cjson.decode(response)
-      assert.are.equal("Basic dXNlcm5hbWU6cGFzc3dvcmQ=", parsed_response.headers.authorization)
+      assert.equal("Basic dXNlcm5hbWU6cGFzc3dvcmQ=", parsed_response.headers.authorization)
     end)
 
   end)

spec/plugins/keyauth/access_spec.lua

@@ -11,18 +11,18 @@ describe("Authentication Plugin", function()
     spec_helper.prepare_db()
     spec_helper.insert_fixtures {
       api = {
-        { name = "tests auth 1", public_dns = "keyauth1.com", target_url = "http://mockbin.com" },
-        { name = "tests auth 2", public_dns = "keyauth2.com", target_url = "http://mockbin.com" }
+        {name = "tests auth 1", public_dns = "keyauth1.com", target_url = "http://mockbin.com"},
+        {name = "tests auth 2", public_dns = "keyauth2.com", target_url = "http://mockbin.com"}
       },
       consumer = {
-        { username = "auth_tests_consumer" }
+        {username = "auth_tests_consumer"}
       },
       plugin_configuration = {
-        { name = "keyauth", value = { key_names = { "apikey" }}, __api = 1 },
-        { name = "keyauth", value = {key_names = {"apikey"}, hide_credentials = true}, __api = 2 }
+        {name = "keyauth", value = {key_names = {"apikey"}}, __api = 1},
+        {name = "keyauth", value = {key_names = {"apikey"}, hide_credentials = true}, __api = 2}
       },
       keyauth_credential = {
-        { key = "apikey123", __consumer = 1 }
+        {key = "apikey123", __consumer = 1}
       }
     }
 
@@ -38,119 +38,126 @@ describe("Authentication Plugin", function()
     it("should return invalid credentials when the credential value is wrong", function()
       local response, status = http_client.get(STUB_GET_URL, {apikey = "asd"}, {host = "keyauth1.com"})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(403, status)
+      assert.equal("Invalid authentication credentials", body.message)
     end)
 
-    it("should return invalid credentials when the credential parameter name is wrong in GET", function()
+    it("should reply 401 when the credential parameter is missing", function()
       local response, status = http_client.get(STUB_GET_URL, {apikey123 = "apikey123"}, {host = "keyauth1.com"})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(401, status)
+      assert.equal("No API key found in headers or querystring", body.message)
     end)
 
-    it("should return invalid credentials when the credential parameter name is wrong in POST", function()
+    it("should reply 401 when the credential parameter name is wrong in GET", function()
+      local response, status = http_client.get(STUB_GET_URL, {apikey123 = "apikey123"}, {host = "keyauth1.com"})
+      local body = cjson.decode(response)
+      assert.equal(401, status)
+      assert.equal("No API key found in headers or querystring", body.message)
+    end)
+
+    it("should reply 401 when the credential parameter name is wrong in POST", function()
       local response, status = http_client.post(STUB_POST_URL, {apikey123 = "apikey123"}, {host = "keyauth1.com"})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(401, status)
+      assert.equal("No API key found in headers or querystring", body.message)
     end)
 
     it("should pass with GET", function()
       local response, status = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "keyauth1.com"})
-      assert.are.equal(200, status)
+      assert.equal(200, status)
       local parsed_response = cjson.decode(response)
-      assert.are.equal("apikey123", parsed_response.queryString.apikey)
+      assert.equal("apikey123", parsed_response.queryString.apikey)
     end)
 
     it("should pass with POST", function()
       local response, status = http_client.post(STUB_POST_URL, {apikey = "apikey123"}, {host = "keyauth1.com"})
-      assert.are.equal(200, status)
+      assert.equal(200, status)
       local parsed_response = cjson.decode(response)
-      assert.are.equal("apikey123", parsed_response.postData.params.apikey)
+      assert.equal("apikey123", parsed_response.postData.params.apikey)
     end)
 
-    it("should return invalid credentials when the credential parameter name is wrong in GET header", function()
+    it("should reply 401 when the credential parameter name is wrong in GET header", function()
       local response, status = http_client.get(STUB_GET_URL, {}, {host = "keyauth1.com", apikey123 = "apikey123"})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(401, status)
+      assert.equal("No API key found in headers or querystring", body.message)
     end)
 
-    it("should return invalid credentials when the credential parameter name is wrong in POST header", function()
+    it("should reply 401 when the credential parameter name is wrong in POST header", function()
       local response, status = http_client.post(STUB_POST_URL, {}, {host = "keyauth1.com", apikey123 = "apikey123"})
       local body = cjson.decode(response)
-      assert.are.equal(403, status)
-      assert.are.equal("Invalid authentication credentials", body.message)
+      assert.equal(401, status)
+      assert.equal("No API key found in headers or querystring", body.message)
     end)
 
     it("should set right headers", function()
       local response, status = http_client.post(STUB_POST_URL, {apikey = "apikey123"}, {host = "keyauth1.com"})
-      assert.are.equal(200, status)
+      assert.equal(200, status)
       local parsed_response = cjson.decode(response)
       assert.truthy(parsed_response.headers["x-consumer-id"])
       assert.truthy(parsed_response.headers["x-consumer-username"])
-      assert.are.equal("auth_tests_consumer", parsed_response.headers["x-consumer-username"])
+      assert.equal("auth_tests_consumer", parsed_response.headers["x-consumer-username"])
     end)
 
     describe("Hide credentials", function()
 
       it("should pass with POST and hide credentials", function()
         local response, status = http_client.post(STUB_POST_URL, {apikey = "apikey123", wot = "wat"}, {host = "keyauth2.com"})
-        assert.are.equal(200, status)
+        assert.equal(200, status)
         local parsed_response = cjson.decode(response)
         assert.falsy(parsed_response.postData.params.apikey)
-        assert.are.equal("wat", parsed_response.postData.params.wot)
+        assert.equal("wat", parsed_response.postData.params.wot)
       end)
 
       it("should pass with POST multipart and hide credentials", function()
         local response, status = http_client.post_multipart(STUB_POST_URL, {apikey = "apikey123", wot = "wat"}, {host = "keyauth2.com"})
-        assert.are.equal(200, status)
+        assert.equal(200, status)
         local parsed_response = cjson.decode(response)
         assert.falsy(parsed_response.postData.params.apikey)
-        assert.are.equal("wat", parsed_response.postData.params.wot)
+        assert.equal("wat", parsed_response.postData.params.wot)
       end)
 
       it("should pass with GET and hide credentials", function()
         local response, status = http_client.get(STUB_GET_URL, {}, {host = "keyauth2.com", apikey = "apikey123"})
-        assert.are.equal(200, status)
+        assert.equal(200, status)
         local parsed_response = cjson.decode(response)
         assert.falsy(parsed_response.headers.apikey)
       end)
 
       it("should pass with GET and hide credentials and another param", function()
         local response, status = http_client.get(STUB_GET_URL, {}, {host = "keyauth2.com", apikey = "apikey123", foo = "bar"})
-        assert.are.equal(200, status)
+        assert.equal(200, status)
         local parsed_response = cjson.decode(response)
         assert.falsy(parsed_response.headers.apikey)
-        assert.are.equal("bar", parsed_response.headers.foo)
+        assert.equal("bar", parsed_response.headers.foo)
       end)
 
       it("should not pass with GET and hide credentials", function()
         local response, status = http_client.get(STUB_GET_URL, {}, {host = "keyauth2.com", apikey = "apikey123123"})
         local body = cjson.decode(response)
-        assert.are.equal(403, status)
-        assert.are.equal("Invalid authentication credentials", body.message)
+        assert.equal(403, status)
+        assert.equal("Invalid authentication credentials", body.message)
       end)
 
       it("should pass with GET and hide credentials and another param", function()
         local response, status = http_client.get(STUB_GET_URL, {}, {host = "keyauth2.com", apikey = "apikey123", wot = "wat"})
-        assert.are.equal(200, status)
+        assert.equal(200, status)
         local parsed_response = cjson.decode(response)
         assert.falsy(parsed_response.headers.apikey)
-        assert.are.equal("wat", parsed_response.headers.wot)
+        assert.equal("wat", parsed_response.headers.wot)
       end)
 
       it("should not pass with GET and hide credentials", function()
         local response, status = http_client.get(STUB_GET_URL, {}, {host = "keyauth2.com", apikey = "apikey123123"})
         local body = cjson.decode(response)
-        assert.are.equal(403, status)
-        assert.are.equal("Invalid authentication credentials", body.message)
+        assert.equal(403, status)
+        assert.equal("Invalid authentication credentials", body.message)
       end)
 
       it("should pass with GET and hide credentials in querystring", function()
         local response, status = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "keyauth2.com"})
-        assert.are.equal(200, status)
+        assert.equal(200, status)
         local parsed_response = cjson.decode(response)
         assert.falsy(parsed_response.queryString.apikey)
       end)