Copilot commented on code in PR #995:
URL: https://github.com/apache/ranger/pull/995#discussion_r3458036673
##########
security-admin/src/main/webapp/react-webapp/src/views/PolicyListing/PolicyPermissionItem.jsx:
##########
@@ -105,15 +107,53 @@ export default function PolicyPermissionItem(props) {
};
const grpResourcesKeys = useMemo(() => {
- const { resources = [] } = serviceCompDetails;
+ const { resources = [] } = serviceCompDetails || {};
const grpResources = groupBy(resources, "level");
let grpResourcesKeys = [];
for (const resourceKey in grpResources) {
grpResourcesKeys.push(+resourceKey);
}
grpResourcesKeys = grpResourcesKeys.sort();
Review Comment:
Sorting `grpResourcesKeys` without a comparator sorts lexicographically,
which can misorder numeric levels (e.g., `10` before `2`) and lead to incorrect
resource-depth handling. Use a numeric comparator (e.g., `(a, b) => a - b`)
after ensuring the keys are numbers.
##########
security-admin/src/main/webapp/react-webapp/src/views/PolicyListing/PolicyPermissionItem.jsx:
##########
@@ -105,15 +107,53 @@ export default function PolicyPermissionItem(props) {
};
const grpResourcesKeys = useMemo(() => {
- const { resources = [] } = serviceCompDetails;
+ const { resources = [] } = serviceCompDetails || {};
const grpResources = groupBy(resources, "level");
let grpResourcesKeys = [];
for (const resourceKey in grpResources) {
grpResourcesKeys.push(+resourceKey);
}
grpResourcesKeys = grpResourcesKeys.sort();
return grpResourcesKeys;
- }, []);
+ }, [serviceCompDetails?.resources]);
+
+ // Narrow dependency: only recompute when resource selection fields change,
+ // not on every keystroke in user/group/permission fields.
+ const resourceBlocks = Array.isArray(formValues?.additionalResources)
+ ? formValues.additionalResources
+ : [formValues];
+
+ const serializedResources = JSON.stringify(
+ resourceBlocks.map((b) =>
+ grpResourcesKeys.map((level) => b?.[`resourceName-${level}`]?.name ||
null)
+ )
+ );
Review Comment:
`serializedResources` is computed via `JSON.stringify(...)` on every render
(even when unrelated fields change), which can be expensive for larger
policies. Consider memoizing `serializedResources` itself (or subscribing only
to the relevant resource fields) so the stringify work is avoided when resource
selections haven't changed.
##########
security-admin/src/main/webapp/react-webapp/src/hooks/usePruneStaleConditions.js:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Keeps policyItems[].conditions["action-matches"] in sync when permissions or
+ * resources change. Mounted from PolicyPermissionItem; does not touch ip-range
+ * or other per-row conditions.
+ */
+
+import { useEffect } from "react";
+import { isArray, find, isEqual } from "lodash";
+import {
+ getSelectedAccessTypesForRow,
+ getAllowedActionMatchesForCondition,
+ isPerRowCondition,
+ parseConditionUiHint
+} from "Utils/policyConditionUtils";
+
+/**
+ * Stable dependency string: only permission accesses and per-row condition
values.
+ * Avoids re-running when unrelated form fields (user/group names, etc.)
change.
+ */
+const serializePruneDeps = (formValues, attrName) => {
+ const items = formValues?.[attrName];
+ if (!Array.isArray(items)) {
+ return "[]";
+ }
+ return JSON.stringify(
+ items.map((item, index) => ({
+ accesses: getSelectedAccessTypesForRow(formValues, attrName, index),
+ actionMatches: item?.conditions?.["action-matches"] ?? null
+ }))
+ );
+};
+
+export const usePruneStaleConditions = ({
+ formValues,
+ attrName,
+ form,
+ leafResourceTypes,
+ serviceCompDetails,
+ conditionDefVal,
+ actionReqsMap
+}) => {
+ const serializedPruneDeps = serializePruneDeps(formValues, attrName);
+
+ useEffect(() => {
+ const items = formValues?.[attrName];
+ if (!items || !isArray(items)) {
+ return;
+ }
+
+ let hasChanges = false;
+ const newItems = [...items];
+
+ newItems.forEach((item, index) => {
+ if (!item.conditions) {
+ return;
+ }
+
+ const accesses = getSelectedAccessTypesForRow(formValues, attrName,
index);
+ if (accesses.length === 0) {
+ return;
+ }
Review Comment:
When a row's permissions are cleared (`accesses.length === 0`), the hook
currently skips pruning, which can leave a stale `conditions['action-matches']`
saved on that row. If the UI intends actions to be permission-scoped, consider
explicitly deleting `action-matches` when no accesses are selected so invalid
state can't silently persist.
##########
security-admin/src/main/webapp/react-webapp/src/components/Editable.jsx:
##########
@@ -677,14 +776,41 @@ const Editable = (props) => {
const handleApply = () => {
let errors, uiHintVal;
if (selectValRef?.current) {
+ // Re-validate action-matches against current row permissions before
persisting.
sortBy(Object.keys(selectValRef.current)).map((property) => {
let conditionObj = find(conditionDefVal, function (m) {
if (m.name == property) {
return m;
}
Review Comment:
This uses `.map()` only for side effects (the return value is ignored).
Replacing with `.forEach()` (or a `for...of`) avoids allocating an unused array
and makes the intent clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
