From 602d62ebcc322bbb5d2fa3e65814781affcc3904 Mon Sep 17 00:00:00 2001 From: Ieva Date: Thu, 3 Feb 2022 15:27:05 +0000 Subject: [PATCH] Access control: FGAC for team sync endpoints (#44673) * add actions for team group sync * extend the hook to allow specifying whether the user is external * move user struct to type package * interface for permission service to allow mocking it * reuse existing permissions * test fix * refactor * linting --- pkg/api/team_members.go | 5 +-- pkg/services/accesscontrol/accesscontrol.go | 7 +++- .../accesscontrol/database/database_test.go | 3 +- .../database/resource_permissions.go | 8 ++--- .../resource_permissions_bench_test.go | 2 +- .../database/resource_permissions_test.go | 6 ++-- .../accesscontrol/resourcepermissions/api.go | 2 +- .../resourcepermissions/api_test.go | 2 +- .../resourcepermissions/options.go | 3 +- .../resourcepermissions/service.go | 9 ++--- .../resourcepermissions/service_mock.go | 33 +++++++++++++++++++ .../resourcepermissions/service_test.go | 4 +-- .../resourcepermissions/types/hook.go | 12 +++++-- .../resourceservices/resource_services.go | 8 ++--- 14 files changed, 76 insertions(+), 28 deletions(-) create mode 100644 pkg/services/accesscontrol/resourcepermissions/service_mock.go diff --git a/pkg/api/team_members.go b/pkg/api/team_members.go index cc0cd45daf4..3c094622086 100644 --- a/pkg/api/team_members.go +++ b/pkg/api/team_members.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/util" @@ -152,7 +153,7 @@ func (hs *HTTPServer) RemoveTeamMember(c *models.ReqContext) response.Response { } teamIDString := strconv.FormatInt(teamId, 10) - if _, err := hs.TeamPermissionsService.SetUserPermission(c.Req.Context(), orgId, userId, teamIDString, ""); err != nil { + if _, err := hs.TeamPermissionsService.SetUserPermission(c.Req.Context(), orgId, accesscontrol.User{ID: userId}, teamIDString, ""); err != nil { if errors.Is(err, models.ErrTeamNotFound) { return response.Error(404, "Team not found", nil) } @@ -171,7 +172,7 @@ func (hs *HTTPServer) RemoveTeamMember(c *models.ReqContext) response.Response { // Stubbable by tests. var addOrUpdateTeamMember = func(ctx context.Context, resourcePermissionService *resourcepermissions.Service, userID, orgID, teamID int64, permission string) error { teamIDString := strconv.FormatInt(teamID, 10) - if _, err := resourcePermissionService.SetUserPermission(ctx, orgID, userID, teamIDString, permission); err != nil { + if _, err := resourcePermissionService.SetUserPermission(ctx, orgID, accesscontrol.User{ID: userID}, teamIDString, permission); err != nil { return fmt.Errorf("failed setting permissions for user %d in team %d: %w", userID, teamID, err) } return nil diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index 83fbeeaf4d3..3bf514d6371 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -37,13 +37,18 @@ type ResourcePermissionsService interface { // GetPermissions returns all permissions for given resourceID GetPermissions(ctx context.Context, orgID int64, resourceID string) ([]ResourcePermission, error) // SetUserPermission sets permission on resource for a user - SetUserPermission(ctx context.Context, orgID, userID int64, resourceID, permission string) (*ResourcePermission, error) + SetUserPermission(ctx context.Context, orgID int64, user User, resourceID, permission string) (*ResourcePermission, error) // SetTeamPermission sets permission on resource for a team SetTeamPermission(ctx context.Context, orgID, teamID int64, resourceID, permission string) (*ResourcePermission, error) // SetBuiltInRolePermission sets permission on resource for a built-in role (Admin, Editor, Viewer) SetBuiltInRolePermission(ctx context.Context, orgID int64, builtInRole string, resourceID string, permission string) (*ResourcePermission, error) } +type User struct { + ID int64 + IsExternal bool +} + // Metadata contains user accesses for a given resource // Ex: map[string]bool{"create":true, "delete": true} type Metadata map[string]bool diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go index d4fa790178f..441b0deea29 100644 --- a/pkg/services/accesscontrol/database/database_test.go +++ b/pkg/services/accesscontrol/database/database_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/models" @@ -81,7 +80,7 @@ func TestAccessControlStore_GetUserPermissions(t *testing.T) { user, team := createUserAndTeam(t, sql, tt.orgID) for _, id := range tt.userPermissions { - _, err := store.SetUserResourcePermission(context.Background(), tt.orgID, user.Id, accesscontrol.SetResourcePermissionCommand{ + _, err := store.SetUserResourcePermission(context.Background(), tt.orgID, accesscontrol.User{ID: user.Id}, accesscontrol.SetResourcePermissionCommand{ Actions: []string{"dashboards:write"}, Resource: "dashboards", ResourceID: id, diff --git a/pkg/services/accesscontrol/database/resource_permissions.go b/pkg/services/accesscontrol/database/resource_permissions.go index 436d6179df5..651fad1ee12 100644 --- a/pkg/services/accesscontrol/database/resource_permissions.go +++ b/pkg/services/accesscontrol/database/resource_permissions.go @@ -34,20 +34,20 @@ func (p *flatResourcePermission) Managed() bool { } func (s *AccessControlStore) SetUserResourcePermission( - ctx context.Context, orgID, userID int64, + ctx context.Context, orgID int64, user accesscontrol.User, cmd accesscontrol.SetResourcePermissionCommand, hook types.UserResourceHookFunc, ) (*accesscontrol.ResourcePermission, error) { - if userID == 0 { + if user.ID == 0 { return nil, models.ErrUserNotFound } var err error var permission *accesscontrol.ResourcePermission err = s.sql.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - permission, err = s.setResourcePermission(sess, orgID, managedUserRoleName(userID), s.userAdder(sess, orgID, userID), cmd) + permission, err = s.setResourcePermission(sess, orgID, managedUserRoleName(user.ID), s.userAdder(sess, orgID, user.ID), cmd) if err == nil && hook != nil { - return hook(sess, orgID, userID, cmd.ResourceID, cmd.Permission) + return hook(sess, orgID, user, cmd.ResourceID, cmd.Permission) } return err diff --git a/pkg/services/accesscontrol/database/resource_permissions_bench_test.go b/pkg/services/accesscontrol/database/resource_permissions_bench_test.go index 77a6a9fa398..27047433870 100644 --- a/pkg/services/accesscontrol/database/resource_permissions_bench_test.go +++ b/pkg/services/accesscontrol/database/resource_permissions_bench_test.go @@ -93,7 +93,7 @@ func GenerateDatasourcePermissions(b *testing.B, db *sqlstore.SQLStore, ac *Acce _, err := ac.SetUserResourcePermission( context.Background(), accesscontrol.GlobalOrgID, - userIds[i], + accesscontrol.User{ID: userIds[i]}, accesscontrol.SetResourcePermissionCommand{ Actions: []string{dsAction}, Resource: dsResource, diff --git a/pkg/services/accesscontrol/database/resource_permissions_test.go b/pkg/services/accesscontrol/database/resource_permissions_test.go index e029da2b1f5..00d6a5bfae9 100644 --- a/pkg/services/accesscontrol/database/resource_permissions_test.go +++ b/pkg/services/accesscontrol/database/resource_permissions_test.go @@ -70,11 +70,11 @@ func TestAccessControlStore_SetUserResourcePermission(t *testing.T) { store, _ := setupTestEnv(t) for _, s := range test.seeds { - _, err := store.SetUserResourcePermission(context.Background(), test.orgID, test.userID, s, nil) + _, err := store.SetUserResourcePermission(context.Background(), test.orgID, accesscontrol.User{ID: test.userID}, s, nil) require.NoError(t, err) } - added, err := store.SetUserResourcePermission(context.Background(), test.userID, test.userID, accesscontrol.SetResourcePermissionCommand{ + added, err := store.SetUserResourcePermission(context.Background(), test.userID, accesscontrol.User{ID: test.userID}, accesscontrol.SetResourcePermissionCommand{ Actions: test.actions, Resource: test.resource, ResourceID: test.resourceID, @@ -352,7 +352,7 @@ func seedResourcePermissions(t *testing.T, store *AccessControlStore, sql *sqlst }) require.NoError(t, err) - _, err = store.SetUserResourcePermission(context.Background(), 1, u.Id, accesscontrol.SetResourcePermissionCommand{ + _, err = store.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: u.Id}, accesscontrol.SetResourcePermissionCommand{ Actions: actions, Resource: resource, ResourceID: resourceID, diff --git a/pkg/services/accesscontrol/resourcepermissions/api.go b/pkg/services/accesscontrol/resourcepermissions/api.go index 84a93834854..987a57948fd 100644 --- a/pkg/services/accesscontrol/resourcepermissions/api.go +++ b/pkg/services/accesscontrol/resourcepermissions/api.go @@ -131,7 +131,7 @@ func (a *api) setUserPermission(c *models.ReqContext) response.Response { return response.Error(http.StatusBadRequest, "bad request data", err) } - _, err = a.service.SetUserPermission(c.Req.Context(), c.OrgId, userID, resourceID, cmd.Permission) + _, err = a.service.SetUserPermission(c.Req.Context(), c.OrgId, accesscontrol.User{ID: userID}, resourceID, cmd.Permission) if err != nil { return response.Error(http.StatusBadRequest, "failed to set user permission", err) } diff --git a/pkg/services/accesscontrol/resourcepermissions/api_test.go b/pkg/services/accesscontrol/resourcepermissions/api_test.go index 6e596875cd4..bf64c93d0d4 100644 --- a/pkg/services/accesscontrol/resourcepermissions/api_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/api_test.go @@ -160,7 +160,7 @@ func TestApi_getPermissions(t *testing.T) { // seed user 1 with "View" permission on dashboard 1 u, err := sql.CreateUser(context.Background(), models.CreateUserCommand{Login: "test", OrgId: 1}) require.NoError(t, err) - _, err = service.SetUserPermission(context.Background(), u.OrgId, u.Id, tt.resourceID, "View") + _, err = service.SetUserPermission(context.Background(), u.OrgId, accesscontrol.User{ID: u.Id}, tt.resourceID, "View") require.NoError(t, err) // seed built in role Admin with "Edit" permission on dashboard 1 diff --git a/pkg/services/accesscontrol/resourcepermissions/options.go b/pkg/services/accesscontrol/resourcepermissions/options.go index 907a202ad5c..12fcb5acdb9 100644 --- a/pkg/services/accesscontrol/resourcepermissions/options.go +++ b/pkg/services/accesscontrol/resourcepermissions/options.go @@ -3,6 +3,7 @@ package resourcepermissions import ( "context" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/sqlstore" ) @@ -28,7 +29,7 @@ type Options struct { // RoleGroup is the group name for the generated fixed roles RoleGroup string // OnSetUser if configured will be called each time a permission is set for a user - OnSetUser func(session *sqlstore.DBSession, orgID, userID int64, resourceID, permission string) error + OnSetUser func(session *sqlstore.DBSession, orgID int64, user accesscontrol.User, resourceID, permission string) error // OnSetTeam if configured will be called each time a permission is set for a team OnSetTeam func(session *sqlstore.DBSession, orgID, teamID int64, resourceID, permission string) error // OnSetBuiltInRole if configured will be called each time a permission is set for a built-in role diff --git a/pkg/services/accesscontrol/resourcepermissions/service.go b/pkg/services/accesscontrol/resourcepermissions/service.go index 6ab670a313e..ca5a587baa9 100644 --- a/pkg/services/accesscontrol/resourcepermissions/service.go +++ b/pkg/services/accesscontrol/resourcepermissions/service.go @@ -16,7 +16,8 @@ import ( type Store interface { // SetUserResourcePermission sets permission for managed user role on a resource SetUserResourcePermission( - ctx context.Context, orgID, userID int64, + ctx context.Context, orgID int64, + user accesscontrol.User, cmd accesscontrol.SetResourcePermissionCommand, hook types.UserResourceHookFunc, ) (*accesscontrol.ResourcePermission, error) @@ -100,7 +101,7 @@ func (s *Service) GetPermissions(ctx context.Context, orgID int64, resourceID st }) } -func (s *Service) SetUserPermission(ctx context.Context, orgID, userID int64, resourceID, permission string) (*accesscontrol.ResourcePermission, error) { +func (s *Service) SetUserPermission(ctx context.Context, orgID int64, user accesscontrol.User, resourceID, permission string) (*accesscontrol.ResourcePermission, error) { if !s.options.Assignments.Users { return nil, ErrInvalidAssignment } @@ -114,11 +115,11 @@ func (s *Service) SetUserPermission(ctx context.Context, orgID, userID int64, re return nil, err } - if err := s.validateUser(ctx, orgID, userID); err != nil { + if err := s.validateUser(ctx, orgID, user.ID); err != nil { return nil, err } - return s.store.SetUserResourcePermission(ctx, orgID, userID, accesscontrol.SetResourcePermissionCommand{ + return s.store.SetUserResourcePermission(ctx, orgID, user, accesscontrol.SetResourcePermissionCommand{ Actions: actions, Permission: permission, ResourceID: resourceID, diff --git a/pkg/services/accesscontrol/resourcepermissions/service_mock.go b/pkg/services/accesscontrol/resourcepermissions/service_mock.go new file mode 100644 index 00000000000..a1ad509e7cd --- /dev/null +++ b/pkg/services/accesscontrol/resourcepermissions/service_mock.go @@ -0,0 +1,33 @@ +package resourcepermissions + +import ( + "context" + + "github.com/stretchr/testify/mock" + + "github.com/grafana/grafana/pkg/services/accesscontrol" +) + +type MockService struct { + mock.Mock +} + +func (m *MockService) GetPermissions(ctx context.Context, orgID int64, resourceID string) ([]accesscontrol.ResourcePermission, error) { + mockedArgs := m.Called(ctx, orgID, resourceID) + return mockedArgs.Get(0).([]accesscontrol.ResourcePermission), mockedArgs.Error(1) +} + +func (m *MockService) SetUserPermission(ctx context.Context, orgID int64, user accesscontrol.User, resourceID, permission string) (*accesscontrol.ResourcePermission, error) { + mockedArgs := m.Called(ctx, orgID, user, resourceID, permission) + return mockedArgs.Get(0).(*accesscontrol.ResourcePermission), mockedArgs.Error(1) +} + +func (m *MockService) SetTeamPermission(ctx context.Context, orgID, teamID int64, resourceID, permission string) (*accesscontrol.ResourcePermission, error) { + mockedArgs := m.Called(ctx, orgID, teamID, resourceID, permission) + return mockedArgs.Get(0).(*accesscontrol.ResourcePermission), mockedArgs.Error(1) +} + +func (m *MockService) SetBuiltInRolePermission(ctx context.Context, orgID int64, builtInRole, resourceID, permission string) (*accesscontrol.ResourcePermission, error) { + mockedArgs := m.Called(ctx, orgID, builtInRole, resourceID, permission) + return mockedArgs.Get(0).(*accesscontrol.ResourcePermission), mockedArgs.Error(1) +} diff --git a/pkg/services/accesscontrol/resourcepermissions/service_test.go b/pkg/services/accesscontrol/resourcepermissions/service_test.go index dbdcd3e4b5e..cec11542dd4 100644 --- a/pkg/services/accesscontrol/resourcepermissions/service_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/service_test.go @@ -46,13 +46,13 @@ func TestService_SetUserPermission(t *testing.T) { var hookCalled bool if tt.callHook { - service.options.OnSetUser = func(session *sqlstore.DBSession, orgID, userID int64, resourceID, permission string) error { + service.options.OnSetUser = func(session *sqlstore.DBSession, orgID int64, user accesscontrol.User, resourceID, permission string) error { hookCalled = true return nil } } - _, err = service.SetUserPermission(context.Background(), user.OrgId, user.Id, "1", "") + _, err = service.SetUserPermission(context.Background(), user.OrgId, accesscontrol.User{ID: user.Id}, "1", "") require.NoError(t, err) assert.Equal(t, tt.callHook, hookCalled) }) diff --git a/pkg/services/accesscontrol/resourcepermissions/types/hook.go b/pkg/services/accesscontrol/resourcepermissions/types/hook.go index 5e86cc3893c..b389e8d101c 100644 --- a/pkg/services/accesscontrol/resourcepermissions/types/hook.go +++ b/pkg/services/accesscontrol/resourcepermissions/types/hook.go @@ -1,7 +1,15 @@ package types -import "github.com/grafana/grafana/pkg/services/sqlstore" +import ( + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/sqlstore" +) -type UserResourceHookFunc func(session *sqlstore.DBSession, orgID, userID int64, resourceID, permission string) error +type UserResourceHookFunc func(session *sqlstore.DBSession, orgID int64, user accesscontrol.User, resourceID, permission string) error type TeamResourceHookFunc func(session *sqlstore.DBSession, orgID, teamID int64, resourceID, permission string) error type BuiltinResourceHookFunc func(session *sqlstore.DBSession, orgID int64, builtInRole, resourceID, permission string) error + +type User struct { + ID int64 + IsExternal bool +} diff --git a/pkg/services/accesscontrol/resourceservices/resource_services.go b/pkg/services/accesscontrol/resourceservices/resource_services.go index 077e6cd93a5..3cf2adfb95f 100644 --- a/pkg/services/accesscontrol/resourceservices/resource_services.go +++ b/pkg/services/accesscontrol/resourceservices/resource_services.go @@ -77,20 +77,20 @@ func ProvideTeamPermissions(router routing.RouteRegister, sql *sqlstore.SQLStore ReaderRoleName: "Team permission reader", WriterRoleName: "Team permission writer", RoleGroup: "Teams", - OnSetUser: func(session *sqlstore.DBSession, orgID, userID int64, resourceID, permission string) error { + OnSetUser: func(session *sqlstore.DBSession, orgID int64, user accesscontrol.User, resourceID, permission string) error { teamId, err := strconv.ParseInt(resourceID, 10, 64) if err != nil { return err } switch permission { case "Member": - return sqlstore.AddOrUpdateTeamMemberHook(session, userID, orgID, teamId, false, 0) + return sqlstore.AddOrUpdateTeamMemberHook(session, user.ID, orgID, teamId, user.IsExternal, 0) case "Admin": - return sqlstore.AddOrUpdateTeamMemberHook(session, userID, orgID, teamId, false, models.PERMISSION_ADMIN) + return sqlstore.AddOrUpdateTeamMemberHook(session, user.ID, orgID, teamId, user.IsExternal, models.PERMISSION_ADMIN) case "": return sqlstore.RemoveTeamMemberHook(session, &models.RemoveTeamMemberCommand{ OrgId: orgID, - UserId: userID, + UserId: user.ID, TeamId: teamId, }) default: