From 0942e0a815f02fd7778d51b8efa2ce3011006c12 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Mon, 23 Jan 2023 10:54:29 +0100 Subject: [PATCH] RBAC: Rewrite data source api tests (#61783) RBAC: Rewrite datasource rbac api tests --- pkg/api/common_test.go | 28 --- pkg/api/dashboard_test.go | 3 +- pkg/api/datasources_test.go | 420 +++++++----------------------------- 3 files changed, 80 insertions(+), 371 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 2f5ace440ce..c4dfeda1659 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -26,7 +26,6 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" - "github.com/grafana/grafana/pkg/services/accesscontrol/actest" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol" "github.com/grafana/grafana/pkg/services/annotations/annotationstest" @@ -42,7 +41,6 @@ import ( dashver "github.com/grafana/grafana/pkg/services/dashboardversion" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/guardian" - "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/licensing" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/login/loginservice" @@ -56,7 +54,6 @@ import ( "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/searchusers/filters" "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/services/stats/statsimpl" "github.com/grafana/grafana/pkg/services/tag/tagimpl" "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/services/team/teamimpl" @@ -252,31 +249,6 @@ func (s *fakeRenderService) Init() error { return nil } -func setupAccessControlScenarioContext(t *testing.T, cfg *setting.Cfg, url string, permissions []accesscontrol.Permission) (*scenarioContext, *HTTPServer) { - store := sqlstore.InitTestDB(t) - statsService := statsimpl.ProvideService(store) - hs := &HTTPServer{ - Cfg: cfg, - Live: newTestLive(t, store), - License: &licensing.OSSLicensingService{}, - Features: featuremgmt.WithFeatures(), - QuotaService: quotatest.New(false, nil), - RouteRegister: routing.NewRouteRegister(), - AccessControl: accesscontrolmock.New().WithPermissions(permissions), - searchUsersService: searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), usertest.NewUserServiceFake()), - ldapGroups: ldap.ProvideGroupsService(), - accesscontrolService: actest.FakeService{}, - statsService: statsService, - } - - sc := setupScenarioContext(t, url) - - hs.registerRoutes() - hs.RouteRegister.Register(sc.m.Router) - - return sc, hs -} - type accessControlTestCase struct { expectedCode int desc string diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index d988fd5659f..5daa4816767 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -23,6 +23,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/registry/corekind" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/alerting" @@ -115,7 +116,7 @@ func newTestLive(t *testing.T, store db.DB) *live.GrafanaLive { nil, &usagestats.UsageStatsMock{T: t}, nil, - features, accesscontrolmock.New(), &dashboards.FakeDashboardService{}, annotationstest.NewFakeAnnotationsRepo(), nil) + features, acimpl.ProvideAccessControl(cfg), &dashboards.FakeDashboardService{}, annotationstest.NewFakeAnnotationsRepo(), nil) require.NoError(t, err) return gLive } diff --git a/pkg/api/datasources_test.go b/pkg/api/datasources_test.go index 08cced21be0..f9b313c6b5a 100644 --- a/pkg/api/datasources_test.go +++ b/pkg/api/datasources_test.go @@ -1,15 +1,14 @@ package api import ( - "bytes" "context" "encoding/json" - "fmt" "io" "net/http" - "net/http/httptest" + "strings" "testing" + "github.com/grafana/grafana/pkg/web/webtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -24,7 +23,6 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol/actest" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/datasources/permissions" - "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/setting" ) @@ -250,369 +248,107 @@ func TestUpdateDataSource_URLWithoutProtocol(t *testing.T) { assert.Equal(t, 200, sc.resp.Code) } -func TestAPI_Datasources_AccessControl(t *testing.T) { - testDatasource := datasources.DataSource{ - Id: 3, - Uid: "testUID", - OrgId: testOrgID, - Name: "test", - Url: "http://localhost:5432", - Type: "postgresql", - Access: "Proxy", - } - testDatasourceReadOnly := datasources.DataSource{ - Id: 4, - Uid: "testUID", - OrgId: testOrgID, - Name: "test", - Url: "http://localhost:5432", - Type: "postgresql", - Access: "Proxy", - ReadOnly: true, +func TestAPI_datasources_AccessControl(t *testing.T) { + type testCase struct { + desc string + urls []string + method string + body string + permission []ac.Permission + expectedCode int } - addDatasourceBody := func() io.Reader { - s, _ := json.Marshal(datasources.AddDataSourceCommand{ - Name: "test", - Url: "http://localhost:5432", - Type: "postgresql", - Access: "Proxy", - }) - return bytes.NewReader(s) - } - - dsServiceMock := &dataSourcesServiceMock{ - expectedDatasource: &testDatasource, - } - dsPermissionService := permissions.NewMockDatasourcePermissionService() - dsPermissionService.DsResult = []*datasources.DataSource{ - &testDatasource, - } - - updateDatasourceBody := func() io.Reader { - s, _ := json.Marshal(datasources.UpdateDataSourceCommand{ - Name: "test", - Url: "http://localhost:5432", - Type: "postgresql", - Access: "Proxy", - }) - return bytes.NewReader(s) - } - type acTestCaseWithHandler struct { - body func() io.Reader - accessControlTestCase - expectedDS *datasources.DataSource - expectedSQLError error - } - tests := []acTestCaseWithHandler{ + tests := []testCase{ { - body: updateDatasourceBody, - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusNotFound, - desc: "DatasourcesPut should return 404 if datasource not found", - url: fmt.Sprintf("/api/datasources/%v", "12345678"), - method: http.MethodPut, - permissions: []ac.Permission{ - { - Action: datasources.ActionWrite, - Scope: datasources.ScopeAll, - }, - }, + desc: "should be able to update datasource with correct permission", + urls: []string{"api/datasources/1", "/api/datasources/uid/1"}, + method: http.MethodPut, + body: `{"name": "test", "url": "http://localhost:5432", "type": "postgresql", "access": "Proxy"}`, + permission: []ac.Permission{ + {Action: datasources.ActionWrite, Scope: datasources.ScopeProvider.GetResourceScope("1")}, + {Action: datasources.ActionWrite, Scope: datasources.ScopeProvider.GetResourceScopeUID("1")}, }, - expectedSQLError: datasources.ErrDataSourceNotFound, + expectedCode: http.StatusOK, }, { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesGet should return 200 for user with correct permissions", - url: "/api/datasources/", - method: http.MethodGet, - permissions: []ac.Permission{{Action: datasources.ActionRead, Scope: datasources.ScopeAll}}, - }, + desc: "should not be able to update datasource without correct permission", + urls: []string{"api/datasources/1", "/api/datasources/uid/1"}, + method: http.MethodPut, + permission: []ac.Permission{}, + expectedCode: http.StatusForbidden, }, { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesGet should return 403 for user without required permissions", - url: "/api/datasources/", - method: http.MethodGet, - permissions: []ac.Permission{{Action: "wrong"}}, + desc: "should be able to fetch datasource with correct permission", + urls: []string{"api/datasources/1", "/api/datasources/uid/1", "/api/datasources/name/test"}, + method: http.MethodGet, + permission: []ac.Permission{ + {Action: datasources.ActionRead, Scope: datasources.ScopeProvider.GetResourceScope("1")}, + {Action: datasources.ActionRead, Scope: datasources.ScopeProvider.GetResourceScopeUID("1")}, + {Action: datasources.ActionRead, Scope: datasources.ScopeProvider.GetResourceScopeName("test")}, }, + expectedCode: http.StatusOK, }, { - body: addDatasourceBody, - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesPost should return 200 for user with correct permissions", - url: "/api/datasources/", - method: http.MethodPost, - permissions: []ac.Permission{{Action: datasources.ActionCreate}}, - }, - expectedDS: &testDatasource, + desc: "should not be able to fetch datasource without correct permission", + urls: []string{"api/datasources/1", "/api/datasources/uid/1"}, + method: http.MethodGet, + permission: []ac.Permission{}, + expectedCode: http.StatusForbidden, }, { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesPost should return 403 for user without required permissions", - url: "/api/datasources/", - method: http.MethodPost, - permissions: []ac.Permission{{Action: "wrong"}}, - }, + desc: "should be able to create datasource with correct permission", + urls: []string{"/api/datasources"}, + method: http.MethodPost, + body: `{"name": "test", "url": "http://localhost:5432", "type": "postgresql", "access": "Proxy"}`, + permission: []ac.Permission{{Action: datasources.ActionCreate}}, + expectedCode: http.StatusOK, }, { - body: updateDatasourceBody, - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesPut should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/%v", testDatasource.Id), - method: http.MethodPut, - permissions: []ac.Permission{ - { - Action: datasources.ActionWrite, - Scope: fmt.Sprintf("datasources:id:%v", testDatasource.Id), - }, - }, - }, - expectedDS: &testDatasource, + desc: "should not be able to create datasource without correct permission", + urls: []string{"/api/datasources"}, + method: http.MethodPost, + permission: []ac.Permission{}, + expectedCode: http.StatusForbidden, }, { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesPut should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/%v", testDatasource.Id), - method: http.MethodPut, - permissions: []ac.Permission{{Action: "wrong"}}, + desc: "should be able to delete datasource with correct permission", + urls: []string{"/api/datasources/1", "/api/datasources/uid/1"}, + method: http.MethodDelete, + permission: []ac.Permission{ + {Action: datasources.ActionDelete, Scope: datasources.ScopeProvider.GetResourceScope("1")}, + {Action: datasources.ActionDelete, Scope: datasources.ScopeProvider.GetResourceScopeUID("1")}, }, + expectedCode: http.StatusOK, }, { - body: updateDatasourceBody, - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesPut should return 403 for read only datasource", - url: fmt.Sprintf("/api/datasources/%v", testDatasourceReadOnly.Id), - method: http.MethodPut, - permissions: []ac.Permission{ - { - Action: datasources.ActionWrite, - Scope: fmt.Sprintf("datasources:id:%v", testDatasourceReadOnly.Id), - }, - }, - }, - expectedDS: &testDatasourceReadOnly, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesDeleteByID should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/%v", testDatasource.Id), - method: http.MethodDelete, - permissions: []ac.Permission{ - { - Action: datasources.ActionDelete, - Scope: fmt.Sprintf("datasources:id:%v", testDatasource.Id), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesDeleteByID should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/%v", testDatasource.Id), - method: http.MethodDelete, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesDeleteByUID should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/uid/%v", testDatasource.Uid), - method: http.MethodDelete, - permissions: []ac.Permission{ - { - Action: datasources.ActionDelete, - Scope: fmt.Sprintf("datasources:uid:%v", testDatasource.Uid), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesDeleteByUID should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/uid/%v", testDatasource.Uid), - method: http.MethodDelete, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesDeleteByName should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/name/%v", testDatasource.Name), - method: http.MethodDelete, - permissions: []ac.Permission{ - { - Action: datasources.ActionDelete, - Scope: fmt.Sprintf("datasources:name:%v", testDatasource.Name), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesDeleteByName should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/name/%v", testDatasource.Name), - method: http.MethodDelete, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesGetByID should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/%v", testDatasource.Id), - method: http.MethodGet, - permissions: []ac.Permission{ - { - Action: datasources.ActionRead, - Scope: fmt.Sprintf("datasources:id:%v", testDatasource.Id), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesGetByID should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/%v", testDatasource.Id), - method: http.MethodGet, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesGetByUID should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/uid/%v", testDatasource.Uid), - method: http.MethodGet, - permissions: []ac.Permission{ - { - Action: datasources.ActionRead, - Scope: fmt.Sprintf("datasources:uid:%v", testDatasource.Uid), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesGetByUID should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/uid/%v", testDatasource.Uid), - method: http.MethodGet, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesGetByName should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/name/%v", testDatasource.Name), - method: http.MethodGet, - permissions: []ac.Permission{ - { - Action: datasources.ActionRead, - Scope: fmt.Sprintf("datasources:name:%v", testDatasource.Name), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesGetByName should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/name/%v", testDatasource.Name), - method: http.MethodGet, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusOK, - desc: "DatasourcesGetIdByName should return 200 for user with correct permissions", - url: fmt.Sprintf("/api/datasources/id/%v", testDatasource.Name), - method: http.MethodGet, - permissions: []ac.Permission{ - { - Action: datasources.ActionIDRead, - Scope: fmt.Sprintf("datasources:name:%v", testDatasource.Name), - }, - }, - }, - expectedDS: &testDatasource, - }, - { - accessControlTestCase: accessControlTestCase{ - expectedCode: http.StatusForbidden, - desc: "DatasourcesGetIdByName should return 403 for user without required permissions", - url: fmt.Sprintf("/api/datasources/id/%v", testDatasource.Name), - method: http.MethodGet, - permissions: []ac.Permission{{Action: "wrong"}}, - }, - expectedDS: &testDatasource, + desc: "should not be able to delete datasource without correct permission", + urls: []string{"/api/datasources/1", "/api/datasources/uid/1"}, + method: http.MethodDelete, + permission: []ac.Permission{}, + expectedCode: http.StatusForbidden, }, } - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { - cfg := setting.NewCfg() - sc, hs := setupAccessControlScenarioContext(t, cfg, test.url, test.permissions) + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + server := SetupAPITestServer(t, func(hs *HTTPServer) { + hs.Cfg = setting.NewCfg() + hs.DataSourcesService = &dataSourcesServiceMock{expectedDatasource: &datasources.DataSource{}} + hs.accesscontrolService = actest.FakeService{} + hs.Live = newTestLive(t, hs.SQLStore) + }) - // mock sqlStore and datasource permission service - dsServiceMock.expectedError = test.expectedSQLError - dsServiceMock.expectedDatasource = test.expectedDS - dsPermissionService.DsResult = []*datasources.DataSource{test.expectedDS} - if test.expectedDS == nil { - dsPermissionService.DsResult = nil + for _, url := range tt.urls { + var body io.Reader + if tt.body != "" { + body = strings.NewReader(tt.body) + } + + res, err := server.SendJSON(webtest.RequestWithSignedInUser(server.NewRequest(tt.method, url, body), userWithPermissions(1, tt.permission))) + require.NoError(t, err) + assert.Equal(t, tt.expectedCode, res.StatusCode) + require.NoError(t, res.Body.Close()) } - hs.DataSourcesService = dsServiceMock - hs.DatasourcePermissionsService = dsPermissionService - - // Create a middleware to pretend user is logged in - pretendSignInMiddleware := func(c *models.ReqContext) { - sc.context = c - sc.context.UserID = testUserID - sc.context.OrgID = testOrgID - sc.context.Login = testUserLogin - sc.context.OrgRole = org.RoleViewer - sc.context.IsSignedIn = true - } - sc.m.Use(pretendSignInMiddleware) - - sc.resp = httptest.NewRecorder() - hs.SettingsProvider = &setting.OSSImpl{Cfg: cfg} - - var err error - if test.body != nil { - sc.req, err = http.NewRequest(test.method, test.url, test.body()) - sc.req.Header.Add("Content-Type", "application/json") - } else { - sc.req, err = http.NewRequest(test.method, test.url, nil) - } - - assert.NoError(t, err) - - sc.exec() - assert.Equal(t, test.expectedCode, sc.resp.Code) }) } }