diff --git a/CHANGELOG.md b/CHANGELOG.md index 08c7c4b3d7..fdb703f76a 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 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 [cyberark/conjur#2926](https://github.com/cyberark/conjur/pull/2926) 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..f09e0809bc --- /dev/null +++ b/app/models/exceptions/disallowed_policy_operation.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Exceptions + class DisallowedPolicyOperation < RuntimeError + + 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/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..3f608ef44d 100644 --- a/app/models/loader/orchestrate.rb +++ b/app/models/loader/orchestrate.rb @@ -120,7 +120,12 @@ def delete_shadowed_and_duplicate_rows end # TODO: consider renaming this method - def store_policy_in_db + def store_policy_in_db(reject_duplicates: false) + if reject_duplicates + duplicates = detect_duplicates_pk + Rails.logger.warn(Exceptions::DisallowedPolicyOperation.new(duplicates)) if duplicates.present? + end + eliminate_duplicates_pk insert_new @@ -243,12 +248,99 @@ 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 + # 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 cc2e52c8b5..764eb4f375 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 - 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 @@ -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 201 + 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": [ - - ] - } + 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 diff --git a/cucumber/api/features/policy_load_modes.feature b/cucumber/api/features/policy_load_modes.feature index 3f0a66ac20..ec7a031414 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 422 + 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