perf(ip-restriction) leaner schema validation + tests

kong/dao/schemas_validation.lua

@@ -24,7 +24,9 @@ local custom_types_validation = {
       return parsed_url and parsed_url.path and parsed_url.host and parsed_url.scheme
     end
   end,
-  ["array"] = function(v) return utils.is_array(v) end
+  ["array"] = function(v)
+    return utils.is_array(v)
+  end
 }
 
 local function validate_type(field_type, value)

kong/plugins/ip-restriction/schema.lua

@@ -1,6 +1,5 @@
 local iputils = require "resty.iputils"
 local DaoError = require "kong.dao.error"
-local utils = require "kong.tools.utils"
 local constants = require "kong.constants"
 
 local function validate_ips(v, t, column)
@@ -9,29 +8,33 @@ local function validate_ips(v, t, column)
     for _, ip in ipairs(v) do
       local _, err = iputils.parse_cidr(ip)
       if type(err) == "string" then -- It's an error only if the second variable is a string
-        return false, err
+        return false, "cannot parse '"..ip.."': "..err
       end
     end
-    new_fields = { ["_"..column.."_cache"] = iputils.parse_cidrs(v) }
+    new_fields = {["_"..column.."_cache"] = iputils.parse_cidrs(v)}
   end
   return true, nil, new_fields
 end
 
 return {
   fields = {
-    whitelist = { type = "array", func = validate_ips },
-    blacklist = { type = "array", func = validate_ips },
+    whitelist = {type = "array", func = validate_ips},
+    blacklist = {type = "array", func = validate_ips},
 
     -- Internal use
-    _whitelist_cache = { type = "array" },
-    _blacklist_cache = { type = "array" }
+    _whitelist_cache = {type = "array"},
+    _blacklist_cache = {type = "array"}
   },
   self_check = function(schema, plugin_t, dao, is_update)
-    if utils.table_size(plugin_t.whitelist) > 0 and utils.table_size(plugin_t.blacklist) > 0 then
-      return false, DaoError("You cannot set both a whitelist and a blacklist", constants.DATABASE_ERROR_TYPES.SCHEMA)
-    elseif utils.table_size(plugin_t.whitelist) == 0 and utils.table_size(plugin_t.blacklist) == 0 then
-      return false, DaoError("You must set at least a whitelist or blacklist", constants.DATABASE_ERROR_TYPES.SCHEMA)
+    local wl = type(plugin_t.whitelist) == "table" and plugin_t.whitelist or {}
+    local bl = type(plugin_t.blacklist) == "table" and plugin_t.blacklist or {}
+
+    if #wl > 0 and #bl > 0 then
+      return false, DaoError("you cannot set both a whitelist and a blacklist", constants.DATABASE_ERROR_TYPES.SCHEMA)
+    elseif #wl == 0 and #bl == 0 then
+      return false, DaoError("you must set at least a whitelist or blacklist", constants.DATABASE_ERROR_TYPES.SCHEMA)
     end
+
     return true
   end
 }

spec/plugins/ip-restriction/access_spec.lua

