From e9807aad6b4e32735d7d59a95d5766752b68022a Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Wed, 26 Jun 2024 15:13:11 -0400 Subject: [PATCH] feat(rbac): nested condition Signed-off-by: Yi Cai --- .../rbac/src/__fixtures__/mockConditions.ts | 81 ++ .../ConditionalAccess/ConditionsForm.tsx | 92 +- .../ConditionalAccess/ConditionsFormRow.tsx | 784 +++++++++++++++--- .../ConditionsFormRowFields.tsx | 166 +++- .../src/components/ConditionalAccess/types.ts | 8 +- plugins/rbac/src/utils/rbac-utils.test.ts | 66 +- plugins/rbac/src/utils/rbac-utils.ts | 31 +- plugins/rbac/tests/rbac.spec.ts | 42 + 8 files changed, 1103 insertions(+), 167 deletions(-) diff --git a/plugins/rbac/src/__fixtures__/mockConditions.ts b/plugins/rbac/src/__fixtures__/mockConditions.ts index 604c135135..5770d53790 100644 --- a/plugins/rbac/src/__fixtures__/mockConditions.ts +++ b/plugins/rbac/src/__fixtures__/mockConditions.ts @@ -42,4 +42,85 @@ export const mockConditions: RoleConditionalPolicyDecision[] = roleEntityRef: 'role:default/rbac_admin', permissionMapping: ['delete', 'update'], }, + { + id: 3, + result: AuthorizeResult.CONDITIONAL, + pluginId: 'catalog', + resourceType: 'catalog-entity', + conditions: { + anyOf: [ + { + rule: 'IS_ENTITY_OWNER', + resourceType: 'catalog-entity', + params: { + claims: ['user:default/ciiay'], + }, + }, + { + rule: 'IS_ENTITY_KIND', + resourceType: 'catalog-entity', + params: { kinds: ['Group'] }, + }, + { + allOf: [ + { + rule: 'IS_ENTITY_OWNER', + resourceType: 'catalog-entity', + params: { + claims: ['user:default/ciiay'], + }, + }, + { + rule: 'IS_ENTITY_KIND', + resourceType: 'catalog-entity', + params: { + kinds: ['User'], + }, + }, + ], + }, + ], + }, + roleEntityRef: 'role:default/rbac_admin', + permissionMapping: ['read', 'delete', 'update'], + }, + { + id: 4, + result: AuthorizeResult.CONDITIONAL, + pluginId: 'catalog', + resourceType: 'catalog-entity', + conditions: { + not: { + rule: 'HAS_LABEL', + resourceType: 'catalog-entity', + params: { label: 'temp' }, + }, + }, + roleEntityRef: 'role:default/rbac_admin', + permissionMapping: ['delete', 'update'], + }, + { + id: 5, + result: AuthorizeResult.CONDITIONAL, + pluginId: 'scaffolder', + resourceType: 'scaffolder-template', + conditions: { + not: { + anyOf: [ + { + rule: 'HAS_TAG', + resourceType: 'scaffolder-template', + params: { tag: 'dev' }, + }, + { + rule: 'HAS_TAG', + resourceType: 'scaffolder-template', + params: { tag: 'test' }, + }, + ], + }, + }, + roleEntityRef: 'role:default/rbac_admin', + permissionMapping: ['read'], + }, ]; diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx index ac9d37e4e1..3e632240d9 100644 --- a/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx +++ b/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx @@ -1,12 +1,20 @@ import React from 'react'; +import { PermissionCondition } from '@backstage/plugin-permission-common'; + import { makeStyles } from '@material-ui/core'; import Box from '@mui/material/Box'; import Button from '@mui/material/Button'; +import { RJSFValidationError } from '@rjsf/utils'; import { ConditionsFormRow } from './ConditionsFormRow'; import { criterias } from './const'; -import { ConditionsData, RuleParamsErrors, RulesData } from './types'; +import { + Condition, + ConditionsData, + RuleParamsErrors, + RulesData, +} from './types'; const useStyles = makeStyles(theme => ({ form: { @@ -58,22 +66,89 @@ export const ConditionsForm = ({ ); const [errors, setErrors] = React.useState(); + const handleSetErrors = ( + newErrors: RJSFValidationError[], + currentCriteria: string, + nestedCriteria?: string, + nestedConditionIndex?: number, + ruleIndex?: number, + removeErrors = false, + ) => { + setErrors(prevErrors => { + const updatedErrors: RuleParamsErrors = { ...prevErrors }; + + let baseErrorKey = currentCriteria; + if (nestedConditionIndex !== undefined) { + baseErrorKey += `.${nestedConditionIndex}`; + } + if (nestedCriteria !== undefined) { + baseErrorKey += `.${nestedCriteria}`; + } + if (ruleIndex !== undefined) { + baseErrorKey += `.${ruleIndex}`; + } + + if (removeErrors || newErrors.length === 0) { + delete updatedErrors[baseErrorKey]; + Object.keys(updatedErrors).forEach(key => { + if (key.startsWith(`${baseErrorKey}.`)) { + delete updatedErrors[key]; + } + }); + } else { + updatedErrors[baseErrorKey] = newErrors; + } + + return updatedErrors; + }); + }; + const [removeAllClicked, setRemoveAllClicked] = React.useState(false); + const flattenConditions = ( + conditionData: Condition[], + ): PermissionCondition[] => { + const flatConditions: PermissionCondition[] = []; + + const processCondition = (condition: Condition) => { + if ('rule' in condition) { + flatConditions.push(condition); + } else { + if (condition.allOf) { + condition.allOf.forEach(processCondition); + } + if (condition.anyOf) { + condition.anyOf.forEach(processCondition); + } + if (condition.not) { + if ('rule' in condition.not) { + flatConditions.push(condition.not); + } else { + processCondition(condition.not); + } + } + } + }; + conditionData.forEach(processCondition); + return flatConditions; + }; + const isNoRuleSelected = () => { switch (criteria) { - case criterias.condition: { + case criterias.condition: return !conditions.condition?.rule; - } case criterias.not: { - return !conditions.not?.rule; + const flatConditions = flattenConditions([conditions.not as Condition]); + return flatConditions.some(c => !c.rule); } case criterias.allOf: { - return !!conditions.allOf?.find(c => !c.rule); + const flatConditions = flattenConditions(conditions.allOf || []); + return flatConditions.some(c => !c.rule); } case criterias.anyOf: { - return !!conditions.anyOf?.find(c => !c.rule); + const flatConditions = flattenConditions(conditions.anyOf || []); + return flatConditions.some(c => !c.rule); } default: return true; @@ -81,7 +156,8 @@ export const ConditionsForm = ({ }; const isSaveDisabled = () => { - const hasErrors = !!errors?.[criteria]?.length; + const hasErrors = + errors && Object.keys(errors).some(key => errors[key].length > 0); if (removeAllClicked) return false; @@ -102,7 +178,7 @@ export const ConditionsForm = ({ selPluginResourceType={selPluginResourceType} onRuleChange={newCondition => setConditions(newCondition)} setCriteria={setCriteria} - setErrors={setErrors} + handleSetErrors={handleSetErrors} setRemoveAllClicked={setRemoveAllClicked} /> diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRow.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRow.tsx index 0a22376314..b0a2c1eb66 100644 --- a/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRow.tsx +++ b/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRow.tsx @@ -2,17 +2,25 @@ import React from 'react'; import { PermissionCondition } from '@backstage/plugin-permission-common'; -import { IconButton, makeStyles, useTheme } from '@material-ui/core'; +import { + FormControlLabel, + IconButton, + makeStyles, + Radio, + RadioGroup, + useTheme, +} from '@material-ui/core'; import AddIcon from '@mui/icons-material/Add'; import RemoveIcon from '@mui/icons-material/Remove'; import Box from '@mui/material/Box'; import Button from '@mui/material/Button'; import ToggleButton from '@mui/material/ToggleButton'; import ToggleButtonGroup from '@mui/material/ToggleButtonGroup'; +import { RJSFValidationError } from '@rjsf/utils'; import { ConditionsFormRowFields } from './ConditionsFormRowFields'; import { conditionButtons, criterias } from './const'; -import { ConditionsData, RuleParamsErrors, RulesData } from './types'; +import { Condition, ConditionsData, RulesData } from './types'; const useStyles = makeStyles(theme => ({ conditionRow: { @@ -24,16 +32,38 @@ const useStyles = makeStyles(theme => ({ backgroundColor: `${theme.palette.background.paper}!important`, }, }, + nestedConditionRow: { + padding: '20px', + marginLeft: '20px', + border: `1px solid ${theme.palette.border}`, + borderRadius: '4px', + backgroundColor: theme.palette.background.default, + '& input': { + backgroundColor: `${theme.palette.background.paper}!important`, + }, + }, criteriaButtonGroup: { backgroundColor: theme.palette.background.paper, width: '80%', }, + nestedConditioncriteriaButtonGroup: { + backgroundColor: theme.palette.background.paper, + width: '60%', + height: '100%', + }, criteriaButton: { width: '100%', textTransform: 'none', padding: theme.spacing(1), }, addRuleButton: { + display: 'flex', + color: theme.palette.primary.light, + textTransform: 'none', + marginTop: theme.spacing(1), + }, + addNestedConditionButton: { + display: 'flex', color: theme.palette.primary.light, textTransform: 'none', marginTop: theme.spacing(1), @@ -44,6 +74,17 @@ const useStyles = makeStyles(theme => ({ alignSelf: 'baseline', marginTop: theme.spacing(3.3), }, + removeNestedRuleButton: { + color: theme.palette.grey[500], + flexGrow: 0, + alignSelf: 'baseline', + }, + radioGroup: { + margin: '0.5rem', + }, + radioLabel: { + marginTop: '0.5rem', + }, })); type ConditionFormRowProps = { @@ -53,10 +94,19 @@ type ConditionFormRowProps = { selPluginResourceType: string; criteria: string; setCriteria: React.Dispatch>; - setErrors: React.Dispatch>; + handleSetErrors: ( + newErrors: RJSFValidationError[], + criteria: string, + nestedCriteria?: string, + nestedConditionIndex?: number, + ruleIndex?: number, + removeErrors?: boolean, + ) => void; setRemoveAllClicked: React.Dispatch>; }; +type NotConditionType = 'simple-condition' | 'nested-condition'; + export const ConditionsFormRow = ({ conditionRulesData, conditionRow, @@ -64,13 +114,72 @@ export const ConditionsFormRow = ({ criteria, onRuleChange, setCriteria, - setErrors, + handleSetErrors, setRemoveAllClicked, }: ConditionFormRowProps) => { const classes = useStyles(); const theme = useTheme(); + const nestedConditionButtons = conditionButtons.filter( + button => button.val !== 'condition', + ); + + const [nestedConditionRow, setNestedConditionRow] = React.useState< + ConditionsData[] + >([]); + const [notConditionType, setNotConditionType] = + React.useState('simple-condition'); + + const extractNestedConditions = ( + conditions: ConditionsData[], + criteriaTypes: string[], + nestedConditions: ConditionsData[], + ) => { + conditions.forEach(c => { + criteriaTypes.forEach(ct => { + if (Object.keys(c).includes(ct)) { + nestedConditions.push(c); + } + }); + }); + }; + + React.useEffect(() => { + const nestedConditions: ConditionsData[] = []; + const criteriaTypes = [criterias.allOf, criterias.anyOf, criterias.not]; + switch (criteria) { + case criterias.allOf: + extractNestedConditions( + (conditionRow.allOf as ConditionsData[]) || [], + criteriaTypes, + nestedConditions, + ); + break; + case criterias.anyOf: + extractNestedConditions( + (conditionRow.anyOf as ConditionsData[]) || [], + criteriaTypes, + nestedConditions, + ); + break; + case criterias.not: + if ( + criteriaTypes.includes( + Object.keys(conditionRow.not as ConditionsData)[0], + ) + ) { + nestedConditions.push(conditionRow.not as ConditionsData); + setNotConditionType('nested-condition'); + } + break; + default: + break; + } + + setNestedConditionRow(nestedConditions); + }, [conditionRow, criteria]); const handleCriteriaChange = (val: string) => { + handleSetErrors([], criteria, undefined, undefined, undefined, true); setCriteria(val); switch (val) { case criterias.condition: { @@ -108,6 +217,7 @@ export const ConditionsFormRow = ({ break; } case criterias.not: { + setNotConditionType('simple-condition'); onRuleChange({ not: { rule: '', @@ -121,6 +231,205 @@ export const ConditionsFormRow = ({ } }; + const updateRules = ( + updatedNestedConditionRow: ConditionsData[] | ConditionsData, + ) => { + const existingSimpleCondition = + criteria !== criterias.not + ? ( + conditionRow[criteria as keyof ConditionsData] as Condition[] + )?.filter(con => Object.keys(con).includes('rule')) || [] + : []; + + const newCondition: Condition[] = Array.isArray(updatedNestedConditionRow) + ? [...existingSimpleCondition, ...updatedNestedConditionRow] + : [...existingSimpleCondition, updatedNestedConditionRow]; + + if (criteria === criterias.anyOf || criteria === criterias.allOf) { + onRuleChange({ + [criteria]: newCondition, + }); + } else if (criteria === criterias.not) { + onRuleChange({ + not: updatedNestedConditionRow as ConditionsData, + }); + } + }; + + const handleNestedConditionCriteriaChange = ( + val: string, + nestedConditionIndex: number, + ) => { + handleSetErrors( + [], + criteria, + undefined, + nestedConditionIndex, + undefined, + true, + ); + const newNestedConditionNot: ConditionsData = { + [val]: { + rule: '', + resourceType: selPluginResourceType, + params: {}, + }, + }; + const newNestedConditionAllOfOrAnyOf: ConditionsData = { + [val]: [ + { + rule: '', + resourceType: selPluginResourceType, + params: {}, + }, + ], + }; + if (criteria === criterias.not) { + updateRules( + val === criterias.not + ? newNestedConditionNot + : newNestedConditionAllOfOrAnyOf, + ); + } else { + const updatedNestedConditionRow = nestedConditionRow.map((c, index) => { + if (index === nestedConditionIndex) { + return val === criterias.not + ? newNestedConditionNot + : newNestedConditionAllOfOrAnyOf; + } + return c; + }); + updateRules(updatedNestedConditionRow); + } + }; + + const handleAddNestedCondition = (currentCriteria: string) => { + const newNestedCondition = { + [criterias.allOf]: [ + { + rule: '', + resourceType: selPluginResourceType, + params: {}, + }, + ], + }; + const updatedNestedConditionRow = [ + ...nestedConditionRow, + newNestedCondition, + ]; + updateRules( + currentCriteria === criterias.not + ? newNestedCondition + : updatedNestedConditionRow, + ); + }; + + const handleNotConditionTypeChange = (val: string) => { + setNotConditionType(val as NotConditionType); + if (val === 'nested-condition') { + handleAddNestedCondition(criterias.not); + handleSetErrors([], criteria, criterias.not, 0, 0, true); + } else { + onRuleChange({ + not: { + rule: '', + resourceType: selPluginResourceType, + params: {}, + }, + }); + handleSetErrors([], criteria, criterias.not, 0, 0, true); + handleSetErrors([], criteria, undefined, undefined, undefined, true); + } + }; + + const handleAddRuleInNestedCondition = ( + nestedConditionCriteria: string, + nestedConditionIndex: number, + ) => { + const updatedNestedConditionRow: ConditionsData[] = []; + + nestedConditionRow.forEach((c, index) => { + if (index === nestedConditionIndex) { + updatedNestedConditionRow.push({ + [nestedConditionCriteria as keyof ConditionsData]: [ + ...((c[ + nestedConditionCriteria as keyof ConditionsData + ] as PermissionCondition[]) || []), + { + rule: '', + resourceType: selPluginResourceType, + params: {}, + }, + ], + }); + } else { + updatedNestedConditionRow.push(c); + } + }); + updateRules( + criteria === criterias.not + ? updatedNestedConditionRow[0] + : updatedNestedConditionRow, + ); + }; + + const handleRemoveNestedCondition = ( + nestedConditionCriteria: string, + nestedConditionIndex: number, + ) => { + const updatedNestedConditionRow = nestedConditionRow.filter( + (_, index) => index !== nestedConditionIndex, + ); + + updateRules(updatedNestedConditionRow); + handleSetErrors( + [], + criteria, + nestedConditionCriteria, + nestedConditionIndex, + undefined, + true, + ); + }; + + const handleRemoveNestedConditionRule = ( + nestedConditionCriteria: string, + nestedConditionIndex: number, + ruleIndex: number, + ) => { + const updatedNestedConditionRow: ConditionsData[] = []; + + nestedConditionRow.forEach((c, index) => { + if (index === nestedConditionIndex) { + const updatedRules = ( + (c[ + nestedConditionCriteria as keyof ConditionsData + ] as PermissionCondition[]) || [] + ).filter((_r, rindex) => rindex !== ruleIndex); + updatedNestedConditionRow.push({ + [nestedConditionCriteria as keyof ConditionsData]: updatedRules, + }); + } else { + updatedNestedConditionRow.push(c); + } + }); + + updateRules( + criteria === criterias.not + ? updatedNestedConditionRow[0] + : updatedNestedConditionRow, + ); + + handleSetErrors( + [], + criteria, + nestedConditionCriteria, + nestedConditionIndex, + ruleIndex, + true, + ); + }; + const ruleOptionDisabled = ( ruleOption: string, conditions?: PermissionCondition[], @@ -128,6 +437,32 @@ export const ConditionsFormRow = ({ return !!(conditions || []).find(con => con.rule === ruleOption); }; + const renderConditionRule = () => { + return ( + + ruleOptionDisabled( + ruleOption, + conditionRow.condition ? [conditionRow.condition] : undefined, + ) + } + setRemoveAllClicked={setRemoveAllClicked} + /> + ); + }; + return ( ))} - {(criteria === criterias.allOf || criteria === criterias.anyOf) && ( + {(criteria === criterias.allOf || + criteria === criterias.anyOf || + criteria === criterias.not) && ( {criteria === criterias.allOf && - conditionRow.allOf?.map((c, index) => ( -
- - { - const rules = (conditionRow.allOf || []).filter( - (_r, rindex) => index !== rindex, - ); - onRuleChange({ allOf: rules }); - }} - > - - -
- ))} + conditionRow.allOf?.map( + (c, index) => + (c as PermissionCondition).resourceType && ( +
+ + { + const rules = (conditionRow.allOf || []).filter( + (_r, rindex) => index !== rindex, + ); + onRuleChange({ allOf: rules }); + handleSetErrors( + [], + criteria, + undefined, + undefined, + index, + true, + ); + }} + > + + +
+ ), + )} {criteria === criterias.anyOf && - conditionRow.anyOf?.map((c, index) => ( -
+ conditionRow.anyOf?.map((c, index) => { + return ( + (c as PermissionCondition).resourceType && ( +
+ + { + const rules = (conditionRow.anyOf || []).filter( + (_r, rindex) => index !== rindex, + ); + onRuleChange({ anyOf: rules }); + handleSetErrors( + [], + criteria, + undefined, + undefined, + index, + true, + ); + }} + > + + +
+ ) + ); + })} + {criteria === criterias.not && ( + + handleNotConditionTypeChange(value as NotConditionType) + } + > + } + label="Add rule" + className={classes.radioLabel} + /> + {notConditionType === 'simple-condition' && ( + ruleOptionDisabled( + ruleOption, + conditionRow.not + ? [conditionRow.not as PermissionCondition] + : undefined, + ) + } setRemoveAllClicked={setRemoveAllClicked} /> - { - const rules = (conditionRow.anyOf || []).filter( - (_r, rindex) => index !== rindex, - ); - onRuleChange({ anyOf: rules }); - }} + )} + } + label="Add nested condition" + className={classes.radioLabel} + /> + + )} + {(criteria === criterias.allOf || criteria === criterias.anyOf) && ( + + )} + {(criteria === criterias.allOf || criteria === criterias.anyOf) && ( + + )} + {nestedConditionRow?.length > 0 && + nestedConditionRow.map((nc, nestedConditionIndex) => ( + +
- - -
+ + handleNestedConditionCriteriaChange( + newNestedCriteria, + nestedConditionIndex, + ) + } + className={classes.nestedConditioncriteriaButtonGroup} + > + {nestedConditionButtons.map(({ val, label }) => ( + + {label} + + ))} + + {criteria !== criterias.not && ( + + handleRemoveNestedCondition( + Object.keys(nc)[0], + nestedConditionIndex, + ) + } + > + + + )} +
+ + {Object.keys(nc)[0] === criterias.allOf && + nc.allOf?.map((c, index) => ( +
+ + + handleRemoveNestedConditionRule( + criterias.allOf, + nestedConditionIndex, + index, + ) + } + > + + +
+ ))} + {Object.keys(nc)[0] === criterias.anyOf && + nc.anyOf?.map((c, index) => ( +
+ + + handleRemoveNestedConditionRule( + criterias.anyOf, + nestedConditionIndex, + index, + ) + } + > + + +
+ ))} + {Object.keys(nc)[0] === criterias.not && ( + + ruleOptionDisabled( + ruleOption, + nc.not ? [nc.not as PermissionCondition] : undefined, + ) + } + setRemoveAllClicked={setRemoveAllClicked} + nestedConditionRow={nestedConditionRow} + nestedConditionCriteria={Object.keys(nc)[0]} + nestedConditionIndex={nestedConditionIndex} + updateRules={updateRules} + /> + )} + {Object.keys(nc)[0] !== criterias.not && ( + + )} + + ))} - )} - {criteria === criterias.condition && ( - - ruleOptionDisabled( - ruleOption, - conditionRow.condition ? [conditionRow.condition] : undefined, - ) - } - setRemoveAllClicked={setRemoveAllClicked} - /> - )} - {criteria === criterias.not && ( - - ruleOptionDisabled( - ruleOption, - conditionRow.not ? [conditionRow.not] : undefined, - ) - } - setRemoveAllClicked={setRemoveAllClicked} - /> - )} + {criteria === criterias.condition && renderConditionRule()} ); }; diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx index 8829f2dfbf..70cd917fee 100644 --- a/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx +++ b/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx @@ -2,36 +2,74 @@ import React from 'react'; import { PermissionCondition } from '@backstage/plugin-permission-common'; -import { Box, makeStyles, TextField } from '@material-ui/core'; +import { Box, makeStyles, TextField, Theme } from '@material-ui/core'; import { Autocomplete } from '@material-ui/lab'; import Form from '@rjsf/mui'; -import { RegistryFieldsType, RJSFSchema, UiSchema } from '@rjsf/utils'; +import { + RegistryFieldsType, + RJSFSchema, + RJSFValidationError, + UiSchema, +} from '@rjsf/utils'; import validator from '@rjsf/validator-ajv8'; import { criterias } from './const'; import { CustomArrayField } from './CustomArrayField'; import { RulesDropdownOption } from './RulesDropdownOption'; -import { ConditionsData, RuleParamsErrors, RulesData } from './types'; +import { Condition, ConditionsData, RulesData } from './types'; -const useStyles = makeStyles(theme => ({ +interface StyleProps { + isNotSimpleCondition: boolean; +} + +const useStyles = makeStyles(theme => ({ bgPaper: { backgroundColor: theme.palette.background.paper, }, params: { fontFamily: theme.typography.fontFamily, }, + inputFieldContainer: { + display: 'flex', + flexFlow: 'row', + gap: '10px', + flexGrow: 1, + margin: ({ isNotSimpleCondition }) => + isNotSimpleCondition ? '-1.5rem 0 0 1.85rem' : '0', + }, + ruleKeyInput: { + marginTop: '26px', + width: '50%', + }, + ruleValueInput: { + backgroundColor: theme.palette.background.paper, + marginTop: '26px', + width: '100%', + }, })); type ConditionFormRowFieldsProps = { - oldCondition: PermissionCondition; + oldCondition: Condition; index?: number; criteria: string; onRuleChange: (newCondition: ConditionsData) => void; conditionRow: ConditionsData; conditionRulesData?: RulesData; - setErrors: React.Dispatch>; + handleSetErrors: ( + newErrors: RJSFValidationError[], + criteria: string, + nestedCriteria?: string, + nestedConditionIndex?: number, + ruleIndex?: number, + removeErrors?: boolean, + ) => void; optionDisabled?: (ruleOption: string) => boolean; setRemoveAllClicked: React.Dispatch>; + nestedConditionRow?: ConditionsData[]; + nestedConditionCriteria?: string; + nestedConditionIndex?: number; + ruleIndex?: number; + updateRules?: (newCondition: ConditionsData[] | ConditionsData) => void; }; export const ConditionsFormRowFields = ({ @@ -41,13 +79,22 @@ export const ConditionsFormRowFields = ({ onRuleChange, conditionRow, conditionRulesData, - setErrors, + handleSetErrors, optionDisabled, setRemoveAllClicked, + nestedConditionRow, + nestedConditionCriteria, + nestedConditionIndex, + ruleIndex, + updateRules, }: ConditionFormRowFieldsProps) => { - const classes = useStyles(); + const classes = useStyles({ + isNotSimpleCondition: + criteria === criterias.not && !nestedConditionCriteria, + }); const rules = conditionRulesData?.rules ?? []; - const paramsSchema = conditionRulesData?.[oldCondition.rule]?.schema; + const paramsSchema = + conditionRulesData?.[(oldCondition as PermissionCondition).rule]?.schema; const schema: RJSFSchema = paramsSchema; @@ -88,23 +135,79 @@ export const ConditionsFormRowFields = ({ } }; + const handleNestedConditionChange = (newCondition: PermissionCondition) => { + if ( + !nestedConditionRow || + !nestedConditionCriteria || + nestedConditionIndex === undefined || + !updateRules + ) { + return; + } + const updatedNestedConditionRow: ConditionsData[] = nestedConditionRow.map( + (c, i) => { + if (i === nestedConditionIndex) { + if (nestedConditionCriteria === criterias.not) { + return { + [nestedConditionCriteria]: newCondition, + }; + } + const updatedNestedConditionRules = ( + (c[ + nestedConditionCriteria as keyof ConditionsData + ] as PermissionCondition[]) || [] + ).map((rule, rindex) => { + return rindex === ruleIndex ? newCondition : rule; + }); + + return { + [nestedConditionCriteria]: updatedNestedConditionRules, + }; + } + return c; + }, + ); + + updateRules( + criteria === criterias.not + ? updatedNestedConditionRow[0] + : updatedNestedConditionRow, + ); + if (newCondition.params && Object.keys(newCondition.params).length > 0) { + handleSetErrors( + [], + criteria, + nestedConditionCriteria, + nestedConditionIndex, + ruleIndex, + true, + ); + } + }; + + const onConditionChange = (newCondition: PermissionCondition) => { + if (nestedConditionRow) { + handleNestedConditionChange(newCondition); + } else { + handleConditionChange(newCondition); + } + }; + return ( - + optionDisabled ? optionDisabled(option) : false } onChange={(_event, ruleVal?: string | null) => - handleConditionChange({ + onConditionChange({ ...oldCondition, rule: ruleVal ?? '', params: {}, - }) + } as PermissionCondition) } renderOption={option => ( )} /> - {schema ? (
- handleConditionChange({ + onConditionChange({ ...oldCondition, params: data.formData || {}, - }) + } as PermissionCondition) } transformErrors={errors => { - setErrors({ [criteria]: errors }); + const hasErrors = errors.length > 0; + if (nestedConditionRow) { + handleSetErrors( + errors, + criteria, + nestedConditionCriteria, + nestedConditionIndex, + ruleIndex, + !hasErrors, + ); + } else { + handleSetErrors( + errors, + criteria, + undefined, + undefined, + index, + !hasErrors, + ); + } return errors; }} showErrorList={false} @@ -147,8 +268,7 @@ export const ConditionsFormRowFields = ({ /> ) : ( { }); }); - it('should return undefined if nested condition exists', () => { + it('should return nested conditions if nested condition exists', () => { const conditions = { allOf: [mockConditions[1].conditions, mockConditions[0].conditions], } as AllOfCriteria; const result = getConditionsData(conditions); + const expectedResult = { + allOf: [ + { + allOf: [ + { + params: { label: 'temp' }, + resourceType: 'catalog-entity', + rule: 'HAS_LABEL', + }, + { + params: { key: 'status' }, + resourceType: 'catalog-entity', + rule: 'HAS_METADATA', + }, + ], + }, + { + params: { annotation: 'temp' }, + resourceType: 'catalog-entity', + rule: 'HAS_ANNOTATION', + }, + ], + }; - expect(result).toBeUndefined(); + expect(result).toEqual(expectedResult); }); }); @@ -422,7 +445,7 @@ describe('getConditionalPermissionsData', () => { expect(result).toEqual([]); }); - it('should skip conditional permission with nested upper criteria', () => { + it('should return nested conditional permission with nested upper criteria', () => { const conditionalPermissions = [ { id: 1, @@ -455,6 +478,41 @@ describe('getConditionalPermissionsData', () => { permissionPolicies, ); - expect(result).toEqual([]); + const expectedResultConditions = { + allOf: [ + { + allOf: [ + { + params: { + label: 'temp', + }, + resourceType: 'catalog-entity', + rule: 'HAS_LABEL', + }, + { + params: { + key: 'status', + }, + resourceType: 'catalog-entity', + rule: 'HAS_METADATA', + }, + ], + }, + { + params: { + annotation: 'temp', + }, + resourceType: 'catalog-entity', + rule: 'HAS_ANNOTATION', + }, + ], + }; + + expect(result[0].conditions?.allOf).toHaveLength(2); + + const allOfConditions = result[0].conditions?.allOf || []; + expect(allOfConditions[0]).toHaveProperty('allOf'); + expect(allOfConditions[1]).toHaveProperty('params'); + expect(result[0].conditions).toEqual(expectedResultConditions); }); }); diff --git a/plugins/rbac/src/utils/rbac-utils.ts b/plugins/rbac/src/utils/rbac-utils.ts index 1784a75e5d..95f5cf26bd 100644 --- a/plugins/rbac/src/utils/rbac-utils.ts +++ b/plugins/rbac/src/utils/rbac-utils.ts @@ -228,7 +228,7 @@ export const getPermissionsData = ( }; export const getConditionUpperCriteria = ( - conditions: PermissionCriteria, + conditions: PermissionCriteria | string, ): string | undefined => { return Object.keys(conditions).find(key => [criterias.allOf, criterias.anyOf, criterias.not].includes(key), @@ -244,26 +244,31 @@ export const getConditionsData = ( case criterias.allOf: { const allOfConditions = (conditions as AllOfCriteria) .allOf; - if (allOfConditions.find(c => !!getConditionUpperCriteria(c))) { - return undefined; - } + allOfConditions.map(aoc => { + if (getConditionUpperCriteria(aoc)) { + return getConditionsData(aoc); + } + return aoc; + }); return { allOf: allOfConditions as PermissionCondition[] }; } case criterias.anyOf: { const anyOfConditions = (conditions as AnyOfCriteria) .anyOf; - if (anyOfConditions.find(c => !!getConditionUpperCriteria(c))) { - return undefined; - } + anyOfConditions.map(aoc => { + if (getConditionUpperCriteria(aoc)) { + return getConditionsData(aoc); + } + return aoc; + }); return { anyOf: anyOfConditions as PermissionCondition[] }; } case criterias.not: { - const notConditions = (conditions as NotCriteria) - .not; - if (getConditionUpperCriteria(notConditions)) { - return undefined; - } - return { not: notConditions as PermissionCondition }; + const notCondition = (conditions as NotCriteria).not; + const nestedCondition = getConditionUpperCriteria(notCondition) + ? getConditionsData(notCondition) + : notCondition; + return { not: nestedCondition as PermissionCondition }; } default: return { condition: conditions as PermissionCondition }; diff --git a/plugins/rbac/tests/rbac.spec.ts b/plugins/rbac/tests/rbac.spec.ts index 12453a3b54..e376e44cdf 100644 --- a/plugins/rbac/tests/rbac.spec.ts +++ b/plugins/rbac/tests/rbac.spec.ts @@ -279,4 +279,46 @@ test.describe('RBAC plugin', () => { await page.locator(`a`).filter({ hasText: 'RBAC' }).click(); }); + + test('Edit role to convert conditional policy into nested conditional policy', async () => { + await expect( + page.getByRole('heading', { name: 'All roles (2)' }), + ).toBeVisible({ timeout: 20000 }); + + // edit/update policies + await page.locator(`a`).filter({ hasText: 'role:default/guests' }).click(); + await expect( + page.getByRole('heading', { name: 'role:default/guests' }), + ).toBeVisible({ + timeout: 20000, + }); + await page.getByRole('tab', { name: 'Overview' }).click(); + + await page.locator(RoleOverviewPO.updatePolicies).click(); + await expect(page.getByRole('heading', { name: 'Edit Role' })).toBeVisible({ + timeout: 20000, + }); + + // update conditional policy to nested conditions + await page.getByText('Configure access', { exact: true }).click(); + await page.getByText('AllOf', { exact: true }).click(); + await page.getByPlaceholder('Select a rule').first().click(); + await page.getByText('HAS_LABEL').click(); + await page.getByLabel('label').fill('dev'); + await page.getByText('Add nested condition').click(); + await page.getByPlaceholder('Select a rule').last().click(); + await page.getByText('HAS_METADATA').click(); + await page.getByLabel('key').fill('status'); + await page.getByTestId('save-conditions').click(); + + expect( + page.getByText('Configure access (2 rules)', { exact: true }), + ).toBeVisible(); + + await common.clickButton('Next'); + await common.clickButton('Save'); + await verifyText('Role role:default/guests updated successfully', page); + + await page.locator(`a`).filter({ hasText: 'RBAC' }).click(); + }); });