From 9ae81584c537eebb44363faff4cc4ec988126865 Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Wed, 6 Sep 2023 11:08:38 -0400 Subject: [PATCH 1/3] Reinstate "Fail additive policy loads that update existing resources" This reverts commit 4f137d407435c7341f5c419d950d27d70ca41577. This reinstates commit 9141a2dc60c9097eafcbc0db5e0d317f7a4d7a8f. --- CHANGELOG.md | 3 ++ app/models/loader/create_policy.rb | 2 +- app/models/loader/orchestrate.rb | 8 ++-- .../features/policy_load_annotations.feature | 46 +++---------------- .../api/features/policy_load_modes.feature | 15 ++---- 5 files changed, 21 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08c7c4b3d7..e651619191 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. mitigates the possibility of a web worker becoming starved while waiting for a connection to become available. [cyberark/conjur#2875](https://github.com/cyberark/conjur/pull/2875) +- Additive policy requests submitted via POST are rejected with a 400 status if + they attempt to update an existing resource. + [cyberark/conjur#2888](https://github.com/cyberark/conjur/pull/2888) - Changed base-image tagging strategy [cyberark/conjur#2926](https://github.com/cyberark/conjur/pull/2926) diff --git a/app/models/loader/create_policy.rb b/app/models/loader/create_policy.rb index f3aac3dc67..128a179edc 100644 --- a/app/models/loader/create_policy.rb +++ b/app/models/loader/create_policy.rb @@ -16,7 +16,7 @@ def call @loader.delete_shadowed_and_duplicate_rows - @loader.store_policy_in_db + @loader.store_policy_in_db(reject_duplicates: true) @loader.release_db_connection end diff --git a/app/models/loader/orchestrate.rb b/app/models/loader/orchestrate.rb index f9666d302b..299fe8d218 100644 --- a/app/models/loader/orchestrate.rb +++ b/app/models/loader/orchestrate.rb @@ -120,8 +120,9 @@ def delete_shadowed_and_duplicate_rows end # TODO: consider renaming this method - def store_policy_in_db - eliminate_duplicates_pk + def store_policy_in_db(reject_duplicates: false) + removed_duplicates_count = eliminate_duplicates_pk + raise ApplicationController::BadRequest, "Updating existing resource disallowed in additive policy operation" if removed_duplicates_count.positive? && reject_duplicates insert_new @@ -243,8 +244,9 @@ def eliminate_duplicates_exact end # Delete rows from the new policy which have the same primary keys as existing rows. + # Returns the total number of deleted rows. def eliminate_duplicates_pk - TABLES.each do |table| + TABLES.sum do |table| eliminate_duplicates(table, Array(model_for_table(table).primary_key) + [ :policy_id ]) end end diff --git a/cucumber/api/features/policy_load_annotations.feature b/cucumber/api/features/policy_load_annotations.feature index cc2e52c8b5..c21c547409 100644 --- a/cucumber/api/features/policy_load_annotations.feature +++ b/cucumber/api/features/policy_load_annotations.feature @@ -17,7 +17,7 @@ Feature: Updating Policies with Annotations - create / add new / POST : EXPECTED SUCCESS - create / add new / PATCH : EXPECTED FAIL - 403 on policy load - create / update existing / PUT : EXPECTED FAIL - 403 on policy load - - create / update existing / POST : EXPECTED FAIL - 20x on policy load, annot not updated + - create / update existing / POST : EXPECTED FAIL - 400 on policy load - create / update existing / PATCH : EXPECTED FAIL - 403 on policy load - update / add new / PUT : EXPECTED SUCCESS - update / add new / POST : EXPECTED FAIL - 403 on policy load @@ -26,13 +26,6 @@ Feature: Updating Policies with Annotations - update / update existing / POST : EXPECTED FAIL - 403 on policy load - update / update existing / PATCH : EXPECTED SUCCESS - All these outcomes align with our expectations, but one may not align with - user expectations: ( create / update existing / POST ). A user may expect that - a policy load that tries and fails to update the content of a given annotation - should either provide a warning or fail outright. - - How can we update how we handle policy to fail in this case? - Background: Given I am the super-user And I successfully PUT "/policies/cucumber/policy/root" with body: @@ -183,45 +176,20 @@ Feature: Updating Policies with Annotations @negative @acceptance - Scenario: User with create privilege CAN NOT update existing annotations with POST, but policy loads successfully + Scenario: User with create privilege CAN NOT update existing annotations with POST When I login as "alice" - Then I successfully POST "/policies/cucumber/policy/hosts" with body: + And I save my place in the log file + Then I POST "/policies/cucumber/policy/hosts" with body: """ - !host id: annotated annotations: description: Success """ - And I successfully GET "/resources/cucumber/host/hosts%2Fannotated" - Then the JSON should be: + Then the HTTP response status code is 400 + And The following appears in the log after my savepoint: """ - { - "annotations": [ - { - "name": "description", - "policy": "cucumber:policy:hosts", - "value": "Already annotated" - } - ], - "id": "cucumber:host:hosts/annotated", - "owner": "cucumber:policy:hosts", - "permissions": [ - { - "policy": "cucumber:policy:root", - "privilege": "read", - "role": "cucumber:user:bob" - }, - { - "policy": "cucumber:policy:root", - "privilege": "read", - "role": "cucumber:user:alice" - } - ], - "policy": "cucumber:policy:hosts", - "restricted_to": [ - - ] - } + Updating existing resource disallowed in additive policy operation """ @negative diff --git a/cucumber/api/features/policy_load_modes.feature b/cucumber/api/features/policy_load_modes.feature index 3f0a66ac20..eed60df3ef 100644 --- a/cucumber/api/features/policy_load_modes.feature +++ b/cucumber/api/features/policy_load_modes.feature @@ -172,22 +172,17 @@ Feature: Updating policies @acceptance Scenario: POST cannot update existing policy records - When I successfully POST "/policies/cucumber/policy/dev/db" with body: + When I save my place in the log file + And I POST "/policies/cucumber/policy/dev/db" with body: """ - !variable id: b kind: private key """ - When I successfully GET "/resources/cucumber/variable/dev/db/b" - Then the JSON at "annotations" should be: + Then the HTTP response status code is 400 + And The following appears in the log after my savepoint: """ - [ - { - "name": "conjur/kind", - "policy": "cucumber:policy:dev/db", - "value": "password" - } - ] + Updating existing resource disallowed in additive policy operation """ @negative @acceptance From 3aab880e2516092ee91d97192f01518e28ee726d Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Wed, 6 Sep 2023 11:09:58 -0400 Subject: [PATCH 2/3] Reinstate "Create and use unique DisallowedPolicyOperation exception" This reverts commit 36b780379ba7d2a83df25afa0083949e33b7ca67. This reinstates commit b1028a102c5c604b460d941acb60976729f4a4b3. --- app/controllers/application_controller.rb | 12 ++++++++++++ app/models/exceptions/disallowed_policy_operation.rb | 11 +++++++++++ app/models/loader/orchestrate.rb | 2 +- .../api/features/policy_load_annotations.feature | 4 ++-- cucumber/api/features/policy_load_modes.feature | 2 +- 5 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 app/models/exceptions/disallowed_policy_operation.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2b53930016..f079873542 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -59,6 +59,7 @@ class UnprocessableEntity < RuntimeError rescue_from Sequel::ForeignKeyConstraintViolation, with: :foreign_key_constraint_violation rescue_from Conjur::PolicyParser::Invalid, with: :policy_invalid rescue_from Exceptions::InvalidPolicyObject, with: :policy_invalid + rescue_from Exceptions::DisallowedPolicyOperation, with: :disallowed_policy_operation rescue_from ArgumentError, with: :argument_error rescue_from ActionController::ParameterMissing, with: :argument_error rescue_from UnprocessableEntity, with: :unprocessable_entity @@ -193,6 +194,17 @@ def policy_invalid e render(json: { error: error }, status: :unprocessable_entity) end + def disallowed_policy_operation e + logger.debug("#{e}\n#{e.backtrace.join("\n")}") + + render(json: { + error: { + code: "disallowed_policy_operation", + message: e.message + } + }, status: :unprocessable_entity) + end + def argument_error e logger.debug("#{e}\n#{e.backtrace.join("\n")}") diff --git a/app/models/exceptions/disallowed_policy_operation.rb b/app/models/exceptions/disallowed_policy_operation.rb new file mode 100644 index 0000000000..84d34f0bbf --- /dev/null +++ b/app/models/exceptions/disallowed_policy_operation.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Exceptions + class DisallowedPolicyOperation < RuntimeError + + def initialize + super("Updating existing resource disallowed in additive policy operation") + end + + end +end diff --git a/app/models/loader/orchestrate.rb b/app/models/loader/orchestrate.rb index 299fe8d218..4b2dda2f59 100644 --- a/app/models/loader/orchestrate.rb +++ b/app/models/loader/orchestrate.rb @@ -122,7 +122,7 @@ def delete_shadowed_and_duplicate_rows # TODO: consider renaming this method def store_policy_in_db(reject_duplicates: false) removed_duplicates_count = eliminate_duplicates_pk - raise ApplicationController::BadRequest, "Updating existing resource disallowed in additive policy operation" if removed_duplicates_count.positive? && reject_duplicates + raise Exceptions::DisallowedPolicyOperation if removed_duplicates_count.positive? && reject_duplicates insert_new diff --git a/cucumber/api/features/policy_load_annotations.feature b/cucumber/api/features/policy_load_annotations.feature index c21c547409..4ecad06154 100644 --- a/cucumber/api/features/policy_load_annotations.feature +++ b/cucumber/api/features/policy_load_annotations.feature @@ -17,7 +17,7 @@ Feature: Updating Policies with Annotations - create / add new / POST : EXPECTED SUCCESS - create / add new / PATCH : EXPECTED FAIL - 403 on policy load - create / update existing / PUT : EXPECTED FAIL - 403 on policy load - - create / update existing / POST : EXPECTED FAIL - 400 on policy load + - create / update existing / POST : EXPECTED FAIL - 422 on policy load - create / update existing / PATCH : EXPECTED FAIL - 403 on policy load - update / add new / PUT : EXPECTED SUCCESS - update / add new / POST : EXPECTED FAIL - 403 on policy load @@ -186,7 +186,7 @@ Feature: Updating Policies with Annotations annotations: description: Success """ - Then the HTTP response status code is 400 + Then the HTTP response status code is 422 And The following appears in the log after my savepoint: """ Updating existing resource disallowed in additive policy operation diff --git a/cucumber/api/features/policy_load_modes.feature b/cucumber/api/features/policy_load_modes.feature index eed60df3ef..ec7a031414 100644 --- a/cucumber/api/features/policy_load_modes.feature +++ b/cucumber/api/features/policy_load_modes.feature @@ -179,7 +179,7 @@ Feature: Updating policies id: b kind: private key """ - Then the HTTP response status code is 400 + Then the HTTP response status code is 422 And The following appears in the log after my savepoint: """ Updating existing resource disallowed in additive policy operation From 290f9e8ed476696d6674900a1cadcc894530f4de Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Thu, 7 Sep 2023 09:14:50 -0400 Subject: [PATCH 3/3] Update existing object w/ policy POST: descriptive warn instead of fail --- CHANGELOG.md | 2 +- .../exceptions/disallowed_policy_operation.rb | 14 ++- app/models/loader/orchestrate.rb | 94 ++++++++++++++++++- .../features/policy_load_annotations.feature | 4 +- 4 files changed, 107 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e651619191..fdb703f76a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. mitigates the possibility of a web worker becoming starved while waiting for a connection to become available. [cyberark/conjur#2875](https://github.com/cyberark/conjur/pull/2875) -- Additive policy requests submitted via POST are rejected with a 400 status if +- Additive policy requests submitted via POST are rejected with a 422 status if they attempt to update an existing resource. [cyberark/conjur#2888](https://github.com/cyberark/conjur/pull/2888) - Changed base-image tagging strategy diff --git a/app/models/exceptions/disallowed_policy_operation.rb b/app/models/exceptions/disallowed_policy_operation.rb index 84d34f0bbf..f09e0809bc 100644 --- a/app/models/exceptions/disallowed_policy_operation.rb +++ b/app/models/exceptions/disallowed_policy_operation.rb @@ -3,9 +3,19 @@ module Exceptions class DisallowedPolicyOperation < RuntimeError - def initialize - super("Updating existing resource disallowed in additive policy operation") + def initialize(context) + super(self.class.build_message(context)) + + @context = context end + class << self + def build_message(context) + "WARNING: Updating existing resource disallowed in additive policy operations (POST). " \ + "In a future release, loading this policy file will fail with a 422 error code. " \ + "The following updates have not been applied, and have been discarded: " \ + "#{context.inspect}" + end + end end end diff --git a/app/models/loader/orchestrate.rb b/app/models/loader/orchestrate.rb index 4b2dda2f59..3f608ef44d 100644 --- a/app/models/loader/orchestrate.rb +++ b/app/models/loader/orchestrate.rb @@ -121,8 +121,12 @@ def delete_shadowed_and_duplicate_rows # TODO: consider renaming this method def store_policy_in_db(reject_duplicates: false) - removed_duplicates_count = eliminate_duplicates_pk - raise Exceptions::DisallowedPolicyOperation if removed_duplicates_count.positive? && reject_duplicates + if reject_duplicates + duplicates = detect_duplicates_pk + Rails.logger.warn(Exceptions::DisallowedPolicyOperation.new(duplicates)) if duplicates.present? + end + + eliminate_duplicates_pk insert_new @@ -251,6 +255,92 @@ def eliminate_duplicates_pk end end + # Returns a hash of table names related to an array of hashes describing + # changes to existing resources made by the current policy load operation. + def detect_duplicates_pk + TABLES.each_with_object({}) do |table, duplicates| + table_dupes = duplicates_in_table( + table, + primary_keys(table) + [ :policy_id ], + non_primary_keys(table) + ) + duplicates[table] = table_dupes if table_dupes.present? + end + end + + # Finds entries in the temporary incoming schema's table that duplicate + # entries in the primary schema's table along it's primary_columns. Returns + # an array of hashes, where each hash describes an existing resource, and + # has a :diff entry describes changes made to the resource by the current + # policy load operation. + def duplicates_in_table table, primary_columns, non_primary_columns + primary_selections = primary_columns.map do |column| + "new_#{table}.#{column}" + end.join(', ') + + non_primary_selections = non_primary_columns.map do |column| + "new_#{table}.#{column} AS new_#{column}, old_#{table}.#{column} AS old_#{column}" + end.join(', ') + + selections = "#{primary_selections}" + selections << ", #{non_primary_selections}" unless non_primary_selections.blank? + + comparisons = primary_columns.map do |column| + "new_#{table}.#{column} = old_#{table}.#{column}" + end.join(' AND ') + + # This query returns an array of hashes. Each hash describes an existing + # resource that is being updated by the current policy load operation and + # the desired updates. Each hash contains: + # - a single entry for each primary key in the target table, and + # - two entries for each non-primary key in the target table, prepended + # with "old_" and "new_". + query = <<~QUERY + SELECT #{selections} + FROM #{table} AS new_#{table} + JOIN #{qualify_table(table)} AS old_#{table} + ON #{comparisons} + QUERY + + # Convert the hash returned by query into an easily consumable format. + consumable_diff(table, db[query].all) + end + + # This method expects the duplicates argument to be an array of hashes as + # described in the method above. This method returns an array of hashes, + # where each contains: + # - a single entry for each primary key in the target table, and + # - a :diff entry, which stores a hash of non-primary keys related to both + # its original and updated values. + def consumable_diff table, duplicates + duplicates.each_with_object([]) do |duplicate, dupes_a| + dupe_h = {} + pk_columns = primary_keys(table) + pk_columns.each do |column| + dupe_h[column] = duplicate[column] + end + + dupe_h[:diff] = non_primary_keys(table).each_with_object({}) do |column, diff| + if duplicate["old_#{column}".to_sym] != duplicate["new_#{column}".to_sym] + diff[column] = [ + duplicate["old_#{column}".to_sym], + duplicate["new_#{column}".to_sym] + ] + end + end + + dupes_a << dupe_h + end + end + + def primary_keys table + Array(model_for_table(table).primary_key) + end + + def non_primary_keys table + TABLE_EQUIVALENCE_COLUMNS[table] - primary_keys(table) + end + # Eliminate duplicates from a table, using the specified comparison columns. def eliminate_duplicates table, columns comparisons = columns.map do |column| diff --git a/cucumber/api/features/policy_load_annotations.feature b/cucumber/api/features/policy_load_annotations.feature index 4ecad06154..764eb4f375 100644 --- a/cucumber/api/features/policy_load_annotations.feature +++ b/cucumber/api/features/policy_load_annotations.feature @@ -186,10 +186,10 @@ Feature: Updating Policies with Annotations annotations: description: Success """ - Then the HTTP response status code is 422 + Then the HTTP response status code is 201 And The following appears in the log after my savepoint: """ - Updating existing resource disallowed in additive policy operation + WARNING: Updating existing resource disallowed in additive policy operations (POST). In a future release, loading this policy file will fail with a 422 error code. The following updates have not been applied, and have been discarded: {:annotations=>[{:resource_id=>"cucumber:host:hosts/annotated", :name=>"description", :diff=>{:value=>["Already annotated", "Success"]}}]} """ @negative