Skip to content

refactor(schema): drop the retired org/group ownership relations #1815

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

Merged
whoAbhishekSah merged 3 commits into from
Jul 29, 2026
Merged

refactor(schema): drop the retired org/group ownership relations #1815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
627ba89
refactor(schema): drop the retired org/group ownership relations
whoAbhishekSah Jul 29, 2026
b3396db
refactor(membership): rename removeMemberRelation to removeGroupMembe…
whoAbhishekSah Jul 29, 2026
adb02fc
Merge branch 'main' into refactor/drop-ownership-relations
whoAbhishekSah Jul 29, 2026
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
45 changes: 15 additions & 30 deletions core/membership/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,13 +509,10 @@ func (s *Service) cascadeRemovePrincipal(ctx context.Context, org organization.O

// clean up SpiceDB relations
for _, g := range orgGroups {
if err := s.removeRelations(ctx, g.ID, schema.GroupNamespace, principalID, principalType); err != nil {
if err := s.removeGroupMemberRelation(ctx, g.ID, principalID, principalType); err != nil {
return fmt.Errorf("remove group %s relations: %w", g.ID, err)
}
}
if err := s.removeRelations(ctx, orgID, schema.OrganizationNamespace, principalID, principalType); err != nil {
return fmt.Errorf("remove org relations: %w", err)
}

// remove identity link for service users
if principalType == schema.ServiceUserPrincipal {
Expand Down Expand Up @@ -549,15 +546,15 @@ func (s *Service) removeCustomResourceAccess(ctx context.Context, principalID, p
return nil
}

// removeRelations deletes owner and member relations for a principal on a resource.
func (s *Service) removeRelations(ctx context.Context, resourceID, resourceType, principalID, principalType string) error {
obj := relation.Object{ID: resourceID, Namespace: resourceType}
sub := relation.Subject{ID: principalID, Namespace: principalType}
for _, name := range []string{schema.OwnerRelationName, schema.MemberRelationName} {
err := s.relationService.Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: name})
if err != nil && !errors.Is(err, relation.ErrNotExist) {
return fmt.Errorf("delete relation %s: %w", name, err)
}
// removeGroupMemberRelation deletes the member relation for a principal on a group.
func (s *Service) removeGroupMemberRelation(ctx context.Context, groupID, principalID, principalType string) error {
err := s.relationService.Delete(ctx, relation.Relation{
Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace},
Subject: relation.Subject{ID: principalID, Namespace: principalType},
RelationName: schema.MemberRelationName,
})
if err != nil && !errors.Is(err, relation.ErrNotExist) {
return fmt.Errorf("delete relation %s: %w", schema.MemberRelationName, err)
}
return nil
}
Expand Down Expand Up @@ -1395,7 +1392,7 @@ func (s *Service) RemoveGroupMember(ctx context.Context, groupID, principalID, p
}
}

if err := s.removeRelations(ctx, groupID, schema.GroupNamespace, principalID, principalType); err != nil {
if err := s.removeGroupMemberRelation(ctx, groupID, principalID, principalType); err != nil {
s.log.ErrorContext(ctx, "membership state inconsistent: group policies removed but relation cleanup failed, needs manual fix",
"group_id", groupID,
"principal_id", principalID,
Expand All @@ -1410,7 +1407,7 @@ func (s *Service) RemoveGroupMember(ctx context.Context, groupID, principalID, p
}

// RemoveAllGroupMembers tears down membership for a group that is being
// destroyed: deletes every policy on the group and every owner/member
// destroyed: deletes every policy on the group and every member
// relation per principal. No min-owner check — the group itself is going
// away, so the invariant doesn't apply. Errors are joined; partial failures
// are logged so a retry can complete the cleanup.
Expand Down Expand Up @@ -1442,7 +1439,7 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err
if _, hadFailure := failed[key]; hadFailure {
continue
}
if relErr := s.removeRelations(ctx, groupID, schema.GroupNamespace, p.PrincipalID, p.PrincipalType); relErr != nil {
if relErr := s.removeGroupMemberRelation(ctx, groupID, p.PrincipalID, p.PrincipalType); relErr != nil {
errs = errors.Join(errs, fmt.Errorf("remove relations for %s:%s: %w", p.PrincipalType, p.PrincipalID, relErr))
}
}
Expand Down Expand Up @@ -1540,8 +1537,8 @@ func (s *Service) linkGroupToOrg(ctx context.Context, groupID, orgID string) err
return nil
}

// unlinkGroupFromOrg removes both hierarchy relations between a group and its
// org. Used as best-effort cleanup when group-create wiring fails partway.
// unlinkGroupFromOrg removes the group#org identity link. Used as best-effort
// cleanup when group-create wiring fails partway.
// relation.ErrNotExist is ignored; any other error is returned.
func (s *Service) unlinkGroupFromOrg(ctx context.Context, groupID, orgID string) error {
if err := s.relationService.Delete(ctx, relation.Relation{
Expand All @@ -1551,18 +1548,6 @@ func (s *Service) unlinkGroupFromOrg(ctx context.Context, groupID, orgID string)
}); err != nil && !errors.Is(err, relation.ErrNotExist) {
return err
}

if err := s.relationService.Delete(ctx, relation.Relation{
Object: relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace},
Subject: relation.Subject{
ID: groupID,
Namespace: schema.GroupNamespace,
SubRelationName: schema.MemberRelationName,
},
RelationName: schema.MemberRelationName,
}); err != nil && !errors.Is(err, relation.ErrNotExist) {
return err
}
return nil
}

Expand Down
61 changes: 7 additions & 54 deletions core/membership/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,6 @@ func TestService_RemoveOrganizationMember(t *testing.T) {

enabledOrg := organization.Organization{ID: orgID, Title: "Test Org"}

orgObj := relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace}
grpObj := relation.Object{ID: groupID, Namespace: schema.GroupNamespace}
userSub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}

Expand Down Expand Up @@ -828,8 +827,6 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
wantErr: nil,
Expand All @@ -853,9 +850,6 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
d.policySvc.EXPECT().Delete(ctx, "proj-p1").Return(nil)
d.policySvc.EXPECT().Delete(ctx, "grp-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{projectID}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: grpObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: grpObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
Expand All @@ -878,8 +872,6 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
wantErr: nil,
Expand All @@ -901,23 +893,23 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
wantErrContain: "delete sub-resource policy",
},
{
name: "should return error if org relation removal fails after org policies deleted",
name: "should return error if group relation removal fails after org policies deleted",
setup: func(d testDeps) {
d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: viewerRoleID}}, nil)
d.roleSvc.EXPECT().Get(ctx, schema.RoleOrganizationOwner).Return(role.Role{ID: ownerRoleID, Name: schema.RoleOrganizationOwner}, nil)
d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil)
d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil)
d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{{ID: groupID}}, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{
{ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID, RoleID: viewerRoleID},
}, nil)
// org policy deleted first (viewer, plain Delete)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{}).Return(nil)
// then relation removal fails
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(errors.New("spicedb down"))
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: grpObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(errors.New("spicedb down"))
},
wantErrContain: "remove org relations",
wantErrContain: "relations",
},
{
name: "should remove custom-resource access across every project in the org",
Expand All @@ -933,8 +925,6 @@ func TestService_RemoveOrganizationMember(t *testing.T) {
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{projectID, secondProjectID}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(nil)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
wantErr: nil,
Expand Down Expand Up @@ -1018,8 +1008,6 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) {
ownerRoleID := uuid.New().String()

enabledOrg := organization.Organization{ID: orgID, Title: "Test Org"}
orgObj := relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace}
userSub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}

