From 4be9ec8f722149a48d7da367b1b3ccc2d2ed2c69 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Fri, 17 Sep 2021 09:19:36 +0200 Subject: [PATCH] AccessControl: Protect org users lookup (#38981) * Move legacy accesscontrol to middleware layer * Remove bus usage for this endpoint * Add tests for legacy accesscontrol * Fix tests for org user and remove one more bus usage * Added test for FolderAdmin as suggested in the review --- pkg/api/api.go | 3 +- pkg/api/org_users.go | 34 +----- pkg/api/org_users_test.go | 233 ++++++++++++++++++++++++++++---------- pkg/middleware/auth.go | 27 +++++ 4 files changed, 205 insertions(+), 92 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 140f243534f..6a2ceeb76e6 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -28,6 +28,7 @@ func (hs *HTTPServer) registerRoutes() { reqGrafanaAdmin := middleware.ReqGrafanaAdmin reqEditorRole := middleware.ReqEditorRole reqOrgAdmin := middleware.ReqOrgAdmin + reqOrgAdminFolderAdminOrTeamAdmin := middleware.OrgAdminFolderAdminOrTeamAdmin reqCanAccessTeams := middleware.AdminOrFeatureEnabled(hs.Cfg.EditorsCanAdmin) reqSnapshotPublicModeOrSignedIn := middleware.SnapshotPublicModeOrSignedIn(hs.Cfg) redirectFromLegacyPanelEditURL := middleware.RedirectFromLegacyPanelEditURL(hs.Cfg) @@ -226,7 +227,7 @@ func (hs *HTTPServer) registerRoutes() { // current org without requirement of user to be org admin apiRoute.Group("/org", func(orgRoute routing.RouteRegister) { - orgRoute.Get("/users/lookup", routing.Wrap(hs.GetOrgUsersForCurrentOrgLookup)) + orgRoute.Get("/users/lookup", authorize(reqOrgAdminFolderAdminOrTeamAdmin, ac.EvalPermission(ac.ActionOrgUsersRead, ac.ScopeUsersAll)), routing.Wrap(hs.GetOrgUsersForCurrentOrgLookup)) }) // create new org diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 61f8f19c1e9..b3b7c717e6a 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/util" ) @@ -70,15 +71,6 @@ func (hs *HTTPServer) GetOrgUsersForCurrentOrg(c *models.ReqContext) response.Re // GET /api/org/users/lookup func (hs *HTTPServer) GetOrgUsersForCurrentOrgLookup(c *models.ReqContext) response.Response { - isAdmin, err := isOrgAdminFolderAdminOrTeamAdmin(c) - if err != nil { - return response.Error(500, "Failed to get users for current organization", err) - } - - if !isAdmin { - return response.Error(403, "Permission denied", nil) - } - orgUsers, err := hs.getOrgUsersHelper(&models.GetOrgUsersQuery{ OrgId: c.OrgId, Query: c.Query("query"), @@ -102,28 +94,6 @@ func (hs *HTTPServer) GetOrgUsersForCurrentOrgLookup(c *models.ReqContext) respo return response.JSON(200, result) } -func isOrgAdminFolderAdminOrTeamAdmin(c *models.ReqContext) (bool, error) { - if c.OrgRole == models.ROLE_ADMIN { - return true, nil - } - - hasAdminPermissionInFoldersQuery := models.HasAdminPermissionInFoldersQuery{SignedInUser: c.SignedInUser} - if err := bus.Dispatch(&hasAdminPermissionInFoldersQuery); err != nil { - return false, err - } - - if hasAdminPermissionInFoldersQuery.Result { - return true, nil - } - - isAdminOfTeamsQuery := models.IsAdminOfTeamsQuery{SignedInUser: c.SignedInUser} - if err := bus.Dispatch(&isAdminOfTeamsQuery); err != nil { - return false, err - } - - return isAdminOfTeamsQuery.Result, nil -} - // GET /api/orgs/:orgId/users func (hs *HTTPServer) GetOrgUsers(c *models.ReqContext) response.Response { result, err := hs.getOrgUsersHelper(&models.GetOrgUsersQuery{ @@ -140,7 +110,7 @@ func (hs *HTTPServer) GetOrgUsers(c *models.ReqContext) response.Response { } func (hs *HTTPServer) getOrgUsersHelper(query *models.GetOrgUsersQuery, signedInUser *models.SignedInUser) ([]*models.OrgUserDTO, error) { - if err := bus.Dispatch(query); err != nil { + if err := sqlstore.GetOrgUsers(query); err != nil { return nil, err } diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 8b71a024212..c073dbff0bf 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -4,33 +4,28 @@ import ( "context" "encoding/json" "net/http" + "net/http/httptest" "testing" + "time" "github.com/grafana/grafana/pkg/api/dtos" - "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" + accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func setUpGetOrgUsersHandler() { - bus.AddHandler("test", func(query *models.GetOrgUsersQuery) error { - query.Result = []*models.OrgUserDTO{ - {Email: "testUser@grafana.com", Login: testUserLogin}, - {Email: "user1@grafana.com", Login: "user1"}, - {Email: "user2@grafana.com", Login: "user2"}, - } - return nil - }) -} - func setUpGetOrgUsersDB(t *testing.T, sqlStore *sqlstore.SQLStore) { setting.AutoAssignOrg = true - setting.AutoAssignOrgId = 1 + setting.AutoAssignOrgId = int(testOrgID) - _, err := sqlStore.CreateUser(context.Background(), models.CreateUserCommand{Email: "testUser@grafana.com", Login: "testUserLogin"}) + _, err := sqlStore.CreateUser(context.Background(), models.CreateUserCommand{Email: "testUser@grafana.com", Login: testUserLogin}) require.NoError(t, err) _, err = sqlStore.CreateUser(context.Background(), models.CreateUserCommand{Email: "user1@grafana.com", Login: "user1"}) require.NoError(t, err) @@ -45,7 +40,7 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { sqlStore := sqlstore.InitTestDB(t) loggedInUserScenario(t, "When calling GET on", "api/org/users", func(sc *scenarioContext) { - setUpGetOrgUsersHandler() + setUpGetOrgUsersDB(t, sqlStore) sc.handlerFunc = hs.GetOrgUsersForCurrentOrg sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() @@ -94,48 +89,7 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { assert.Equal(t, 2, resp.Page) }) - loggedInUserScenario(t, "When calling GET as an editor with no team / folder permissions on", - "api/org/users/lookup", func(sc *scenarioContext) { - setUpGetOrgUsersHandler() - bus.AddHandler("test", func(query *models.HasAdminPermissionInFoldersQuery) error { - query.Result = false - return nil - }) - bus.AddHandler("test", func(query *models.IsAdminOfTeamsQuery) error { - query.Result = false - return nil - }) - - sc.handlerFunc = hs.GetOrgUsersForCurrentOrgLookup - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - assert.Equal(t, http.StatusForbidden, sc.resp.Code) - - var resp struct { - Message string - } - err := json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - - assert.Equal(t, "Permission denied", resp.Message) - }) - - loggedInUserScenarioWithRole(t, "When calling GET as an admin on", "GET", "api/org/users/lookup", - "api/org/users/lookup", models.ROLE_ADMIN, func(sc *scenarioContext) { - setUpGetOrgUsersHandler() - - sc.handlerFunc = hs.GetOrgUsersForCurrentOrgLookup - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - require.Equal(t, http.StatusOK, sc.resp.Code) - - var resp []dtos.UserLookupDTO - err := json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - assert.Len(t, resp, 3) - }) - - t.Run("Given there is two hidden users", func(t *testing.T) { + t.Run("Given there are two hidden users", func(t *testing.T) { settings.HiddenUsers = map[string]struct{}{ "user1": {}, testUserLogin: {}, @@ -143,7 +97,7 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { t.Cleanup(func() { settings.HiddenUsers = make(map[string]struct{}) }) loggedInUserScenario(t, "When calling GET on", "api/org/users", func(sc *scenarioContext) { - setUpGetOrgUsersHandler() + setUpGetOrgUsersDB(t, sqlStore) sc.handlerFunc = hs.GetOrgUsersForCurrentOrg sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() @@ -160,7 +114,7 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { loggedInUserScenarioWithRole(t, "When calling GET as an admin on", "GET", "api/org/users/lookup", "api/org/users/lookup", models.ROLE_ADMIN, func(sc *scenarioContext) { - setUpGetOrgUsersHandler() + setUpGetOrgUsersDB(t, sqlStore) sc.handlerFunc = hs.GetOrgUsersForCurrentOrgLookup sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() @@ -176,3 +130,164 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { }) }) } + +func setupOrgUsersAPIcontext(t *testing.T, role models.RoleType) (*scenarioContext, *sqlstore.SQLStore) { + cfg := setting.NewCfg() + db := sqlstore.InitTestDB(t) + + hs := &HTTPServer{ + Cfg: cfg, + QuotaService: "a.QuotaService{Cfg: cfg}, + RouteRegister: routing.NewRouteRegister(), + AccessControl: accesscontrolmock.New().WithDisabled(), + SQLStore: db, + } + + sc := setupScenarioContext(t, "/api/org/users/lookup") + // 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 = role + sc.context.IsSignedIn = true + } + sc.m.Use(pretendSignInMiddleware) + + hs.registerRoutes() + hs.RouteRegister.Register(sc.m.Router) + + return sc, db +} + +func TestOrgUsersAPIEndpoint_LegacyAccessControl_FolderAdmin(t *testing.T) { + sc, db := setupOrgUsersAPIcontext(t, models.ROLE_VIEWER) + + // Create a dashboard folder + cmd := models.SaveDashboardCommand{ + OrgId: testOrgID, + FolderId: 1, + IsFolder: true, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "id": nil, + "title": "1 test dash folder", + "tags": "prod", + }), + } + folder, err := db.SaveDashboard(cmd) + require.NoError(t, err) + require.NotNil(t, folder) + + // Grant our test Viewer with permission to admin the folder + acls := []*models.DashboardAcl{ + { + DashboardID: folder.Id, + OrgID: testOrgID, + UserID: testUserID, + Permission: models.PERMISSION_ADMIN, + Created: time.Now(), + Updated: time.Now(), + }, + } + err = db.UpdateDashboardACL(folder.Id, acls) + require.NoError(t, err) + + sc.resp = httptest.NewRecorder() + + sc.req, err = http.NewRequest(http.MethodGet, "/api/org/users/lookup", nil) + require.NoError(t, err) + + sc.exec() + assert.Equal(t, http.StatusOK, sc.resp.Code) +} + +func TestOrgUsersAPIEndpoint_LegacyAccessControl_TeamAdmin(t *testing.T) { + sc, db := setupOrgUsersAPIcontext(t, models.ROLE_VIEWER) + + // Setup store teams + team1, err := db.CreateTeam("testteam1", "testteam1@example.org", testOrgID) + require.NoError(t, err) + err = db.AddTeamMember(testUserID, testOrgID, team1.Id, false, models.PERMISSION_ADMIN) + require.NoError(t, err) + + sc.resp = httptest.NewRecorder() + + sc.req, err = http.NewRequest(http.MethodGet, "/api/org/users/lookup", nil) + require.NoError(t, err) + + sc.exec() + assert.Equal(t, http.StatusOK, sc.resp.Code) +} + +func TestOrgUsersAPIEndpoint_LegacyAccessControl_Admin(t *testing.T) { + sc, _ := setupOrgUsersAPIcontext(t, models.ROLE_ADMIN) + sc.resp = httptest.NewRecorder() + + var err error + sc.req, err = http.NewRequest(http.MethodGet, "/api/org/users/lookup", nil) + require.NoError(t, err) + + sc.exec() + assert.Equal(t, http.StatusOK, sc.resp.Code) +} + +func TestOrgUsersAPIEndpoint_LegacyAccessControl_Viewer(t *testing.T) { + sc, _ := setupOrgUsersAPIcontext(t, models.ROLE_VIEWER) + sc.resp = httptest.NewRecorder() + + var err error + sc.req, err = http.NewRequest(http.MethodGet, "/api/org/users/lookup", nil) + require.NoError(t, err) + + sc.exec() + assert.Equal(t, http.StatusForbidden, sc.resp.Code) +} + +func TestOrgUsersAPIEndpoint_AccessControl(t *testing.T) { + tests := []accessControlTestCase{ + { + expectedCode: http.StatusOK, + desc: "UsersLookupGet should return 200 for user with correct permissions", + url: "/api/org/users/lookup", + method: http.MethodGet, + permissions: []*accesscontrol.Permission{{Action: accesscontrol.ActionOrgUsersRead, Scope: accesscontrol.ScopeUsersAll}}, + }, + { + + expectedCode: http.StatusForbidden, + desc: "UsersLookupGet should return 403 for user without required permissions", + url: "/api/org/users/lookup", + method: http.MethodGet, + permissions: []*accesscontrol.Permission{{Action: "wrong"}}, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + cfg := setting.NewCfg() + sc, hs := setupAccessControlScenarioContext(t, cfg, test.url, test.permissions) + + // 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 = models.ROLE_VIEWER + sc.context.IsSignedIn = true + } + sc.m.Use(pretendSignInMiddleware) + + sc.resp = httptest.NewRecorder() + hs.SettingsProvider = &setting.OSSImpl{Cfg: cfg} + + var err error + sc.req, err = http.NewRequest(test.method, test.url, nil) + require.NoError(t, err) + + sc.exec() + assert.Equal(t, test.expectedCode, sc.resp.Code) + }) + } +} diff --git a/pkg/middleware/auth.go b/pkg/middleware/auth.go index ff9b66433d9..41cef022e7e 100644 --- a/pkg/middleware/auth.go +++ b/pkg/middleware/auth.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/middleware/cookies" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" ) @@ -188,3 +189,29 @@ func shouldForceLogin(c *models.ReqContext) bool { return forceLogin } + +func OrgAdminFolderAdminOrTeamAdmin(c *models.ReqContext) { + if c.OrgRole == models.ROLE_ADMIN { + return + } + + hasAdminPermissionInFoldersQuery := models.HasAdminPermissionInFoldersQuery{SignedInUser: c.SignedInUser} + if err := sqlstore.HasAdminPermissionInFolders(&hasAdminPermissionInFoldersQuery); err != nil { + c.JsonApiErr(500, "Failed to check if user is a folder admin", err) + } + + if hasAdminPermissionInFoldersQuery.Result { + return + } + + isAdminOfTeamsQuery := models.IsAdminOfTeamsQuery{SignedInUser: c.SignedInUser} + if err := sqlstore.IsAdminOfTeams(&isAdminOfTeamsQuery); err != nil { + c.JsonApiErr(500, "Failed to check if user is a team admin", err) + } + + if isAdminOfTeamsQuery.Result { + return + } + + accessForbidden(c) +}