@@ -5,75 +5,66 @@ local cjson = require "cjson"
 local STUB_GET_URL = spec_helper.STUB_GET_URL
 
 describe("IP Restriction", function()
-
   setup(function()
     spec_helper.prepare_db()
     spec_helper.insert_fixtures {
       api = {
-        { name = "iprestriction1", request_host = "test1.com", upstream_url = "http://mockbin.com" },
-        { name = "iprestriction2", request_host = "test2.com", upstream_url = "http://mockbin.com" },
-        { name = "iprestriction3", request_host = "test3.com", upstream_url = "http://mockbin.com" },
-        { name = "iprestriction4", request_host = "test4.com", upstream_url = "http://mockbin.com" },
-        { name = "iprestriction5", request_host = "test5.com", upstream_url = "http://mockbin.com" },
-        { name = "iprestriction6", request_host = "test6.com", upstream_url = "http://mockbin.com" }
+        {name = "iprestriction1", request_host = "test1.com", upstream_url = "http://mockbin.com"},
+        {name = "iprestriction2", request_host = "test2.com", upstream_url = "http://mockbin.com"},
+        {name = "iprestriction3", request_host = "test3.com", upstream_url = "http://mockbin.com"},
+        {name = "iprestriction4", request_host = "test4.com", upstream_url = "http://mockbin.com"},
+        {name = "iprestriction5", request_host = "test5.com", upstream_url = "http://mockbin.com"},
+        {name = "iprestriction6", request_host = "test6.com", upstream_url = "http://mockbin.com"},
+        {name = "iprestriction7", request_host = "test7.com", upstream_url = "http://mockbin.com"}
       },
       plugin = {
-        { name = "ip-restriction", config = { blacklist = { "127.0.0.1" }}, __api = 1 },
-        { name = "ip-restriction", config = { blacklist = { "127.0.0.2" }}, __api = 2 },
-        { name = "ip-restriction", config = { whitelist = { "127.0.0.2" }}, __api = 3 },
-        { name = "ip-restriction", config = { whitelist = { "127.0.0.1" }}, __api = 4 },
-        { name = "ip-restriction", config = { blacklist = { "127.0.0.0/24" }}, __api = 5 },
-        { name = "ip-restriction", config = { whitelist = { "127.0.0.1", "127.0.0.2" }}, __api = 6 },
+        {name = "ip-restriction", config = {blacklist = {"127.0.0.1"}}, __api = 1},
+        {name = "ip-restriction", config = {blacklist = {"127.0.0.2"}}, __api = 2},
+        {name = "ip-restriction", config = {whitelist = {"127.0.0.2"}}, __api = 3},
+        {name = "ip-restriction", config = {whitelist = {"127.0.0.1"}}, __api = 4},
+        {name = "ip-restriction", config = {blacklist = {"127.0.0.0/24"}}, __api = 5},
+        {name = "ip-restriction", config = {whitelist = {"127.0.0.1", "127.0.0.2"}}, __api = 6}
       }
     }
-
     spec_helper.start_kong()
   end)
-
   teardown(function()
     spec_helper.stop_kong()
   end)
-
   it("should block request when IP is in blacklist", function()
     local response, status = http_client.get(STUB_GET_URL, {}, {host = "test1.com"})
     local body = cjson.decode(response)
-    assert.are.equal(403, status)
-    assert.are.equal("Your IP address is not allowed", body.message)
+    assert.equal(403, status)
+    assert.equal("Your IP address is not allowed", body.message)
   end)
-
   it("should allow request when IP is not in blacklist", function()
     local response, status = http_client.get(STUB_GET_URL, {}, {host = "test2.com"})
     local body = cjson.decode(response)
-    assert.are.equal(200, status)
-    assert.are.equal("127.0.0.1", body.clientIPAddress)
+    assert.equal(200, status)
+    assert.equal("127.0.0.1", body.clientIPAddress)
   end)
-
   it("should block request when IP is not in whitelist", function()
     local response, status = http_client.get(STUB_GET_URL, {}, {host = "test3.com"})
     local body = cjson.decode(response)
-    assert.are.equal(403, status)
-    assert.are.equal("Your IP address is not allowed", body.message)
+    assert.equal(403, status)
+    assert.equal("Your IP address is not allowed", body.message)
   end)
-
   it("should allow request when IP is in whitelist", function()
     local response, status = http_client.get(STUB_GET_URL, {}, {host = "test4.com"})
     local body = cjson.decode(response)
-    assert.are.equal(200, status)
-    assert.are.equal("127.0.0.1", body.clientIPAddress)
+    assert.equal(200, status)
+    assert.equal("127.0.0.1", body.clientIPAddress)
   end)
-
   it("should block request when IP is blacklisted with CIDR", function()
     local response, status = http_client.get(STUB_GET_URL, {}, {host = "test5.com"})
     local body = cjson.decode(response)
-    assert.are.equal(403, status)
-    assert.are.equal("Your IP address is not allowed", body.message)
+    assert.equal(403, status)
+    assert.equal("Your IP address is not allowed", body.message)
   end)
-
   it("should allow request when IP is in whitelist with another IP", function()
     local response, status = http_client.get(STUB_GET_URL, {}, {host = "test6.com"})
     local body = cjson.decode(response)
-    assert.are.equal(200, status)
-    assert.are.equal("127.0.0.1", body.clientIPAddress)
+    assert.equal(200, status)
+    assert.equal("127.0.0.1", body.clientIPAddress)
   end)
-
 end)

