Skip to content

Commit

Permalink
Never match existing asset-* entities when patch-existing=false (#1395)
Browse files Browse the repository at this point in the history
<!-- Specify linked issues and REMOVE THE UNUSED LINES -->

This is the first of two fixes to this area of code.

There are two modes for `POST /bundle/import`: `patch-existing={true,false}`. If true, then existing entities are supposed to be detected and patched.

There is a bug where `asset_mappings` and `asset_properties` are erroneously mapped onto existing assets and essentially get merged when they should be created fresh.

This PR fixes this behavior for `patch-existing=false`, since no-one is using `patch-existing=true` yet and everyone is using `patch-existing=false`.

The merging code was made on the faulty assumption that there can only be one asset_mapping / asset_properties per asset.

There is a red test that is fixed by commit [Revert "Revert "green""](79f4978). The previous commit fails with the wrong number of results:

```
FAIL in ctia.bundle.routes-test/ (bundle-asset-relationships-test) (routes_test.clj:1504)
es-store relationships are created for asset mappings/properties
...
expected: (= 4 (count (filter (comp #{"created"} :result) create-results2)))
  actual: (not (= 4 2))

FAIL in ctia.bundle.routes-test/ (bundle-asset-relationships-test) (routes_test.clj:1506)
es-store relationships are created for asset mappings/properties
...
expected: (= 1 (count (filter (comp #{"exists"} :result) create-results2)))
  actual: (not (= 1 3))
```

<!--

Describe your PR for reviewers.
Don't forget to set correct labels (User Facing / Beta / Feature Flag)
If there is UI change please add a screen capture.
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="iroh-services-clients">[§](#iroh-services-clients)</a> IROH Services Clients
=====================================================================================

Put all informations that need to be communicated to IROH Services Clients.
Typically IROH-UI, ATS Integration, Orbital, etc...
 -->

<a name="qa">[§](#qa)</a> QA
============================

<!--
Describe the steps to test your PR.

1.
2.
3.

Or if no QA is needed keep it as is.
 -->
No QA is needed.

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="ops">[§](#ops)</a> Ops
===============================

  Specify Ops related issues and documentation
- Config change needed: threatgrid/tenzin#
- Migration needed: threatgrid/tenzin#
- How to enable/disable that feature: (ex remove service from `bootstrap.cfg`, add scope to org)
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="documentation">[§](#documentation)</a> Documentation
=============================================================

  Public Facing documentation section;
- Public documentation updated needed: threatgrid/iroh-ui#
  See internal [doc file](./services/iroh-auth/doc/public-doc.org)
 -->

<a name="release-notes">[§](#release-notes)</a> Release Notes
=============================================================

<!-- REMOVE UNUSED LINES -->

```
intern: Never match existing asset-* entities when patch-existing=false
```

<a name="squashed-commits">[§](#squashed-commits)</a> Squashed Commits
======================================================================
  • Loading branch information
frenchy64 authored and ISPHOST committed Dec 11, 2023
1 parent 8e2010a commit 4259a5c
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 25 deletions.
3 changes: 2 additions & 1 deletion src/ctia/bundle/core.clj
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,8 @@
(let [bundle-import-data (prepare-import bundle-entities external-key-prefixes auth-identity services)
tempids (bundle-import-data->tempids bundle-import-data tempids)
bundle-import-data (-> bundle-import-data
(resolve-asset-properties+mappings tempids auth-identity asset_properties-merge-strategy services)
(cond-> patch-existing
(resolve-asset-properties+mappings tempids auth-identity asset_properties-merge-strategy services))
(cond-> (and patch-existing
(= :merge-previous incident-tactics-techniques-merge-strategy))
merge-existing-incident-tactics+techniques))
Expand Down
113 changes: 89 additions & 24 deletions test/ctia/bundle/routes_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -1317,13 +1317,23 @@

(let [[oldv1 oldv2 newv1 newv2 newv3] (map (comp str gensym) '[oldv1 oldv2 newv1 newv2 newv3])
[relationship1-original-id
relationship2-original-id
relationship3-original-id
relationship4-original-id
asset_mapping1-original-id
asset_mapping2-original-id
asset_property1-original-id
asset_property2-original-id
asset1-original-id
asset2-original-id] (map #(str "transient:" % "_" (random-uuid))
'[relationship1-original-id
relationship2-original-id
relationship3-original-id
relationship4-original-id
asset_mapping1-original-id
asset_mapping2-original-id
asset_property1-original-id
asset_property2-original-id
asset1-original-id
asset2-original-id])
asset1 {:asset_type "device"
Expand All @@ -1349,6 +1359,7 @@
:timestamp #inst "2023-03-02T19:14:46.660-00:00"
:specificity "Unique"
:confidence "Unknown"}
asset_mapping2 (assoc asset_mapping1 :id asset_mapping2-original-id)
old-properties [{:name "something1" :value oldv1}
{:name "something2" :value oldv2}]
asset_property1 {:properties old-properties
Expand All @@ -1360,32 +1371,86 @@
:source_uri "https://something"
:id asset_property1-original-id
:timestamp #inst "2023-03-02T19:14:46.783-00:00"}
new-bundle (-> bundle-minimal
(assoc :assets #{asset1}
:asset_mappings #{asset_mapping1}
:asset_properties #{asset_property1}
:relationships #{{:id relationship1-original-id
:source_ref "https://private.intel.int.iroh.site:443/ctia/incident/incident-4fb91401-36a5-46d1-b0aa-01af02f00a7a"
:target_ref asset_mapping1-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}
{:source_ref "https://private.intel.int.iroh.site:443/ctia/incident/incident-4fb91401-36a5-46d1-b0aa-01af02f00a7a"
:target_ref asset_property1-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}
{:source_ref "https://private.intel.int.iroh.site:443/ctia/incident/incident-4fb91401-36a5-46d1-b0aa-01af02f00a7a"
:target_ref asset1-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}}))
create-response (POST app
"ctia/bundle/import"
:body new-bundle
:headers {"Authorization" "45c1f5e3f05d0"})
{create-results :results :as create-bundle-results} (:parsed-body create-response)
asset_property2 (assoc asset_property1 :id asset_property2-original-id)
incident-id "https://private.intel.int.iroh.site:443/ctia/incident/incident-4fb91401-36a5-46d1-b0aa-01af02f00a7a"
base-new-bundle (assoc bundle-minimal :assets #{asset1})
;; try and induce a bug where asset_mappings / asset_properties erroneously "exists" then they should be "created".
;; the conditions are if an asset_mappings / asset_properties already exists with the same asset_ref as the
;; one we're about to import, then the new entity will erroneously merge with the existing one.
;; we know the bug is fixed if we get the right number of result=created in the second bundle import result.
new-bundle1 (-> base-new-bundle
(assoc :asset_mappings #{asset_mapping1}
:asset_properties #{asset_property1}
:relationships #{{:id relationship1-original-id
:source_ref incident-id
:target_ref asset_mapping1-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}
{:id relationship3-original-id
:source_ref incident-id
:target_ref asset_property1-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}
{:source_ref incident-id
:target_ref asset1-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}}))
new-bundle2 (-> base-new-bundle
(assoc :asset_mappings #{asset_mapping2}
:asset_properties #{asset_property2}
:relationships #{{:id relationship2-original-id
:source_ref incident-id
:target_ref asset_mapping2-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}
{:id relationship4-original-id
:source_ref incident-id
:target_ref asset_property2-original-id, :relationship_type "related-to", :source "IROH Risk Score Service"}}))
;; create the asset and an asset-mappings
create-response1 (POST app
"ctia/bundle/import"
:body new-bundle1
:headers {"Authorization" "45c1f5e3f05d0"})
{create-results1 :results :as create-bundle-results1} (:parsed-body create-response1)
;; try and erroneously match a new asset-mappings with an old one based on asset_ref
create-response2 (POST app
"ctia/bundle/import"
:body new-bundle2
:headers {"Authorization" "45c1f5e3f05d0"})
{create-results2 :results :as create-bundle-results2} (:parsed-body create-response2)
create-results (concat create-results1 create-results2)
;; resolve in order of creation/patch for easier debugging
asset1-id (find-id-by-original-id :asset1-id create-bundle-results asset1-original-id)
asset_property1-id (find-id-by-original-id :asset_property1-id create-bundle-results asset_property1-original-id)
asset_mapping1-id (find-id-by-original-id :asset_mapping1-id create-bundle-results asset_mapping1-original-id)
relationship1-id (find-id-by-original-id :relationship1-id create-bundle-results relationship1-original-id)]
asset1-id (find-id-by-original-id :asset1-id create-bundle-results1 asset1-original-id)
asset_property1-id (find-id-by-original-id :asset_property1-id create-bundle-results1 asset_property1-original-id)
asset_property2-id (find-id-by-original-id :asset_property2-id create-bundle-results2 asset_property2-original-id)
_ (assert (not= asset_property1-id asset_property2-id))
asset_mapping1-id (find-id-by-original-id :asset_mapping1-id create-bundle-results1 asset_mapping1-original-id)
asset_mapping2-id (find-id-by-original-id :asset_mapping2-id create-bundle-results2 asset_mapping2-original-id)
_ (assert (not= asset_mapping1-id asset_mapping2-id))
relationship1-id (find-id-by-original-id :relationship1-id create-bundle-results1 relationship1-original-id)
relationship2-id (find-id-by-original-id :relationship2-id create-bundle-results2 relationship2-original-id)
relationship3-id (find-id-by-original-id :relationship3-id create-bundle-results1 relationship3-original-id)
relationship4-id (find-id-by-original-id :relationship4-id create-bundle-results2 relationship4-original-id)]
(testing "relationships are created for asset mappings/properties"
(when (is (= 200 (:status create-response)))
(is (= 6 (count create-results)))
(is (every? (comp #{"created"} :result) create-results)
(pr-str (mapv :result create-results)))))
(when (and (is (= 200 (:status create-response1)))
(is (= 200 (:status create-response2))))
(is (= 6 (count (filter (comp #{"created"} :result) create-results1)))
(pr-str create-results))
(is (= 0 (count (filter (comp #{"exists"} :result) create-results1)))
(pr-str create-results))
;; this is the most important line: only the asset exists, all other entities are
;; created in the second import.
(is (= 4 (count (filter (comp #{"created"} :result) create-results2)))
(pr-str (mapv :result create-results)))
(is (= 1 (count (filter (comp #{"exists"} :result) create-results2)))
(pr-str create-results))
(let [{{:keys [relationships]} :parsed-body} (GET app
"ctia/bundle/export"
:query-params {:ids [relationship1-id relationship2-id relationship3-id relationship4-id]}
:headers {"Authorization" "45c1f5e3f05d0"})
groups (update-vals (group-by :id relationships)
#(mapv (fn [e] (select-keys e [:source_ref :target_ref])) %))]
(is (= {relationship1-id [{:source_ref incident-id
:target_ref asset_mapping1-id}]
relationship2-id [{:source_ref incident-id
:target_ref asset_mapping2-id}]
relationship3-id [{:source_ref incident-id
:target_ref asset_property1-id}]
relationship4-id [{:source_ref incident-id
:target_ref asset_property2-id}]}
groups)))))
(testing "asset-properties and asset-mappings are patched"
(let [new-properties [{:name "something1" :value newv1}
{:name "something-else" :value newv2}]
Expand Down

0 comments on commit 4259a5c

Please sign in to comment.