From 0cd4d778144846e0273d1113bf6ba302a6e2a172 Mon Sep 17 00:00:00 2001
From: Allison King <allisonjuliaking@gmail.com>
Date: Fri, 30 Sep 2022 12:26:45 -0400
Subject: [PATCH] Restrict startsWith comparisons in CheckboxTree more (#1126)

* Restrict startsWith comparisons in CheckboxTree more

To fix a bug where checking user.biometric would also check user.biometric_health

* Update changelog
---
 CHANGELOG.md                                  |  1 +
 .../admin-ui/__tests__/checkbox-tree.test.tsx | 13 ++++++++++
 .../src/features/common/CheckboxTree.tsx      | 24 +++++++++++++++----
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0af3be085f..5c27b0c40d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -56,6 +56,7 @@ The types of changes are:
 ### Fixed
 
 * Fixed the "help" link in the UI header [#1078](https://github.com/ethyca/fides/pull/1078)
+* Fixed a bug in Data Category Dropdowns where checking i.e. `user.biometric` would also check `user.biometric_health` [#1126](https://github.com/ethyca/fides/pull/1126)
 
 ### Security
 
diff --git a/clients/ctl/admin-ui/__tests__/checkbox-tree.test.tsx b/clients/ctl/admin-ui/__tests__/checkbox-tree.test.tsx
index 5aad377360..a0f7e22a2a 100644
--- a/clients/ctl/admin-ui/__tests__/checkbox-tree.test.tsx
+++ b/clients/ctl/admin-ui/__tests__/checkbox-tree.test.tsx
@@ -28,6 +28,11 @@ const MOCK_NODES = [
         value: "grandparent.aunt",
         children: [],
       },
+      {
+        label: "aunt_second",
+        value: "grandparent.aunt_second",
+        children: [],
+      },
     ],
   },
   {
@@ -236,6 +241,7 @@ describe("Checkbox tree", () => {
           "grandparent.parent.child",
           "grandparent.parent.sibling",
           "grandparent.aunt",
+          "grandparent.aunt_second",
         ].sort()
       );
       expect(
@@ -254,6 +260,13 @@ describe("Checkbox tree", () => {
           (d) => d.value
         )
       ).toEqual(["grandparent.parent.child"]);
+
+      // make sure aunt_second does not sneak in
+      expect(
+        getDescendantsAndCurrent(MOCK_NODES, "grandparent.aunt").map(
+          (d) => d.value
+        )
+      ).toEqual(["grandparent.aunt"]);
     });
 
     it("can determine if an ancestor is selected", () => {
diff --git a/clients/ctl/admin-ui/src/features/common/CheckboxTree.tsx b/clients/ctl/admin-ui/src/features/common/CheckboxTree.tsx
index 0c08357aeb..e6c2ea9400 100644
--- a/clients/ctl/admin-ui/src/features/common/CheckboxTree.tsx
+++ b/clients/ctl/admin-ui/src/features/common/CheckboxTree.tsx
@@ -36,6 +36,16 @@ export const ancestorIsSelected = (selected: string[], nodeName: string) => {
   return intersection.length > 0;
 };
 
+const matchNodeOrDescendant = (value: string, match: string) => {
+  if (value === match) {
+    return true;
+  }
+  if (value.startsWith(`${match}.`)) {
+    return true;
+  }
+  return false;
+};
+
 export const getDescendantsAndCurrent = (
   nodes: TreeNode[],
   nodeName: string,
@@ -46,7 +56,7 @@ export const getDescendantsAndCurrent = (
     if (node.children) {
       getDescendantsAndCurrent(node.children, nodeName, descendants);
     }
-    if (node.value.startsWith(nodeName)) {
+    if (matchNodeOrDescendant(node.value, nodeName)) {
       descendants.push(node);
     }
   });
@@ -151,8 +161,10 @@ const CheckboxTree = ({ nodes, selected, onSelected }: CheckboxTreeProps) => {
     let newSelected: string[] = [];
     if (checked.indexOf(node.value) >= 0) {
       // take advantage of dot notation here for unchecking children
-      newChecked = checked.filter((s) => !s.startsWith(node.value));
-      newSelected = selected.filter((s) => !s.startsWith(node.value));
+      newChecked = checked.filter((s) => !matchNodeOrDescendant(s, node.value));
+      newSelected = selected.filter(
+        (s) => !matchNodeOrDescendant(s, node.value)
+      );
     } else {
       // we need to mark all descendants as checked, though these are not
       // technically 'selected'
@@ -169,7 +181,9 @@ const CheckboxTree = ({ nodes, selected, onSelected }: CheckboxTreeProps) => {
   const handleExpanded = (node: TreeNode) => {
     if (expanded.indexOf(node.value) >= 0) {
       // take advantage of dot notation here for unexpanding children
-      setExpanded(expanded.filter((c) => !c.startsWith(node.value)));
+      setExpanded(
+        expanded.filter((c) => !matchNodeOrDescendant(c, node.value))
+      );
     } else {
       setExpanded([...expanded, node.value]);
     }
@@ -186,7 +200,7 @@ const CheckboxTree = ({ nodes, selected, onSelected }: CheckboxTreeProps) => {
       const isIndeterminate =
         isChecked &&
         node.children.length > 0 &&
-        checked.filter((s) => s.startsWith(node.value)).length !==
+        checked.filter((s) => s.startsWith(`${node.value}.`)).length + 1 !==
           thisDescendants.length;
       const isDisabled = ancestorIsSelected(selected, node.value);