RBAC: Remove user permissions in org when user is removed (grafana#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 <[email protected]>

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")
 }
 

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)
 			}
 		})
 	}

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 {

pkg/services/accesscontrol/database/database.go

@@ -131,41 +131,63 @@ 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
 		}
 
 		if len(roleIDs) == 0 {
 			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
 		}
 

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)
-
-	user, _ := createUserAndTeam(t, sql, 1)
+	t.Run("expect permissions in all orgs 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(), accesscontrol.GlobalOrgID, 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)
 
-	_, 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)
+		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)
+	})
 
-	err = store.DeleteUserPermissions(context.Background(), user.ID)
-	require.NoError(t, err)
+	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:   1,
-		UserID:  user.ID,
-		Roles:   []string{"Admin"},
-		Actions: []string{"dashboards:write"},
+		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) {

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)

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)
 }

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
 		}

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

public/app/features/admin/UserOrgs.tsx

@@ -158,16 +158,8 @@ class UnThemedOrgRow extends PureComponent<OrgRowProps> {
   }
 
   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 = () => {