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.

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

feat(rbac): add ownership conditional rule to backend plugin #3265

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

PatAKnight
Copy link
Contributor

@PatAKnight PatAKnight commented Mar 18, 2025

Hey, I just made a Pull Request!

Adds a new conditional rule to the RBAC backend plugin. This rule can be used for multi tenant support. This will grant admins of Backstage the ability to allow team leads access to the RBAC plugins conditionally. This conditional access would limit these team leads to only being able to read, edit, and delete permission policies and roles that they are an owner of.

This also includes the new optional field owner for the role metadata that can be used alongside the newly added conditional rule.

Include are some docs with a step by step example from both the admin's and the team lead's point of view. This can be used to help with testing.

Changes to the frontend:

Admin's point of view
bzi-qmqo-zzi (2025-03-19 20_32 GMT-4)

Team lead's point of view
bzi-qmqo-zzi (2025-03-19 20_42 GMT-4)

User's point of view
bzi-qmqo-zzi (2025-03-19 20_44 GMT-4)

Note with the user's point of view, the first 404 page is from trying to access the RBAC page directly. In this case http://localhost:3000/rbac. The second is from trying to access the edit page directly for a particular role. Example http://localhost:3000/rbac/role/role/default/team.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

Notes

Worth some discussion on

Backend:

  • To prevent created a breaking change and dealing with conditional policies with the create action, I included some custom code to prevent those policies from being created.
  • I originally included an ownership check for creating permission policies and conditional policies. However, because I started preventing conditional policies with the create action from being created, I had to remove this check.

Frontend:

  • I had to change the permission that we used for the RBAC page from policy.entity.read to policy.entity.create. This is because we were using the usePermission incorrectly and this was leading to issues whenever a condition was applied to policy.entity.read. An alternative to this could be how we handle the Administration here. Ended up updating the RBAC page so that it is once again protected by the policy.entity.read permission.

@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Mar 18, 2025

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-rbac-backend workspaces/rbac/plugins/rbac-backend minor v6.0.0
@backstage-community/plugin-rbac-common workspaces/rbac/plugins/rbac-common minor v1.14.0
@backstage-community/plugin-rbac workspaces/rbac/plugins/rbac minor v1.39.2

@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch 2 times, most recently from ba72894 to d740702 Compare March 20, 2025 02:57
@ShiranHi
Copy link

Thank you, Patrick. I have 2 questions:

  1. Why didn't you use the owner field in the first step?
  2. Is "Permissions", appears on the Plugins dropdown, the RBAC plugin? I think it would be better to use a more descriptive name. What do you think?

@PatAKnight
Copy link
Contributor Author

Thank you, Patrick. I have 2 questions:

  1. Why didn't you use the owner field in the first step?
  2. Is "Permissions", appears on the Plugins dropdown, the RBAC plugin? I think it would be better to use a more descriptive name. What do you think?
  1. We don't set it in the first step because we would be giving access the team lead access to the role role:default/team-lead. This would then allow them to make changes like removing the conditional policy, which would in turn grant them full access to the RBAC plugins.
  2. Permissions appears in the dropdown because officially our pluginId is permission.

@PatAKnight PatAKnight changed the title [WIP] feat(rbac): add ownership conditional rule to backend plugin feat(rbac): add ownership conditional rule to backend plugin Mar 21, 2025
@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch 2 times, most recently from fd86f6c to de5f9c9 Compare March 24, 2025 15:33
@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch from de5f9c9 to 49ecc23 Compare March 25, 2025 04:30
@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch from 49ecc23 to a630493 Compare March 25, 2025 14:14
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr left a comment

Choose a reason for hiding this comment

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

Backend looks good.

Copy link
Contributor

@its-mitesh-kumar its-mitesh-kumar left a comment

Choose a reason for hiding this comment

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

@PatAKnight
Should we provide edit icon on role details page in About section , since some user will like to edit owner , currently if they wish to do so , the need to click edit icon in Users and groups and they will need to click back . Or is it finr for now .
Screenshot 2025-03-26 at 2 01 21 PM

Copy link
Contributor

@its-mitesh-kumar its-mitesh-kumar left a comment

Choose a reason for hiding this comment

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

Not able to edit owner attaching screen recording for the same .

RBAC-owner-edit-1.mov

@ShiranHi
Copy link

Should we provide edit icon on role details page in About section , since some user will like to edit owner , currently if they wish to do so , the need to click edit icon in Users and groups and they will need to click back . Or is it finr for now .

That's a good idea!
Also, if we have no owner can we add "--" like in the design?

@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch from a630493 to aa8de92 Compare March 26, 2025 13:50
@PatAKnight
Copy link
Contributor Author

@PatAKnight Should we provide edit icon on role details page in About section , since some user will like to edit owner , currently if they wish to do so , the need to click edit icon in Users and groups and they will need to click back . Or is it finr for now .

@its-mitesh-kumar I think that it should be fine for now and we can create a follow up task for this. Users are still able to make the edit from the RBAC page as well. So, at least there are a couple of avenues that they can take. Sorry, just trying to keep the scope down as much as possible

Not able to edit owner attaching screen recording for the same .

@its-mitesh-kumar, thanks for the catch! Looks like I missed the assignment in the backend. Should be fixed now.

Should we provide edit icon on role details page in About section , since some user will like to edit owner , currently if they wish to do so , the need to click edit icon in Users and groups and they will need to click back . Or is it finr for now .

That's a good idea! Also, if we have no owner can we add "--" like in the design?

@ShiranHi, looks like there was actually a bug with this. It is fixed now and will display No owners matching the No information and No description for the modified and description fields.

@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch from 47a9f6b to 1f4ca47 Compare March 28, 2025 03:03
@dzemanov
Copy link
Contributor

Tested this after a rebase. Everything works correctly except 1 issue, great work @PatAKnight!

I tested this with yaml and csv permissions. Not sure if these are in scope? But for on startup creation, I get this error:

[backend]: 2025-03-28T10:08:30.849Z permission info permission.condition-write isAuditEvent=true eventId="condition-write" severityLevel="medium" actor={"actorId":"plugin:permission"} request=undefined meta={"actionType":"create","condition":{"result":"CONDITIONAL","roleEntityRef":"role:default/team-lead-csv","pluginId":"permission","resourceType":"policy-entity","permissionMapping":["read","update","delete"],"conditions":{"rule":"IS_OWNER","resourceType":"policy-entity","params":{"owners":["user:default/dzemanov"]}}}} error="Error: Unable to get permission list for plugin permission" status="failed"

Probably because this is on startup and the endpoint is not there yet. I tested with:

g, group:default/team-a, role:default/team-lead-csv
p, role:default/team-lead-csv, catalog.entity.read, read, allow
p, role:default/team-lead-csv, policy.entity.create, create, allow
---
result: CONDITIONAL
roleEntityRef: "role:default/team-lead-csv"
pluginId: permission
resourceType: policy-entity
permissionMapping:
  - read
  - update
  - delete
conditions:
  rule: IS_OWNER
  resourceType: policy-entity
  params:
    owners:
      - "user:default/dzemanov"

When I later updated the yaml or csv file, permissions were correctly created and applied.

Copy link
Contributor

@divyanshiGupta divyanshiGupta left a comment

Choose a reason for hiding this comment

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

@PatAKnight I am able to see the role created by admin for the team lead in team leads account and its editable. IMO role created for team lead by admin should not be visible to the lead.

lead.able.to.see.adminrole.mp4

Also I am able to create role from lead account without adding the owner and the roles gets created but is not visible to the lead. The role created by the lead irrespective of if they added themselves as owner or not should be visible otherwise its not a good user experience as they can create the role but not see it.

Screen.Recording.2025-03-28.at.4.56.17.PM.mov

Copy link
Contributor

@divyanshiGupta divyanshiGupta left a comment

Choose a reason for hiding this comment

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

@PatAKnight Instead of No owner can we just show - as this is what we showed previously for all other fields?

Screenshot 2025-03-28 at 4 27 54 PM

Here also instaed of leaving owner empty can we show - so that it is clear that the owner is not added here.

Screenshot 2025-03-28 at 4 30 35 PM

@PatAKnight
Copy link
Contributor Author

@PatAKnight I am able to see the role created by admin for the team lead in team leads account and its editable. IMO role created for team lead by admin should not be visible to the lead.

@divyanshiGupta this was because you assigned the team lead to be the owner of that role. If you were to remove them from ownership and instead leave it blank or assign the admin as the owner, then they would not be able to see it.

Also I am able to create role from lead account without adding the owner and the roles gets created but is not visible to the lead. The role created by the lead irrespective of if they added themselves as owner or not should be visible otherwise its not a good user experience as they can create the role but not see it.

The conditional rule acts as a filter based on what the rule is targeting, in this case the owner field of the role. If it is not set, then the role will be filtered out by the condition. This is similar behavior for all conditional rules within Backstage.

An example would be if you have catalog.entity.create and a condition for ownership with catalog.entity.read. In this event you can add a catalog entity that you are not an owner of to the catalog and not be able to see it.

@PatAKnight Instead of No owner can we just show - as this is what we showed previously for all other fields?

So, I mentioned above that there was actually a bug related to this and we are suppose to be showing 'No description' and 'No information' for the fields that are not set. I can change the owner field to display '-', if we are fine with it being different from the other fields.

Here also instaed of leaving owner empty can we show - so that it is clear that the owner is not added here.

Yes, I can update the review section

@PatAKnight
Copy link
Contributor Author

Here also instaed of leaving owner empty can we show - so that it is clear that the owner is not added here.

@divyanshiGupta forgot to ask, but would you like for me to do the same with the description as well?

@PatAKnight PatAKnight force-pushed the rbac-conditional-rules branch from 9f5f783 to 63c5488 Compare March 28, 2025 15:59
@ShiranHi
Copy link

@PatAKnight , @divyanshiGupta it is better to use a double dash (--) when you want to indicate "no value" more explicitly.

@PatAKnight
Copy link
Contributor Author

@ShiranHi, I am fine with swapping 'No owner' to '--'. Do we also want to update 'No description' and 'No information' for the description and modified fields?

@ShiranHi
Copy link

@ShiranHi, I am fine with swapping 'No owner' to '--'. Do we also want to update 'No description' and 'No information' for the description and modified fields?

Yes please, let's apply it everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants