Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(rbac): conditional access form validation #1699

Merged
merged 1 commit into from
May 28, 2024
Merged

fix(rbac): conditional access form validation #1699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion plugins/rbac/src/__fixtures__/mockConditionRules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ export const mockConditionRules = [
properties: {
kinds: {
type: 'array',
items: { type: 'string' },
items: { type: 'string' }, // TODO: should be enum?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ciiay this actually depends on the plugin devs/maintainer as what condition rules they add to their plugins resource-types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove the comment.

minItems: 1,
description: 'List of kinds to match at least one of',
},
},
Expand All @@ -110,6 +111,7 @@ export const mockConditionRules = [
claims: {
type: 'array',
items: { type: 'string' },
minItems: 1,
description:
'List of claims to match at least one on within ownedBy',
},
Expand Down
4 changes: 2 additions & 2 deletions plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,14 @@ export const ConditionsFormRowFields = ({
{schema ? (
<Form
schema={paramsSchema}
formData={oldCondition?.params ?? null}
formData={oldCondition?.params || {}}
validator={validator}
uiSchema={uiSchema}
fields={customFields}
onChange={data =>
handleConditionChange({
...oldCondition,
params: data.formData ?? {},
params: data.formData || {},
})
}
transformErrors={errors => {
Expand Down
5 changes: 3 additions & 2 deletions plugins/rbac/src/components/ConditionalAccess/CustomArrayField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ export const CustomArrayField = (props: FieldProps) => {
label={name}
value={fieldVal}
onChange={e => {
setFieldVal(e.target.value);
onChange(e.target.value.split(',').map(val => val.trim()));
const value = e.target.value;
setFieldVal(value);
onChange(value ? value.split(',').map(val => val.trim()) : []);
Copy link
Collaborator

@divyanshiGupta divyanshiGupta May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if a minItems attribute is available in the paramsSchema of the array param then an empty array will return a validation error. But when the array param for example kinds is just listed as required this will not return any error as we are returning [] and its an acceptable value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it returns an empty array, the item account will be 0 which doesn't meet the minItem: 1 requirement and it throws an error.

Copy link
Collaborator

@divyanshiGupta divyanshiGupta May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrienkoAleksandr I am able to add below condition on catalog-entity :

        {
            "rule": "IS_ENTITY_KIND",
            "resourceType": "catalog-entity",
            "params": {
                "kinds": []
            }
         }

On applying above the corresponding role members cannot access any type of catalog-entity kinds. Does this look correct to you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can deny such conditions with help of validator in the frame of the https://issues.redhat.com/browse/RHIDP-2157 . But maybe it can be feature...

Copy link
Collaborator

@divyanshiGupta divyanshiGupta May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But here https://docs.google.com/document/d/1XMEmSlwVog1cOD0X5ADwg53Vnm8PL-HH_8LZriUM7vs/edit?disco=AAABNnL7BR8 kinds is just required which mean kinds cannot be undefined but it can be [] as it doesnt have minItems: 1 to restrict an empty array. So can this might be what plugin condition rules author expects or its a backend bug?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrienkoAleksandr ^^

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So can this might be what plugin condition rules author expects or its a backend bug?

I believe we are not responsible for such arrays. The plugin author designs and controls this aspect.

}}
className={classes.bgPaper}
required={required}
Expand Down
Loading