type testDeps struct {
policySvc *mocks.PolicyService
Expand Down Expand Up @@ -1051,8 +1039,6 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) {
// plain delete, not DeleteWithMinRoleGuard
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
},
Expand All @@ -1065,8 +1051,6 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) {
d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil)
d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
},
Expand All @@ -1085,8 +1069,6 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) {
}, nil)
d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
d.resSvc.EXPECT().RemovePrincipalAccess(ctx, userID, schema.UserPrincipal, []string{}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(nil)
d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
},
Expand Down Expand Up @@ -1137,9 +1119,6 @@ func TestService_RemoveOrganizationMember_WithoutResourceService(t *testing.T) {
ownerRoleID := uuid.New().String()
viewerRoleID := uuid.New().String()

orgObj := relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace}
userSub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}

policySvc := mocks.NewPolicyService(t)
relSvc := mocks.NewRelationService(t)
roleSvc := mocks.NewRoleService(t)
Expand All @@ -1157,8 +1136,6 @@ func TestService_RemoveOrganizationMember_WithoutResourceService(t *testing.T) {
{ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID},
}, nil)
policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil)
relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(nil)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, roleSvc, orgSvc, mocks.NewUserService(t), projSvc, grpSvc, mocks.NewServiceuserService(t), auditRepo)
Expand Down Expand Up @@ -2080,15 +2057,6 @@ func TestService_OnGroupCreated(t *testing.T) {
Subject: relation.Subject{ID: orgID, Namespace: schema.OrganizationNamespace},
RelationName: schema.OrganizationRelationName,
}
orgGroupMemberRelation := relation.Relation{
Object: relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace},
Subject: relation.Subject{
ID: groupID,
Namespace: schema.GroupNamespace,
SubRelationName: schema.MemberRelationName,
},
RelationName: schema.MemberRelationName,
}
creatorMemberRelation := relation.Relation{
Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace},
Subject: relation.Subject{ID: creatorID, Namespace: schema.UserPrincipal},
Expand Down Expand Up @@ -2150,9 +2118,8 @@ func TestService_OnGroupCreated(t *testing.T) {
_ = mockRoleSvc
_ = mockUserSvc

// rollback: delete the identity link plus any legacy org-member relation
// rollback: delete the identity link
mockRelSvc.EXPECT().Delete(ctx, groupOrgRelation).Return(nil)
mockRelSvc.EXPECT().Delete(ctx, orgGroupMemberRelation).Return(relation.ErrNotExist)

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), mockPolicySvc, mockRelSvc, mockRoleSvc, mocks.NewOrgService(t), mockUserSvc, mocks.NewProjectService(t), mockGrpSvc, mocks.NewServiceuserService(t), mocks.NewAuditRecordRepository(t))

