Log warning of dropped updates to existing resources in policy POST

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)
 

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")}")
 

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

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

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|

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

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