feat(oauth2) set generic X-Credential-Identifier (deprecating X-Credential-Username)

bungle · bungle · commit 84b3006c9e53 · 2020-02-04T17:45:14.000+02:00
### Summary The PR Kong#4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided at time that we should add support for this less opinionated field name on other auth plugins too. This commit adds it to `OAuth2 Plugin`. This feature was originally implemented by @lucasmoreno on PR Kong#5201. Thank you, Lucas, grab your T-shirt: https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt! I just refactored @lucasmoreno's code to use our generic `X-Credential-Identifier` header instead of the proposed `X-Credential-Client-Id`. ### Issues Resolved Closes Kong#5201
diff --git a/kong/plugins/oauth2/access.lua b/kong/plugins/oauth2/access.lua
@@ -97,6 +97,7 @@ local function generate_token(conf, service, credential, authenticated_userid,
   }
 end
 
+
 local function load_oauth2_credential_by_client_id(client_id)
   local credential, err = kong.db.oauth2_credentials:select_by_client_id(client_id)
   if err then
@@ -106,6 +107,7 @@ local function load_oauth2_credential_by_client_id(client_id)
   return credential
 end
 
+
 local function get_redirect_uris(client_id)
   local client, err
   if client_id and client_id ~= "" then
@@ -121,6 +123,7 @@ local function get_redirect_uris(client_id)
   return client and client.redirect_uris or nil, client
 end
 
+
 local function retrieve_parameters()
   -- OAuth2 parameters could be in both the querystring or body
   local uri_args = kong.request.get_query()
@@ -135,6 +138,7 @@ local function retrieve_parameters()
   return uri_args
 end
 
+
 local function retrieve_scope(parameters, conf)
   local scope = parameters[SCOPE]
   local scopes = {}
@@ -161,6 +165,7 @@ local function retrieve_scope(parameters, conf)
   end -- else return nil
 end
 
+
 local function authorize(conf)
   local response_params = {}
   local parameters = retrieve_parameters()
@@ -309,6 +314,7 @@ local function authorize(conf)
   })
 end
 
+
 local function retrieve_client_credentials(parameters, conf)
   local client_id, client_secret, from_authorization_header
   local authorization_header = kong.request.get_header(conf.auth_header_name)
@@ -346,6 +352,7 @@ local function retrieve_client_credentials(parameters, conf)
   return client_id, client_secret, from_authorization_header
 end
 
+
 local function issue_token(conf)
   local response_params = {}
   local invalid_client_properties = {}
@@ -561,17 +568,18 @@ local function issue_token(conf)
 
   -- Sending response in JSON format
   return kong.response.exit(response_params[ERROR] and
-                             (invalid_client_properties and
-                              invalid_client_properties.status or 400) or 200,
-                              response_params, {
-                                ["cache-control"] = "no-store",
-                                ["pragma"] = "no-cache",
-                                ["www-authenticate"] = invalid_client_properties and
-                                                       invalid_client_properties.www_authenticate
-                              }
-                            )
+                            (invalid_client_properties and
+                             invalid_client_properties.status or 400) or 200,
+                             response_params, {
+                               ["cache-control"] = "no-store",
+                               ["pragma"] = "no-cache",
+                               ["www-authenticate"] = invalid_client_properties and
+                                                      invalid_client_properties.www_authenticate
+                             }
+                           )
 end
 
+
 local function load_token(conf, service, access_token)
   local credentials, err =
     kong.db.oauth2_tokens:select_by_access_token(access_token)
@@ -601,6 +609,7 @@ local function load_token(conf, service, access_token)
   return credentials
 end
 
+
 local function retrieve_token(conf, access_token)
   local token, err
 
@@ -618,6 +627,7 @@ local function retrieve_token(conf, access_token)
   return token
 end
 
+
 local function parse_access_token(conf)
   local found_in = {}
 
@@ -667,6 +677,7 @@ local function parse_access_token(conf)
   return access_token
 end
 
+
 local function load_oauth2_credential_into_memory(credential_id)
   local result, err = kong.db.oauth2_credentials:select { id = credential_id }
   if err then
@@ -676,7 +687,10 @@ local function load_oauth2_credential_into_memory(credential_id)
   return result
 end
 
+
 local function set_consumer(consumer, credential, token)
