Merge pull request #857 from Conjur-Enterprise/422

CNJR-9138: Return 422 when policy references non-existent resource

Author: szh

CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Nothing should go in this section, please add to the latest unreleased version
   (and update the corresponding date), or add a new version.
 
-## [1.22.0] - 2025-01-13
+## [1.22.0] - 2025-04-02
 
 ### Added
 - Added the dynamic secrets Issuers API and data model. CNJR-7828
@@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Attempt to authenticate using the built-in authenticator (`authn`) with a GET
   request now results in a `404` response, rather than logging an authenticator
   not enabled message. CNJR-5854
+- Attempt to load a policy that references a non-existent resource now
+  results in a `422` response, rather than a `404` error. CNJR-9122
 - Set the default and maximal limit value for resources list API to 1000 in order
   to align with the documentation. CNJR-8485
 - Ensure Kubernetes authenticator websocket connections are closed when a

app/controllers/application_controller.rb

@@ -67,6 +67,7 @@ class UnprocessableEntity < RuntimeError
   rescue_from Sequel::ForeignKeyConstraintViolation, with: :foreign_key_constraint_violation
   rescue_from Exceptions::EnhancedPolicyError, with: :enhanced_policy_error
   rescue_from Exceptions::InvalidPolicyObject, with: :policy_invalid
+  rescue_from Exceptions::PolicyLoadRecordNotFound, with: :policy_invalid
   rescue_from Conjur::PolicyParser::Invalid, with: :policy_invalid
   rescue_from Conjur::PolicyParser::ResolverError, with: :policy_invalid
   rescue_from NoMethodError, with: :validation_failed
@@ -138,7 +139,7 @@ def foreign_key_constraint_violation e
     if e.is_a?(Sequel::ForeignKeyConstraintViolation) &&
       e.cause.is_a?(PG::ForeignKeyViolation) &&
       (e.cause.result.error_field(PG::PG_DIAG_MESSAGE_DETAIL) =~ /Key \(([^)]+)\)=\(([^)]+)\) is not present in table "([^"]+)"/  rescue false)
-      violating_key = $2
+      violating_key = ::Regexp.last_match(2)
 
       exc = Exceptions::RecordNotFound.new(violating_key)
       render_record_not_found(exc)
@@ -178,7 +179,7 @@ def validation_failed e
   def policy_invalid e
     logger.debug("#{e}\n#{e.backtrace.join("\n")}")
 
-    msg = e.message == nil ? e.to_s : e.message
+    msg = e.message.nil? ? e.to_s : e.message
     error = { code: "policy_invalid", message: msg }
 
     if e.instance_of?(Conjur::PolicyParser::Invalid)

app/controllers/policies_controller.rb

@@ -18,7 +18,7 @@ class PoliciesController < RestController
   def get
     action = :read
     unless params[:kind] == 'policy'
-      raise(Errors::EffectivePolicy::PathParamError.new(params[:kind]))
+      raise Errors::EffectivePolicy::PathParamError, params[:kind]
     end
 
     authorize_ownership
@@ -152,6 +152,7 @@ def load_policy(mode, mode_class, delete_permitted:)
         get_policy_mode.call(strategy_class)
         policy_mode.call_pr(policy_result) unless policy_erred.call
       rescue => e
+        e = sql_to_policy_error(e)
         policy_result.error=(enhance_error(e))
       end
     }
@@ -212,7 +213,7 @@ def load_policy(mode, mode_class, delete_permitted:)
   rescue => e
     original_error = e
     if e.instance_of?(Exceptions::EnhancedPolicyError) && e.original_error
-        original_error = e.original_error
+      original_error = e.original_error
     end
 
     audit_failure(original_error, mode)
@@ -337,4 +338,20 @@ def parse_submitted_policy(policy_result)
   def publish_event
     Monitoring::PubSub.instance.publish('conjur.policy_loaded')
   end
+
+  # This method is used to convert a Sequel::ForeignKeyConstraintViolation error
+  # into a PolicyLoadRecordNotFound error.
+  def sql_to_policy_error(exception)
+    if !exception.is_a?(Sequel::ForeignKeyConstraintViolation) ||
+        !exception.cause.is_a?(PG::ForeignKeyViolation)
+      return exception
+    end
+
+    # Try to parse the error message to find the violating key. This is based on
+    # `foreign_key_constraint_violation` in application_controller.rb.
+    return exception unless exception.cause.result.error_field(PG::PG_DIAG_MESSAGE_DETAIL) =~ /Key \(([^)]+)\)=\(([^)]+)\) is not present in table "([^"]+)"/
+
+    violating_key = ::Regexp.last_match(2)
+    Exceptions::PolicyLoadRecordNotFound.new(violating_key)
+  end
 end

app/models/exceptions/policy_load_record_not_found.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module Exceptions
+  # This exception is raised when a record required for policy loading is not found.
+  # It is different from a standard RecordNotFound exception because it is used in the
+  # context of policy loading, where the record not existing should be treated as a
+  # validation error of the policy load object and return a 422 status code, rather
+  # than a 404.
+  class PolicyLoadRecordNotFound < RuntimeError
+    attr_reader :id, :account, :kind, :identifier
+
+    def initialize id, message: nil
+      super(message || self.class.build_message(id))
+
+      @id = id
+      @account, @kind, @identifier = self.class.parse_id(id)
+    end
+
+    class << self
+      def parse_id id
+        id.split(':', 3)
+      end
+
+      def build_message id
+        account, kind, id = parse_id(id)
+        kind ||= 'unknown kind'
+        account ||= 'unknown account'
+        "#{kind.capitalize} '#{id}' not found in account '#{account}'"
+      end
+    end
+  end
+end

app/models/loader/types.rb

@@ -54,11 +54,11 @@ def find_ownerid
       end
 
       def find_roleid id
-        (::Role[id] || public_roles_model[id]).try(:role_id) || raise(Exceptions::RecordNotFound, id)
+        (::Role[id] || public_roles_model[id]).try(:role_id) || raise(Exceptions::PolicyLoadRecordNotFound, id)
       end
 
       def find_resourceid id
-        (::Resource[id] || public_resources_model[id]).try(:resource_id) || raise(Exceptions::RecordNotFound, id)
+        (::Resource[id] || public_resources_model[id]).try(:resource_id) || raise(Exceptions::PolicyLoadRecordNotFound, id)
       end
 
       protected
@@ -318,7 +318,7 @@ def verify_dynamic_secret
 
             if issuer.nil?
               issuer_exception_id = "#{@policy_object.account}:issuer:#{issuer_id}"
-              raise Exceptions::RecordNotFound, issuer_exception_id
+              raise Exceptions::PolicyLoadRecordNotFound, issuer_exception_id
             end
 
             if annotations["#{Issuer::DYNAMIC_ANNOTATION_PREFIX}method"].nil?
@@ -361,7 +361,7 @@ def auth_issuer_resource(privilege, resource_id, issuer_id, account)
         resource = ::Resource[resource_id]
         unless current_user.allowed_to?(privilege, resource)
           issuer_exception_id = "#{account}:issuer:#{issuer_id}"
-          raise Exceptions::RecordNotFound, issuer_exception_id
+          raise Exceptions::PolicyLoadRecordNotFound, issuer_exception_id
         end
       end
 

cucumber/api/features/policy_load_errors.feature

@@ -16,19 +16,13 @@ Feature: Policy loading error messages
       privilege: [ execute ]
       resource: !variable password
     """
-    Then the HTTP response status code is 404
+    Then the HTTP response status code is 422
     And the JSON response should be:
     """
     {
       "error": {
-        "code": "not_found",
-        "message": "User 'bob' not found in account 'cucumber'",
-        "target": "user",
-        "details": {
-          "code": "not_found",
-          "target": "id",
-          "message": "cucumber:user:bob"
-        }
+        "code": "policy_invalid",
+        "message": "User 'bob' not found in account 'cucumber'"
       }
     }
     """

cucumber/policy/features/host_factory.feature

@@ -44,7 +44,7 @@ Feature: Host Factories can be managed through policies.
           layers: [ !layer ../default ]
       """
     Then there's an error
-    And the error code is "not_found"
+    And the error code is "policy_invalid"
     And the error message is "Layer 'myapp/../default' not found in account 'cucumber'"
 
   @acceptance

cucumber/policy/features/replace.feature

@@ -48,7 +48,7 @@ A policy can be reloaded using the --replace flag
       - !user developer2
     """
     Then there's an error
-    And the error code is "not_found"
+    And the error code is "policy_invalid"
     And the error message is "Group 'developers' not found in account 'cucumber'"
 
   @negative @acceptance
@@ -85,7 +85,7 @@ A policy can be reloaded using the --replace flag
       - !user developer2
     """
     Then there's an error
-    And the error code is "not_found"
+    And the error code is "policy_invalid"
     And the error message is "Group 'security-admin' not found in account 'cucumber'"
 
   @smoke
@@ -281,7 +281,7 @@ A policy can be reloaded using the --replace flag
       resource: !layer ops
     """
     Then there's an error
-    And the error code is "not_found"
+    And the error code is "policy_invalid"
     And the error message is "Layer 'ops' not found in account 'cucumber'"
 
   @acceptance

cucumber/policy/features/step_definitions/policy_steps.rb

@@ -25,7 +25,7 @@
 Given(/^I try to load a policy with an unresolvable reference:$/) do |policy|
   @client = Client.for("user", "admin")
   @result = @client.load_policy(id: 'root', policy: policy)
-  expect(@result.code).to eq(404)
+  expect(@result.code).to eq(422)
 end
 
 Then("the result includes an API key for {string}") do |role_id|

spec/controllers/policy_validation_spec.rb

@@ -27,8 +27,8 @@ def validation_error_text
 def validation_explanation
   body = JSON.parse(response.body)
   msg = body['errors'][0]['message']
-  adv = msg.match(/^[^\n]*\n{1,1}(.*)$/).to_s
-  adv
+  msg.match(/^[^\n]*\n{1,1}(.*)$/).to_s
+  
 end
 
 describe PoliciesController, type: :request do
@@ -156,14 +156,10 @@ def validation_explanation
                 resource: !variable password
             YAML
           )
-          expect(response.code).to eq("404")
+          expect(response.code).to eq("422")
           body = JSON.parse(response.body)
-          expect(body['error']['code']).to eq("not_found")
+          expect(body['error']['code']).to eq("policy_invalid")
           expect(body['error']['message']).to eq("User 'bob' not found in account 'rspec'")
-          expect(body['error']['target']).to eq("user")
-          expect(body['error']['details']["code"]).to eq("not_found")
-          expect(body['error']['details']["target"]).to eq("id")
-          expect(body['error']['details']["message"]).to eq("rspec:user:bob")
         end
       end