From e81fa0e4c527ee75451bc5f5ea9f4544c88ee084 Mon Sep 17 00:00:00 2001 From: Haris Rozajac <58232930+harisrozajac@users.noreply.github.com> Date: Wed, 31 Jul 2024 12:10:52 -0600 Subject: [PATCH] Explore: Check for RBAC permissions when hitting query history endpoints (#91156) * Check for RBAC permissions when hitting query history endpoints; extract checking logic into a middleware * Fix lint errors * Fix test * Use permissions for patch path; rename callback handler --- pkg/services/queryhistory/api.go | 43 ++++++++----------- pkg/services/queryhistory/queryhistory.go | 5 ++- .../queryhistory/queryhistory_create_test.go | 22 +++++++++- .../queryhistory/queryhistory_search_test.go | 7 +-- .../queryhistory/queryhistory_test.go | 38 ++++++++++------ 5 files changed, 71 insertions(+), 44 deletions(-) diff --git a/pkg/services/queryhistory/api.go b/pkg/services/queryhistory/api.go index d9115479da9..ef8ca98ad4d 100644 --- a/pkg/services/queryhistory/api.go +++ b/pkg/services/queryhistory/api.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/middleware" + ac "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/util" @@ -15,15 +16,27 @@ import ( func (s *QueryHistoryService) registerAPIEndpoints() { s.RouteRegister.Group("/api/query-history", func(entities routing.RouteRegister) { - entities.Post("/", middleware.ReqSignedIn, routing.Wrap(s.createHandler)) - entities.Get("/", middleware.ReqSignedIn, routing.Wrap(s.searchHandler)) - entities.Delete("/:uid", middleware.ReqSignedIn, routing.Wrap(s.deleteHandler)) - entities.Post("/star/:uid", middleware.ReqSignedIn, routing.Wrap(s.starHandler)) - entities.Delete("/star/:uid", middleware.ReqSignedIn, routing.Wrap(s.unstarHandler)) - entities.Patch("/:uid", middleware.ReqSignedIn, routing.Wrap(s.patchCommentHandler)) + entities.Post("/", middleware.ReqSignedIn, routing.Wrap(s.permissionsMiddleware(s.createHandler, "Failed to create query history"))) + entities.Get("/", middleware.ReqSignedIn, routing.Wrap(s.permissionsMiddleware(s.searchHandler, "Failed to get query history"))) + entities.Delete("/:uid", middleware.ReqSignedIn, routing.Wrap(s.permissionsMiddleware(s.deleteHandler, "Failed to delete query history"))) + entities.Post("/star/:uid", middleware.ReqSignedIn, routing.Wrap(s.permissionsMiddleware(s.starHandler, "Failed to star query history"))) + entities.Delete("/star/:uid", middleware.ReqSignedIn, routing.Wrap(s.permissionsMiddleware(s.unstarHandler, "Failed to unstar query history"))) + entities.Patch("/:uid", middleware.ReqSignedIn, routing.Wrap(s.permissionsMiddleware(s.patchCommentHandler, "Failed to update comment of query in query history"))) }) } +type CallbackHandler func(c *contextmodel.ReqContext) response.Response + +func (s *QueryHistoryService) permissionsMiddleware(handler CallbackHandler, errorMessage string) CallbackHandler { + return func(c *contextmodel.ReqContext) response.Response { + hasAccess := ac.HasAccess(s.accessControl, c) + if c.GetOrgRole() == org.RoleViewer && !s.Cfg.ViewersCanEdit && !hasAccess(ac.EvalPermission(ac.ActionDatasourcesExplore)) { + return response.Error(http.StatusUnauthorized, errorMessage, nil) + } + return handler(c) + } +} + // swagger:route POST /query-history query_history createQuery // // Add query to query history. @@ -36,10 +49,6 @@ func (s *QueryHistoryService) registerAPIEndpoints() { // 401: unauthorisedError // 500: internalServerError func (s *QueryHistoryService) createHandler(c *contextmodel.ReqContext) response.Response { - if c.GetOrgRole() == org.RoleViewer && !s.Cfg.ViewersCanEdit { - return response.Error(http.StatusUnauthorized, "Failed to create query history", nil) - } - cmd := CreateQueryInQueryHistoryCommand{} if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) @@ -66,10 +75,6 @@ func (s *QueryHistoryService) createHandler(c *contextmodel.ReqContext) response // 401: unauthorisedError // 500: internalServerError func (s *QueryHistoryService) searchHandler(c *contextmodel.ReqContext) response.Response { - if c.GetOrgRole() == org.RoleViewer && !s.Cfg.ViewersCanEdit { - return response.Error(http.StatusUnauthorized, "Failed to get query history", nil) - } - timeRange := gtime.NewTimeRange(c.Query("from"), c.Query("to")) query := SearchInQueryHistoryQuery{ @@ -102,10 +107,6 @@ func (s *QueryHistoryService) searchHandler(c *contextmodel.ReqContext) response // 401: unauthorisedError // 500: internalServerError func (s *QueryHistoryService) deleteHandler(c *contextmodel.ReqContext) response.Response { - if c.GetOrgRole() == org.RoleViewer && !s.Cfg.ViewersCanEdit { - return response.Error(http.StatusUnauthorized, "Failed to delete query history", nil) - } - queryUID := web.Params(c.Req)[":uid"] if len(queryUID) > 0 && !util.IsValidShortUID(queryUID) { return response.Error(http.StatusNotFound, "Query in query history not found", nil) @@ -163,9 +164,6 @@ func (s *QueryHistoryService) patchCommentHandler(c *contextmodel.ReqContext) re // 401: unauthorisedError // 500: internalServerError func (s *QueryHistoryService) starHandler(c *contextmodel.ReqContext) response.Response { - if c.GetOrgRole() == org.RoleViewer && !s.Cfg.ViewersCanEdit { - return response.Error(http.StatusUnauthorized, "Failed to star query history", nil) - } queryUID := web.Params(c.Req)[":uid"] if len(queryUID) > 0 && !util.IsValidShortUID(queryUID) { return response.Error(http.StatusNotFound, "Query in query history not found", nil) @@ -190,9 +188,6 @@ func (s *QueryHistoryService) starHandler(c *contextmodel.ReqContext) response.R // 401: unauthorisedError // 500: internalServerError func (s *QueryHistoryService) unstarHandler(c *contextmodel.ReqContext) response.Response { - if c.GetOrgRole() == org.RoleViewer && !s.Cfg.ViewersCanEdit { - return response.Error(http.StatusUnauthorized, "Failed to unstar query history", nil) - } queryUID := web.Params(c.Req)[":uid"] if len(queryUID) > 0 && !util.IsValidShortUID(queryUID) { return response.Error(http.StatusNotFound, "Query in query history not found", nil) diff --git a/pkg/services/queryhistory/queryhistory.go b/pkg/services/queryhistory/queryhistory.go index deb7d8d640e..d2f1e90e8bc 100644 --- a/pkg/services/queryhistory/queryhistory.go +++ b/pkg/services/queryhistory/queryhistory.go @@ -7,17 +7,19 @@ import ( "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" + ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) -func ProvideService(cfg *setting.Cfg, sqlStore db.DB, routeRegister routing.RouteRegister) *QueryHistoryService { +func ProvideService(cfg *setting.Cfg, sqlStore db.DB, routeRegister routing.RouteRegister, accessControl ac.AccessControl) *QueryHistoryService { s := &QueryHistoryService{ store: sqlStore, Cfg: cfg, RouteRegister: routeRegister, log: log.New("query-history"), now: time.Now, + accessControl: accessControl, } // Register routes only when query history is enabled @@ -45,6 +47,7 @@ type QueryHistoryService struct { RouteRegister routing.RouteRegister log log.Logger now func() time.Time + accessControl ac.AccessControl } func (s QueryHistoryService) CreateQueryInQueryHistory(ctx context.Context, user *user.SignedInUser, cmd CreateQueryInQueryHistoryCommand) (QueryHistoryDTO, error) { diff --git a/pkg/services/queryhistory/queryhistory_create_test.go b/pkg/services/queryhistory/queryhistory_create_test.go index c9fb59d7c77..2d06d1d044f 100644 --- a/pkg/services/queryhistory/queryhistory_create_test.go +++ b/pkg/services/queryhistory/queryhistory_create_test.go @@ -12,7 +12,7 @@ func TestIntegrationCreateQueryInQueryHistory(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - testScenario(t, "When users tries to create query in query history it should succeed", false, + testScenario(t, "When users tries to create query in query history it should succeed", true, true, func(t *testing.T, sc scenarioContext) { command := CreateQueryInQueryHistoryCommand{ DatasourceUID: "NCzh67i", @@ -21,7 +21,25 @@ func TestIntegrationCreateQueryInQueryHistory(t *testing.T) { }), } sc.reqContext.Req.Body = mockRequestBody(command) - resp := sc.service.createHandler(sc.reqContext) + permissionsMiddlewareCallback := sc.service.permissionsMiddleware(sc.service.createHandler, "Failed to create query history") + + resp := permissionsMiddlewareCallback(sc.reqContext) require.Equal(t, 200, resp.Status()) }) + + testScenario(t, "When users tries to create query in query history without permissions it should fail", true, false, + func(t *testing.T, sc scenarioContext) { + command := CreateQueryInQueryHistoryCommand{ + DatasourceUID: "NCzh67i", + Queries: simplejson.NewFromAny(map[string]any{ + "expr": "test", + }), + } + + sc.reqContext.Req.Body = mockRequestBody(command) + permissionsMiddlewareCallback := sc.service.permissionsMiddleware(sc.service.createHandler, "Failed to create query history") + + resp := permissionsMiddlewareCallback(sc.reqContext) + require.Equal(t, 401, resp.Status()) + }) } diff --git a/pkg/services/queryhistory/queryhistory_search_test.go b/pkg/services/queryhistory/queryhistory_search_test.go index 18e1bcc9e7f..bd8aa3559dc 100644 --- a/pkg/services/queryhistory/queryhistory_search_test.go +++ b/pkg/services/queryhistory/queryhistory_search_test.go @@ -12,7 +12,7 @@ func TestIntegrationGetQueriesFromQueryHistory(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - testScenario(t, "When users tries to get query in empty query history, it should return empty result", false, + testScenario(t, "When users tries to get query in empty query history, it should return empty result", false, false, func(t *testing.T, sc scenarioContext) { sc.reqContext.Req.Form.Add("datasourceUid", "test") resp := sc.service.searchHandler(sc.reqContext) @@ -262,10 +262,11 @@ func TestIntegrationGetQueriesFromQueryHistory(t *testing.T) { require.Equal(t, 0, response.Result.TotalCount) }) - testScenario(t, "When user is viewer, return 401", true, + testScenario(t, "When user is viewer and has no RBAC permissions, return 401", true, false, func(t *testing.T, sc scenarioContext) { sc.reqContext.Req.Form.Add("datasourceUid", "test") - resp := sc.service.searchHandler(sc.reqContext) + permissionsMiddlewareCallback := sc.service.permissionsMiddleware(sc.service.searchHandler, "Failed to get query history") + resp := permissionsMiddlewareCallback(sc.reqContext) require.Equal(t, 401, resp.Status()) }) diff --git a/pkg/services/queryhistory/queryhistory_test.go b/pkg/services/queryhistory/queryhistory_test.go index e546524ac72..3b79f478780 100644 --- a/pkg/services/queryhistory/queryhistory_test.go +++ b/pkg/services/queryhistory/queryhistory_test.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/tracing" + accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/org/orgimpl" @@ -48,7 +49,7 @@ type scenarioContext struct { initialResult QueryHistoryResponse } -func testScenario(t *testing.T, desc string, isViewer bool, fn func(t *testing.T, sc scenarioContext)) { +func testScenario(t *testing.T, desc string, isViewer bool, hasDatasourceExplorePermission bool, fn func(t *testing.T, sc scenarioContext)) { t.Helper() t.Run(desc, func(t *testing.T) { @@ -59,9 +60,10 @@ func testScenario(t *testing.T, desc string, isViewer bool, fn func(t *testing.T ctx.Req.Header.Add("Content-Type", "application/json") sqlStore, cfg := db.InitTestDBWithCfg(t) service := QueryHistoryService{ - Cfg: setting.NewCfg(), - store: sqlStore, - now: time.Now, + Cfg: setting.NewCfg(), + store: sqlStore, + now: time.Now, + accessControl: accesscontrolmock.New(), } service.Cfg.QueryHistoryEnabled = true quotaService := quotatest.New(false, nil) @@ -80,14 +82,22 @@ func testScenario(t *testing.T, desc string, isViewer bool, fn func(t *testing.T role = org.RoleEditor } + permissions := make(map[int64]map[string][]string) + + if hasDatasourceExplorePermission { + permissions[testUserID] = make(map[string][]string) + permissions[testUserID]["datasources:explore"] = []string{} + } + usr := user.SignedInUser{ - UserID: testUserID, - Name: "Signed In User", - Login: "signed_in_user", - Email: "signed.in.user@test.com", - OrgID: testOrgID, - OrgRole: role, - LastSeenAt: service.now(), + UserID: testUserID, + Name: "Signed In User", + Login: "signed_in_user", + Email: "signed.in.user@test.com", + OrgID: testOrgID, + OrgRole: role, + LastSeenAt: service.now(), + Permissions: permissions, } _, err = usrSvc.Create(context.Background(), &user.CreateUserCommand{ @@ -113,7 +123,7 @@ func testScenario(t *testing.T, desc string, isViewer bool, fn func(t *testing.T func testScenarioWithQueryInQueryHistory(t *testing.T, desc string, fn func(t *testing.T, sc scenarioContext)) { t.Helper() - testScenario(t, desc, false, func(t *testing.T, sc scenarioContext) { + testScenario(t, desc, false, false, func(t *testing.T, sc scenarioContext) { command := CreateQueryInQueryHistoryCommand{ DatasourceUID: testDsUID1, Queries: simplejson.NewFromAny([]interface{}{ @@ -132,7 +142,7 @@ func testScenarioWithQueryInQueryHistory(t *testing.T, desc string, fn func(t *t func testScenarioWithMultipleQueriesInQueryHistory(t *testing.T, desc string, fn func(t *testing.T, sc scenarioContext)) { t.Helper() - testScenario(t, desc, false, func(t *testing.T, sc scenarioContext) { + testScenario(t, desc, false, false, func(t *testing.T, sc scenarioContext) { start := time.Now().Add(-3 * time.Second) sc.service.now = func() time.Time { return start } command1 := CreateQueryInQueryHistoryCommand{ @@ -193,7 +203,7 @@ func testScenarioWithMultipleQueriesInQueryHistory(t *testing.T, desc string, fn func testScenarioWithMixedQueriesInQueryHistory(t *testing.T, desc string, fn func(t *testing.T, sc scenarioContext)) { t.Helper() - testScenario(t, desc, false, func(t *testing.T, sc scenarioContext) { + testScenario(t, desc, false, false, func(t *testing.T, sc scenarioContext) { start := time.Now() sc.service.now = func() time.Time { return start } command1 := CreateQueryInQueryHistoryCommand{