+  kong.client.authenticate(consumer, credential)
+
   local set_header = kong.service.request.set_header
   local clear_header = kong.service.request.clear_header
 
@@ -698,31 +712,34 @@ local function set_consumer(consumer, credential, token)
     clear_header(constants.HEADERS.CONSUMER_USERNAME)
   end
 
-  kong.client.authenticate(consumer, credential)
-
-  if credential then
-    if token.scope then
-      set_header("x-authenticated-scope", token.scope)
-    else
-      clear_header("x-authenticated-scope")
-    end
-
-    if token.authenticated_userid then
-      set_header("x-authenticated-userid", token.authenticated_userid)
-    else
-      clear_header("x-authenticated-userid")
-    end
+  if credential and credential.client_id then
+    set_header(constants.HEADERS.CREDENTIAL_IDENTIFIER, credential.client_id)
+  else
+    clear_header(constants.HEADERS.CREDENTIAL_IDENTIFIER)
+  end
 
-    clear_header(constants.HEADERS.ANONYMOUS) -- in case of auth plugins concatenation
+  clear_header(constants.HEADERS.CREDENTIAL_USERNAME)
 
+  if credential then
+    clear_header(constants.HEADERS.ANONYMOUS)
   else
     set_header(constants.HEADERS.ANONYMOUS, true)
-    clear_header("x-authenticated-scope")
-    clear_header("x-authenticated-userid")
   end
 
+  if token and token.scope then
+    set_header("X-Authenticated-Scope", token.scope)
+  else
+    clear_header("X-Authenticated-Scope")
+  end
+
+  if token and token.authenticated_userid then
+    set_header("X-Authenticated-UserId", token.authenticated_userid)
+  else
+    clear_header("X-Authenticated-UserId")
+  end
 end
 
+
 local function do_authentication(conf)
   local access_token = parse_access_token(conf);
   if not access_token then
@@ -852,7 +869,7 @@ function _M.execute(conf)
         return kong.response.exit(500, { message = "An unexpected error occurred" })
       end
 
-      set_consumer(consumer, nil, nil)
+      set_consumer(consumer)
 
     else
       return kong.response.exit(err.status, err.message, err.headers)
diff --git a/kong/plugins/oauth2/handler.lua b/kong/plugins/oauth2/handler.lua
@@ -1,12 +1,15 @@
 local access = require "kong.plugins.oauth2.access"
 
-local OAuthHandler = {}
+
+local OAuthHandler = {
+  PRIORITY = 1004,
+  VERSION = "2.1.0",
+}
+
 
 function OAuthHandler:access(conf)
   access.execute(conf)
 end
 
-OAuthHandler.PRIORITY = 1004
-OAuthHandler.VERSION = "2.0.0"
 
 return OAuthHandler
diff --git a/spec/03-plugins/25-oauth2/03-access_spec.lua b/spec/03-plugins/25-oauth2/03-access_spec.lua
@@ -907,6 +907,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
           assert.are.equal("bob", body.headers["x-consumer-username"])
           assert.are.equal("email profile", body.headers["x-authenticated-scope"])
           assert.are.equal("userid123", body.headers["x-authenticated-userid"])
+          assert.are.equal("clientid123", body.headers["x-credential-identifier"])
+          assert.are.equal(nil, body.headers["x-credential-username"])
         end)
       end)
 
@@ -1208,6 +1210,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
           assert.are.equal("bob", body.headers["x-consumer-username"])
           assert.are.equal("email", body.headers["x-authenticated-scope"])
           assert.are.equal("hello", body.headers["x-authenticated-userid"])
+          assert.are.equal("clientid123", body.headers["x-credential-identifier"])
+          assert.are.equal(nil, body.headers["x-credential-username"])
         end)
         it("works in a multipart request", function()
           local res = assert(proxy_ssl_client:send {
@@ -1442,6 +1446,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
           assert.are.equal("bob", body.headers["x-consumer-username"])
           assert.are.equal("email", body.headers["x-authenticated-scope"])
           assert.are.equal("id123", body.headers["x-authenticated-userid"])
+          assert.are.equal("clientid123", body.headers["x-credential-identifier"])
+          assert.are.equal(nil, body.headers["x-credential-username"])
         end)
       end)
     end)
@@ -1638,6 +1644,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.are.equal("bob", body.headers["x-consumer-username"])
         assert.are.equal("email", body.headers["x-authenticated-scope"])
         assert.are.equal("userid123", body.headers["x-authenticated-userid"])
