Spike: adding/updating annotations via PUT/POST/PATCH #2888

john-odonnell · 2023-08-07T13:27:30Z

Desired Outcome

Capture the current behavior of policy load operations to add new or update existing annotations on an existing resource.

From CNJR-2111:

POST should allow adding new annotations to an existing resource and PATCH should allow adding or modifying existing annotations on an existing resource.

Implemented Changes

Added Cucumber tests in cucumber/api/features/policy_load_annotations.feature that confirm whether a user with [create|update] privilege on a policy branch can [PUT|POST|PATCH] a policy file to [add a new|update an existing] annotation on an existing resource.

Outcome Summary

All these cases behave as we expect:

- create / add new         / PUT   :   EXPECTED FAIL
- create / add new         / POST  :   SUCCESS
- create / add new         / PATCH :   EXPECTED FAIL
- create / update existing / PUT   :   EXPECTED FAIL
- create / update existing / POST  :   EXPECTED FAIL*
- create / update existing / PATCH :   EXPECTED FAIL
- update / add new         / PUT   :   SUCCESS
- update / add new         / POST  :   EXPECTED FAIL
- update / add new         / PATCH :   SUCCESS
- update / update existing / PUT   :   SUCCESS
- update / update existing / POST  :   EXPECTED FAIL
- update / update existing / PATCH :   SUCCESS

The create / update existing / POST does not add a new annotation - as expected - but the policy load operation succeeds, which could be misleading to a user. This is caused by how the different policy loader classes - Loader::ReplacePolicy (PUT), Loader::CreatePolicy (POST), and Loader::ModifyPolicy (PATCH) - use the Loader::Orchastrate methods eliminate_duplicates_exact, eliminate_duplicates_pk and update_changed.

eliminate_duplicates_exact is used to cull objects specified in the incoming policy file that already existing in the current policy. Incoming updated annotations are not culled by this method.
update_changed is used to persist updated objects from the incoming policy into the current policy.
eliminate_duplicates_pk is used to cull objects that exist in the current policy that may have been updated by the incoming policy. Incoming updated annotations are always culled by this method, regardless of whether they have been persisted.

The Loader::CreatePolicy (POST) loader is the only loader class that does not call update_changed - this means eliminate_duplicates_exact and eliminate_duplicates_pk are run in sequence, deleting all objects that appear in both the existing and incoming policy files, regardless of updates.

Commit 211e432 implements a 400 error condition when a policy load via POST attempts to update an existing resource.

Connected Issue/Story

CNJR-2111

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a
CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code
changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: [insert issue ID]
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

codeclimate · 2023-08-17T23:39:15Z

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)


Loader::Orchestrate#store_policy_in_db has boolean parameter 'reject_duplicates'

codeclimate · 2023-08-17T23:39:15Z

app/models/loader/orchestrate.rb

-      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


Loader::Orchestrate#store_policy_in_db is controlled by argument 'reject_duplicates'

doodlesbykumbi

Looking good. Left a comment about defining a local error type for rejecting duplicates to avoid coupling.

doodlesbykumbi · 2023-08-18T18:56:57Z

cucumber/api/features/support/env.rb

@@ -5,7 +5,7 @@
 ENV['CONJUR_LOG_LEVEL'] ||= 'debug'

 parallel_cuke_vars = {}
-parallel_cuke_vars['CONJUR_APPLIANCE_URL'] = "http://conjur#{ENV['TEST_ENV_NUMBER']}"
+parallel_cuke_vars['CONJUR_APPLIANCE_URL'] = ENV.fetch('CONJUR_APPLIANCE_URL', "http://conjur#{ENV['TEST_ENV_NUMBER']}")


Curious about the motivation for this change ?

This enabled running cucumber tests in the dev env, where you run them from the Conjur container. In that case, you need to use http://localhost:3000 instead of http://conjur.

doodlesbykumbi · 2023-08-18T19:01:32Z

app/models/loader/orchestrate.rb

-      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


It's better to raise a local exception to avoid coupling the loader to the somewhat global ApplicationController. Have a look at Exceptions::InvalidPolicyObject for inspiration, and see how it is rescued from in ApplicationController, see

conjur/app/controllers/application_controller.rb

Line 61 in 956e8b6

rescue_from Exceptions::InvalidPolicyObject, with: :policy_invalid

Created Exceptions::DisallowedPolicyOperation

codeclimate · 2023-08-18T20:30:32Z

app/controllers/application_controller.rb

@@ -193,6 +194,17 @@ def policy_invalid e
    render(json: { error: error }, status: :unprocessable_entity)
  end

+  def disallowed_policy_operation e


ApplicationController#disallowed_policy_operation has the parameter name 'e'

codeclimate · 2023-08-18T20:30:32Z

app/models/loader/orchestrate.rb

-      eliminate_duplicates_pk
+    def store_policy_in_db(reject_duplicates: false)
+      removed_duplicates_count = eliminate_duplicates_pk
+      raise Exceptions::DisallowedPolicyOperation if removed_duplicates_count.positive? && reject_duplicates