Expand Down Expand Up @@ -2232,7 +2199,7 @@ func TestService_RemoveGroupMember(t *testing.T) {
wantErr: membership.ErrLastGroupOwnerRole,
},
{
name: "should remove a member (non-owner) and delete both relations",
name: "should remove a member (non-owner) and delete the member relation",
setup: func(policySvc *mocks.PolicyService, relSvc *mocks.RelationService, roleSvc *mocks.RoleService, grpSvc *mocks.GroupService, userSvc *mocks.UserService, auditRepo *mocks.AuditRecordRepository) {
grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil)
userSvc.EXPECT().GetByID(ctx, userID).Return(enabledUser, nil)
Expand All @@ -2242,7 +2209,6 @@ func TestService_RemoveGroupMember(t *testing.T) {
policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
obj := relation.Object{ID: groupID, Namespace: schema.GroupNamespace}
sub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}
relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.MemberRelationName}).Return(nil)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
Expand All @@ -2259,7 +2225,6 @@ func TestService_RemoveGroupMember(t *testing.T) {
policySvc.EXPECT().DeleteWithMinRoleGuard(ctx, "p1", ownerRoleID).Return(nil)
obj := relation.Object{ID: groupID, Namespace: schema.GroupNamespace}
sub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}
relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.OwnerRelationName}).Return(nil)
relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
Expand Down Expand Up @@ -2325,9 +2290,7 @@ func TestService_RemoveAllGroupMembers(t *testing.T) {
policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
policySvc.EXPECT().Delete(ctx, "p2").Return(nil)
policySvc.EXPECT().Delete(ctx, "p3").Return(nil)
relSvc.EXPECT().Delete(ctx, relFor(schema.OwnerRelationName, userA)).Return(relation.ErrNotExist)
relSvc.EXPECT().Delete(ctx, relFor(schema.MemberRelationName, userA)).Return(nil)
relSvc.EXPECT().Delete(ctx, relFor(schema.OwnerRelationName, userB)).Return(nil)
relSvc.EXPECT().Delete(ctx, relFor(schema.MemberRelationName, userB)).Return(relation.ErrNotExist)

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc,
Expand All @@ -2348,7 +2311,6 @@ func TestService_RemoveAllGroupMembers(t *testing.T) {
}, nil)
policySvc.EXPECT().Delete(ctx, "p1").Return(errors.New("db down"))
policySvc.EXPECT().Delete(ctx, "p2").Return(nil)
relSvc.EXPECT().Delete(ctx, relFor(schema.OwnerRelationName, userB)).Return(nil)
relSvc.EXPECT().Delete(ctx, relFor(schema.MemberRelationName, userB)).Return(nil)

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc,
Expand Down Expand Up @@ -2382,21 +2344,12 @@ func TestService_OnGroupDeleted(t *testing.T) {
}).Return([]policy.Policy{{ID: "principal-p1"}}, nil)
policySvc.EXPECT().Delete(ctx, "principal-p1").Return(nil)

// unlinkGroupFromOrg: both hierarchy relations
// unlinkGroupFromOrg: the identity link
relSvc.EXPECT().Delete(ctx, relation.Relation{
Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace},
Subject: relation.Subject{ID: orgID, Namespace: schema.OrganizationNamespace},
RelationName: schema.OrganizationRelationName,
}).Return(nil)
relSvc.EXPECT().Delete(ctx, relation.Relation{
Object: relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace},
Subject: relation.Subject{
ID: groupID,
Namespace: schema.GroupNamespace,
SubRelationName: schema.MemberRelationName,
},
RelationName: schema.MemberRelationName,
}).Return(nil)

svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc,
mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t),
Expand Down
Loading
Loading