feat(rbac): nested condition

[ciiay]

For RHIDP-2553: Add support for creating nested conditions

Screen recording(updated on 7/1):

https://github.com/janus-idp/backstage-plugins/assets/26255262/7127740e-e988-43e2-a5da-df621e2d0b1d

Screenshot for multiple level nested condition warning message:

[openshift-ci[bot]]

[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 assign gashcrumb for approval. 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](https://github.com/janus-idp/backstage-plugins/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[sonarcloud[bot]]

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!