+        assert.are.equal("clientid123", body.headers["x-credential-identifier"])
+        assert.are.equal(nil, body.headers["x-credential-username"])
       end)
       it("fails when an authorization code is used more than once", function()
         local code = provision_code()
@@ -1883,6 +1891,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.are.equal(consumer.username, body.headers["x-consumer-username"])
         assert.are.equal("userid123", body.headers["x-authenticated-userid"])
         assert.are.equal("email", body.headers["x-authenticated-scope"])
+        assert.are.equal("clientid123", body.headers["x-credential-identifier"])
+        assert.are.equal(nil, body.headers["x-credential-username"])
         assert.is_nil(body.headers["x-anonymous-consumer"])
       end)
       it("returns HTTP 400 when scope is not a string", function()
@@ -1934,6 +1944,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.are.equal(consumer.username, body.headers["x-consumer-username"])
         assert.are.equal("userid123", body.headers["x-authenticated-userid"])
         assert.are.equal("email", body.headers["x-authenticated-scope"])
+        assert.are.equal("clientid123", body.headers["x-credential-identifier"])
+        assert.are.equal(nil, body.headers["x-credential-username"])
         assert.is_nil(body.headers["x-anonymous-consumer"])
       end)
       it("works with wrong credentials and anonymous", function()
@@ -1947,6 +1959,9 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         local body = cjson.decode(assert.res_status(200, res))
         assert.are.equal("true", body.headers["x-anonymous-consumer"])
         assert.equal('no-body', body.headers["x-consumer-username"])
+        assert.are.equal(nil, body.headers["x-credential-identifier"])
+        assert.are.equal(nil, body.headers["x-credential-username"])
+
       end)
       it("errors when anonymous user doesn't exist", function()
         finally(function()
@@ -2401,6 +2416,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
     local user1
     local user2
     local anonymous
+    local keyauth
 
     lazy_setup(function()
       local service1 = admin_api.services:insert({
@@ -2461,7 +2477,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         },
       }
 
-      admin_api.keyauth_credentials:insert({
+      keyauth = admin_api.keyauth_credentials:insert({
         key      = "Mouse",
         consumer = { id = user1.id },
       })
@@ -2504,6 +2520,10 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         local id = assert.request(res).has.header("x-consumer-id")
         assert.not_equal(id, anonymous.id)
         assert(id == user1.id or id == user2.id)
+
+        local client_id = assert.request(res).has.header("x-credential-identifier")
+        assert.equal(keyauth.id, client_id)
+        assert.request(res).has.no.header("x-credential-username")
       end)
 
       it("fails 401, with only the first credential provided", function()
@@ -2566,6 +2586,9 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         local id = assert.request(res).has.header("x-consumer-id")
         assert.not_equal(id, anonymous.id)
         assert(id == user1.id or id == user2.id)
+        local client_id = assert.request(res).has.header("x-credential-identifier")
+        assert.equal("clientid4567", client_id)
+        assert.request(res).has.no.header("x-credential-username")
       end)
 
       it("passes with only the first credential provided", function()
@@ -2582,6 +2605,9 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         local id = assert.request(res).has.header("x-consumer-id")
         assert.not_equal(id, anonymous.id)
         assert.equal(user1.id, id)
+        local client_id = assert.request(res).has.header("x-credential-identifier")
+        assert.equal(keyauth.id, client_id)
+        assert.request(res).has.no.header("x-credential-username")
       end)
 
       it("passes with only the second credential provided", function()
@@ -2600,6 +2626,9 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         local id = assert.request(res).has.header("x-consumer-id")
         assert.not_equal(id, anonymous.id)
         assert.equal(user2.id, id)
+        local client_id = assert.request(res).has.header("x-credential-identifier")
+        assert.equal("clientid4567", client_id)
+        assert.request(res).has.no.header("x-credential-username")
       end)
 
       it("passes with no credential provided", function()
@@ -2614,6 +2643,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.request(res).has.header("x-anonymous-consumer")
         local id = assert.request(res).has.header("x-consumer-id")
         assert.equal(id, anonymous.id)
+        assert.request(res).has.no.header("x-credential-identifier")
+        assert.request(res).has.no.header("x-credential-username")
       end)
     end)
   end)