spec/plugins/ip-restriction/api_spec.lua

@@ -0,0 +1,85 @@
+local schemas_validation = require "kong.dao.schemas_validation"
+local schema = require "kong.plugins.ip-restriction.schema"
+
+local v = schemas_validation.validate_entity
+
+describe("ip-restriction schema", function()
+  describe("errors", function()
+    it("whitelist should not accept invalid types", function()
+      local t = {whitelist = 12}
+      local ok, err = v(t, schema)
+      assert.False(ok)
+      assert.same({whitelist = "whitelist is not a array"}, err)
+    end)
+    it("whitelist should not accept invalid IPs", function()
+      local t = {whitelist = "hello"}
+      local ok, err = v(t, schema)
+      assert.False(ok)
+      assert.same({whitelist = "cannot parse 'hello': Invalid IP"}, err)
+
+      t = {whitelist = {"127.0.0.1", "127.0.0.2", "hello"}}
+      ok, err = v(t, schema)
+      assert.False(ok)
+      assert.same({whitelist = "cannot parse 'hello': Invalid IP"}, err)
+    end)
+    it("blacklist should not accept invalid types", function()
+      local t = {blacklist = 12}
+      local ok, err = v(t, schema)
+      assert.False(ok)
+      assert.same({blacklist = "blacklist is not a array"}, err)
+    end)
+    it("blacklist should not accept invalid IPs", function()
+      local t = {blacklist = "hello"}
+      local ok, err = v(t, schema)
+      assert.False(ok)
+      assert.same({blacklist = "cannot parse 'hello': Invalid IP"}, err)
+
+      t = {blacklist = {"127.0.0.1", "127.0.0.2", "hello"}}
+      ok, err = v(t, schema)
+      assert.False(ok)
+      assert.same({blacklist = "cannot parse 'hello': Invalid IP"}, err)
+    end)
+    it("should not accept both a whitelist and a blacklist", function()
+      local t = {blacklist = {"127.0.0.1"}, whitelist = {"127.0.0.2"}}
+      local ok, err, self_err = v(t, schema)
+      assert.False(ok)
+      assert.falsy(err)
+      assert.equal("you cannot set both a whitelist and a blacklist", self_err.message)
+    end)
+    it("should not accept both empty whitelist and blacklist", function()
+      local t = {blacklist = {}, whitelist = {}}
+      local ok, err, self_err = v(t, schema)
+      assert.False(ok)
+      assert.falsy(err)
+      assert.equal("you must set at least a whitelist or blacklist", self_err.message)
+    end)
+  end)
+  describe("ok", function()
+    it("should accept a valid whitelist", function()
+      local t = {whitelist = {"127.0.0.1", "127.0.0.2"}}
+      local ok, err = v(t, schema)
+      assert.True(ok)
+      assert.falsy(err)
+    end)
+    it("should accept a valid blacklist", function()
+      local t = {blacklist = {"127.0.0.1", "127.0.0.2"}}
+      local ok, err = v(t, schema)
+      assert.True(ok)
+      assert.falsy(err)
+    end)
+    it("should build _whitelist_cache", function()
+      local t = {whitelist = {"127.0.0.1", "127.0.0.2"}}
+      local ok, err = v(t, schema)
+      assert.True(ok)
+      assert.falsy(err)
+      assert.is_table(t._whitelist_cache)
+    end)
+    it("should build _blacklist_cache", function()
+      local t = {blacklist = {"127.0.0.1", "127.0.0.2"}}
+      local ok, err = v(t, schema)
+      assert.True(ok)
+      assert.falsy(err)
+      assert.is_table(t._blacklist_cache)
+    end)
+  end)
+end)