RBAC: Fix delete team permissions on team delete (#83442)

* RBAC: Remove team permissions on delete

* Remove unecessary deletes from store function

* Nit on mock

* Add test to the database

* Nit on comment

* Add another test to check that other permissions remain

pkg/services/accesscontrol/accesscontrol.go

@@ -35,6 +35,9 @@ type Service interface {
 	// 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
+	// DeleteTeamPermissions removes all role assignments and permissions granted to a team
+	// and removes permissions scoped to the team.
+	DeleteTeamPermissions(ctx context.Context, orgID, teamID int64) error
 	// DeclareFixedRoles allows the caller to declare, to the service, fixed roles and their
 	// assignments to organization roles ("Viewer", "Editor", "Admin") or "Grafana Admin"
 	DeclareFixedRoles(registrations ...RoleRegistration) error
@@ -52,6 +55,7 @@ type Store interface {
 	SearchUsersPermissions(ctx context.Context, orgID int64, options SearchOptions) (map[int64][]Permission, error)
 	GetUsersBasicRoles(ctx context.Context, userFilter []int64, orgID int64) (map[int64][]string, error)
 	DeleteUserPermissions(ctx context.Context, orgID, userID int64) error
+	DeleteTeamPermissions(ctx context.Context, orgID, teamID int64) error
 	SaveExternalServiceRole(ctx context.Context, cmd SaveExternalServiceRoleCommand) error
 	DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error
 }

pkg/services/accesscontrol/acimpl/service.go

@@ -166,6 +166,10 @@ func (s *Service) DeleteUserPermissions(ctx context.Context, orgID int64, userID
 	return s.store.DeleteUserPermissions(ctx, orgID, userID)
 }
 
+func (s *Service) DeleteTeamPermissions(ctx context.Context, orgID int64, teamID int64) error {
+	return s.store.DeleteTeamPermissions(ctx, orgID, teamID)
+}
+
 // DeclareFixedRoles allow the caller to declare, to the service, fixed roles and their assignments
 // to organization roles ("Viewer", "Editor", "Admin") or "Grafana Admin"
 func (s *Service) DeclareFixedRoles(registrations ...accesscontrol.RoleRegistration) error {

pkg/services/accesscontrol/actest/fake.go

@@ -41,6 +41,10 @@ func (f FakeService) DeleteUserPermissions(ctx context.Context, orgID, userID in
 	return f.ExpectedErr
 }
 
+func (f FakeService) DeleteTeamPermissions(ctx context.Context, orgID, teamID int64) error {
+	return f.ExpectedErr
+}
+
 func (f FakeService) DeclareFixedRoles(registrations ...accesscontrol.RoleRegistration) error {
 	return f.ExpectedErr
 }
@@ -94,6 +98,10 @@ func (f FakeStore) DeleteUserPermissions(ctx context.Context, orgID, userID int6
 	return f.ExpectedErr
 }
 
+func (f FakeStore) DeleteTeamPermissions(ctx context.Context, orgID, teamID int64) error {
+	return f.ExpectedErr
+}
+
 func (f FakeStore) SaveExternalServiceRole(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error {
 	return f.ExpectedErr
 }

pkg/services/accesscontrol/actest/store_mock.go

@@ -51,6 +51,24 @@ func (_m *MockStore) DeleteUserPermissions(ctx context.Context, orgID int64, use
 	return r0
 }
 
+// DeleteTeamPermissions provides a mock function with given fields: ctx, orgID, teamID
+func (_m *MockStore) DeleteTeamPermissions(ctx context.Context, orgID int64, teamID int64) error {
+	ret := _m.Called(ctx, orgID, teamID)
+
+	if len(ret) == 0 {
+		panic("no return value specified for DeleteTeamPermissions")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, int64, int64) error); ok {
+		r0 = rf(ctx, orgID, teamID)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 // GetUserPermissions provides a mock function with given fields: ctx, query
 func (_m *MockStore) GetUserPermissions(ctx context.Context, query accesscontrol.GetUserPermissionsQuery) ([]accesscontrol.Permission, error) {
 	ret := _m.Called(ctx, query)

pkg/services/accesscontrol/database/database.go

@@ -241,3 +241,58 @@ func (s *AccessControlStore) DeleteUserPermissions(ctx context.Context, orgID, u
 	})
 	return err
 }
+
+func (s *AccessControlStore) DeleteTeamPermissions(ctx context.Context, orgID, teamID int64) error {
+	err := s.sql.WithDbSession(ctx, func(sess *db.Session) error {
+		roleDeleteQuery := "DELETE FROM team_role WHERE team_id = ? AND org_id = ?"
+		roleDeleteParams := []any{roleDeleteQuery, teamID, orgID}
+
+		// Delete team role assignments
+		if _, err := sess.Exec(roleDeleteParams...); err != nil {
+			return err
+		}
+
+		// Delete permissions that are scoped to the team
+		if _, err := sess.Exec("DELETE FROM permission WHERE scope = ?", accesscontrol.Scope("teams", "id", strconv.FormatInt(teamID, 10))); err != nil {
+			return err
+		}
+
+		// Delete the team managed role
+		roleQuery := "SELECT id FROM role WHERE name = ? AND org_id = ?"
+		roleParams := []any{accesscontrol.ManagedTeamRoleName(teamID), orgID}
+
+		var roleIDs []int64
+		if err := sess.SQL(roleQuery, roleParams...).Find(&roleIDs); err != nil {
+			return err
+		}
+
+		if len(roleIDs) == 0 {
+			return nil
+		}
+
+		permissionDeleteQuery := "DELETE FROM permission WHERE role_id IN(? " + strings.Repeat(",?", len(roleIDs)-1) + ")"
+		permissionDeleteParams := make([]any, 0, len(roleIDs)+1)
+		permissionDeleteParams = append(permissionDeleteParams, permissionDeleteQuery)
+		for _, id := range roleIDs {
+			permissionDeleteParams = append(permissionDeleteParams, id)
+		}
+
+		// Delete managed team permissions
+		if _, err := sess.Exec(permissionDeleteParams...); err != nil {
+			return err
+		}
+
+		managedRoleDeleteQuery := "DELETE FROM role WHERE id IN(? " + strings.Repeat(",?", len(roleIDs)-1) + ")"
+		managedRoleDeleteParams := []any{managedRoleDeleteQuery}
+		for _, id := range roleIDs {
+			managedRoleDeleteParams = append(managedRoleDeleteParams, id)
+		}
+		// Delete managed team role
+		if _, err := sess.Exec(managedRoleDeleteParams...); err != nil {
+			return err
+		}
+
+		return nil
+	})
+	return err
+}

pkg/services/accesscontrol/database/database_test.go

@@ -243,6 +243,77 @@ func TestAccessControlStore_DeleteUserPermissions(t *testing.T) {
 	})
 }
 
+func TestAccessControlStore_DeleteTeamPermissions(t *testing.T) {
+	t.Run("expect permissions related to team to be deleted", func(t *testing.T) {
+		store, permissionsStore, sql, teamSvc, _ := setupTestEnv(t)
+		user, team := createUserAndTeam(t, sql, teamSvc, 1)
+
+		// grant permission to the team
+		_, err := permissionsStore.SetTeamResourcePermission(context.Background(), 1, team.ID, rs.SetResourcePermissionCommand{
+			Actions:           []string{"dashboards:write"},
+			Resource:          "dashboards",
+			ResourceAttribute: "uid",
+			ResourceID:        "xxYYzz",
+		}, nil)
+		require.NoError(t, err)
+
+		// generate permissions scoped to the team
+		_, err = permissionsStore.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: user.ID}, rs.SetResourcePermissionCommand{
+			Actions:           []string{"team:read"},
+			Resource:          "teams",
+			ResourceAttribute: "id",
+			ResourceID:        fmt.Sprintf("%d", team.ID),
+		}, nil)
+		require.NoError(t, err)
+
+		err = store.DeleteTeamPermissions(context.Background(), 1, team.ID)
+		require.NoError(t, err)
+
+		permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{
+			OrgID:   1,
+			UserID:  user.ID,
+			Roles:   []string{"Admin"},
+			TeamIDs: []int64{team.ID},
+		})
+		require.NoError(t, err)
+		assert.Len(t, permissions, 0)
+	})
+	t.Run("expect permissions not related to team to be kept", func(t *testing.T) {
+		store, permissionsStore, sql, teamSvc, _ := setupTestEnv(t)
+		user, team := createUserAndTeam(t, sql, teamSvc, 1)
+
+		// grant permission to the team
+		_, err := permissionsStore.SetTeamResourcePermission(context.Background(), 1, team.ID, rs.SetResourcePermissionCommand{
+			Actions:           []string{"dashboards:write"},
+			Resource:          "dashboards",
+			ResourceAttribute: "uid",
+			ResourceID:        "xxYYzz",
+		}, nil)
+		require.NoError(t, err)
+
+		// generate permissions scoped to another team
+		_, err = permissionsStore.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: user.ID}, rs.SetResourcePermissionCommand{
+			Actions:           []string{"team:read"},
+			Resource:          "teams",
+			ResourceAttribute: "id",
+			ResourceID:        fmt.Sprintf("%d", team.ID+1),
+		}, nil)
+		require.NoError(t, err)
+
+		err = store.DeleteTeamPermissions(context.Background(), 1, team.ID)
+		require.NoError(t, err)
+
+		permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{
+			OrgID:   1,
+			UserID:  user.ID,
+			Roles:   []string{"Admin"},
+			TeamIDs: []int64{team.ID},
+		})
+		require.NoError(t, err)
+		assert.Len(t, permissions, 1)
+	})
+}
+
 func createUserAndTeam(t *testing.T, userSrv user.Service, teamSvc team.Service, orgID int64) (*user.User, team.Team) {
 	t.Helper()
 

pkg/services/accesscontrol/mock/mock.go

@@ -28,6 +28,7 @@ type Calls struct {
 	RegisterFixedRoles             []interface{}
 	RegisterAttributeScopeResolver []interface{}
 	DeleteUserPermissions          []interface{}
+	DeleteTeamPermissions          []interface{}
 	SearchUsersPermissions         []interface{}
 	SearchUserPermissions          []interface{}
 	SaveExternalServiceRole        []interface{}
@@ -53,6 +54,7 @@ type Mock struct {
 	RegisterFixedRolesFunc             func() error
 	RegisterScopeAttributeResolverFunc func(string, accesscontrol.ScopeAttributeResolver)
 	DeleteUserPermissionsFunc          func(context.Context, int64) error
+	DeleteTeamPermissionsFunc          func(context.Context, int64) error
 	SearchUsersPermissionsFunc         func(context.Context, identity.Requester, int64, accesscontrol.SearchOptions) (map[int64][]accesscontrol.Permission, error)
 	SearchUserPermissionsFunc          func(ctx context.Context, orgID int64, searchOptions accesscontrol.SearchOptions) ([]accesscontrol.Permission, error)
 	SaveExternalServiceRoleFunc        func(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error
@@ -199,6 +201,15 @@ func (m *Mock) DeleteUserPermissions(ctx context.Context, orgID, userID int64) e
 	return nil
 }
 
+func (m *Mock) DeleteTeamPermissions(ctx context.Context, orgID, teamID int64) error {
+	m.Calls.DeleteTeamPermissions = append(m.Calls.DeleteTeamPermissions, []interface{}{ctx, orgID, teamID})
+	// Use override if provided
+	if m.DeleteTeamPermissionsFunc != nil {
+		return m.DeleteTeamPermissionsFunc(ctx, teamID)
+	}
+	return nil
+}
+
 // SearchUsersPermissions returns all users' permissions filtered by an action prefix
 func (m *Mock) SearchUsersPermissions(ctx context.Context, usr identity.Requester, options accesscontrol.SearchOptions) (map[int64][]accesscontrol.Permission, error) {
 	user := usr.(*user.SignedInUser)

pkg/services/team/teamapi/team.go

@@ -127,6 +127,12 @@ func (tapi *TeamAPI) deleteTeamByID(c *contextmodel.ReqContext) response.Respons
 		}
 		return response.Error(http.StatusInternalServerError, "Failed to delete Team", err)
 	}
+
+	// Clear associated team assignments, managed role and permissions
+	if err := tapi.ac.DeleteTeamPermissions(c.Req.Context(), orgID, teamID); err != nil {
+		return response.Error(http.StatusInternalServerError, "Failed to delete Team permissions", err)
+	}
+
 	return response.Success("Team deleted")
 }
 

pkg/services/team/teamimpl/store.go

@@ -143,7 +143,6 @@ func (ss *xormStore) Delete(ctx context.Context, cmd *team.DeleteTeamCommand) er
 			"DELETE FROM team_member WHERE org_id=? and team_id = ?",
 			"DELETE FROM team WHERE org_id=? and id = ?",
 			"DELETE FROM dashboard_acl WHERE org_id=? and team_id = ?",
-			"DELETE FROM team_role WHERE org_id=? and team_id = ?",
 		}
 
 		deletes = append(deletes, ss.deletes...)
@@ -154,10 +153,7 @@ func (ss *xormStore) Delete(ctx context.Context, cmd *team.DeleteTeamCommand) er
 				return err
 			}
 		}
-
-		_, err := sess.Exec("DELETE FROM permission WHERE scope=?", ac.Scope("teams", "id", fmt.Sprint(cmd.ID)))
-
-		return err
+		return nil
 	})
 }