RBAC: Make access control metadata for folders work with nested folders #66464

IevaVasiljeva · 2023-04-13T12:39:11Z

What is this feature?

Removing access control metadata from search results. This was used when filtering for folders where an alert rule can be created - this has been substituted by searching for folder of type dash-folder-alerting that the user has edit permissions on.

Also change the logic for how AC metadata is computed for a single folder - permissions from parents should cascade down to children.

Why do we need this feature?

The logic to add access control metadata was not taking into account nested folders, which lead to a broken UI (for alert rules etc).

Who is this feature for?

Internal change to make nested folders work with RBAC.

Which issue(s) does this PR fix?:

Fixes #https://github.com/grafana/grafana-authnz-team/issues/196

Special notes for your reviewer:

This PR changes the objects returned by search APIs. However, accesscontrol query parameter is not documented, so we have no guarantees around it, therefore I think it's fine to remove AC metadata.

Eventually, we might need to rethink the way we return access related information for folders (ie, when other objects will be stored in folders as well).

IevaVasiljeva · 2023-04-13T13:07:24Z

public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx

-      const canUpdateInCurrentFolder =
-        existingRuleForm &&
-        hit.folderId === existingRuleForm.id &&
-        contextSrv.hasAccessInMetadata(AccessControlAction.AlertingRuleUpdate, hit, rbacDisabledFallback);


This is the one bit that might be different after this PR - all the folders that the user has AlertingRuleCreate action on will be listed, but we wouldn't list a folder that the user can update rules in but can't create rules in. This would be a really uncommon AC setup though, so I think it's fine not to account for this case. Please correct me if I'm wrong though.

I agree this is a relatively uncommon setup. However, if it happens, we'll be displaying totally incorrect values when the user Edit an alert rule. We'll show a list of folders with AlertingRuleCreate permission, whereas all of them might be wrong (because the user can have edit permission to none of them)
So, in the described scenario, rule Editing will be completely broken
@yuri-tceretian wdyt?

If I'm reading the old code correctly, it would not be completely incorrect values. Previously it would have shown folders where the user can create alerts + the folder that the alert rule that is being updated is in (if the user is allowed to update alerts in it).

After this change, the risk is that we might not show the current folder of the alert rule that is being updated.

It's still a risk though, so if you think that this is bad enough and is likely to be an issue for some users, we'll need to think about another way to solve this.

I think the situation when a user can only update rules but not create one is not something uncommon.

I created a role

role

{ "version": 1, "uid": "ce004a84-4757-4151-8803-b43469f05c4c", "name": "custom:alerting_3", "displayName": "Custom Alerting in folder", "description": "Control alerts in folder", "group": "", "permissions": [ { "action": "folders:read", "scope": "folders:*", "updated": "2023-04-17T15:07:00-04:00", "created": "2023-04-17T15:07:00-04:00" }, { "action": "datasources:query", "scope": "datasources:uid:*", "updated": "2023-04-17T15:07:00-04:00", "created": "2023-04-17T15:07:00-04:00" }, { "action": "alert.rules:write", "scope": "folders:uid:d40049df-f4cd-4106-a9ae-fefffd26d8df", "updated": "2023-04-17T15:07:00-04:00", "created": "2023-04-17T15:07:00-04:00" }, { "action": "alert.rules:write", "scope": "folders:uid:f750b100-662a-4ccb-9ce8-97bb8e28988d", "updated": "2023-04-17T15:07:00-04:00", "created": "2023-04-17T15:07:00-04:00" }, { "action": "alert.rules:create", "scope": "folders:uid:c43e5f3d-be81-4684-b349-a08c8db29bfb", "updated": "2023-04-17T15:07:00-04:00", "created": "2023-04-17T15:07:00-04:00" } ], "updated": "2023-04-17T15:07:00.722346-04:00", "created": "2023-04-17T15:07:00.722346-04:00", "global": false }

that gives write (update) access to two folders and create-only in the 3rd and then assigned to a user with a Viewer base role.

When that user edits a rule in either f750b1 or d4004 folders with write access, the dropdown shows only two folders: the current folder that stores the alert and c43e5 for which user has create-only access. That happens in the main and PR branches.
The only difference I could find is what search API call returns: in PR branch it returns only those 2 folders whereas in main it returns all folders available for read with access control metadata.

