fix(permission): clean up leftover access in SpiceDB when a permission is deleted

cmd/serve.go

@@ -376,12 +376,12 @@ func buildAPIDependencies(
 	}
 	authzRelationRepository := spicedb.NewRelationRepository(sdb, consistencyLevel, cfg.SpiceDB.CheckTrace)
 
-	permissionRepository := postgres.NewPermissionRepository(dbc)
-	permissionService := permission.NewService(permissionRepository)
-
 	relationPGRepository := postgres.NewRelationRepository(dbc)
 	relationService := relation.NewService(relationPGRepository, authzRelationRepository)
 
+	permissionRepository := postgres.NewPermissionRepository(dbc)
+	permissionService := permission.NewService(permissionRepository, relationService)
+
 	auditRecordRepository := postgres.NewAuditRecordRepository(dbc)
 
 	roleRepository := postgres.NewRoleRepository(dbc)
@@ -428,6 +428,9 @@ func buildAPIDependencies(
 	organizationRepository := postgres.NewOrganizationRepository(dbc)
 
 	roleService := role.NewService(roleRepository, relationService, permissionService, auditRecordRepository, cfg.App.PAT.DeniedPermissionsSet())
+	// permission deletion prunes the deleted slug from role definitions; wired
+	// back here because role.Service depends on permission.Service
+	permissionService.SetRoleService(roleService)
 	policyService := policy.NewService(policyPGRepository, relationService, roleService)
 	userService := user.NewService(userRepository, relationService, sessionService)
 	patValidator := userpat.NewValidator(logger, userPATRepo, cfg.App.PAT)

core/permission/service.go

@@ -2,20 +2,49 @@ package permission
 
 import (
 	"context"
+	"errors"
 
+	"github.com/raystack/frontier/core/relation"
+	"github.com/raystack/frontier/internal/bootstrap/schema"
 	"github.com/raystack/frontier/pkg/utils"
 )
 
+// RelationService is the slice of relation.Service permission deletion needs to
+// sweep the role->permission tuples a deleted permission would otherwise leave
+// behind.
+type RelationService interface {
+	Delete(ctx context.Context, rel relation.Relation) error
+}
+
+// RoleService is the slice of role.Service permission deletion needs to strip
+// the deleted slug from role definitions. Primitive-typed and injected via
+// SetRoleService after construction because role.Service depends on
+// permission.Service (so they can't both be set at construction time) — the
+// same pattern organization/project use with SetMembershipService.
+type RoleService interface {
+	RemovePermissionFromRoles(ctx context.Context, slug string) error
+}
+
 type Service struct {
-	repository Repository
+	repository      Repository
+	relationService RelationService
+	roleService     RoleService
 }
 
-func NewService(repository Repository) *Service {
+func NewService(repository Repository, relationService RelationService) *Service {
 	return &Service{
-		repository: repository,
+		repository:      repository,
+		relationService: relationService,
 	}
 }
 
+// SetRoleService injects the role service used to prune a deleted permission's
+// slug from role definitions. Done post-construction to break the mutual
+// dependency between permission.Service and role.Service.
+func (s *Service) SetRoleService(roleService RoleService) {
+	s.roleService = roleService
+}
+
 func (s Service) Get(ctx context.Context, id string) (Permission, error) {
 	if utils.IsValidUUID(id) {
 		return s.repository.Get(ctx, id)
@@ -41,8 +70,36 @@ func (s Service) Update(ctx context.Context, perm Permission) (Permission, error
 	return s.repository.Update(ctx, perm)
 }
 
-// Delete call over a service could be dangerous without removing all of its relations
-// the method does not do it by default
+// Delete removes the permission along with everything that references it:
+//   - the role->permission SpiceDB tuples (app/role:<role>#<slug>@<*>, one per
+//     principal type) written by createRolePermissionRelation — keyed by the
+//     permission slug as the relation name, so one object-namespace +
+//     relation-name delete clears them across all roles and principal types;
+//   - the slug from each role's permissions list, so no role keeps listing a
+//     permission that no longer exists;
+//   - the permission row itself.
 func (s Service) Delete(ctx context.Context, id string) error {
-	return s.repository.Delete(ctx, id)
+	perm, err := s.Get(ctx, id)
+	if err != nil {
+		return err
+	}
+
+	slug := perm.Slug
+	if slug == "" {
+		slug = perm.GenerateSlug()
+	}
+	if err := s.relationService.Delete(ctx, relation.Relation{
+		Object: relation.Object{
+			Namespace: schema.RoleNamespace,
+		},
+		RelationName: slug,
+	}); err != nil && !errors.Is(err, relation.ErrNotExist) {
+		return err
+	}
+
+	if err := s.roleService.RemovePermissionFromRoles(ctx, slug); err != nil {
+		return err
+	}
+
+	return s.repository.Delete(ctx, perm.ID)
 }

core/permission/service_test.go

@@ -14,7 +14,7 @@ import (
 
 func TestService_Get(t *testing.T) {
 	mockRepo := mocks.NewRepository(t)
-	svc := permission.NewService(mockRepo)
+	svc := permission.NewService(mockRepo, mocks.NewRelationService(t))
 
 	t.Run("should get permission by id", func(t *testing.T) {
 		inputID := uuid.New().String()
@@ -61,7 +61,7 @@ func TestService_Get(t *testing.T) {
 
 func TestService_Upsert(t *testing.T) {
 	mockRepo := mocks.NewRepository(t)
-	svc := permission.NewService(mockRepo)
+	svc := permission.NewService(mockRepo, mocks.NewRelationService(t))
 
 	t.Run("should upsert permission", func(t *testing.T) {
 		inputPermission := permission.Permission{
@@ -112,7 +112,7 @@ func TestService_Upsert(t *testing.T) {
 
 func TestService_List(t *testing.T) {
 	mockRepo := mocks.NewRepository(t)
-	svc := permission.NewService(mockRepo)
+	svc := permission.NewService(mockRepo, mocks.NewRelationService(t))
 
 	t.Run("should list permissions", func(t *testing.T) {
 		filters := permission.Filter{
@@ -157,7 +157,7 @@ func TestService_List(t *testing.T) {
 }
 func TestService_Update(t *testing.T) {
 	mockRepo := mocks.NewRepository(t)
-	svc := permission.NewService(mockRepo)
+	svc := permission.NewService(mockRepo, mocks.NewRelationService(t))
 
 	t.Run("should update permission", func(t *testing.T) {
 		inputPermission := permission.Permission{
@@ -207,24 +207,52 @@ func TestService_Update(t *testing.T) {
 }
 
 func TestService_Delete(t *testing.T) {
-	mockRepo := mocks.NewRepository(t)
-	svc := permission.NewService(mockRepo)
+	t.Run("should sweep relations, prune role lists, then delete permission", func(t *testing.T) {
+		mockRepo := mocks.NewRepository(t)
+		mockRelation := mocks.NewRelationService(t)
+		mockRole := mocks.NewRoleService(t)
+		svc := permission.NewService(mockRepo, mockRelation)
+		svc.SetRoleService(mockRole)
 
-	t.Run("should delete permission", func(t *testing.T) {
 		permissionID := uuid.New().String()
+		perm := permission.Permission{ID: permissionID, Name: "delete", Slug: "app_organization_delete"}
 
+		mockRepo.On("Get", mock.Anything, permissionID).Return(perm, nil).Once()
+		mockRelation.On("Delete", mock.Anything, mock.Anything).Return(nil).Once()
+		mockRole.On("RemovePermissionFromRoles", mock.Anything, "app_organization_delete").Return(nil).Once()
 		mockRepo.On("Delete", mock.Anything, permissionID).Return(nil).Once()
 
 		err := svc.Delete(context.Background(), permissionID)
 
 		assert.Nil(t, err)
 	})
 
-	t.Run("should return an error if permissions cannot be list", func(t *testing.T) {
+	t.Run("should return an error if permission cannot be resolved", func(t *testing.T) {
+		mockRepo := mocks.NewRepository(t)
+		svc := permission.NewService(mockRepo, mocks.NewRelationService(t))
+
+		permissionID := uuid.New().String()
+		expectedErr := errors.New("An error occurred")
+
+		mockRepo.On("Get", mock.Anything, permissionID).Return(permission.Permission{}, expectedErr).Once()
+
+		err := svc.Delete(context.Background(), permissionID)
+
+		assert.NotNil(t, err)
+		assert.Equal(t, expectedErr, err)
+	})
+
+	t.Run("should return an error if the relation sweep fails", func(t *testing.T) {
+		mockRepo := mocks.NewRepository(t)
+		mockRelation := mocks.NewRelationService(t)
+		svc := permission.NewService(mockRepo, mockRelation)
+
 		permissionID := uuid.New().String()
+		perm := permission.Permission{ID: permissionID, Name: "delete", Slug: "app_organization_delete"}
 		expectedErr := errors.New("An error occurred")
 
-		mockRepo.On("Delete", mock.Anything, permissionID).Return(expectedErr).Once()
+		mockRepo.On("Get", mock.Anything, permissionID).Return(perm, nil).Once()
+		mockRelation.On("Delete", mock.Anything, mock.Anything).Return(expectedErr).Once()
 
 		err := svc.Delete(context.Background(), permissionID)