Loader::Orchestrate#store_policy_in_db is controlled by argument 'reject_duplicates'

codeclimate · 2023-08-18T20:30:34Z

Code Climate has analyzed commit b1028a1 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.4% (0.0% change).

View more on Code Climate.

doodlesbykumbi

LGTM! Left a comment about adding more context to errors

doodlesbykumbi · 2023-08-21T15:06:51Z

app/models/exceptions/disallowed_policy_operation.rb

+  class DisallowedPolicyOperation < RuntimeError
+
+    def initialize
+      super("Updating existing resource disallowed in additive policy operation")


Thanks for addressing this. I was thinking it might be useful to provide any meaningful context around the error like how it's done here.

conjur/app/models/exceptions/invalid_policy_object.rb

Line 7 in 956e8b6

def initialize id, message: nil

Specifically I had in mind

Policy ID

Offending lines etc.

Revert breaking changes to policy API from #2888

john-odonnell force-pushed the CNJR-2102 branch 2 times, most recently from 87d24a1 to 9730a9b Compare August 14, 2023 13:07

john-odonnell marked this pull request as ready for review August 14, 2023 13:31

john-odonnell requested a review from a team as a code owner August 14, 2023 13:31

john-odonnell force-pushed the CNJR-2102 branch 5 times, most recently from 6d4f4fa to 6262fd7 Compare August 17, 2023 22:52

codeclimate bot reviewed Aug 17, 2023

View reviewed changes

john-odonnell force-pushed the CNJR-2102 branch from 6262fd7 to 2f9ed46 Compare August 18, 2023 15:02

john-odonnell added 4 commits August 18, 2023 13:53

Cukes: use CONJUR_APPLIANCE_URL before http://conjur

7f2be27

Capture current behavior of policy load to create/update annot

c50cdea

Tag new Cucumber tests

36c8dbe

Fail additive policy load requests that update existing resources

9141a2d

john-odonnell force-pushed the CNJR-2102 branch from 2f9ed46 to 9141a2d Compare August 18, 2023 17:53

doodlesbykumbi reviewed Aug 18, 2023

View reviewed changes

Create and use unique DisallowedPolicyOperation exception

b1028a1

codeclimate bot reviewed Aug 18, 2023

View reviewed changes

doodlesbykumbi approved these changes Aug 21, 2023

View reviewed changes

john-odonnell merged commit bf420cd into master Aug 21, 2023
2 checks passed

john-odonnell deleted the CNJR-2102 branch August 21, 2023 15:30

john-odonnell mentioned this pull request Sep 5, 2023

Revert breaking changes to policy API from #2888 #2940

Merged

13 tasks

john-odonnell added a commit that referenced this pull request Sep 5, 2023

Merge pull request #2940 from cyberark/revert-policy-api-changes

eb4b353

Revert breaking changes to policy API from #2888

john-odonnell mentioned this pull request Sep 7, 2023

Log warning of dropped updates to existing resources in policy POST #2944

Draft

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike: adding/updating annotations via PUT/POST/PATCH #2888

Spike: adding/updating annotations via PUT/POST/PATCH #2888

john-odonnell commented Aug 7, 2023 •

edited

Loading

codeclimate bot Aug 17, 2023

codeclimate bot Aug 17, 2023

doodlesbykumbi left a comment

doodlesbykumbi Aug 18, 2023

john-odonnell Aug 18, 2023

doodlesbykumbi Aug 18, 2023

john-odonnell Aug 18, 2023

codeclimate bot Aug 18, 2023

codeclimate bot Aug 18, 2023

codeclimate bot commented Aug 18, 2023

doodlesbykumbi left a comment

doodlesbykumbi Aug 21, 2023

Spike: adding/updating annotations via PUT/POST/PATCH #2888

Spike: adding/updating annotations via PUT/POST/PATCH #2888

Conversation

john-odonnell commented Aug 7, 2023 • edited Loading

Desired Outcome

Implemented Changes

Outcome Summary

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Aug 17, 2023

Choose a reason for hiding this comment

codeclimate bot Aug 17, 2023

Choose a reason for hiding this comment

doodlesbykumbi left a comment

Choose a reason for hiding this comment

doodlesbykumbi Aug 18, 2023

Choose a reason for hiding this comment

john-odonnell Aug 18, 2023

Choose a reason for hiding this comment

doodlesbykumbi Aug 18, 2023

Choose a reason for hiding this comment

john-odonnell Aug 18, 2023

Choose a reason for hiding this comment

codeclimate bot Aug 18, 2023

Choose a reason for hiding this comment

codeclimate bot Aug 18, 2023

Choose a reason for hiding this comment

codeclimate bot commented Aug 18, 2023

doodlesbykumbi left a comment

Choose a reason for hiding this comment

doodlesbykumbi Aug 21, 2023

Choose a reason for hiding this comment

john-odonnell commented Aug 7, 2023 •

edited

Loading