From 9a7388d695c9de105917cde23a684a7d6722a3ca Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Thu, 25 Jun 2015 20:33:59 +0200 Subject: [PATCH] fix(api) Prevent PATCH from overriding plugin's value Since plugin_configuration's `value` is stored as plain text in Cassandra, if one made a PATCH request it could potentially override the stored value because the DAO treated the field just like any other. Instead, we need to set each property of a `value` that is not updated by the PATCH before updating the row. That behaviour doesn't apply to PUT requests. --- kong/api/crud_helpers.lua | 4 +- kong/dao/cassandra/base_dao.lua | 22 ++++++++ kong/dao/schemas_validation.lua | 4 +- .../admin_api/apis_routes_spec.lua | 51 +++++++++++++++++-- 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/kong/api/crud_helpers.lua b/kong/api/crud_helpers.lua index 60c859432cd1..9a394e4d220a 100644 --- a/kong/api/crud_helpers.lua +++ b/kong/api/crud_helpers.lua @@ -106,8 +106,8 @@ function _M.post(params, dao_collection, success) end end -function _M.patch(new_entity, old_entity, dao_collection) - for k, v in pairs(new_entity) do +function _M.patch(params, old_entity, dao_collection) + for k, v in pairs(params) do old_entity[k] = v end diff --git a/kong/dao/cassandra/base_dao.lua b/kong/dao/cassandra/base_dao.lua index 84188ed140eb..07661e5eaef9 100644 --- a/kong/dao/cassandra/base_dao.lua +++ b/kong/dao/cassandra/base_dao.lua @@ -411,6 +411,24 @@ local function extract_primary_key(t, primary_key, clustering_key) return t_primary_key, t_no_primary_key end +-- When updating a row that has a json-as-text column (ex: plugin_configuration.value), +-- we want to avoid overriding it with a partial value. +-- Ex: value.key_name + value.hide_credential, if we update only one field, +-- the other should be preserved. Of course this only applies in partial update. +local function fix_tables(t, old_t, schema) + for k, v in pairs(schema.fields) do + if v.schema then + local s = type(v.schema) == "function" and v.schema(t) or v.schema + for s_k, s_v in pairs(s.fields) do + if not t[k][s_k] and old_t[k] then + t[k][s_k] = old_t[k][s_k] + end + end + fix_tables(t[k], old_t[k], s) + end + end +end + -- Update a row: find the row with the given PRIMARY KEY and update the other values -- If `full`, sets to NULL values that are not included in the schema. -- Performs schema validation, UNIQUE and FOREIGN checks. @@ -432,6 +450,10 @@ function BaseDao:update(t, full) return false end + if not full then + fix_tables(t, res, self._schema) + end + -- Validate schema errors = validations.validate(t, self, {partial_update = not full, full_update = full}) if errors then diff --git a/kong/dao/schemas_validation.lua b/kong/dao/schemas_validation.lua index 8f9065c78338..062564a30ee3 100644 --- a/kong/dao/schemas_validation.lua +++ b/kong/dao/schemas_validation.lua @@ -132,8 +132,8 @@ function _M.validate_fields(t, schema, options) if sub_schema then -- Check for sub-schema defaults and required properties in advance - for sub_field_k, sub_field in pairs(sub_schema.fields) do - if t[column] == nil then + if t[column] == nil then + for sub_field_k, sub_field in pairs(sub_schema.fields) do if sub_field.default ~= nil then -- Sub-value has a default, be polite and pre-assign the sub-value t[column] = {} elseif sub_field.required then -- Only check required if field doesn't have a default diff --git a/spec/integration/admin_api/apis_routes_spec.lua b/spec/integration/admin_api/apis_routes_spec.lua index 2f49592a272a..67ebc7638a75 100644 --- a/spec/integration/admin_api/apis_routes_spec.lua +++ b/spec/integration/admin_api/apis_routes_spec.lua @@ -249,6 +249,7 @@ describe("Admin API", function() end) describe("PUT", function() + local plugin_id it("[FAILURE] should return proper errors", function() send_content_types(BASE_URL, "PUT", {}, @@ -268,21 +269,44 @@ describe("Admin API", function() response, status = http_client.put(BASE_URL, { name = "keyauth", - value = {key_names={"apikey"}} - }, {["content-type"]="application/json"}) + value = {key_names = {"apikey"}} + }, {["content-type"] = "application/json"}) assert.equal(201, status) body = json.decode(response) + plugin_id = body.id + response, status = http_client.put(BASE_URL, { - id = body.id, + id = plugin_id, name = "keyauth", - value = {key_names={"updated_apikey"}} - }, {["content-type"]="application/json"}) + value = {key_names = {"updated_apikey"}} + }, {["content-type"] = "application/json"}) assert.equal(200, status) body = json.decode(response) assert.equal("updated_apikey", body.value.key_names[1]) end) + it("should override a plugin's `value` if partial", function() + local response, status = http_client.put(BASE_URL, { + id = plugin_id, + name = "keyauth", + ["value.key_names"] = {"api_key"}, + ["value.hide_credentials"] = true + }) + assert.equal(200, status) + assert.truthy(response) + + response, status = http_client.put(BASE_URL, { + id = plugin_id, + name = "keyauth", + ["value.key_names"] = {"api_key_updated"} + }) + assert.equal(200, status) + local body = json.decode(response) + assert.same({"api_key_updated"}, body.value.key_names) + assert.falsy(body.hide_credentials) + end) + end) describe("GET", function() @@ -349,6 +373,23 @@ describe("Admin API", function() assert.equal(404, status) end) + it("should not override a plugin's `value` if partial", function() + -- This is delicate since a plugin's `value` is a text field in a DB like Cassandra + local response, status = http_client.patch(BASE_URL..plugin.name, { + ["value.key_names"] = {"key_set_null_test"}, + ["value.hide_credentials"] = true + }) + assert.equal(200, status) + + response, status = http_client.patch(BASE_URL..plugin.name, { + ["value.key_names"] = {"key_set_null_test_updated"} + }) + assert.equal(200, status) + local body = json.decode(response) + assert.same({"key_set_null_test_updated"}, body.value.key_names) + assert.equal(true, body.value.hide_credentials) + end) + end) describe("DELETE", function()