fix(admin) lookup entities by secondary field as uuid (Kong#2420)

when the name/key/whatever field is formatted as a uuid Kong would assume that field to be the `id` field. This change will make it first try the `id` field in that case, and then the alternative field.
thienhv · Apr 21, 2017 · 254befa · 254befa
1 parent 25038d3
commit 254befa
Show file tree

Hide file tree

Showing 9 changed files with 108 additions and 65 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,11 @@
     weight Targets, instead of all nonzero weight targets. This is to provide
     a better picture of the Targets currently in use by the Kong load balancer.
     [#2310](https://github.com/Mashape/kong/pull/2310)
+  - Endpoints with parameters `xxx_or_id` will now also yield the proper
+    result if the `xxx` field is formatted as a uuid. Most notably this fixes
+    a problem with consumers (where the username is a uuid) not being found,
+    but also other endpoints suffering from the same flaw have also been fixed.
+    [#2420](https://github.com/Mashape/kong/pull/2420)
 - Plugins:
   - key-auth: Allow setting API key header names with an underscore.
     [#2370](https://github.com/Mashape/kong/pull/2370)

diff --git a/kong/api/crud_helpers.lua b/kong/api/crud_helpers.lua
@@ -5,16 +5,45 @@ local app_helpers = require "lapis.application"
 
 local _M = {}
 
+--- Will look up a value in the dao.
+-- Either by `id` field or by the field named by 'alternate_field'. If the value
+-- is NOT a uuid, then by the 'alternate_field'. If it is a uuid then it will
+-- first try the `id` field, if that doesn't yield anything it will try again
+-- with the 'alternate_field'.
+-- @param dao the specific dao to search
+-- @param filter filter table to use, tries will add to this table
+-- @param value the value to look up
+-- @param alternate_field the field to use if it is not a uuid, or not found in `id`
+function _M.find_by_id_or_field(dao, filter, value, alternate_field)
+  filter = filter or {}
+  local is_uuid = utils.is_valid_uuid(value)
+  filter[is_uuid and "id" or alternate_field] = value
+
+  local rows, err = dao:find_all(filter)
+  if err then
+    return nil, err
+  end
+
+  if is_uuid and not next(rows) then
+    -- it's a uuid, but yielded no results, so retry with the alternate field
+    filter.id = nil
+    filter[alternate_field] = value
+    rows, err = dao:find_all(filter)
+    if err then
+      return nil, err
+    end
+  end
+  return rows
+end
+
 function _M.find_api_by_name_or_id(self, dao_factory, helpers)
-  local filter_keys = {
-    [utils.is_valid_uuid(self.params.name_or_id) and "id" or "name"] = self.params.name_or_id
-  }
-  self.params.name_or_id = nil
+  local rows, err = _M.find_by_id_or_field(dao_factory.apis, {},
+                                           self.params.name_or_id, "name")
 
-  local rows, err = dao_factory.apis:find_all(filter_keys)
   if err then
     return helpers.yield_error(err)
   end
+  self.params.name_or_id = nil
 
   -- We know name and id are unique for APIs, hence if we have a row, it must be the only one
   self.api = rows[1]
@@ -24,15 +53,13 @@ function _M.find_api_by_name_or_id(self, dao_factory, helpers)
 end
 
 function _M.find_consumer_by_username_or_id(self, dao_factory, helpers)
-  local filter_keys = {
-    [utils.is_valid_uuid(self.params.username_or_id) and "id" or "username"] = self.params.username_or_id
-  }
-  self.params.username_or_id = nil
+  local rows, err = _M.find_by_id_or_field(dao_factory.consumers, {},
+                                           self.params.username_or_id, "username")
 
-  local rows, err = dao_factory.consumers:find_all(filter_keys)
   if err then
     return helpers.yield_error(err)
   end
+  self.params.username_or_id = nil
 
   -- We know username and id are unique, so if we have a row, it must be the only one
   self.consumer = rows[1]
@@ -42,15 +69,13 @@ function _M.find_consumer_by_username_or_id(self, dao_factory, helpers)
 end
 
 function _M.find_upstream_by_name_or_id(self, dao_factory, helpers)
-  local filter_keys = {
-    [utils.is_valid_uuid(self.params.name_or_id) and "id" or "name"] = self.params.name_or_id
-  }
-  self.params.name_or_id = nil
+  local rows, err = _M.find_by_id_or_field(dao_factory.upstreams, {},
+                                           self.params.name_or_id, "name")
 
-  local rows, err = dao_factory.upstreams:find_all(filter_keys)
   if err then
     return helpers.yield_error(err)
   end
+  self.params.name_or_id = nil
 
   -- We know name and id are unique, so if we have a row, it must be the only one
   self.upstream = rows[1]

diff --git a/kong/plugins/acl/api.lua b/kong/plugins/acl/api.lua
@@ -1,5 +1,4 @@
 local crud = require "kong.api.crud_helpers"
-local utils = require "kong.tools.utils"
 
 return {
   ["/consumers/:username_or_id/acls/"] = {
@@ -26,18 +25,19 @@ return {
       crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
       self.params.consumer_id = self.consumer.id
 
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.group_or_id) and "id" or "group"] = self.params.group_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.group_or_id = nil
+      local acls, err = crud.find_by_id_or_field(
+        dao_factory.acls,
+        { consumer_id = self.params.consumer_id },
+        self.params.group_or_id,
+        "group"
+      )
 
-      local acls, err = dao_factory.acls:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif #acls == 0 then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.group_or_id = nil
 
       self.acl = acls[1]
     end,

diff --git a/kong/plugins/basic-auth/api.lua b/kong/plugins/basic-auth/api.lua
@@ -1,5 +1,4 @@
 local crud = require "kong.api.crud_helpers"
-local utils = require "kong.tools.utils"
 
 return {
   ["/consumers/:username_or_id/basic-auth/"] = {
@@ -25,18 +24,19 @@ return {
       crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
       self.params.consumer_id = self.consumer.id
 
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.credential_username_or_id) and "id" or "username"] = self.params.credential_username_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.credential_username_or_id = nil
+      local credentials, err = crud.find_by_id_or_field(
+        dao_factory.basicauth_credentials,
+        { consumer_id = self.params.consumer_id },
+        self.params.credential_username_or_id,
+        "username"
+      )
 
-      local credentials, err = dao_factory.basicauth_credentials:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif next(credentials) == nil then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.credential_username_or_id = nil
 
       self.basicauth_credential = credentials[1]
     end,

diff --git a/kong/plugins/hmac-auth/api.lua b/kong/plugins/hmac-auth/api.lua
@@ -1,5 +1,4 @@
 local crud = require "kong.api.crud_helpers"
-local utils = require "kong.tools.utils"
 
 return{
   ["/consumers/:username_or_id/hmac-auth/"] = {
@@ -26,19 +25,19 @@ return{
       crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
       self.params.consumer_id = self.consumer.id
 
+      local credentials, err = crud.find_by_id_or_field(
+        dao_factory.hmacauth_credentials,
+        { consumer_id = self.params.consumer_id },
+        self.params.credential_username_or_id,
+        "username"
+      )
 
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.credential_username_or_id) and "id" or "username"] = self.params.credential_username_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.credential_username_or_id = nil
-
-      local credentials, err = dao_factory.hmacauth_credentials:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif next(credentials) == nil then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.credential_username_or_id = nil
 
       self.hmacauth_credential = credentials[1]
     end,

diff --git a/kong/plugins/jwt/api.lua b/kong/plugins/jwt/api.lua
@@ -1,5 +1,4 @@
 local crud = require "kong.api.crud_helpers"
-local utils = require "kong.tools.utils"
 
 return {
   ["/consumers/:username_or_id/jwt/"] = {
@@ -26,18 +25,19 @@ return {
       crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
       self.params.consumer_id = self.consumer.id
 
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.credential_key_or_id) and "id" or "key"] = self.params.credential_key_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.credential_key_or_id = nil
+      local credentials, err = crud.find_by_id_or_field(
+        dao_factory.jwt_secrets,
+        { consumer_id = self.params.consumer_id },
+        self.params.credential_key_or_id,
+        "key"
+      )
 
-      local credentials, err = dao_factory.jwt_secrets:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif next(credentials) == nil then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.credential_key_or_id = nil
 
       self.jwt_secret = credentials[1]
     end,

diff --git a/kong/plugins/key-auth/api.lua b/kong/plugins/key-auth/api.lua
@@ -1,5 +1,4 @@
 local crud = require "kong.api.crud_helpers"
-local utils = require "kong.tools.utils"
 
 return {
   ["/consumers/:username_or_id/key-auth/"] = {
@@ -25,18 +24,19 @@ return {
       crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
       self.params.consumer_id = self.consumer.id
 
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.credential_key_or_id) and "id" or "key"] = self.params.credential_key_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.credential_key_or_id = nil
+      local credentials, err = crud.find_by_id_or_field(
+        dao_factory.keyauth_credentials,
+        { consumer_id = self.params.consumer_id },
+        self.params.credential_key_or_id,
+        "key"
+      )
 
-      local credentials, err = dao_factory.keyauth_credentials:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif next(credentials) == nil then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.credential_key_or_id = nil
 
       self.keyauth_credential = credentials[1]
     end,

diff --git a/kong/plugins/oauth2/api.lua b/kong/plugins/oauth2/api.lua
@@ -1,5 +1,4 @@
 local crud = require "kong.api.crud_helpers"
-local utils = require "kong.tools.utils"
 
 return {
   ["/oauth2_tokens/"] = {
@@ -18,18 +17,19 @@ return {
 
   ["/oauth2_tokens/:token_or_id"] = {
     before = function(self, dao_factory, helpers)
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.token_or_id) and "id" or "access_token"] = self.params.token_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.token_or_id = nil
+      local credentials, err = crud.find_by_id_or_field(
+        dao_factory.oauth2_tokens,
+        { consumer_id = self.params.consumer_id },
+        self.params.token_or_id,
+        "access_token"
+      )
 
-      local credentials, err = dao_factory.oauth2_tokens:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif next(credentials) == nil then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.token_or_id = nil
 
       self.oauth2_token = credentials[1]
     end,
@@ -77,18 +77,19 @@ return {
       crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
       self.params.consumer_id = self.consumer.id
 
-      local filter_keys = {
-        [utils.is_valid_uuid(self.params.clientid_or_id) and "id" or "client_id"] = self.params.clientid_or_id,
-        consumer_id = self.params.consumer_id,
-      }
-      self.params.clientid_or_id = nil
+      local credentials, err = crud.find_by_id_or_field(
+        dao_factory.oauth2_credentials,
+        { consumer_id = self.params.consumer_id },
+        self.params.clientid_or_id,
+        "client_id"
+      )
 
-      local credentials, err = dao_factory.oauth2_credentials:find_all(filter_keys)
       if err then
         return helpers.yield_error(err)
       elseif next(credentials) == nil then
         return helpers.responses.send_HTTP_NOT_FOUND()
       end
+      self.params.clientid_or_id = nil
 
       self.oauth2_credential = credentials[1]
     end,

diff --git a/spec/02-integration/03-admin_api/03-consumers_routes_spec.lua b/spec/02-integration/03-admin_api/03-consumers_routes_spec.lua
@@ -20,7 +20,7 @@ describe("Admin API", function()
     helpers.stop_kong()
   end)
 
-  local consumer, consumer2
+  local consumer, consumer2, consumer3
   before_each(function()
     helpers.dao:truncate_tables()
     consumer = assert(helpers.dao.consumers:insert {
@@ -31,6 +31,10 @@ describe("Admin API", function()
       username = "bob pop",  -- containing space for urlencoded test
       custom_id = "abcd"
     })
+    consumer3 = assert(helpers.dao.consumers:insert {
+      username = "83825bb5-38c7-4160-8c23-54dd2b007f31",  -- uuid format
+      custom_id = "1a2b"
+    })
   end)
 
   describe("/consumers", function()
@@ -305,6 +309,15 @@ describe("Admin API", function()
           local json = cjson.decode(body)
           assert.same(consumer, json)
         end)
+        it("retrieves by username in uuid format", function()
+          local res = assert(client:send {
+            method = "GET",
+            path = "/consumers/"..consumer3.username
+          })
+          local body = assert.res_status(200, res)
+          local json = cjson.decode(body)
+          assert.same(consumer3, json)
+        end)
         it("retrieves by urlencoded username", function()
           local res = assert(client:send {
             method = "GET",