Skip to content

fix(authz): symmetric SpiceDB cleanup on policy delete #1624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
whoAbhishekSah wants to merge 1 commit into
base: main
Choose a base branch
from
+150 −9
Open

fix(authz): symmetric SpiceDB cleanup on policy delete #1624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions core/policy/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package policy

import (
"context"
"errors"
"fmt"

"github.com/raystack/frontier/pkg/utils"
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
128 changes: 127 additions & 1 deletion core/policy/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
},
Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
Loading