From 09105c178fe8e2c061361855e73aa75e31d1ec3d Mon Sep 17 00:00:00 2001 From: Aapo Talvensaari Date: Mon, 20 Jan 2020 19:17:32 +0200 Subject: [PATCH] fix(declarative) schema validation in db-less mode (#5464) ### Summary This was reported with #5401. The changes for adding DAO transformations ended up breaking schema validation on declarative mode. This commit fixes that. While fixing this, I also found out that original validation didn't work correctly when you had entity validations that relied on values of foreign keys. This is now also corrected. ### Issues Resolved Fix #5401 --- kong/db/schema/others/declarative_config.lua | 168 +++++---- .../11-declarative_config/03-flatten_spec.lua | 328 ++++++++++++++++++ 2 files changed, 425 insertions(+), 71 deletions(-) diff --git a/kong/db/schema/others/declarative_config.lua b/kong/db/schema/others/declarative_config.lua index e27825753f8f..62f40f8cbf19 100644 --- a/kong/db/schema/others/declarative_config.lua +++ b/kong/db/schema/others/declarative_config.lua @@ -121,8 +121,12 @@ local function add_top_level_entities(fields, entities) end -local function copy_without_foreign(record) +local function copy_record(record, include_foreign) local copy = utils.deep_copy(record, false) + if include_foreign then + return copy + end + for i = #copy.fields, 1, -1 do local f = copy.fields[i] local _, fdata = next(f) @@ -144,10 +148,10 @@ end -- (e.g. `service` as a string key in the `routes` entry). -- @tparam map records A map of top-level record definitions, -- indexable by entity name. These records are modified in-place. -local function nest_foreign_relationships(records) +local function nest_foreign_relationships(records, include_foreign) for entity, record in pairs(records) do for _, f in ipairs(record.fields) do - local fname, fdata = next(f) + local _, fdata = next(f) if fdata.type == "foreign" then local ref = fdata.reference -- allow nested entities @@ -155,7 +159,7 @@ local function nest_foreign_relationships(records) table.insert(records[ref].fields, { [entity] = { type = "array", - elements = copy_without_foreign(record, fname), + elements = copy_record(record, include_foreign), }, }) end @@ -187,7 +191,7 @@ local function reference_foreign_by_name(records) end -local function build_fields(entities) +local function build_fields(entities, include_foreign) local fields = { { _format_version = { type = "string", required = true, eq = "1.1" } }, } @@ -197,7 +201,7 @@ local function build_fields(entities) }) local records = add_top_level_entities(fields, entities) - nest_foreign_relationships(records) + nest_foreign_relationships(records, include_foreign) return fields, records end @@ -482,41 +486,98 @@ local function generate_ids(input, known_entities, parent_entity) end +local function populate_ids(input, known_entities, parent_entity, by_id, by_key) + local by_id = by_id or {} + local by_key = by_key or {} + for _, entity in ipairs(known_entities) do + if type(input[entity]) ~= "table" then + goto continue + end + + local parent_fk + local child_key + if parent_entity then + local parent_schema = all_schemas[parent_entity] + if parent_schema.fields[entity] then + goto continue + end + parent_fk = parent_schema:extract_pk_values(input) + child_key = foreign_children[parent_entity][entity] + end + + local schema = all_schemas[entity] + for _, item in ipairs(input[entity]) do + local pk_name, key = get_key_for_uuid_gen(entity, item, schema, + parent_fk, child_key) + if key and not item[pk_name] then + item[pk_name] = generate_uuid(schema.name, key) + end + + populate_ids(item, known_entities, entity, by_id, by_key) + + local item_id = DeclarativeConfig.pk_string(schema, item) + by_id[entity] = by_id[entity] or {} + by_id[entity][item_id] = item + + local key + if schema.endpoint_key then + key = item[schema.endpoint_key] + if key then + by_key[entity] = by_key[entity] or {} + by_key[entity][key] = item + end + end + + if parent_fk and not item[child_key] then + item[child_key] = utils.deep_copy(parent_fk, false) + end + end + + ::continue:: + end + + if not parent_entity then + for entity, entries in pairs(by_id) do + local schema = all_schemas[entity] + for _, entry in pairs(entries) do + for name, field in schema:each_field(entry) do + if field.type == "foreign" and type(entry[name]) == "string" then + local found = find_entity(entry[name], field.reference, by_key, by_id) + if found then + entry[name] = all_schemas[field.reference]:extract_pk_values(found) + end + end + end + end + end + end +end + + local function flatten(self, input) local output = {} - -- This may give invalid errors as it validates input that does not have - -- foreign keys filled (e.g. basicauth_credentials fail on this with this - -- config: - -- - -- consumers: - -- username: consumer - -- basicauth_credentials: - -- - username: username - -- password: password - -- - -- because it tries to validate the credential that has no consumer.id at - -- this point filled in, and transformation implicitly adds mutually_required - -- check on that entity. - -- - -- As a short-term solution, we don't error right away here. local ok, err = self:validate(input) + if not ok then + -- the error may be due entity validation that depends on foreign entity, + -- and that is the reason why we try to validate the input again with the + -- filled foreign keys + local input_copy = utils.deep_copy(input, false) + populate_ids(input_copy, self.known_entities) + local schema = DeclarativeConfig.load(self.plugin_set, true) + if not schema:validate(input_copy) then + return nil, err + end + end generate_ids(input, self.known_entities) local processed = self:process_auto_fields(input, "insert") - local by_id, by_key_or_err = validate_references(self, processed) + local by_id, by_key = validate_references(self, processed) if not by_id then - -- let's return original error in case it is defined, as the reference - -- validation could have failed because of that. - if not ok then - return nil, err - end - - return nil, by_key_or_err + return nil, by_key end - local by_key = by_key_or_err for entity, entries in pairs(by_id) do local schema = all_schemas[entity] @@ -535,44 +596,6 @@ local function flatten(self, input) end end - -- TODO: is the plugin not enabled a sign of something wrong somewhere? - local ok2, err2 = schema:validate(flat_entry) - if not ok2 - and err2 - and err2.name ~= string.format("plugin '%s' not enabled; add it to " .. - "the 'plugins' configuration property", - flat_entry.name) - then - - if err then - if err2["@entity"] then - for _, errmsg2 in ipairs(err2["@entity"]) do - local found - if err["@entity"] then - for _, errmsg in ipairs(err["@entity"]) do - if errmsg2 == errmsg then - found = true - break - end - end - end - - if not found then - if not err["@entity"] then - err["@entity"] = {} - end - - table.insert(err["@entity"], errmsg2) - end - end - end - - return nil, err - end - - return nil, err2 - end - output[entity][id] = flat_entry end end @@ -596,7 +619,7 @@ local function load_entity_subschemas(entity_name, entity) end -function DeclarativeConfig.load(plugin_set) +function DeclarativeConfig.load(plugin_set, include_foreign) if not core_entities then -- a copy of constants.CORE_ENTITIES without "tags" core_entities = {} @@ -633,7 +656,7 @@ function DeclarativeConfig.load(plugin_set) end end - local fields, records = build_fields(known_entities) + local fields, records = build_fields(known_entities, include_foreign) -- assert(no_foreign(fields)) local ok, err = load_plugin_subschemas(fields, plugin_set) @@ -644,7 +667,9 @@ function DeclarativeConfig.load(plugin_set) -- we replace the "foreign"-type fields at the top-level -- with "string"-type fields only after the subschemas have been loaded, -- otherwise they will detect the mismatch. - reference_foreign_by_name(records) + if not include_foreign then + reference_foreign_by_name(records) + end local def = { name = "declarative_config", @@ -656,6 +681,7 @@ function DeclarativeConfig.load(plugin_set) schema.known_entities = known_entities schema.flatten = flatten + schema.plugin_set = plugin_set return schema, nil, def end diff --git a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua index 5ea0cac4ec05..9e5637431d30 100644 --- a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua +++ b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua @@ -1151,6 +1151,334 @@ describe("declarative config: flatten", function() end) describe("custom entities", function() + describe("basicauth_credentials:", function() + it("accepts an empty list", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + basicauth_credentials: + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({}, idempotent(config)) + + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + basicauth_credentials: + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({}, idempotent(config)) + + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("accepts as a nested entity", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - username: username + password: password + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("accepts as a nested entity by id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - username: username + password: password + consumer: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as a nested entity by incorrect id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - username: username + password: password + consumer: 00000000-0000-0000-0000-000000000000 + ]])) + local config, err = DeclarativeConfig:flatten(config) + + assert.equal(nil, config) + assert.same({ + consumers = { + { + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + consumer = 'value must be null', + }, + }, + }, + }, + }, idempotent(err)) + end) + + it("accepts as a nested entity by username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - username: username + password: password + consumer: consumer + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as a nested entity by incorrect username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - username: username + password: password + consumer: incorrect + ]])) + local config, err = DeclarativeConfig:flatten(config) + assert.equal(nil, config) + assert.same({ + consumers = { + { + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + consumer = 'value must be null', + }, + }, + }, + }, + }, idempotent(err)) + end) + + it("accepts as an unnested entity by id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - consumer: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: username + password: password + + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as an unnested entity by incorrect id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - consumer: 00000000-0000-0000-0000-000000000000 + username: username + password: password + + ]])) + local config, err = DeclarativeConfig:flatten(config) + assert.equal(nil, config) + assert.same({ + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + }, + }, + }, idempotent(err)) + end) + + it("accepts as an unnested entity by username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - consumer: consumer + username: username + password: password + + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as an unnested entity by incorrect username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - consumer: incorrect + username: username + password: password + + ]])) + local config, err = DeclarativeConfig:flatten(config) + assert.equal(nil, config) + assert.same({ + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + }, + }, + }, idempotent(err)) + end) + end) + describe("oauth2_credentials:", function() it("accepts an empty list", function() local config = assert(lyaml.load([[