Skip to content

refactor(schema): remove member relation from app/organization #1701

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

Draft
rohilsurana wants to merge 1 commit into
base: schema-membership-policy-driven
Choose a base branch
from
+110 −121
Draft

refactor(schema): remove member relation from app/organization #1701

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
125 changes: 66 additions & 59 deletions core/membership/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,17 +164,19 @@ func (s *Service) AddOrganizationMember(ctx context.Context, orgID, principalID,
}

relationName := orgRoleToRelation(fetchedRole)
if err := s.createRelation(ctx, orgID, schema.OrganizationNamespace, principalID, principalType, relationName); err != nil {
// best-effort cleanup to avoid orphaned policy
if deleteErr := s.policyService.Delete(ctx, createdPolicy.ID); deleteErr != nil {
s.log.WarnContext(ctx, "orphaned policy: relation creation failed and policy cleanup also failed",
"policy_id", createdPolicy.ID,
"org_id", orgID,
"principal_id", principalID,
"policy_delete_error", deleteErr,
)
if relationName != "" {
if err := s.createRelation(ctx, orgID, schema.OrganizationNamespace, principalID, principalType, relationName); err != nil {
// best-effort cleanup to avoid orphaned policy
if deleteErr := s.policyService.Delete(ctx, createdPolicy.ID); deleteErr != nil {
s.log.WarnContext(ctx, "orphaned policy: relation creation failed and policy cleanup also failed",
"policy_id", createdPolicy.ID,
"org_id", orgID,
"principal_id", principalID,
"policy_delete_error", deleteErr,
)
}
return err
}
return err
}

// create identity link for service users (serviceuser#org@organization)
Expand Down Expand Up @@ -260,8 +262,8 @@ func (s *Service) SetOrganizationMemberRole(ctx context.Context, orgID, principa
}

newRelation := orgRoleToRelation(fetchedRole)
oldRelations := []string{schema.OwnerRelationName, schema.MemberRelationName}
err = s.replaceRelation(ctx, orgID, schema.OrganizationNamespace, principalID, principalType, oldRelations, newRelation)
oldRelations := []string{schema.OwnerRelationName}
err = s.replaceOrRemoveRelation(ctx, orgID, schema.OrganizationNamespace, principalID, principalType, oldRelations, newRelation)
if err != nil {
s.log.ErrorContext(ctx, "membership state inconsistent: policy replaced but relation update failed, needs manual fix",
"org_id", orgID,
Expand Down Expand Up @@ -531,11 +533,11 @@ 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.removeRelations(ctx, g.ID, schema.GroupNamespace, principalID, principalType, groupRelationNames); 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 {
if err := s.removeRelations(ctx, orgID, schema.OrganizationNamespace, principalID, principalType, orgRelationNames); err != nil {
return fmt.Errorf("remove org relations: %w", err)
}

Expand All @@ -554,11 +556,11 @@ func (s *Service) cascadeRemovePrincipal(ctx context.Context, org organization.O
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 {
// removeRelations deletes the given relations for a principal on a resource.
func (s *Service) removeRelations(ctx context.Context, resourceID, resourceType, principalID, principalType string, relationNames []string) error {
obj := relation.Object{ID: resourceID, Namespace: resourceType}
sub := relation.Subject{ID: principalID, Namespace: principalType}
for _, name := range []string{schema.OwnerRelationName, schema.MemberRelationName} {
for _, name := range relationNames {
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)
Expand All @@ -567,6 +569,11 @@ func (s *Service) removeRelations(ctx context.Context, resourceID, resourceType,
return nil
}

var (
orgRelationNames = []string{schema.OwnerRelationName}
groupRelationNames = []string{schema.OwnerRelationName, schema.MemberRelationName}
)

// validateMinOwnerConstraint ensures the org always has at least one owner after a role change.
// Returns the resolved owner role ID for reuse by callers.
func (s *Service) validateMinOwnerConstraint(ctx context.Context, orgID, newRoleID string, existing []policy.Policy) (string, error) {
Expand Down Expand Up @@ -672,6 +679,35 @@ func (s *Service) replaceRelation(ctx context.Context, resourceID, resourceType,
return nil
}

// replaceOrRemoveRelation deletes the given old relations and, if
// newRelationName is non-empty, creates the replacement. When
// newRelationName is "" the old relations are simply removed with no
// replacement — used when the target role has no corresponding SpiceDB
// relation (e.g. non-owner org roles after the member relation was removed).
func (s *Service) replaceOrRemoveRelation(ctx context.Context, resourceID, resourceType, principalID, principalType string, oldRelations []string, newRelationName string) error {
obj := relation.Object{ID: resourceID, Namespace: resourceType}
sub := relation.Subject{ID: principalID, Namespace: principalType}

for _, name := range oldRelations {
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)
}
}

if newRelationName == "" {
return nil
}

_, err := s.relationService.Create(ctx, relation.Relation{
Object: obj, Subject: sub, RelationName: newRelationName,
})
if err != nil {
return fmt.Errorf("create relation: %w", err)
}
return nil
}

// validateOrgRole checks that the role is valid for organization scope and returns it.
// A role is valid if it is either:
// - a platform-wide role scoped to organizations, or
Expand Down Expand Up @@ -768,12 +804,14 @@ func (s *Service) validatePrincipal(ctx context.Context, orgID, principalID, pri
}

// orgRoleToRelation maps an org role to the corresponding SpiceDB relation name.
// Owner role gets "owner" relation, everything else gets "member" relation.
// Owner role gets the "owner" relation; all other roles return "" because the
// org member relation has been removed from the schema — non-owner access is
// resolved entirely through rolebindings (granted->).
func orgRoleToRelation(r role.Role) string {
if r.Name == schema.RoleOrganizationOwner {
return schema.OwnerRelationName
}
return schema.MemberRelationName
return ""
}

func (s *Service) createPolicy(ctx context.Context, resourceID, resourceType, principalID, principalType, roleID string) (policy.Policy, error) {
Expand Down Expand Up @@ -1415,7 +1453,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.removeRelations(ctx, groupID, schema.GroupNamespace, principalID, principalType, groupRelationNames); 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 Down Expand Up @@ -1462,7 +1500,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.removeRelations(ctx, groupID, schema.GroupNamespace, p.PrincipalID, p.PrincipalType, groupRelationNames); relErr != nil {
errs = errors.Join(errs, fmt.Errorf("remove relations for %s:%s: %w", p.PrincipalType, p.PrincipalID, relErr))
}
}
Expand Down Expand Up @@ -1553,41 +1591,22 @@ func (s *Service) OnGroupCreated(ctx context.Context, groupID, orgID, creatorID,
//
// If the second relation fails, the first is best-effort rolled back so we
// don't leave a one-way link.
// linkGroupToOrg creates the group→org hierarchy relation in SpiceDB.
// This is the only relation needed: group.org is used by the group
// permission definitions (e.g. group.delete = org->group_delete + ...).
func (s *Service) linkGroupToOrg(ctx context.Context, groupID, orgID string) error {
groupOrg := relation.Relation{
if _, err := s.relationService.Create(ctx, relation.Relation{
Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace},
Subject: relation.Subject{ID: orgID, Namespace: schema.OrganizationNamespace},
RelationName: schema.OrganizationRelationName,
}
if _, err := s.relationService.Create(ctx, groupOrg); err != nil {
return fmt.Errorf("link group to org: %w", err)
}

if _, err := s.relationService.Create(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 {
if delErr := s.relationService.Delete(ctx, groupOrg); delErr != nil && !errors.Is(delErr, relation.ErrNotExist) {
s.log.WarnContext(ctx, "group->org rollback failed after org member relation failure",
"group_id", groupID,
"org_id", orgID,
"error", delErr,
)
}
return fmt.Errorf("add group as org member: %w", err)
return fmt.Errorf("link group to org: %w", 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.
// relation.ErrNotExist is ignored; any other error is returned.
// unlinkGroupFromOrg removes the group→org hierarchy relation.
// Used as best-effort cleanup when group-create wiring fails partway.
func (s *Service) unlinkGroupFromOrg(ctx context.Context, groupID, orgID string) error {
if err := s.relationService.Delete(ctx, relation.Relation{
Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace},
Expand All @@ -1596,18 +1615,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
Loading
Loading