So, I think nothing really changes from alerting editor point of view.

The only difference I could find is what search API call returns: in PR branch it returns only those 2 folders whereas in main it returns all folders available for read with access control metadata.

@yuri-tceretian, I did your setup but I seem to have a different output, on my side only the folder I have alert.rules:create on is returned when the search API is called with type=dash-folder-alerting&permission=Edit. And all three folders are being returned when queried with: type=dash-folder. Which seems to be normal.

That being said, @IevaVasiljeva I investigated your worry:

Previously it would have shown folders where the user can create alerts + the folder that the alert rule that is being updated is in (if the user is allowed to update alerts in it).

When editing a rule in the folder I have alert.rules:write in, I correctly see the folder listed and I also see the folder I have alert.rules:create in.

I checked again, and yes! The API returned only the one user has create permission but UI displayed both: current rule's folder and the one that user has create access to.

Thanks so much for testing this @yuri-tceretian and @gamab! It's great to hear that everything works as it should.

eleijonmarck · 2023-04-14T12:59:15Z

@IevaVasiljeva hey, trying to test this out. is there any particular tests you think might be relevant to try out? sort of like a mini bugbash for this?

IevaVasiljeva · 2023-04-14T13:25:00Z

@IevaVasiljeva hey, trying to test this out. is there any particular tests you think might be relevant to try out? sort of like a mini bugbash for this?

Sure. Rough steps to test:

enabled nestedFolders feature flag;
create some nested folders and grant access to a non-admin user to some of them (say, have folders A < B < C, where A is the parent of B etc, and grant a viewer user edit access to B);
now try to create an alert rule and see what options you get for folders to save this rule in;
then try to edit this alert rule and see whether you can edit it and what options you have;
play around with alerts some more, check if nothing else looks broken;
check if the option to create a new dashboard correctly shows up under the folder that the viewer is an editor of.

gamab

The code is looking good so far 👍 I have done some basic testing with regular folders and it seems to work fine.
I plan to review it again tomorrow with a fresh mind and check the following:

Whether metadata is still needed or not in the places where the modified endpoints (/api/folders/ and /api/search) were used.
Test the code with nested folders.

My only concerns at this time are:

Having metadata for folders was a step towards making our API data transfer objects more standardized, but it seems like we may be moving away from this approach with these changes.
As you pointed out these eliminative changes could be considered breaking. Even if the access-control parameter wasn’t technically documented, it’s still relatively easy to find and understand.

That being said, I assume you spent time evaluating the pros and cons of making metadata work with nested folders and I'd value your findings on this.

pkg/api/folder_test.go

gamab · 2023-04-18T12:30:07Z

