Merge pull request #1241 from agrare/reject_invalid_configuration_scr…

…ipt_payload_credentials_credential_field

Reject invalid script_payload credential_fields

(cherry picked from commit dfca21a)

app/controllers/api/configuration_script_payloads_controller.rb

@@ -29,12 +29,14 @@ def edit_resource(type, id, data)
       unless data["credentials"].nil?
         # Credentials can be a static string or a payload with an external
         # Authentication record referenced by credential_ref and credential_field.
-        credential_refs = data["credentials"].values.select { |val| val.kind_of?(Hash) }.pluck("credential_ref")
+        auth_references = data["credentials"].values.select { |val| val.kind_of?(Hash) }
+        credential_refs = auth_references.pluck("credential_ref")
         # Lookup the Authentication record by ems_ref in the parent manager's
         # list of authentications.
         credentials     = resource.manager&.authentications&.where(:ems_ref => credential_refs) || []
         # Filter the collection based on the current user's RBAC roles.
         credentials, _  = collection_filterer(credentials, "authentications", ::Authentication)
+        credentials_by_ems_ref = credentials.index_by(&:ems_ref)
         # If any requested authentications were unable to be found, either due
         # to a bad credential_ref or due to RBAC then raise a 400 BadRequestError.
         missing_credential_refs = credential_refs - credentials.pluck(:ems_ref)
@@ -43,6 +45,23 @@ def edit_resource(type, id, data)
                 _("Could not find credentials %{missing_credential_refs}") %
                 {:missing_credential_refs => missing_credential_refs}
         end
+        # Ensure that the only values allowed in credential_field are attributes
+        # which the credential class allows the user to set.  These values
+        # are present in the API_ATTRIBUTES and are already used for DDF
+        # parameters and API payload validation.
+        auth_references.each do |ref|
+          credential_ref, credential_field = ref.values_at("credential_ref", "credential_field")
+
+          credential_class          = credentials_by_ems_ref[credential_ref].class
+          allowed_credential_fields = defined?(credential_class::API_ATTRIBUTES) ? credential_class::API_ATTRIBUTES.pluck(:id) : []
+
+          next if allowed_credential_fields.include?(credential_field)
+
+          raise BadRequestError,
+                _("Invalid credential_field %{field}, allowed values are %{allowed_fields}" %
+                {:field => credential_field, :allowed_fields => allowed_credential_fields.join(", ")})
+        end
+
         # Reset the authentications collection with the current set of credentials.
         # This will also remove any credential references not in the new payload.
         resource.authentications = credentials

spec/requests/configuration_script_payloads_spec.rb

@@ -80,7 +80,7 @@
       end
 
       context "with an authentication reference in credentials" do
-        let!(:authentication) { FactoryBot.create(:authentication, :ems_ref => "my-credential", :resource => manager) }
+        let!(:authentication) { FactoryBot.create(:embedded_workflows_workflow_credential, :ems_ref => "my-credential", :resource => manager) }
 
         context "owned by another tenant" do
           let(:tenant_1)        { FactoryBot.create(:tenant) }
@@ -118,12 +118,31 @@
 
           post(api_configuration_script_payloads_url, :params => {:action => 'edit', :resources => [resource]})
 
+          expect(response).to have_http_status(:ok)
           expect(response.parsed_body["results"].first).to include(
             "credentials" => expected_credentials
           )
           expect(script_payload.reload.authentications).to include(authentication)
         end
 
+        it "fails if the credential_field isn't one of the allowed API_ATTRIBUTES" do
+          api_basic_authorize collection_action_identifier(:configuration_script_payloads, :edit, :post)
+
+          expected_credentials = {
+            "my-cred-user"     => {"credential_ref" => "my-credential", "credential_field" => "userid"},
+            "my-cred-password" => {"credential_ref" => "my-credential", "credential_field" => "evm_owner_id"},
+          }
+
+          resource = {
+            :id          => script_payload.id,
+            :name        => 'foo',
+            :credentials => expected_credentials
+          }
+
+          post(api_configuration_script_payloads_url, :params => {:action => 'edit', :resources => [resource]})
+          expect(response).to have_http_status(:bad_request)
+        end
+
         context "with an existing associated authentication record" do
           before { script_payload.authentications << authentication }