LibraryPanels: Add RBAC support #73475

kaydelaney · 2023-08-18T10:45:25Z

Adds RBAC support to library panels / elements.

Fixes #50665

Things left to do:

Refactor permission evaluation code

Testing scenarios:

Creating a library panel with insufficient permissions
Reading a library panel with insufficient permissions
Updating a library panel with insufficient permissions
Deleting a library panel with insufficient permissions
Ensure folder permissions are inherited

IevaVasiljeva

Great to see RBAC being extended to library panels! 🎉

I've added some small suggestions. My main question is about how access to folders impacts access to library elements? We have several different approaches to it out there (for instance, alert actions use folder scopes while accessing annotations requires being able to access the dashboard that they belong to), all with their pros and cons.

pkg/services/libraryelements/accesscontrol.go

IevaVasiljeva · 2023-08-23T14:15:27Z

pkg/services/libraryelements/api.go

+		ac.EvalAny(
+			ac.EvalPermission(
+				ActionLibraryPanelsCreate,
+			),
+			ac.EvalPermission(
+				dashboards.ActionFoldersWrite,
+				dashboards.ScopeFoldersRoot+":id:"+fmt.Sprint(cmd.FolderID),
+			),
+		),


The access control middleware for this endpoint already checks ActionLibraryPanelsCreate, so this check will always pass, as user needs ActionLibraryPanelsCreate to get to this point. What is the intention? Do we want to make sure that they also have access to the folder? Or would it maybe make sense to scope the ActionLibraryPanelsCreate action to folders?

Maybe it should be EvalAll() in this case? but as @IevaVasiljeva mentioned, ActionLibraryPanelsCreate already checked, so it might be omitted.

IevaVasiljeva · 2023-08-23T14:19:52Z

pkg/services/libraryelements/api.go

+		c.SignedInUser,
+		ac.EvalAny(
+			ac.EvalPermission(
+				ScopeLibraryPanelsRoot+":"+action,


Nit: you could also use a scope provider pattern here: https://github.com/grafana/grafana/blob/main/pkg/services/accesscontrol/scope.go#L95

gamab

Great to see people adding RBAC to new features :)

I only left a few comments on my first findings to help a little bit :)
I agree with @IevaVasiljeva I just wonder how libraryelements permissions should be tied to folders 👍

pkg/services/libraryelements/api.go

mildwonkey

I can't helpfully review RBAC changes, but from a general backend platform perspective, this looks reasonable. You have more qualified folks looking at the important content of the PR, so no approvals from me, but no blockers either 😁

alexanderzobnin

Left some comments

pkg/services/libraryelements/api.go

alexanderzobnin · 2023-08-23T16:03:53Z

pkg/services/libraryelements/api.go

+		ac.EvalAny(
+			ac.EvalPermission(
+				ActionLibraryPanelsCreate,
+			),
+			ac.EvalPermission(
+				dashboards.ActionFoldersWrite,
+				dashboards.ScopeFoldersRoot+":id:"+fmt.Sprint(cmd.FolderID),
+			),
+		),


Maybe it should be EvalAll() in this case? but as @IevaVasiljeva mentioned, ActionLibraryPanelsCreate already checked, so it might be omitted.

alexanderzobnin · 2023-08-23T16:14:03Z

pkg/services/libraryelements/api.go

