From dce39cdfcd99419e18403186b94b9c2e1280cd58 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Mon, 15 Jun 2026 15:30:50 +0530 Subject: [PATCH] refactor(schema): remove member relation from app/organization --- core/membership/service.go | 125 +++++++++--------- core/membership/service_test.go | 104 ++++++--------- internal/bootstrap/schema/base_schema.zed | 1 - .../bootstrap/testdata/compiled_schema.zed | 1 - 4 files changed, 110 insertions(+), 121 deletions(-) diff --git a/core/membership/service.go b/core/membership/service.go index 4f0de1c05..1bccecb80 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -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) @@ -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, @@ -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) } @@ -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) @@ -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) { @@ -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 @@ -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) { @@ -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, @@ -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)) } } @@ -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}, @@ -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 } diff --git a/core/membership/service_test.go b/core/membership/service_test.go index dfb8cf568..d1a12169f 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -152,11 +152,7 @@ func TestService_AddOrganizationMember(t *testing.T) { RoleID: viewerRoleID, ResourceID: orgID, ResourceType: schema.OrganizationNamespace, PrincipalID: userID, PrincipalType: schema.UserPrincipal, }).Return(policy.Policy{}, nil) - relSvc.EXPECT().Create(ctx, relation.Relation{ - Object: relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace}, - Subject: relation.Subject{ID: userID, Namespace: schema.UserPrincipal}, - RelationName: schema.MemberRelationName, - }).Return(relation.Relation{}, nil) + // non-owner roles have no org relation (member relation removed from schema) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, orgID: orgID, @@ -195,7 +191,7 @@ func TestService_AddOrganizationMember(t *testing.T) { roleSvc.EXPECT().Get(ctx, viewerRoleID).Return(role.Role{ID: viewerRoleID, OrgID: orgID, Scopes: []string{schema.OrganizationNamespace}}, nil) policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) policySvc.EXPECT().Create(ctx, mock.Anything).Return(policy.Policy{}, nil) - relSvc.EXPECT().Create(ctx, mock.Anything).Return(relation.Relation{}, nil) + // non-owner roles have no org relation (member relation removed from schema) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, orgID: orgID, @@ -235,7 +231,8 @@ func TestService_AddOrganizationMember(t *testing.T) { setup: func(policySvc *mocks.PolicyService, relSvc *mocks.RelationService, roleSvc *mocks.RoleService, orgSvc *mocks.OrgService, userSvc *mocks.UserService, _ *mocks.AuditRecordRepository) { orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil) userSvc.EXPECT().GetByID(ctx, userID).Return(enabledUser, nil) - roleSvc.EXPECT().Get(ctx, viewerRoleID).Return(role.Role{ID: viewerRoleID, Scopes: []string{schema.OrganizationNamespace}}, nil) + // use owner role — only owner still writes an org relation + roleSvc.EXPECT().Get(ctx, ownerRoleID).Return(role.Role{ID: ownerRoleID, Name: schema.RoleOrganizationOwner, Scopes: []string{schema.OrganizationNamespace}}, nil) policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) policySvc.EXPECT().Create(ctx, mock.Anything).Return(policy.Policy{ID: "created-policy-1"}, nil) relSvc.EXPECT().Create(ctx, mock.Anything).Return(relation.Relation{}, errors.New("spicedb unavailable")) @@ -244,7 +241,7 @@ func TestService_AddOrganizationMember(t *testing.T) { }, orgID: orgID, userID: userID, - roleID: viewerRoleID, + roleID: ownerRoleID, wantErrContain: "spicedb unavailable", }, } @@ -531,10 +528,9 @@ func TestService_SetOrganizationMemberRole(t *testing.T) { RoleID: viewerRoleID, ResourceID: orgID, ResourceType: schema.OrganizationNamespace, PrincipalID: userID, PrincipalType: schema.UserPrincipal, }).Return(policy.Policy{ID: "new-p"}, nil) - // replace relation: delete both owner and member, then create member + // replace relation: delete owner only (member relation removed from org schema) + // viewer role → orgRoleToRelation returns "" → no new relation created relSvc.EXPECT().Delete(ctx, orgRelation(schema.OwnerRelationName)).Return(nil) - relSvc.EXPECT().Delete(ctx, orgRelation(schema.MemberRelationName)).Return(nil) - relSvc.EXPECT().Create(ctx, orgRelation(schema.MemberRelationName)).Return(relation.Relation{}, nil) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, roleID: viewerRoleID, @@ -555,9 +551,9 @@ func TestService_SetOrganizationMemberRole(t *testing.T) { RoleID: ownerRoleID, ResourceID: orgID, ResourceType: schema.OrganizationNamespace, PrincipalID: userID, PrincipalType: schema.UserPrincipal, }).Return(policy.Policy{ID: "new-p"}, nil) - // replace relation: delete both, create owner + // replace relation: delete owner only (member relation removed from org schema), + // then create owner relation relSvc.EXPECT().Delete(ctx, orgRelation(schema.OwnerRelationName)).Return(nil) - relSvc.EXPECT().Delete(ctx, orgRelation(schema.MemberRelationName)).Return(nil) relSvc.EXPECT().Create(ctx, orgRelation(schema.OwnerRelationName)).Return(relation.Relation{}, nil) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, @@ -592,10 +588,9 @@ func TestService_SetOrganizationMemberRole(t *testing.T) { // existing policy is manager (non-owner), uses plain Delete policySvc.EXPECT().Delete(ctx, "p1").Return(nil) policySvc.EXPECT().Create(ctx, mock.Anything).Return(policy.Policy{}, nil) - // both relation deletes return not-found — that's fine, should continue + // owner relation delete returns not-found — that's fine, should continue + // member relation no longer exists in org schema; viewer role → no new relation relSvc.EXPECT().Delete(ctx, orgRelation(schema.OwnerRelationName)).Return(relation.ErrNotExist) - relSvc.EXPECT().Delete(ctx, orgRelation(schema.MemberRelationName)).Return(relation.ErrNotExist) - relSvc.EXPECT().Create(ctx, orgRelation(schema.MemberRelationName)).Return(relation.Relation{}, nil) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, roleID: viewerRoleID, @@ -658,8 +653,8 @@ func TestService_SetOrganizationMemberRole_ServiceUser(t *testing.T) { mockPolicySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal}).Return([]policy.Policy{{ID: "p1", RoleID: ownerRoleID}}, nil) mockPolicySvc.EXPECT().Delete(ctx, "p1").Return(nil) mockPolicySvc.EXPECT().Create(ctx, mock.Anything).Return(policy.Policy{}, nil) - mockRelSvc.EXPECT().Delete(ctx, mock.Anything).Return(relation.ErrNotExist).Times(2) - mockRelSvc.EXPECT().Create(ctx, mock.Anything).Return(relation.Relation{}, nil) + // only owner relation is swept; viewer role → no new relation (member removed from org schema) + mockRelSvc.EXPECT().Delete(ctx, mock.Anything).Return(relation.ErrNotExist).Times(1) mockAuditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), mockPolicySvc, mockRelSvc, mockRoleSvc, mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mockAuditRepo) @@ -873,8 +868,8 @@ func TestService_RemoveOrganizationMember(t *testing.T) { {ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID}, }, nil) d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + // only owner relation is swept (member relation removed from org schema) 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, @@ -897,8 +892,8 @@ func TestService_RemoveOrganizationMember(t *testing.T) { d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) d.policySvc.EXPECT().Delete(ctx, "proj-p1").Return(nil) d.policySvc.EXPECT().Delete(ctx, "grp-p1").Return(nil) + // only owner relation swept for org (member relation removed from org schema) 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) @@ -921,8 +916,8 @@ func TestService_RemoveOrganizationMember(t *testing.T) { {ID: "other-proj-p", ResourceType: schema.ProjectNamespace, ResourceID: otherProjectID}, }, nil) d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + // only owner relation swept for org (member relation removed from org schema) 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, @@ -1034,8 +1029,8 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { }, nil) // plain delete, not DeleteWithMinRoleGuard d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + // only owner relation swept for org (member relation removed from org schema) 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) }, }, @@ -1047,8 +1042,8 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { 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.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) + // only owner relation swept for org (member relation removed from org schema) 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) }, }, @@ -1066,8 +1061,8 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { {ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID, RoleID: ownerRoleID}, }, nil) d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + // only owner relation swept for org (member relation removed from org schema) 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) }, }, @@ -2027,15 +2022,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, - } creatorOwnerRelation := relation.Relation{ Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, Subject: relation.Subject{ID: creatorID, Namespace: schema.UserPrincipal}, @@ -2050,8 +2036,8 @@ func TestService_OnGroupCreated(t *testing.T) { mockUserSvc := mocks.NewUserService(t) mockAuditRepo := mocks.NewAuditRecordRepository(t) + // linkGroupToOrg creates only the group→org relation (org#member@group#member removed from schema) mockRelSvc.EXPECT().Create(ctx, groupOrgRelation).Return(relation.Relation{}, nil) - mockRelSvc.EXPECT().Create(ctx, orgGroupMemberRelation).Return(relation.Relation{}, nil) mockGrpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) mockUserSvc.EXPECT().GetByID(ctx, creatorID).Return(enabledUser, nil) @@ -2081,42 +2067,49 @@ func TestService_OnGroupCreated(t *testing.T) { assert.ErrorContains(t, err, "link group to org") }) - t.Run("should rollback first hierarchy relation if second fails", func(t *testing.T) { + t.Run("should rollback hierarchy relation if owner add fails", func(t *testing.T) { mockPolicySvc := mocks.NewPolicyService(t) mockRelSvc := mocks.NewRelationService(t) + mockRoleSvc := mocks.NewRoleService(t) + mockGrpSvc := mocks.NewGroupService(t) + mockUserSvc := mocks.NewUserService(t) + // org#member@group#member relation removed from schema; only group→org relation exists mockRelSvc.EXPECT().Create(ctx, groupOrgRelation).Return(relation.Relation{}, nil) - mockRelSvc.EXPECT().Create(ctx, orgGroupMemberRelation).Return(relation.Relation{}, errors.New("spicedb unavailable")) - // rollback: delete the first hierarchy relation + + // SetGroupMemberRole fails because group fetch fails + mockGrpSvc.EXPECT().Get(ctx, groupID).Return(group.Group{}, errors.New("db down")) + + // rollback: delete the single hierarchy relation mockRelSvc.EXPECT().Delete(ctx, groupOrgRelation).Return(nil) - svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), mockPolicySvc, mockRelSvc, mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mocks.NewServiceuserService(t), mocks.NewAuditRecordRepository(t)) + _ = mockRoleSvc + _ = mockUserSvc + + 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)) err := svc.OnGroupCreated(ctx, groupID, orgID, creatorID, schema.UserPrincipal) - assert.ErrorContains(t, err, "add group as org member") + assert.ErrorContains(t, err, "db down") }) - t.Run("should rollback both hierarchy relations if owner add fails", func(t *testing.T) { + t.Run("should rollback hierarchy relation if SetGroupMemberRole fails after group fetch", func(t *testing.T) { mockPolicySvc := mocks.NewPolicyService(t) mockRelSvc := mocks.NewRelationService(t) mockRoleSvc := mocks.NewRoleService(t) mockGrpSvc := mocks.NewGroupService(t) mockUserSvc := mocks.NewUserService(t) - // linkGroupToOrg succeeds + // linkGroupToOrg succeeds — only one relation now (org#member@group#member removed) mockRelSvc.EXPECT().Create(ctx, groupOrgRelation).Return(relation.Relation{}, nil) - mockRelSvc.EXPECT().Create(ctx, orgGroupMemberRelation).Return(relation.Relation{}, nil) - - // AddGroupMember fails before policy creation (group fetch fails) - mockGrpSvc.EXPECT().Get(ctx, groupID).Return(group.Group{}, errors.New("db down")) - // unused mocks: only here for completeness, won't be called - _ = mockRoleSvc - _ = mockUserSvc + // SetGroupMemberRole fails at existing-policy listing + mockGrpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) + mockUserSvc.EXPECT().GetByID(ctx, creatorID).Return(enabledUser, nil) + mockRoleSvc.EXPECT().Get(ctx, schema.GroupOwnerRole).Return(role.Role{ID: ownerRoleID, Name: schema.GroupOwnerRole, Scopes: []string{schema.GroupNamespace}}, nil) + mockPolicySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: creatorID, PrincipalType: schema.UserPrincipal}).Return(nil, errors.New("db down")) - // rollback: delete both hierarchy relations + // rollback: delete the single hierarchy relation mockRelSvc.EXPECT().Delete(ctx, groupOrgRelation).Return(nil) - mockRelSvc.EXPECT().Delete(ctx, orgGroupMemberRelation).Return(nil) 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)) @@ -2346,21 +2339,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: only group→org relation (org#member@group#member removed from schema) 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), diff --git a/internal/bootstrap/schema/base_schema.zed b/internal/bootstrap/schema/base_schema.zed index 985c48b30..a58087a60 100644 --- a/internal/bootstrap/schema/base_schema.zed +++ b/internal/bootstrap/schema/base_schema.zed @@ -33,7 +33,6 @@ definition app/organization { relation platform: app/platform relation granted: app/rolebinding relation pat_granted: app/rolebinding - relation member: app/user | app/group#member | app/serviceuser relation owner: app/user | app/serviceuser // permissions diff --git a/internal/bootstrap/testdata/compiled_schema.zed b/internal/bootstrap/testdata/compiled_schema.zed index ac991d7ce..d3621d101 100644 --- a/internal/bootstrap/testdata/compiled_schema.zed +++ b/internal/bootstrap/testdata/compiled_schema.zed @@ -45,7 +45,6 @@ definition app/organization { permission grouplist = platform->superuser + granted->app_organization_administer + granted->app_organization_grouplist + owner permission invitationcreate = platform->superuser + granted->app_organization_administer + granted->app_organization_invitationcreate + owner permission invitationlist = platform->superuser + granted->app_organization_administer + granted->app_organization_invitationlist + owner - relation member: app/user | app/group#member | app/serviceuser relation owner: app/user | app/serviceuser relation pat_granted: app/rolebinding