Fixing RBAC feature + parameters (#813)

alfredorubin96 · nielsdejong · web-flow · commit 4b779a0cd3e9 · 2024-03-06T11:57:12.000+01:00
* fixed race condition, to work on other points in PR

* Fixes for complex parameter types in forms

* Added special case for handling cross-db label access

* handling fixed grants without non-fixed grants

* Added error handling to RBAC extension

* Added back async modifier

* bug fixin grbac

---------

Co-authored-by: Alfred Rubin &lt;alfredo.rubin@neo4j.com&gt;
Co-authored-by: Niels de Jong &lt;niels-121@hotmail.com&gt;
diff --git a/src/extensions/forms/FormsReportConfig.tsx b/src/extensions/forms/FormsReportConfig.tsx
@@ -46,7 +46,7 @@ export const FORMS = {
         label: 'Clear parameters after submit',
         type: SELECTION_TYPES.LIST,
         values: [true, false],
-        default: false,
+        default: true,
       },
       hasResetButton: {
         label: 'Has Reset Button',
diff --git a/src/extensions/forms/chart/NeoForm.tsx b/src/extensions/forms/chart/NeoForm.tsx
@@ -26,7 +26,7 @@ const NeoForm = (props: ChartProps) => {
   const hasResetButton = settings?.hasResetButton ?? true;
   const hasSubmitButton = settings?.hasSubmitButton ?? true;
   const hasSubmitMessage = settings?.hasSubmitMessage ?? true;
-  const clearParametersAfterSubmit = settings?.clearParametersAfterSubmit ?? false;
+  const clearParametersAfterSubmit = settings?.clearParametersAfterSubmit ?? true;
   const [submitButtonActive, setSubmitButtonActive] = React.useState(true);
   const [status, setStatus] = React.useState(FormStatus.DATA_ENTRY);
   const [formResults, setFormResults] = React.useState([]);
diff --git a/src/extensions/forms/settings/NeoFormCardSettingsModal.tsx b/src/extensions/forms/settings/NeoFormCardSettingsModal.tsx
@@ -4,6 +4,7 @@ import React from 'react';
 import { Button, Dialog } from '@neo4j-ndl/react';
 import ParameterSelectCardSettings from '../../../chart/parameter/ParameterSelectCardSettings';
 import NeoCardSettingsFooter from '../../../card/settings/CardSettingsFooter';
+import { objMerge } from '../../../utils/ObjectManipulation';
 
 const NeoFormCardSettingsModal = ({ open, setOpen, index, formFields, setFormFields, database, extensions }) => {
   const [advancedSettingsOpen, setAdvancedSettingsOpen] = React.useState(false);
@@ -24,15 +25,19 @@ const NeoFormCardSettingsModal = ({ open, setOpen, index, formFields, setFormFie
               query={formFields[index].query}
               type={'select'}
               database={database}
-              settings={formFields[index].settings}
+              settings={objMerge({ inputMode: 'cypher' }, formFields[index].settings)}
               extensions={extensions}
               onReportSettingUpdate={(key, value) => {
                 const newFormFields = [...formFields];
                 newFormFields[index].settings[key] = value;
+                if (key == 'type') {
+                  newFormFields[index].type = value;
+                }
                 setFormFields(newFormFields);
               }}
               onQueryUpdate={(query) => {
                 const newFormFields = [...formFields];
+
                 newFormFields[index].query = query;
                 setFormFields(newFormFields);
               }}
diff --git a/src/extensions/rbac/RBACManagementMenu.tsx b/src/extensions/rbac/RBACManagementMenu.tsx
@@ -19,7 +19,7 @@ export const RBACManagementMenu = ({ anchorEl, MenuOpen, handleClose, createNoti
     if (!MenuOpen) {
       return;
     }
-    const query = `SHOW ROLES YIELD role WHERE role <> "PUBLIC" return role`;
+    const query = `SHOW PRIVILEGES YIELD role, action WHERE role <> "PUBLIC" RETURN role, 'dbms_actions' in collect(action)`;
     runCypherQuery(
       driver,
       'system',
@@ -32,7 +32,8 @@ export const RBACManagementMenu = ({ anchorEl, MenuOpen, handleClose, createNoti
           createNotification('Unable to retrieve roles', records[0].error);
           return;
         }
-        setRoles(records.map((record) => record._fields[0]));
+        // Only display roles which are not able to do 'dbms_actions', i.e. they are not admins.
+        setRoles(records.filter((r) => r._fields[1] == false).map((record) => record._fields[0]));
       }
     );
   }, [MenuOpen]);
@@ -71,7 +72,7 @@ export const RBACManagementMenu = ({ anchorEl, MenuOpen, handleClose, createNoti
       </Menu>
 
       <RBACManagementModal
-        open={isModalOpen == true}
+        open={isModalOpen}
         handleClose={() => {
           setIsModalOpen(false);
         }}
diff --git a/src/extensions/rbac/RBACManagementModal.tsx b/src/extensions/rbac/RBACManagementModal.tsx
@@ -26,6 +26,12 @@ export const RBACManagementModal = ({ open, handleClose, currentRole, createNoti
   const [labels, setLabels] = useState([]);
   const [allowList, setAllowList] = useState([]);
   const [denyList, setDenyList] = useState([]);
+  const [fixedAllowList, setFixedAllowList] = useState([]);
+  const [fixedDenyList, setFixedDenyList] = useState([]);
+  const [denyCompleted, setDenyCompleted] = useState(false);
+  const [allowCompleted, setAllowCompleted] = useState(false);
+  const [usersCompleted, setUsersCompleted] = useState(false);
+  const [failed, setFailed] = useState(false);
 
   useEffect(() => {
     if (!open) {
@@ -35,10 +41,23 @@ export const RBACManagementModal = ({ open, handleClose, currentRole, createNoti
       setSelectedDatabase('');
       return;
     }
+    setDenyCompleted(false);
+    setAllowCompleted(false);
+    setUsersCompleted(false);
+    setFailed(false);
     retrieveDatabaseList(driver, setDatabases);
     retrieveNeo4jUsers(driver, currentRole, setNeo4jUsers, setSelectedUsers);
   }, [open]);
 
+  useEffect(() => {
+    console.log([denyCompleted, allowCompleted, usersCompleted, failed]);
+    if (failed !== false) {
+      createNotification('Unable to update privileges', `${failed}`);
+    } else if (denyCompleted && allowCompleted && usersCompleted) {
+      createNotification('Success', `Access for role '${currentRole}' updated.`);
+    }
+  }, [denyCompleted, allowCompleted, usersCompleted, failed]);
+
   const parseLabelsList = (database, records) => {
     const allLabels = records.map((record) => record._fields[0]).filter((l) => l !== '_Neodash_Dashboard');
     retrieveAllowAndDenyLists(
@@ -49,6 +68,8 @@ export const RBACManagementModal = ({ open, handleClose, currentRole, createNoti
       setLabels,
       setAllowList,
       setDenyList,
+      setFixedAllowList,
+      setFixedDenyList,
       setLoaded
     );
   };
@@ -58,16 +79,49 @@ export const RBACManagementModal = ({ open, handleClose, currentRole, createNoti
     retrieveLabelsList(driver, selectedOption.value, (records) => parseLabelsList(selectedOption.value, records));
   };
 
-  const handleSave = () => {
-    updateUsers(driver, currentRole, neo4jUsers, selectedUsers);
+  const handleSave = async () => {
+    createNotification('Updating', `Access for role '${currentRole}' is being updated, please wait...`);
+    console.log(selectedUsers);
+    updateUsers(
+      driver,
+      currentRole,
+      neo4jUsers,
+      selectedUsers,
+      () => setUsersCompleted(true),
+      (failReason) => setFailed(`Operation 'ROLE-USER ASSIGNMENT' failed.\n Reason: ${failReason}`)
+    );
+
     if (selectedDatabase) {
-      createNotification('Updating', `Access for role '${currentRole}' is being updated, please wait...`);
-      updatePrivileges(driver, selectedDatabase, currentRole, labels, denyList, Operation.DENY, createNotification);
-      updatePrivileges(driver, selectedDatabase, currentRole, labels, allowList, Operation.GRANT, createNotification);
+      const nonFixedDenyList = denyList.filter((n) => !fixedDenyList.includes(n));
+      const nonFixedAllowList = allowList.filter((n) => !fixedDenyList.includes(n));
+      updatePrivileges(
+        driver,
+        selectedDatabase,
+        currentRole,
+        labels,
+        nonFixedDenyList,
+        Operation.DENY,
+        () => setDenyCompleted(true),
+        (failReason) => setFailed(`Operation 'DENY LABEL ACCESS' failed.\n Reason: ${failReason}`)
+      ).then(() => {
+        updatePrivileges(
+          driver,
+          selectedDatabase,
+          currentRole,
+          labels,
+          nonFixedAllowList,
+          Operation.GRANT,
+          () => setAllowCompleted(true),
+          (failReason) => setFailed(`Operation 'ALLOW LABEL ACCESS' failed.\n Reason: ${failReason}`)
+        );
+      });
     } else {
-      createNotification('Success', `Users have been updated for role '${currentRole}'.`);
+      // Since there is no database selected, we don't run the DENY/ALLOW queries.
+      // We just mark them as completed so the success message shows up.
+      setDenyCompleted(true);
+      setAllowCompleted(true);
     }
-
+    console.log([denyCompleted, allowCompleted, usersCompleted, failed]);
     handleClose();
   };
 
@@ -138,7 +192,17 @@ export const RBACManagementModal = ({ open, handleClose, currentRole, createNoti
                     value: allowList.map((nodelabel) => ({ value: nodelabel, label: nodelabel })),
                     options: labels.map((nodelabel) => ({ value: nodelabel, label: nodelabel })),
                     isMulti: true,
-                    onChange: (val) => setAllowList(val.map((v) => v.value)),
+                    onChange: (val) => {
+                      // Make sure that only database-specific label access rules can be changed from this UI.
+                      if (fixedAllowList.every((v) => val.map((selected) => selected.value).includes(v))) {
+                        setAllowList(val.map((v) => v.value));
+                      } else {
+                        createNotification(
+                          'Label cannot be removed',
+                          'The selected label is allowed access across all databases. You cannot remove this privilege using this interface.'
+                        );
+                      }
+                    },
                   }}
                 />
               </div>
@@ -154,9 +218,21 @@ export const RBACManagementModal = ({ open, handleClose, currentRole, createNoti
                     placeholder: 'Select labels',
                     isClearable: false,
                     value: denyList.map((nodelabel) => ({ value: nodelabel, label: nodelabel })),
-                    options: labels.map((nodelabel) => ({ value: nodelabel, label: nodelabel })),
+                    options: labels
+                      .filter((l) => l !== '*')
+                      .map((nodelabel) => ({ value: nodelabel, label: nodelabel })),
                     isMulti: true,
-                    onChange: (val) => setDenyList(val.map((v) => v.value)),
+                    onChange: (val) => {
+                      // Make sure that only database-specific label access rules can be changed from this UI.
+                      if (fixedDenyList.every((v) => val.map((selected) => selected.value).includes(v))) {
+                        setDenyList(val.map((v) => v.value));
+                      } else {
+                        createNotification(
+                          'Label cannot be removed',
+                          'The selected label is denied access across all databases. You cannot remove this privilege using this interface.'
+                        );
+                      }
+                    },
                   }}
                 />
               </div>
diff --git a/src/extensions/rbac/RBACUtils.ts b/src/extensions/rbac/RBACUtils.ts
@@ -15,15 +15,16 @@ export enum Operation {
  * @param newLabels list of new labels in the database, for which priveleges are changed.
  * @param operation The operation, either 'GRANT' or 'DENY'
  */
-export const updatePrivileges = (
+export async function updatePrivileges(
   driver,
   database,
   role,
   allLabels,
   newLabels,
   operation: Operation,
-  createNotification
-) => {
+  onSuccess,
+  onFail
+) {
   // TODO - should we also drop cross-database DENYs (`ON GRAPH *`) to catch the true full set?
   // TODO - there
   // 1. Special case for '*'. Create it if needed to be there, otherwise revoke it.
@@ -49,7 +50,7 @@ export const updatePrivileges = (
           {},
           1000,
           (status) => {
-            if (status == QueryStatus.NO_DATA || QueryStatus.COMPLETE) {
+            if (status == QueryStatus.NO_DATA || status == QueryStatus.COMPLETE) {
               //  TODO: Neo4j is very slow in updating after the previous query, even though it is technically a finished query.
               // We build in an artificial delay...
               const timeout = setTimeout(() => {
@@ -67,21 +68,38 @@ export const updatePrivileges = (
                     ),
                     {},
                     1000,
-                    () => {
-                      if (status == QueryStatus.NO_DATA || QueryStatus.COMPLETE) {
-                        createNotification('Success', `Access for role '${role}' updated.`);
+                    (status) => {
+                      if (status == QueryStatus.NO_DATA || status == QueryStatus.COMPLETE) {
+                        onSuccess();
+                      }
+                    },
+                    (records) => {
+                      if (records && records[0] && records[0].error) {
+                        onFail(records[0].error);
                       }
                     }
                   );
+                } else {
+                  onSuccess();
                 }
               }, 1000);
             }
+          },
+          (records) => {
+            if (records && records[0] && records[0].error) {
+              onFail(records[0].error);
+            }
           }
         );
       }
+    },
+    (records) => {
+      if (records && records[0] && records[0].error) {
+        onFail(records[0].error);
+      }
     }
   );
-};
+}
 
 /**
  * Generic query builder for adding/removing grants/denies for a list of labels.
@@ -120,6 +138,8 @@ export const retrieveAllowAndDenyLists = (
   setLabels,
   setAllowList,
   setDenyList,
+  setFixedAllowList,
+  setFixedDenyList,
   setLoaded
 ) => {
   runCypherQuery(
@@ -131,7 +151,7 @@ export const retrieveAllowAndDenyLists = (
       AND role = $rolename
       AND action = 'match' 
       AND segment STARTS WITH 'NODE('
-      RETURN access, collect(substring(segment, 5, size(segment)-6)) as nodes`,
+      RETURN access, collect(substring(segment, 5, size(segment)-6)) as nodes, graph = "*" as fixed`,
     { rolename: currentRole, database: database },
     1000,
     (status) => {
@@ -142,12 +162,21 @@ export const retrieveAllowAndDenyLists = (
     },
     (records) => {
       // Extract granted and denied label list from the result of the SHOW PRIVILEGES query
-      const grants = records.filter((r) => r._fields[0] == 'GRANTED');
-      const denies = records.filter((r) => r._fields[0] == 'DENIED');
+      const grants = records.filter((r) => r._fields[0] == 'GRANTED' && r._fields[2] == false);
+      const denies = records.filter((r) => r._fields[0] == 'DENIED' && r._fields[2] == false);
       const grantedLabels = grants[0] ? [...new Set(grants[0]._fields[1])] : [];
       const deniedLabels = denies[0] ? [...new Set(denies[0]._fields[1])] : [];
-      setAllowList(grantedLabels);
-      setDenyList(deniedLabels);
+
+      // Do the same for fixed grants (those stored under the '*' graph permission)
+      const fixedGrants = records.filter((r) => r._fields[0] == 'GRANTED' && r._fields[2] == true);
+      const fixedDenies = records.filter((r) => r._fields[0] == 'DENIED' && r._fields[2] == true);
+      const fixedGrantedLabels = fixedGrants[0] ? [...new Set(fixedGrants[0]._fields[1])] : [];
+      const fixedDeniedLabels = fixedDenies[0] ? [...new Set(fixedDenies[0]._fields[1])] : [];
+
+      setAllowList([...new Set(grantedLabels.concat(fixedGrantedLabels))]);
+      setDenyList([...new Set(deniedLabels.concat(fixedDeniedLabels))]);
+      setFixedAllowList(fixedGrantedLabels);
+      setFixedDenyList(fixedDeniedLabels);
 
       // Here we build a set of all POSSIBLE labels, that includes the list in the database, plus those in denies and grants.
       const possibleLabels = [...new Set(allLabels.concat(grantedLabels).concat(deniedLabels))];
@@ -226,25 +255,42 @@ export function retrieveDatabaseList(driver, setDatabases: React.Dispatch<React.
  * @param allUsers list of all users.
  * @param selectedUsers list of users to have the role after the operation completes.
  */
-export const updateUsers = async (driver, currentRole, allUsers, selectedUsers) => {
+export async function updateUsers(driver, currentRole, allUsers, selectedUsers, onSuccess, onFail) {
   // 1. Build the query that removes all users from the role.
+  let globalStatus = -1;
   await runCypherQuery(
     driver,
     'system',
     `REVOKE ROLE ${currentRole} FROM ${allUsers.join(',')}`,
     {},
     1000,
     (status) => {
-      if (status == QueryStatus.NO_DATA || QueryStatus.COMPLETE) {
-        //  TODO: Neo4j is very slow in updating after the previous query, even though it is technically a finished query.
-        // We build in an artificial delay...
-        const timeout = setTimeout(() => {
-          // 2. Re-assign only selected users to the role.
-          if (selectedUsers.length > 0) {
-            runCypherQuery(driver, 'system', `GRANT ROLE ${currentRole} TO ${selectedUsers.join(',')}`);
-          }
-        }, 1000);
+      globalStatus = status;
+    },
+    (records) => {
+      if (records && records[0] && records[0].error) {
+        onFail(records[0].error);
       }
     }
   );
-};
+  if (globalStatus == QueryStatus.NO_DATA || globalStatus == QueryStatus.COMPLETE) {
+    //  TODO: Neo4j is very slow in updating after the previous query, even though it is technically a finished query.
+    // We build in an artificial delay...
+    if (selectedUsers.length > 0) {
+      await runCypherQuery(
+        driver,
+        'system',
+        `GRANT ROLE ${currentRole} TO ${selectedUsers.join(',')}`,
+        {},
+        1000,
+        (status) => {
+          if (status == QueryStatus.NO_DATA || QueryStatus.COMPLETE) {
+            onSuccess();
+          }
+        }
+      );
+    } else {
+      onSuccess();
+    }
+  }
+}
diff --git a/src/modal/ExportModal.tsx b/src/modal/ExportModal.tsx
@@ -14,7 +14,7 @@ export const NeoExportModal = ({ dashboard }) => {
   return (
     <>
       <Tooltip title='Export' aria-label='export' disableInteractive>
-        <IconButton className='n-mx-1' onClick={() => setOpen(true)} aria-label='Export' title='Export'>
+        <IconButton className='n-mx-1' onClick={() => setOpen(true)} aria-label='Export'>
           <DocumentArrowDownIconOutline />
         </IconButton>
       </Tooltip>
diff --git a/src/report/ReportQueryRunner.ts b/src/report/ReportQueryRunner.ts