+			),
+			ac.EvalPermission(
+				dashboards.ScopeFoldersRoot+":"+action,
+				dashboards.ScopeFoldersRoot+":id:"+fmt.Sprint(folderID),


I don't think we use folder ids for checking the scope, guess uid should be used here. CC @IevaVasiljeva

Alex is right, we use folder UIDs to store folder related permissions. Another option would be to use the folder guardian to check access to the folder (https://github.com/grafana/grafana/blob/main/pkg/services/guardian/accesscontrol_guardian.go#L179).

Isn't there an ID scope resolver? https://github.com/grafana/grafana/blob/lib-rbac/pkg/services/dashboards/accesscontrol.go#L68

alexanderzobnin

Sorry, confused buttons

Wrong button

kaydelaney · 2023-08-31T15:14:56Z

I've been quietly working away addressing the feedback folks have been kind enough to give, and I think I've figured out most of the things I've been stuck on, but I do still have one question:
So, it seems like when a new dashboard is created, some code is executed (setDefaultPermissions) that updates the permissions table for the user creating the dashboard as well as the Viewer and Editor roles if the dashboard is in the General folder. Would I be correct in assuming similar code will need to be written for the library panels use case?

Oh and since I think some people asked about it, I think library panels should more or less share the permission inheritance rules that dashboards currently do. So a library panel will inherit the permissions of the folder that contains it.

kaydelaney · 2023-09-06T15:27:04Z

Lots of changes with that last commit. I more or less just implemented everything for library elements how I saw dashboards was handled. One question I had was about how the permissions for existing library elements should be handled. Will a migration have to be written to add appropriate entries to the permissions table? @IevaVasiljeva

IevaVasiljeva · 2023-09-13T10:08:53Z

It might also be helpful to split this PR into smaller parts. For instance, to tackle read access to library panels first, and then do creating/editing/deleting.

IevaVasiljeva

Thanks for all the responses! I've added some more comments. I think the permission setup that you're going for is closer to what alerting has than what dashboards have, so it's worth taking a look at that.

I'm also happy to chat about this in a call if that would be helpful :)

IevaVasiljeva · 2023-09-14T16:33:26Z

pkg/services/libraryelements/accesscontrol.go

+		if err != nil {
+			return nil, err
+		}
+		return append(inheritedScopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(libElDTO.FolderUID), ScopeLibraryPanelsProvider.GetResourceScopeUID(uid)), nil


I don't think we can mix UID scopes for folder UIDs and library element UID itself, as a library element can probably have the same UID as a folder? Maybe it's enough to only have folder UID based scopes? It would mean that a user can either CRUD all or no library panels in a given folder, do you think that would be fine-grained enough?

Hmm, it seems like the same thing is being done for dashboards though, unless I'm missing something?

grafana/pkg/services/dashboards/accesscontrol.go

Lines 188 to 193 in 0358527

result = append([]string{

ScopeDashboardsProvider.GetResourceScopeUID(dashboard.UID),

ScopeFoldersProvider.GetResourceScopeUID(folderUID),

},

result...,

)

Sorry, I think I misread it earlier. So you can access library panel either if you have library.panels:write with either the scope library.panels:uid:<library_panel_uid> or scope folders:uid:<folder_uid>, where the folder is one of library panel's parents? That seems good!

IevaVasiljeva · 2023-09-14T16:35:57Z

pkg/services/libraryelements/api.go

+	if l.features.IsEnabled(featuremgmt.FlagLibraryPanelRBAC) {
+		allowed, err := l.AccessControl.Evaluate(
+			c.Req.Context(),
+			c.SignedInUser,
+			ac.EvalPermission(
+				dashboards.ActionFoldersWrite,
+				dashboards.ScopeFoldersRoot+":id:"+fmt.Sprint(cmd.FolderID),
+			),
+		)
+		if err != nil {
+			return toLibraryElementError(err, "Failed to evaluate permissions")
+		}
+		if !allowed {
+			return response.Error(http.StatusForbidden, "You do not have the permissions needed to create a library element in this folder", nil)
+		}
+	}


We still evaluate access to the folder from here, right? I think that is probably enough, and this additional check is not needed.

IevaVasiljeva · 2023-09-14T16:39:03Z

pkg/services/libraryelements/database.go

 	return dto, err
 }

+func (l *LibraryElementService) updatePermissions(c context.Context, model *model.LibraryElementDTO, signedInUser identity.Requester) error {


This would be needed if we want to allow setting permissions on an individual library panel level. If we're fine with only granting permissions on a folder level (either you can or can't CRUD library elements in a given folder), then this would not be necessary. What do you think?

Not actually sure quite how fine-grained we need things to be. I'll try and get an answer for you.

Got some clarification and we do actually want permissions to be at the individual library panel level

IevaVasiljeva · 2023-09-14T16:46:58Z

pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go

+var LibraryElementEditActions = append(LibraryElementViewActions, []string{libraryelements.ActionLibraryPanelsWrite, libraryelements.ActionLibraryPanelsDelete}...)
+var LibraryElementAdminActions = LibraryElementEditActions
+
+func ProvideLibraryElementPermissions(


Thanks for linking the usage. The way we've used the permission service framework so far is for resources that have a frontend component that allows setting permissions on them through the UI (like this tab for dashboards). So I feel like it might be an overkill for library elements.

IevaVasiljeva

Great work!

I'd be happy to approve this PR, as long as we make the following changes in this or a follow-up PR:

add tests;
adding name scope resolver for that one endpoint that allows getting a library panel by name (or am I overlooking it?);
add a migration to add the new library panel permissions to folder Viewers, Editors and Admins (similar to this) - otherwise users who can currently use library panels won't be able to use them anymore;
frontend changes (for instance, to make sure that users who can't delete a library panel don't see the delete button etc) - not sure how many changes are necessary there.

Changes that I think we should make, but are ultimately up to your squad to make a call on:

as library panel actions can be scoped to folders, I think we can remove folder permission checks. So, for instance, if a user has library.panels:delete with scope folders:uid:my_folder, then they can delete library panels in my_folder without being able to edit the folder itself;
don't create a permission service and don't set permissions to library panel creator.

IevaVasiljeva · 2023-10-05T09:27:14Z

pkg/api/accesscontrol.go

+			Description: "Create library panel in general folder.",
+			Group:       "Library panels",
+			Permissions: []ac.Permission{
+				{Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(ac.GeneralFolderUID)},


Ah, I see. Fair point! Let's keep it this way then :)

IevaVasiljeva · 2023-10-05T13:12:11Z

pkg/services/libraryelements/accesscontrol.go

+		if err != nil {
+			return nil, err
+		}
+		return append(inheritedScopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(libElDTO.FolderUID), ScopeLibraryPanelsProvider.GetResourceScopeUID(uid)), nil


Sorry, I think I misread it earlier. So you can access library panel either if you have library.panels:write with either the scope library.panels:uid:<library_panel_uid> or scope folders:uid:<folder_uid>, where the folder is one of library panel's parents? That seems good!

IevaVasiljeva · 2023-10-05T13:39:24Z

pkg/services/libraryelements/api.go

+		uidScope := ScopeLibraryPanelsProvider.GetResourceScopeUID(ac.Parameter(":uid"))
+
+		if l.features.IsEnabled(featuremgmt.FlagLibraryPanelRBAC) {
+			entities.Post("/", authorize(ac.EvalPermission(ActionLibraryPanelsCreate)), routing.Wrap(l.createHandler))


Thought: we could scope ActionLibraryPanelsCreate to folders. I think currently we check whether a user has edit access to the folder in order to create a report in it. I think a more flexible permission setup would be scoping the library panel create action to a folder, so the user could create a library panel in a folder without needing to be able to edit the folder itself. What do you think?

Same goes for other folder permission checks - I think it would be better to only check if the library panel action has the right scope, instead of checking folder permissions.

IevaVasiljeva · 2023-10-05T13:43:30Z

pkg/services/libraryelements/database.go

+		})
+	}
+
+	if !inFolder {


Question: is it possible to create a library panel outside a folder?

They can be in the "General" folder, or an actual folder

Interesting, I think I missed it, as I don't see the panels created in the general folder through the UI under the General folder (which I know isn't really a real folder).

Do all the viewers get view access to library panels in the general folder, and editors and admins edit access? If so, these permissions will need to be assigned to the Viewer/Editor roles (very similar to what you've done here, but for viewing, writing and deleting).

I think that would make sense, yes. I'll make the change

IevaVasiljeva · 2023-10-05T14:02:02Z

pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go

+var LibraryElementEditActions = append(LibraryElementViewActions, []string{libraryelements.ActionLibraryPanelsWrite, libraryelements.ActionLibraryPanelsDelete}...)
+var LibraryElementAdminActions = LibraryElementEditActions
+
+func ProvideLibraryElementPermissions(


After reviewing the PR again, I'm still not sure it's worth adding a permission service for library panels. This is my thinking:

Pros:

permission service allows to set library panel permissions on a new resource to the resource creator.

Cons:

if we do not add a permission frontend for library panels (like we've done for dashboards and folders), there is no intuitive (non-API) way to grant or remove the "Viewer" or "Editor" permissions from users;

this adds a lot of complexity to the code (behind the scenes, registering a permission service creates new endpoints for library panel permissions etc).

I've looked through the code, and we have the same problem for other resources (like alerts and reports) where we do not automatically grant resource creator the rights to manipulate the created resource. While it's not ideal, we haven't gotten many (as far as I know) complaints about it.

So I wonder if it's fine to remove this permission service and the code that grants the permissions to the creator for now, until we come up with a smoother solution to it.

IevaVasiljeva · 2023-10-05T14:15:13Z

pkg/services/libraryelements/api.go

+			entities.Get("/", authorize(ac.EvalPermission(ActionLibraryPanelsRead)), routing.Wrap(l.getAllHandler))
+			entities.Get("/:uid", authorize(ac.EvalPermission(ActionLibraryPanelsRead, uidScope)), routing.Wrap(l.getHandler))
+			entities.Get("/:uid/connections/", authorize(ac.EvalPermission(ActionLibraryPanelsRead, uidScope)), routing.Wrap(l.getConnectionsHandler))
+			entities.Get("/name/:name", authorize(ac.EvalPermission(ActionLibraryPanelsRead)), routing.Wrap(l.getByNameHandler))


One more thing - it seems like we're currently checking only the general permission to list any library folder here (as we're omitting the scope). Do we need to add a name scope resolver for this? I see that you've added ErrElementNameNotUnique, so you probably thought of this already :)

Ah right, the permission logic is dealt with in the handler: https://github.com/grafana/grafana/pull/73475/files/43c3043360c4fcac36552fc063a292b935544fc4#diff-fce3aaeec8217549f04cfaebd15b68b1526b692cd62a63258b5bbbcb9ebbb736R292

I think that means I can get rid of the authorize(ac.EvalPermission(ActionLibraryPanelsRead)) part

…Viewer/Editor roles

kaydelaney · 2023-10-06T16:18:14Z

@IevaVasiljeva

Changes that I think we should make, but are ultimately up to your squad to make a call on:

* as library panel actions can be scoped to folders, I think we can remove folder permission checks. So, for instance, if a user has _library.panels:delete_ with scope _folders:uid:my_folder_, then they can delete library panels in _my_folder_ without being able to edit the folder itself;

* don't create a permission service and don't set permissions to library panel creator.

Hmm, so when we are creating a library panel, we check if the user has the ActionLibraryPanelsCreate permission scoped to the folder they want to create the panel in? Would that not require entries in the permission table? How do we do that without the permission service? I feel like I'm misunderstanding something 😅

IevaVasiljeva · 2023-10-09T14:54:15Z

@IevaVasiljeva
Changes that I think we should make, but are ultimately up to your squad to make a call on:
* as library panel actions can be scoped to folders, I think we can remove folder permission checks. So, for instance, if a user has _library.panels:delete_ with scope _folders:uid:my_folder_, then they can delete library panels in _my_folder_ without being able to edit the folder itself;

* don't create a permission service and don't set permissions to library panel creator.
Hmm, so when we are creating a library panel, we check if the user has the ActionLibraryPanelsCreate permission scoped to the folder they want to create the panel in? Would that not require entries in the permission table? How do we do that without the permission service? I feel like I'm misunderstanding something 😅

We could do what we're doing with alerting, and add library panel permissions to the folder permission service. So when someone is granted the permission to view a folder, they'd also get a permission to view the library panels in it. And when someone can edit the folder, they could also edit/create/delete the library panels. The downside is that it would be hard to give someone permissions to view the folder without allowing them to see the library panels, but we're actually working on a project that would mitigate this issue (I'll send you the product DNA in Slack).

But I really don't want to block this work on that. So I'll go ahead and approve this PR, and we can decide later if we want to include library panel permissions in the folder permissions service or not.

IevaVasiljeva

Great work!

Approving this PR, as I imagine it might be easier to tackle any follow-up work in separate PRs, as this one has gotten pretty huge already :)

kaydelaney · 2023-10-09T17:55:06Z

Thanks! I needed to merge anyway, so I added that change to the folder permissions service while I was at it.
Will follow up with some tests in another PR, though I might need some help with that. Tried writing some earlier without much luck

LibraryPanels: Add RBAC support

d3c2810

kaydelaney added area/backend area/frontend add to changelog area/frontend/library-panels no-backport Skip backport of PR area/auth/rbac Grafana role-based access control labels Aug 18, 2023

kaydelaney added this to the 10.2.x milestone Aug 18, 2023

kaydelaney requested a review from a team August 18, 2023 10:45

kaydelaney self-assigned this Aug 18, 2023

kaydelaney requested review from axelavargas and evictorero and removed request for a team August 18, 2023 10:45

IevaVasiljeva reviewed Aug 23, 2023

View reviewed changes

gamab reviewed Aug 23, 2023

View reviewed changes

pkg/services/libraryelements/api.go Outdated Show resolved Hide resolved

pkg/services/libraryelements/api.go Outdated Show resolved Hide resolved

pkg/services/libraryelements/api.go Outdated Show resolved Hide resolved

mildwonkey reviewed Aug 23, 2023

View reviewed changes

alexanderzobnin previously approved these changes Aug 23, 2023

View reviewed changes

alexanderzobnin reviewed Aug 23, 2023

View reviewed changes

alexanderzobnin self-requested a review August 23, 2023 16:16

Change resource name from "library-panels" to "library.panels"

c1bb8ea

Clean up and finish implementation

ae21ea6

Merge remote-tracking branch 'origin/main' into lib-rbac

7dc7ec0

kaydelaney requested review from IevaVasiljeva, mildwonkey and gamab September 7, 2023 12:26

kaydelaney marked this pull request as ready for review September 11, 2023 14:19

kaydelaney requested a review from a team as a code owner September 11, 2023 14:19

Remove unneeded name scope resolver

539e93c

kaydelaney changed the title ~~LibraryPanels: Add RBAC support (WIP)~~ LibraryPanels: Add RBAC support Sep 13, 2023

IevaVasiljeva reviewed Sep 14, 2023

View reviewed changes

kaydelaney added 2 commits September 25, 2023 17:41

Merge remote-tracking branch 'origin/main' into lib-rbac

43c3043

Remove redundant code

e13b12d

kaydelaney requested a review from IevaVasiljeva October 2, 2023 16:50

IevaVasiljeva reviewed Oct 5, 2023

View reviewed changes

kaydelaney added 2 commits October 6, 2023 15:42

Remove permissions service

74881e4

Assign roles for reading/writing library panels in general folder to …

3374a70

…Viewer/Editor roles

IevaVasiljeva approved these changes Oct 9, 2023

View reviewed changes

kaydelaney added 3 commits October 9, 2023 18:28

Remove unnecessary permission eval on /name/:name api

0ebe52c

Add appropriate library panel permissions to folder permissions service

b34351c

Merge remote-tracking branch 'origin/main' into lib-rbac

1e491cc

kaydelaney added 2 commits October 10, 2023 22:25

Add library panel actions folder migration

08b6b75

Fix failing tests

0fd32d3

kaydelaney requested a review from a team as a code owner October 10, 2023 22:27

grafana-pr-automation bot added the area/backend/db/migration label Oct 10, 2023

Fix linting issue

40d3a68

kaydelaney merged commit a12cb8c into main Oct 11, 2023
20 checks passed

kaydelaney deleted the lib-rbac branch October 11, 2023 23:30

natellium mentioned this pull request Oct 12, 2023

Add RBAC note to library panels #76480

Merged

zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023

dnhn mentioned this pull request Oct 24, 2023

grafana 10.2.0 Homebrew/homebrew-core#152264

Closed

BrewTestBot mentioned this pull request Oct 25, 2023

grafana 10.2.0 Homebrew/homebrew-core#152321

Closed

	result = append([]string{
	ScopeDashboardsProvider.GetResourceScopeUID(dashboard.UID),
	ScopeFoldersProvider.GetResourceScopeUID(folderUID),
	},
	result...,
	)

LibraryPanels: Add RBAC support #73475

LibraryPanels: Add RBAC support #73475

Conversation

kaydelaney commented Aug 18, 2023 • edited Loading

Things left to do:

Testing scenarios:

IevaVasiljeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gamab left a comment

Choose a reason for hiding this comment

mildwonkey left a comment

Choose a reason for hiding this comment

alexanderzobnin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IevaVasiljeva Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderzobnin left a comment

Choose a reason for hiding this comment

kaydelaney commented Aug 31, 2023

kaydelaney commented Sep 6, 2023

IevaVasiljeva commented Sep 13, 2023

IevaVasiljeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IevaVasiljeva left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IevaVasiljeva Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaydelaney commented Oct 6, 2023 • edited Loading

IevaVasiljeva commented Oct 9, 2023

IevaVasiljeva left a comment

Choose a reason for hiding this comment

kaydelaney commented Oct 9, 2023

kaydelaney commented Aug 18, 2023 •

edited

Loading

IevaVasiljeva Aug 23, 2023 •

edited

Loading

IevaVasiljeva left a comment •

edited

Loading

IevaVasiljeva Oct 6, 2023 •

edited

Loading

kaydelaney commented Oct 6, 2023 •

edited

Loading