feat(rbac): nested condition

[ciiay]

For RHIDP-2553: Add support for creating nested conditions

Screen recording(updated on 7/1):

rhidp_2553_updated.mov

Screenshot for multiple level nested condition warning message:

[sonarcloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.6% Duplication on New Code

See analysis details on SonarCloud

[divyanshiGupta]

Thanks @ciiay for the PR.

I am not sure if you have seen this https://docs.google.com/document/d/1UtQeeTwPvUndxEInjvEt9JgKIA5Pnrkl5ImFmGsAjR8/edit?disco=AAABPVRBMts but there are plans for adding conditions via a file and it can have any levels of nesting. From our end we should make sure if this happens the UI doesnt break as we are only supporting 1 level of nesting for now. For testing this you can create multi level nested conditions using the CLI and add necessary checks in UI so that it doesnt break in case a multi level nested condition is present.

Added this here as a note and you can take this up while working on edit flow story.

[ciiay]

Hi @divyanshiGupta , thanks for the tips. I have tested with second level conditional permission policies with mocked data in dev env and it shows empty rule rows as shown in the below screenshot. As the nested conditions don't have a rule property, it only shows empty data in rule rows, and it doesn't break anything. My question is, do we have a design for such situation? What needs to be showed in this case to indicate that there are more levels of conditional permission policies? And is it within the scope of this story or out of this story?
cc. @ShiranHi

[ShiranHi]

@ciiay looks good, thank you! Few comments:

1. Can we add the nested condition tooltip (Figma)?

2. Can we increase the padding between the "Add rule" button and the field above it? it seems too small

3. This is really small but I think it will be better if the border of nested condition will be aligned with the beginning of the "Add nested condition" button (Figma)

4. Can we show this error only after users click on "Add rule" or on the side drawer "Save" button?

5. The bottom buttons of the side drawer "Save" and "Cancel" should be sticky, I didn't see that from the video so I write this to confirm we have it

My question is, do we have a design for such situation? What needs to be showed in this case to indicate that there are more levels of conditional permission policies? And is it within the scope of this story or out of this story?

We agreed to limit ourselves to just one level of nested conditions for now, despite the backend's capability to support multiple levels. In case users use the CLI to create multiple levels we will show them one below each other, so I think the nested condition fields should not be empty. Does this make sense?

[ciiay]

Hi @ShiranHi , thanks for the review. Here's updated screenshot.

About the form validation, it has consistent behavior as Plugin drop list and Resource type input field, which is from the form component we implemented with. I don't see a very straight forward solution here. @divyanshiGupta Any thoughts?

In case users use the CLI to create multiple levels we will show them one below each other, so I think the nested condition fields should not be empty

Do you mean we don't allow them to edit the multiple level nested conditions which is more than 1 level, but we need to display the nested condition? It's empty because the nested condition in a nested condition doesn't have a rule field to display in the Rule input field. I can filter the simple conditions in a nested condition which have a Rule field, but not sure what the design is for the nested conditions. Hopefully this screenshot explains what I mean:

The mocked condition I used is

      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'],
                },
              },
              {
                not: {  // this is 2nd level nested condition
                  rule: 'HAS_LABEL',
                  resourceType: 'catalog-entity',
                  params: { label: 'temp' },
                },
              },
              {
                anyOf: [  // this is another 2nd level nested condition
                  {
                    rule: 'HAS_TAG',
                    resourceType: 'catalog-entity',
                    params: { tag: 'dev' },
                  },
                  {
                    rule: 'HAS_TAG',
                    resourceType: 'catalog-entity',
                    params: { tag: 'test' },
                  },
                ],
              },
            ],
          },
        ],
      },

I'm thinking maybe we could give some message like this

[ShiranHi]

About the form validation, it has consistent behavior as Plugin drop list and Resource type input field, which is from the form component we implemented with. I don't see a very straight forward solution here. @divyanshiGupta Any thoughts?

I believe we should display error messages only after users have left the field empty, rather than immediately. If this behavior appears in other areas, I recommend making the same adjustment.

Do you mean we don't allow them to edit the multiple level nested conditions which is more than 1 level, but we need to display the nested condition? It's empty because the nested condition in a nested condition doesn't have a rule field to display in the Rule input field. I can filter the simple conditions in a nested condition which have a Rule field, but not sure what the design is for the nested conditions.

So, in case there are multiple level nested conditions, I suggest showing only the first level as in the screenshot and show a message like this one.

If we can provide a link explaining how to use the CLI for this, we can add it to the yellow notification.
And let's remove the red text "Detailed condition information is hard to display..."

[ciiay]

Hi @ShiranHi , thanks for the reply and quick design on multiple level nested conditions. I have uploaded the updated screen recording along with the screenshot of the multiple-level nested condition sidebar display.

I believe we should display error messages only after users have left the field empty, rather than immediately. If this behavior appears in other areas, I recommend making the same adjustment.

Totally agree that validating on blur provides a more user-friendly experience. In that case, do you mind if we address this requirement in a separate effort? The library(@rjsf/mui) we used for this input field only supports either validate-on-submit or live validation, which is the current behavior of RHDH v1.2. I have tried several ways to implement validation on blur, but no straightforward solution has emerged. Since this PR focuses on reusing the existing form component to implement the nested condition logic, we can track this improvement in a different ticket. What do you think?
@divyanshiGupta Any input on this?

And let's remove the red text "Detailed condition information is hard to display..."

That was only a note on the screenshot to explain the situation, not in the code 😄

[ShiranHi]

I have uploaded the updated screen recording along with the screenshot of the multiple-level nested condition sidebar display.

Thank you!

Since this PR focuses on reusing the existing form component to implement the nested condition logic, we can track this improvement in a different ticket. What do you think?

Sure, it would be great to change that everywhere.

That was only a note on the screenshot to explain the situation, not in the code 😄

Good to know haha, thanks for clarifying it!

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from divyanshigupta. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[divyanshiGupta]

IMO nested condition definition can be improved. We should explain what a nested condition is and then add that the UI currently supports only 1 level of nesting i.e multiple criteria based conditions can only be added to the upper most criteria.

@AndrienkoAleksandr @PatAKnight any suggestions on the definition?

[ShiranHi]

IMO nested condition definition can be improved. We should explain what a nested condition is and then add that the UI currently supports only 1 level of nesting i.e multiple criteria based conditions can only be added to the upper most criteria.

@AndrienkoAleksandr @parvathyvr any suggestions on the definition?

I find this text fairly clear in explaining what a nested condition is and discussing your suggestion. It mentions the limitation of having only 1 layer and how it has been incorporated into the main condition. I'm unsure about what you feel is still unclear or missing. Could you please explain what is missing/unclear?

[divyanshiGupta]

Nested conditions are <b>1 layer rules within a main condition
@ShiranHi I find above line confusing as nested conditions can have multiple levels. It looks like we are stating that in general nested conditions can have only 1 level whereas its the limitation of the UI we are providing. But its just a suggestion, if you think it looks correct then I am fine with it :)

[divyanshiGupta]

cc: @invincibleJai

[ShiranHi]

I find above line confusing as nested conditions can have multiple levels. It looks like we are stating that in general nested conditions can have only 1 level whereas its the limitation of the UI we are providing.

Got it, I see your point. I think that if we allow for one level of nested conditions on the UI, this is the story we should communicate to users, unless it will be confusing. I've tried some other wording but it feels that this sentence is the best option for now. Unless you have a better suggestion, let's stick with this version, as it was suggested by our UX copywriter.