From 57d87389e09132762839c3c7080781c2ad6c84a3 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 17 Aug 2022 16:32:02 +0200 Subject: [PATCH] RBAC: Remove user permissions in org when user is removed (#53782) * RBAC: Add orgID to DeleteUserPermissions * RBAC: Refactor query to delete all permissions in specified org, 0 deletes all permissions * Delete user permission in org when user is removed * Remove call to delete permissions in frontend * Remove user permissions if removed orgs is detected during oauth sync Co-authored-by: Jo --- pkg/api/org_users.go | 10 ++ pkg/api/org_users_test.go | 5 + pkg/services/accesscontrol/accesscontrol.go | 6 +- .../accesscontrol/database/database.go | 46 +++++++--- .../accesscontrol/database/database_test.go | 91 +++++++++++++++---- pkg/services/accesscontrol/mock/mock.go | 4 +- .../ossaccesscontrol/ossaccesscontrol.go | 4 +- .../login/loginservice/loginservice.go | 7 ++ pkg/services/user/userimpl/user.go | 2 +- public/app/features/admin/UserOrgs.tsx | 10 +- 10 files changed, 140 insertions(+), 45 deletions(-) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index d0b593f76f2..e988f7e7d28 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.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/user" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" @@ -409,9 +410,18 @@ func (hs *HTTPServer) removeOrgUserHelper(ctx context.Context, cmd *models.Remov } if cmd.UserWasDeleted { + // This should be called from appropriate service when moved + if err := hs.AccessControl.DeleteUserPermissions(ctx, accesscontrol.GlobalOrgID, cmd.UserId); err != nil { + hs.log.Warn("failed to delete permissions for user", "userID", cmd.UserId, "orgID", accesscontrol.GlobalOrgID, "err", err) + } return response.Success("User deleted") } + // This should be called from appropriate service when moved + if err := hs.AccessControl.DeleteUserPermissions(ctx, cmd.OrgId, cmd.UserId); err != nil { + hs.log.Warn("failed to delete permissions for user", "userID", cmd.UserId, "orgID", cmd.OrgId, "err", err) + } + return response.Success("User removed from organization") } diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index de1c2402239..8f0acefed3e 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -958,6 +958,11 @@ func TestDeleteOrgUsersAPIEndpoint_AccessControl(t *testing.T) { err = sc.db.GetOrgUsers(context.Background(), &getUsersQuery) require.NoError(t, err) assert.Len(t, getUsersQuery.Result, tc.expectedUserCount) + + // check all permissions for user is removed in org + permission, err := sc.hs.AccessControl.GetUserPermissions(context.Background(), &user.SignedInUser{UserID: tc.targetUserId, OrgID: tc.targetOrg}, accesscontrol.Options{}) + require.NoError(t, err) + assert.Len(t, permission, 0) } }) } diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index dc219741fcb..3c7b13c68de 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -36,7 +36,9 @@ type AccessControl interface { // specific scope prefix (ex: datasources:name:) RegisterScopeAttributeResolver(scopePrefix string, resolver ScopeAttributeResolver) - DeleteUserPermissions(ctx context.Context, userID int64) error + // DeleteUserPermissions removes all permissions user has in org and all permission to that user + // If orgID is set to 0 remove permissions from all orgs + DeleteUserPermissions(ctx context.Context, orgID, userID int64) error } type RoleRegistry interface { @@ -47,7 +49,7 @@ type RoleRegistry interface { type PermissionsStore interface { // GetUserPermissions returns user permissions with only action and scope fields set. GetUserPermissions(ctx context.Context, query GetUserPermissionsQuery) ([]Permission, error) - DeleteUserPermissions(ctx context.Context, userID int64) error + DeleteUserPermissions(ctx context.Context, orgID, userID int64) error } type TeamPermissionsService interface { diff --git a/pkg/services/accesscontrol/database/database.go b/pkg/services/accesscontrol/database/database.go index 6822d4c2d58..af0a82cbf4d 100644 --- a/pkg/services/accesscontrol/database/database.go +++ b/pkg/services/accesscontrol/database/database.go @@ -131,20 +131,37 @@ func deletePermissions(sess *sqlstore.DBSession, ids []int64) error { return nil } -func (s *AccessControlStore) DeleteUserPermissions(ctx context.Context, userID int64) error { +func (s *AccessControlStore) DeleteUserPermissions(ctx context.Context, orgID, userID int64) error { err := s.sql.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { + roleDeleteQuery := "DELETE FROM user_role WHERE user_id = ?" + roleDeleteParams := []interface{}{roleDeleteQuery, userID} + if orgID != accesscontrol.GlobalOrgID { + roleDeleteQuery += " AND org_id = ?" + roleDeleteParams = []interface{}{roleDeleteQuery, userID, orgID} + } + // Delete user role assignments - if _, err := sess.Exec("DELETE FROM user_role WHERE user_id = ?", userID); err != nil { + if _, err := sess.Exec(roleDeleteParams...); err != nil { return err } - // Delete permissions that are scoped to user - if _, err := sess.Exec("DELETE FROM permission WHERE scope = ?", accesscontrol.Scope("users", "id", strconv.FormatInt(userID, 10))); err != nil { - return err + // only delete scopes to user if all permissions is removed (i.e. user is removed) + if orgID == accesscontrol.GlobalOrgID { + // Delete permissions that are scoped to user + if _, err := sess.Exec("DELETE FROM permission WHERE scope = ?", accesscontrol.Scope("users", "id", strconv.FormatInt(userID, 10))); err != nil { + return err + } + } + + roleQuery := "SELECT id FROM role WHERE name = ?" + roleParams := []interface{}{accesscontrol.ManagedUserRoleName(userID)} + if orgID != accesscontrol.GlobalOrgID { + roleQuery += " AND org_id = ?" + roleParams = []interface{}{accesscontrol.ManagedUserRoleName(userID), orgID} } var roleIDs []int64 - if err := sess.SQL("SELECT id FROM role WHERE name = ?", accesscontrol.ManagedUserRoleName(userID)).Find(&roleIDs); err != nil { + if err := sess.SQL(roleQuery, roleParams...).Find(&roleIDs); err != nil { return err } @@ -152,20 +169,25 @@ func (s *AccessControlStore) DeleteUserPermissions(ctx context.Context, userID i return nil } - query := "DELETE FROM permission WHERE role_id IN(? " + strings.Repeat(",?", len(roleIDs)-1) + ")" - args := make([]interface{}, 0, len(roleIDs)+1) - args = append(args, query) + permissionDeleteQuery := "DELETE FROM permission WHERE role_id IN(? " + strings.Repeat(",?", len(roleIDs)-1) + ")" + permissionDeleteParams := make([]interface{}, 0, len(roleIDs)+1) + permissionDeleteParams = append(permissionDeleteParams, permissionDeleteQuery) for _, id := range roleIDs { - args = append(args, id) + permissionDeleteParams = append(permissionDeleteParams, id) } // Delete managed user permissions - if _, err := sess.Exec(args...); err != nil { + if _, err := sess.Exec(permissionDeleteParams...); err != nil { return err } + managedRoleDeleteQuery := "DELETE FROM role WHERE id IN(? " + strings.Repeat(",?", len(roleIDs)-1) + ")" + managedRoleDeleteParams := []interface{}{managedRoleDeleteQuery} + for _, id := range roleIDs { + managedRoleDeleteParams = append(managedRoleDeleteParams, id) + } // Delete managed user roles - if _, err := sess.Exec("DELETE FROM role WHERE name = ?", accesscontrol.ManagedUserRoleName(userID)); err != nil { + if _, err := sess.Exec(managedRoleDeleteParams...); err != nil { return err } diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go index 2d90177ecaf..a5010fee3d4 100644 --- a/pkg/services/accesscontrol/database/database_test.go +++ b/pkg/services/accesscontrol/database/database_test.go @@ -141,28 +141,85 @@ func TestAccessControlStore_GetUserPermissions(t *testing.T) { } func TestAccessControlStore_DeleteUserPermissions(t *testing.T) { - store, sql := setupTestEnv(t) + t.Run("expect permissions in all orgs to be deleted", func(t *testing.T) { + store, sql := setupTestEnv(t) + user, _ := createUserAndTeam(t, sql, 1) - user, _ := createUserAndTeam(t, sql, 1) + // generate permissions in org 1 + _, err := store.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: user.ID}, types.SetResourcePermissionCommand{ + Actions: []string{"dashboards:write"}, + Resource: "dashboards", + ResourceID: "1", + }, nil) + require.NoError(t, err) - _, err := store.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: user.ID}, types.SetResourcePermissionCommand{ - Actions: []string{"dashboards:write"}, - Resource: "dashboards", - ResourceID: "1", - }, nil) - require.NoError(t, err) + // generate permissions in org 2 + _, err = store.SetUserResourcePermission(context.Background(), 2, accesscontrol.User{ID: user.ID}, types.SetResourcePermissionCommand{ + Actions: []string{"dashboards:write"}, + Resource: "dashboards", + ResourceID: "1", + }, nil) + require.NoError(t, err) - err = store.DeleteUserPermissions(context.Background(), user.ID) - require.NoError(t, err) + err = store.DeleteUserPermissions(context.Background(), accesscontrol.GlobalOrgID, user.ID) + require.NoError(t, err) - permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{ - OrgID: 1, - UserID: user.ID, - Roles: []string{"Admin"}, - Actions: []string{"dashboards:write"}, + permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{ + OrgID: 1, + UserID: user.ID, + Roles: []string{"Admin"}, + }) + require.NoError(t, err) + assert.Len(t, permissions, 0) + + permissions, err = store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{ + OrgID: 2, + UserID: user.ID, + Roles: []string{"Admin"}, + }) + require.NoError(t, err) + assert.Len(t, permissions, 0) + }) + + t.Run("expect permissions in org 1 to be deleted", func(t *testing.T) { + store, sql := setupTestEnv(t) + user, _ := createUserAndTeam(t, sql, 1) + + // generate permissions in org 1 + _, err := store.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: user.ID}, types.SetResourcePermissionCommand{ + Actions: []string{"dashboards:write"}, + Resource: "dashboards", + ResourceID: "1", + }, nil) + require.NoError(t, err) + + // generate permissions in org 2 + _, err = store.SetUserResourcePermission(context.Background(), 2, accesscontrol.User{ID: user.ID}, types.SetResourcePermissionCommand{ + Actions: []string{"dashboards:write"}, + Resource: "dashboards", + ResourceID: "1", + }, nil) + require.NoError(t, err) + + err = store.DeleteUserPermissions(context.Background(), 1, user.ID) + require.NoError(t, err) + + permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{ + OrgID: 1, + UserID: user.ID, + Roles: []string{"Admin"}, + }) + require.NoError(t, err) + assert.Len(t, permissions, 0) + + permissions, err = store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{ + OrgID: 2, + UserID: user.ID, + Roles: []string{"Admin"}, + }) + require.NoError(t, err) + assert.Len(t, permissions, 1) }) - require.NoError(t, err) - assert.Len(t, permissions, 0) } func createUserAndTeam(t *testing.T, sql *sqlstore.SQLStore, orgID int64) (*user.User, models.Team) { diff --git a/pkg/services/accesscontrol/mock/mock.go b/pkg/services/accesscontrol/mock/mock.go index c843ffc779b..fad60e7e518 100644 --- a/pkg/services/accesscontrol/mock/mock.go +++ b/pkg/services/accesscontrol/mock/mock.go @@ -183,8 +183,8 @@ func (m *Mock) RegisterScopeAttributeResolver(scopePrefix string, resolver acces } } -func (m *Mock) DeleteUserPermissions(ctx context.Context, userID int64) error { - m.Calls.DeleteUserPermissions = append(m.Calls.DeleteUserPermissions, []interface{}{ctx, userID}) +func (m *Mock) DeleteUserPermissions(ctx context.Context, orgID, userID int64) error { + m.Calls.DeleteUserPermissions = append(m.Calls.DeleteUserPermissions, []interface{}{ctx, orgID, userID}) // Use override if provided if m.DeleteUserPermissionsFunc != nil { return m.DeleteUserPermissionsFunc(ctx, userID) diff --git a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go index 14f8c1da05d..3ca1d339173 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go @@ -200,6 +200,6 @@ func (ac *OSSAccessControlService) RegisterScopeAttributeResolver(scopePrefix st ac.scopeResolvers.AddScopeAttributeResolver(scopePrefix, resolver) } -func (ac *OSSAccessControlService) DeleteUserPermissions(ctx context.Context, userID int64) error { - return ac.store.DeleteUserPermissions(ctx, userID) +func (ac *OSSAccessControlService) DeleteUserPermissions(ctx context.Context, orgID int64, userID int64) error { + return ac.store.DeleteUserPermissions(ctx, orgID, userID) } diff --git a/pkg/services/login/loginservice/loginservice.go b/pkg/services/login/loginservice/loginservice.go index 8c91c2dda8b..3fa7c7b4789 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/pkg/services/login/loginservice/loginservice.go @@ -6,6 +6,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -21,12 +22,14 @@ func ProvideService( userService user.Service, quotaService quota.Service, authInfoService login.AuthInfoService, + accessControl accesscontrol.AccessControl, ) *Implementation { s := &Implementation{ SQLStore: sqlStore, userService: userService, QuotaService: quotaService, AuthInfoService: authInfoService, + accessControl: accessControl, } return s } @@ -37,6 +40,7 @@ type Implementation struct { AuthInfoService login.AuthInfoService QuotaService quota.Service TeamSync login.TeamSyncFunc + accessControl accesscontrol.AccessControl } // CreateUser creates inserts a new one. @@ -311,6 +315,9 @@ func (ls *Implementation) syncOrgRoles(ctx context.Context, usr *user.User, extU logger.Error(err.Error(), "userId", cmd.UserId, "orgId", cmd.OrgId) continue } + if err := ls.accessControl.DeleteUserPermissions(ctx, orgId, cmd.UserId); err != nil { + logger.Warn("failed to delete permissions for user", "userID", cmd.UserId, "orgID", orgId) + } return err } diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index 2c788253a3e..dbe0672cfaf 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -223,7 +223,7 @@ func (s *Service) Delete(ctx context.Context, cmd *user.DeleteUserCommand) error return nil }) g.Go(func() error { - if err := s.accessControlStore.DeleteUserPermissions(ctx, cmd.UserID); err != nil { + if err := s.accessControlStore.DeleteUserPermissions(ctx, accesscontrol.GlobalOrgID, cmd.UserID); err != nil { return err } return nil diff --git a/public/app/features/admin/UserOrgs.tsx b/public/app/features/admin/UserOrgs.tsx index f8110c96487..f9dd1335bd3 100644 --- a/public/app/features/admin/UserOrgs.tsx +++ b/public/app/features/admin/UserOrgs.tsx @@ -158,16 +158,8 @@ class UnThemedOrgRow extends PureComponent { } onOrgRemove = async () => { - const { org, user } = this.props; + const { org } = this.props; this.props.onOrgRemove(org.orgId); - if (contextSrv.licensedAccessControlEnabled()) { - if ( - contextSrv.hasPermission(AccessControlAction.ActionUserRolesRemove) && - contextSrv.hasPermission(AccessControlAction.ActionUserRolesAdd) - ) { - user && (await updateUserRoles([], user.id, org.orgId)); - } - } }; onChangeRoleClick = () => {