From 2c244b82d7d69938e94d60f10d513c150a697fed Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Tue, 19 May 2026 15:19:26 +0530 Subject: [PATCH] fix(authz): symmetric SpiceDB cleanup on policy delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit policy.AssignRole writes three tuples per policy but policy.Delete and DeleteWithMinRoleGuard only swept the two where the rolebinding is the Object (role_bearer, role). The resource-grant tuple — where the rolebinding is the Subject — leaked on every per-member removal, ownership swap, or cross-resource principal cleanup. Add a second relation.Delete keyed by Subject on both delete paths via a shared helper. The Subject-side delete tolerates relation.ErrNotExist so resource-level wildcard sweeps in group.DeleteModel / project.DeleteModel (which currently paper over the bug at full-resource teardown) don't cause failures during the overlap period; those sweeps become redundant for this tuple and can be removed in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/policy/service.go | 31 ++++++--- core/policy/service_test.go | 128 +++++++++++++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 9 deletions(-) diff --git a/core/policy/service.go b/core/policy/service.go index e1a432d31..85679d2d5 100644 --- a/core/policy/service.go +++ b/core/policy/service.go @@ -2,6 +2,7 @@ package policy import ( "context" + "errors" "fmt" "github.com/raystack/frontier/pkg/utils" @@ -78,12 +79,7 @@ func (s Service) Create(ctx context.Context, policy Policy) (Policy, error) { } func (s Service) Delete(ctx context.Context, id string) error { - if err := s.relationService.Delete(ctx, relation.Relation{ - Object: relation.Object{ - ID: id, - Namespace: schema.RoleBindingNamespace, - }, - }); err != nil { + if err := s.deleteRoleBindingRelations(ctx, id); err != nil { return err } return s.repository.Delete(ctx, id) @@ -93,12 +89,31 @@ func (s Service) DeleteWithMinRoleGuard(ctx context.Context, id string, guardRol if err := s.repository.DeleteWithMinRoleGuard(ctx, id, guardRoleID); err != nil { return err } - return s.relationService.Delete(ctx, relation.Relation{ + return s.deleteRoleBindingRelations(ctx, id) +} + +// deleteRoleBindingRelations removes all SpiceDB tuples written by AssignRole: +// the rolebinding-as-Object tuples (role_bearer, role) and the resource-grant +// tuple where the rolebinding is the Subject. The Subject-side delete tolerates +// ErrNotExist because resource-delete sweeps may have already removed it. +func (s Service) deleteRoleBindingRelations(ctx context.Context, id string) error { + if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ ID: id, Namespace: schema.RoleBindingNamespace, }, - }) + }); err != nil { + return err + } + if err := s.relationService.Delete(ctx, relation.Relation{ + Subject: relation.Subject{ + ID: id, + Namespace: schema.RoleBindingNamespace, + }, + }); err != nil && !errors.Is(err, relation.ErrNotExist) { + return err + } + return nil } // AssignRole Note: ideally this should be in a single transaction diff --git a/core/policy/service_test.go b/core/policy/service_test.go index 8e897f29c..dc44dfd11 100644 --- a/core/policy/service_test.go +++ b/core/policy/service_test.go @@ -30,7 +30,7 @@ func TestService_Delete(t *testing.T) { setup func() *policy.Service }{ { - name: "delete policy from relation before repository", + name: "delete both rolebinding-as-object and rolebinding-as-subject tuples before repository", id: "test-id", wantErr: false, setup: func() *policy.Service { @@ -41,6 +41,34 @@ func TestService_Delete(t *testing.T) { Namespace: schema.RoleBindingNamespace, }, }).Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Subject: relation.Subject{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(nil) + repo.On("Delete", ctx, "test-id").Return(nil) + return policy.NewService(repo, relationService, roleService) + }, + }, + { + name: "tolerate ErrNotExist on resource-grant tuple delete", + id: "test-id", + wantErr: false, + setup: func() *policy.Service { + repo, roleService, relationService := mockService(t) + relationService.On("Delete", ctx, relation.Relation{ + Object: relation.Object{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Subject: relation.Subject{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(relation.ErrNotExist) repo.On("Delete", ctx, "test-id").Return(nil) return policy.NewService(repo, relationService, roleService) }, @@ -60,6 +88,27 @@ func TestService_Delete(t *testing.T) { return policy.NewService(repo, relationService, roleService) }, }, + { + name: "resource-grant tuple delete failure (non-ErrNotExist) blocks repository delete", + id: "test-id", + wantErr: true, + setup: func() *policy.Service { + repo, roleService, relationService := mockService(t) + relationService.On("Delete", ctx, relation.Relation{ + Object: relation.Object{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Subject: relation.Subject{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(errors.New("spicedb unavailable")) + return policy.NewService(repo, relationService, roleService) + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -71,6 +120,83 @@ func TestService_Delete(t *testing.T) { } } +func TestService_DeleteWithMinRoleGuard(t *testing.T) { + ctx := context.Background() + tests := []struct { + name string + id string + guardID string + wantErr bool + setup func() *policy.Service + }{ + { + name: "delete both rolebinding-as-object and rolebinding-as-subject tuples after repository", + id: "test-id", + guardID: "guard-role-id", + wantErr: false, + setup: func() *policy.Service { + repo, roleService, relationService := mockService(t) + repo.On("DeleteWithMinRoleGuard", ctx, "test-id", "guard-role-id").Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Object: relation.Object{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Subject: relation.Subject{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(nil) + return policy.NewService(repo, relationService, roleService) + }, + }, + { + name: "tolerate ErrNotExist on resource-grant tuple delete", + id: "test-id", + guardID: "guard-role-id", + wantErr: false, + setup: func() *policy.Service { + repo, roleService, relationService := mockService(t) + repo.On("DeleteWithMinRoleGuard", ctx, "test-id", "guard-role-id").Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Object: relation.Object{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(nil) + relationService.On("Delete", ctx, relation.Relation{ + Subject: relation.Subject{ + ID: "test-id", + Namespace: schema.RoleBindingNamespace, + }, + }).Return(relation.ErrNotExist) + return policy.NewService(repo, relationService, roleService) + }, + }, + { + name: "repository guard failure skips relation deletes", + id: "test-id", + guardID: "guard-role-id", + wantErr: true, + setup: func() *policy.Service { + repo, roleService, relationService := mockService(t) + repo.On("DeleteWithMinRoleGuard", ctx, "test-id", "guard-role-id").Return(errors.New("guard violated")) + return policy.NewService(repo, relationService, roleService) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := tt.setup() + if err := s.DeleteWithMinRoleGuard(ctx, tt.id, tt.guardID); (err != nil) != tt.wantErr { + t.Errorf("DeleteWithMinRoleGuard() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + func TestService_Create(t *testing.T) { ctx := context.Background() tests := []struct {