pkg/api/folder_test.go


 		req := server.NewGetRequest("/api/folders/folderUid?accesscontrol=true")
 		webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{
 			1: accesscontrol.GroupScopesByAction([]accesscontrol.Permission{
 				{Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersAll},
-				{Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("folderUid")},
+				{Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("parent_uid")},
+				{Action: dashboards.ActionDashboardsCreate, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("folderUid")},


We could check this one is in the metadata as well.

pkg/api/folder_test.go

gamab · 2023-04-18T13:30:18Z

public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx

-      const canUpdateInCurrentFolder =
-        existingRuleForm &&
-        hit.folderId === existingRuleForm.id &&
-        contextSrv.hasAccessInMetadata(AccessControlAction.AlertingRuleUpdate, hit, rbacDisabledFallback);


The only difference I could find is what search API call returns: in PR branch it returns only those 2 folders whereas in main it returns all folders available for read with access control metadata.

@yuri-tceretian, I did your setup but I seem to have a different output, on my side only the folder I have alert.rules:create on is returned when the search API is called with type=dash-folder-alerting&permission=Edit. And all three folders are being returned when queried with: type=dash-folder. Which seems to be normal.

That being said, @IevaVasiljeva I investigated your worry:

Previously it would have shown folders where the user can create alerts + the folder that the alert rule that is being updated is in (if the user is allowed to update alerts in it).

When editing a rule in the folder I have alert.rules:write in, I correctly see the folder listed and I also see the folder I have alert.rules:create in.

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

IevaVasiljeva · 2023-04-21T10:58:09Z

My only concerns at this time are:

Having metadata for folders was a step towards making our API data transfer objects more standardized, but it seems like we may be moving away from this approach with these changes.

As you pointed out these eliminative changes could be considered breaking. Even if the access-control parameter wasn’t technically documented, it’s still relatively easy to find and understand.

That being said, I assume you spent time evaluating the pros and cons of making metadata work with nested folders and I'd value your findings on this.

I agree with both of your points. My thinking was that adding metadata to the search endpoint is quite expensive computationally. So while we can reintroduce it if needed, I think we should try to use other means of filtering the folder search results by access level if possible. It turned out that we only use the metadata from the search endpoint in one place, and it was quite easy to replace it with a different search query.

Regarding this being a breaking change - again, I agree. But, as it is undocumented, we have no guarantees about it. Plus we still return metadata for endpoints that query for individual folders, so there is a workaround for anyone who was using the metadata from the folder search request. (I don't think that we add metadata to dashboard GET endpoints, but we could do if needed.)

gamab

Thanks for taking the time to explain, it makes a lot of sense and I'm onboard with you. Only nuance I see is that filtering based on permission level does not serve the exact same purpose as AC metadata for the frontend but as you said: it's not needed for now and here the use case was better covered by permission level filtering.

Good job, btw on filtering based on the type and the permission level, it really improved the performance of the alerting frontend queries.

The testing I promised:

✔️ Whether metadata is still needed or not in the places where the modified endpoints (/api/folders/ and /api/search) were used.
✔️ Test the code with nested folders.

I did create a few nested folders and dashboards within them:

▶️ What puzzled me a bit was not to have the new button is the nested folders view to create folders and dashboards.

I played a bit with access-control management for alert rules in nested folders and everything worked correctly.
Only surprises were:
▶️ when you create a rule in a folder, you do not acquire permissions to edit it. (Which I guess makes sense though, since the scope is the folder)
▶️ the alert rule view does not display the folder hierarchy tree.

Alerts in nested folders

Btw, in case you want to test, I created the following role and assigned it to a Viewer:

YAML provisioning file

roles:
  - name: 'custom:alerting_3'
    uid: 'custom_alerting_3'
    displayName: 'Custom Alerting 3'
    description: 'Custom Alerting 3'
    version: 2
    permissions:
    - action: "dashboards:create"
      scope: "folders:*"
    - action: "folders:read"
      scope: "folders:*"
    - action: "datasources:query"
      scope: "datasources:uid:*"
    # Should allow edit on folder1, nested1 and nested2
    - action: "alert.rules:write"
      scope: "folders:uid:aa7f602c-acca-4be6-96bb-46fce7bf81b3"
    # Should allow edit on folder 2nested1
    - action: "alert.rules:write"
      scope: "folders:uid:aa7410d8-c3bc-4b72-9ae3-1a8ecc643d5f"
    # Should allow create on 3nested1 and 3nested2
    - action: "alert.rules:create"
      scope: "folders:uid:bde4c8aa-c825-4031-9685-c28b6dacad95"

gamab · 2023-04-21T13:37:04Z

Really good job on this 🧠

IevaVasiljeva · 2023-04-21T14:05:06Z

Thanks for the amazing review and for your usual thoroughness @gamab ❤️

I'll raise your points with the nested folder group, I think we can definitely improve the UX.

IevaVasiljeva added 3 commits April 13, 2023 11:45

remove metadata for single folder listing

018ac22

extendTests

867740e

remove ac metadata from dash and folder search results

2a3c5c1

IevaVasiljeva added this to the 10.0.0 milestone Apr 13, 2023

IevaVasiljeva changed the title ~~RBAC: remove access control metadata from search results and folder API responses~~ RBAC: Remove access control metadata from search results and folder API responses Apr 13, 2023

grafanabot added area/frontend area/backend labels Apr 13, 2023

IevaVasiljeva added add to changelog no-backport Skip backport of PR and removed area/backend area/frontend labels Apr 13, 2023

grafanabot added area/backend area/frontend labels Apr 13, 2023

IevaVasiljeva requested a review from Jguer April 13, 2023 12:58

remove test

eb94903

IevaVasiljeva marked this pull request as ready for review April 13, 2023 13:00

IevaVasiljeva requested review from a team as code owners April 13, 2023 13:00

IevaVasiljeva requested a review from a team April 13, 2023 13:00

IevaVasiljeva requested a review from a team as a code owner April 13, 2023 13:00

IevaVasiljeva requested review from tskarhed, ashharrison90, ivanortegaalba, polibb, papagian, zserge and yangkb09 and removed request for a team April 13, 2023 13:00

IevaVasiljeva requested review from konrad147, yuri-tceretian and a team April 13, 2023 13:00

IevaVasiljeva commented Apr 13, 2023

View reviewed changes

remove one more test

20ca941

IevaVasiljeva requested a review from gamab April 14, 2023 07:52

ArturWierzbicki requested a review from ryantxu April 14, 2023 08:12

Merge branch 'main' into remove-folder-ac-metadata

2f319d4

IevaVasiljeva marked this pull request as draft April 14, 2023 09:18

put ac metadata back for single folder API responses

fd4ffe2

IevaVasiljeva changed the title ~~RBAC: Remove access control metadata from search results and folder API responses~~ RBAC: Make access control metadata for folders work with nested folders Apr 14, 2023

extend tests

279602a

IevaVasiljeva marked this pull request as ready for review April 14, 2023 12:33

remove ac metadata from folder frontend object

af95b05

undo unneeded change

44972fd

gamab reviewed Apr 18, 2023

View reviewed changes

PR feedback

68fc6cd

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

gamab approved these changes Apr 21, 2023

View reviewed changes

IevaVasiljeva merged commit 5d7433d into main Apr 21, 2023

IevaVasiljeva deleted the remove-folder-ac-metadata branch April 21, 2023 14:05

zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC: Make access control metadata for folders work with nested folders #66464

RBAC: Make access control metadata for folders work with nested folders #66464

IevaVasiljeva commented Apr 13, 2023 •

edited

Loading

IevaVasiljeva Apr 13, 2023

konrad147 Apr 14, 2023

IevaVasiljeva Apr 14, 2023

yuri-tceretian Apr 17, 2023

gamab Apr 18, 2023 •

edited

Loading

yuri-tceretian Apr 18, 2023

IevaVasiljeva Apr 21, 2023

eleijonmarck commented Apr 14, 2023

IevaVasiljeva commented Apr 14, 2023

gamab left a comment •

edited

Loading

gamab Apr 18, 2023

gamab Apr 18, 2023 •

edited

Loading

IevaVasiljeva commented Apr 21, 2023

gamab left a comment •

edited

Loading

gamab commented Apr 21, 2023

IevaVasiljeva commented Apr 21, 2023

RBAC: Make access control metadata for folders work with nested folders #66464

RBAC: Make access control metadata for folders work with nested folders #66464

Conversation

IevaVasiljeva commented Apr 13, 2023 • edited Loading

IevaVasiljeva Apr 13, 2023

Choose a reason for hiding this comment

konrad147 Apr 14, 2023

Choose a reason for hiding this comment

IevaVasiljeva Apr 14, 2023

Choose a reason for hiding this comment

yuri-tceretian Apr 17, 2023

Choose a reason for hiding this comment

gamab Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

yuri-tceretian Apr 18, 2023

Choose a reason for hiding this comment

IevaVasiljeva Apr 21, 2023

Choose a reason for hiding this comment

eleijonmarck commented Apr 14, 2023

IevaVasiljeva commented Apr 14, 2023

gamab left a comment • edited Loading

Choose a reason for hiding this comment

gamab Apr 18, 2023

Choose a reason for hiding this comment

gamab Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

IevaVasiljeva commented Apr 21, 2023

gamab left a comment • edited Loading

Choose a reason for hiding this comment

gamab commented Apr 21, 2023

IevaVasiljeva commented Apr 21, 2023

IevaVasiljeva commented Apr 13, 2023 •

edited

Loading

gamab Apr 18, 2023 •

edited

Loading

gamab left a comment •

edited

Loading

gamab Apr 18, 2023 •

edited

Loading

gamab left a comment •

edited

Loading