From 9cc7caeb0665f4ad2c5dcdc213b125c58a129c0a Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 30 Jul 2026 16:08:17 +0530 Subject: [PATCH 1/3] refactor(membership): deduplicate repeated patterns Behavior unchanged; call sites and public signatures stay the same. - One resourcePolicyFilter builds resource-scoped policy filters; the duplicate policyFilterForResource is gone. removeAllPolicies now goes through it and reuses removePoliciesByFilter, so its filter also pins resource_type (redundant with the per-type ID fields in SQL). - validateOrgRole/validateProjectRole/validateGroupRole are one-line wrappers over a shared validateRoleForScope. - validateMinOwnerConstraint/validateMinGroupOwnerConstraint are wrappers over a shared validateMinRoleConstraint. - The five near-identical org/group audit helpers delegate to one auditMemberChange that writes both audit stores. - hasExactlyRole names the repeated "already has exactly this role" check. - principalKey struct replaces the \x00-joined string map keys, and ListPrincipalsByResource's role enrichment moves into its own function. - listOrgsForPrincipal/listGroupsForPrincipal reuse policyResourceIDs instead of hand-rolled pluck loops. Co-Authored-By: Claude Fable 5 --- core/membership/audit.go | 222 ++++++++------------------------ core/membership/group.go | 57 +------- core/membership/list.go | 118 ++++++++--------- core/membership/org.go | 66 +--------- core/membership/project.go | 24 +--- core/membership/project_test.go | 8 +- core/membership/service.go | 108 +++++++++++++--- 7 files changed, 219 insertions(+), 384 deletions(-) diff --git a/core/membership/audit.go b/core/membership/audit.go index ea8caf54e4..70c16375e1 100644 --- a/core/membership/audit.go +++ b/core/membership/audit.go @@ -49,82 +49,78 @@ func (s *Service) createAuditRecord(ctx context.Context, record auditrecord.Audi } } -func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organization.Organization, p principalInfo, roleID string) { +// auditMemberChange writes a membership change to both audit stores: an audit +// record against the resource and a legacy auditor log with the given attrs. +// The role ID and the principal's email go into the record's target metadata +// when present. +func (s *Service) auditMemberChange(ctx context.Context, event pkgAuditRecord.Event, legacyEvent audit.EventName, res auditrecord.Resource, orgID string, p principalInfo, roleID string, legacyAttrs map[string]string) { targetType, _ := principalTypeToAuditType(p.Type) - meta := map[string]any{"role_id": roleID} + meta := map[string]any{} + if roleID != "" { + meta["role_id"] = roleID + } if p.Email != "" { meta["email"] = p.Email } s.createAuditRecord(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.OrganizationMemberRoleChangedEvent, - Resource: auditrecord.Resource{ - ID: org.ID, - Type: pkgAuditRecord.OrganizationType, - Name: org.Title, - }, + Event: event, + Resource: res, Target: &auditrecord.Target{ ID: p.ID, Type: targetType, Name: p.Name, Metadata: meta, }, - OrgID: org.ID, + OrgID: orgID, OccurredAt: time.Now(), }) - if err := audit.GetAuditor(ctx, org.ID).LogWithAttrs(audit.OrgMemberRoleChangedEvent, audit.Target{ + if err := audit.GetAuditor(ctx, orgID).LogWithAttrs(legacyEvent, audit.Target{ ID: p.ID, Type: p.Type, - }, map[string]string{ - "role_id": roleID, - }); err != nil { - s.log.WarnContext(ctx, "failed to write audit log", "error", err, "event", audit.OrgMemberRoleChangedEvent) + }, legacyAttrs); err != nil { + s.log.WarnContext(ctx, "failed to write audit log", "error", err, "event", legacyEvent) } } +func orgAuditResource(org organization.Organization) auditrecord.Resource { + return auditrecord.Resource{ID: org.ID, Type: pkgAuditRecord.OrganizationType, Name: org.Title} +} + +func groupAuditResource(grp group.Group) auditrecord.Resource { + return auditrecord.Resource{ID: grp.ID, Type: pkgAuditRecord.GroupType, Name: grp.Title} +} + func (s *Service) auditOrgMemberAdded(ctx context.Context, org organization.Organization, p principalInfo, roleID string) { - targetType, _ := principalTypeToAuditType(p.Type) - meta := map[string]any{"role_id": roleID} - if p.Email != "" { - meta["email"] = p.Email - } + s.auditMemberChange(ctx, pkgAuditRecord.OrganizationMemberAddedEvent, audit.OrgMemberCreatedEvent, + orgAuditResource(org), org.ID, p, roleID, map[string]string{"role_id": roleID}) +} - s.createAuditRecord(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.OrganizationMemberAddedEvent, - Resource: auditrecord.Resource{ - ID: org.ID, - Type: pkgAuditRecord.OrganizationType, - Name: org.Title, - }, - Target: &auditrecord.Target{ - ID: p.ID, - Type: targetType, - Name: p.Name, - Metadata: meta, - }, - OrgID: org.ID, - OccurredAt: time.Now(), - }) +func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organization.Organization, p principalInfo, roleID string) { + s.auditMemberChange(ctx, pkgAuditRecord.OrganizationMemberRoleChangedEvent, audit.OrgMemberRoleChangedEvent, + orgAuditResource(org), org.ID, p, roleID, map[string]string{"role_id": roleID}) +} - if err := audit.GetAuditor(ctx, org.ID).LogWithAttrs(audit.OrgMemberCreatedEvent, audit.Target{ - ID: p.ID, - Type: p.Type, - }, map[string]string{ - "role_id": roleID, - }); err != nil { - s.log.WarnContext(ctx, "failed to write audit log", "error", err, "event", audit.OrgMemberCreatedEvent) - } +func (s *Service) auditGroupMemberAdded(ctx context.Context, grp group.Group, p principalInfo, roleID string) { + s.auditMemberChange(ctx, pkgAuditRecord.GroupMemberAddedEvent, audit.GroupMemberCreatedEvent, + groupAuditResource(grp), grp.OrganizationID, p, roleID, map[string]string{"role_id": roleID, "group_id": grp.ID}) +} + +func (s *Service) auditGroupMemberRoleChanged(ctx context.Context, grp group.Group, p principalInfo, roleID string) { + s.auditMemberChange(ctx, pkgAuditRecord.GroupMemberRoleChangedEvent, audit.GroupMemberRoleChangedEvent, + groupAuditResource(grp), grp.OrganizationID, p, roleID, map[string]string{"role_id": roleID, "group_id": grp.ID}) +} + +func (s *Service) auditGroupMemberRemoved(ctx context.Context, grp group.Group, p principalInfo) { + s.auditMemberChange(ctx, pkgAuditRecord.GroupMemberRemovedEvent, audit.GroupMemberRemovedEvent, + groupAuditResource(grp), grp.OrganizationID, p, "", map[string]string{"group_id": grp.ID}) } func (s *Service) auditOrgMemberRemoved(ctx context.Context, org organization.Organization, targetID string, targetType pkgAuditRecord.EntityType) { s.createAuditRecord(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.OrganizationMemberRemovedEvent, - Resource: auditrecord.Resource{ - ID: org.ID, - Type: pkgAuditRecord.OrganizationType, - Name: org.Title, - }, + Event: pkgAuditRecord.OrganizationMemberRemovedEvent, + Resource: orgAuditResource(org), Target: &auditrecord.Target{ ID: targetID, Type: targetType, @@ -134,21 +130,6 @@ func (s *Service) auditOrgMemberRemoved(ctx context.Context, org organization.Or }) } -func principalTypeToAuditType(principalType string) (pkgAuditRecord.EntityType, error) { - switch principalType { - case schema.ServiceUserPrincipal: - return pkgAuditRecord.ServiceUserType, nil - case schema.UserPrincipal: - return pkgAuditRecord.UserType, nil - case schema.GroupPrincipal: - return pkgAuditRecord.GroupType, nil - case schema.PATPrincipal: - return pkgAuditRecord.PATType, nil - default: - return "", ErrInvalidPrincipalType - } -} - func (s *Service) auditProjectMember(ctx context.Context, event pkgAuditRecord.Event, prj project.Project, principalID, principalType string, meta map[string]any) { targetType, _ := principalTypeToAuditType(principalType) if meta == nil { @@ -172,106 +153,17 @@ func (s *Service) auditProjectMember(ctx context.Context, event pkgAuditRecord.E }) } -func (s *Service) auditGroupMemberAdded(ctx context.Context, grp group.Group, p principalInfo, roleID string) { - targetType, _ := principalTypeToAuditType(p.Type) - meta := map[string]any{"role_id": roleID} - if p.Email != "" { - meta["email"] = p.Email - } - - s.createAuditRecord(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.GroupMemberAddedEvent, - Resource: auditrecord.Resource{ - ID: grp.ID, - Type: pkgAuditRecord.GroupType, - Name: grp.Title, - }, - Target: &auditrecord.Target{ - ID: p.ID, - Type: targetType, - Name: p.Name, - Metadata: meta, - }, - OrgID: grp.OrganizationID, - OccurredAt: time.Now(), - }) - - if err := audit.GetAuditor(ctx, grp.OrganizationID).LogWithAttrs(audit.GroupMemberCreatedEvent, audit.Target{ - ID: p.ID, - Type: p.Type, - }, map[string]string{ - "role_id": roleID, - "group_id": grp.ID, - }); err != nil { - s.log.WarnContext(ctx, "failed to write audit log", "error", err, "event", audit.GroupMemberCreatedEvent) - } -} - -func (s *Service) auditGroupMemberRoleChanged(ctx context.Context, grp group.Group, p principalInfo, roleID string) { - targetType, _ := principalTypeToAuditType(p.Type) - meta := map[string]any{"role_id": roleID} - if p.Email != "" { - meta["email"] = p.Email - } - - s.createAuditRecord(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.GroupMemberRoleChangedEvent, - Resource: auditrecord.Resource{ - ID: grp.ID, - Type: pkgAuditRecord.GroupType, - Name: grp.Title, - }, - Target: &auditrecord.Target{ - ID: p.ID, - Type: targetType, - Name: p.Name, - Metadata: meta, - }, - OrgID: grp.OrganizationID, - OccurredAt: time.Now(), - }) - - if err := audit.GetAuditor(ctx, grp.OrganizationID).LogWithAttrs(audit.GroupMemberRoleChangedEvent, audit.Target{ - ID: p.ID, - Type: p.Type, - }, map[string]string{ - "role_id": roleID, - "group_id": grp.ID, - }); err != nil { - s.log.WarnContext(ctx, "failed to write audit log", "error", err, "event", audit.GroupMemberRoleChangedEvent) - } -} - -func (s *Service) auditGroupMemberRemoved(ctx context.Context, grp group.Group, p principalInfo) { - targetType, _ := principalTypeToAuditType(p.Type) - meta := map[string]any{} - if p.Email != "" { - meta["email"] = p.Email - } - - s.createAuditRecord(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.GroupMemberRemovedEvent, - Resource: auditrecord.Resource{ - ID: grp.ID, - Type: pkgAuditRecord.GroupType, - Name: grp.Title, - }, - Target: &auditrecord.Target{ - ID: p.ID, - Type: targetType, - Name: p.Name, - Metadata: meta, - }, - OrgID: grp.OrganizationID, - OccurredAt: time.Now(), - }) - - if err := audit.GetAuditor(ctx, grp.OrganizationID).LogWithAttrs(audit.GroupMemberRemovedEvent, audit.Target{ - ID: p.ID, - Type: p.Type, - }, map[string]string{ - "group_id": grp.ID, - }); err != nil { - s.log.WarnContext(ctx, "failed to write audit log", "error", err, "event", audit.GroupMemberRemovedEvent) +func principalTypeToAuditType(principalType string) (pkgAuditRecord.EntityType, error) { + switch principalType { + case schema.ServiceUserPrincipal: + return pkgAuditRecord.ServiceUserType, nil + case schema.UserPrincipal: + return pkgAuditRecord.UserType, nil + case schema.GroupPrincipal: + return pkgAuditRecord.GroupType, nil + case schema.PATPrincipal: + return pkgAuditRecord.PATType, nil + default: + return "", ErrInvalidPrincipalType } } diff --git a/core/membership/group.go b/core/membership/group.go index 614216c98a..227a432662 100644 --- a/core/membership/group.go +++ b/core/membership/group.go @@ -4,13 +4,11 @@ import ( "context" "errors" "fmt" - "slices" "github.com/raystack/frontier/core/policy" "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/internal/bootstrap/schema" - "github.com/raystack/frontier/pkg/utils" ) // removeGroupMemberRelation deletes the member relation for a principal on a group. @@ -83,7 +81,7 @@ func (s *Service) SetGroupMemberRole(ctx context.Context, groupID, principalID, } // change path: skip if the principal already has exactly this role - if len(existing) == 1 && existing[0].RoleID == resolvedRoleID { + if hasExactlyRole(existing, resolvedRoleID) { return nil } @@ -176,11 +174,11 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err // First pass: delete every policy. Track which principals had any // delete failure so we don't strip their SpiceDB relations while a // surviving policy still references them. - principals := make(map[string]policy.Policy, len(policies)) - failed := make(map[string]struct{}, len(policies)) + principals := make(map[principalKey]policy.Policy, len(policies)) + failed := make(map[principalKey]struct{}, len(policies)) var errs error for _, p := range policies { - key := p.PrincipalType + "\x00" + p.PrincipalID + key := policyPrincipalKey(p) principals[key] = p if delErr := s.policyService.Delete(ctx, p.ID); delErr != nil { failed[key] = struct{}{} @@ -311,20 +309,7 @@ func (s *Service) unlinkGroupFromOrg(ctx context.Context, groupID, orgID string) // - a platform-wide role scoped to groups, or // - a custom role created for the group's parent organization. func (s *Service) validateGroupRole(ctx context.Context, roleID, orgID string) (role.Role, error) { - fetchedRole, err := s.roleService.Get(ctx, roleID) - if err != nil { - return role.Role{}, err - } - if !slices.Contains(fetchedRole.Scopes, schema.GroupNamespace) { - return role.Role{}, ErrInvalidGroupRole - } - if fetchedRole.OrgID == orgID { - return fetchedRole, nil - } - if utils.IsNullUUID(fetchedRole.OrgID) { - return fetchedRole, nil - } - return role.Role{}, ErrInvalidGroupRole + return s.validateRoleForScope(ctx, roleID, orgID, schema.GroupNamespace, ErrInvalidGroupRole) } // validateMinGroupOwnerConstraint ensures the group keeps at least one owner @@ -332,35 +317,5 @@ func (s *Service) validateGroupRole(ctx context.Context, roleID, orgID string) ( // caller can hand it to replacePolicy as a min-role guard, closing the TOCTOU // race between this pre-check and the policy delete. func (s *Service) validateMinGroupOwnerConstraint(ctx context.Context, groupID, newRoleID string, existing []policy.Policy) (string, error) { - ownerRole, err := s.roleService.Get(ctx, schema.GroupOwnerRole) - if err != nil { - return "", fmt.Errorf("get group owner role: %w", err) - } - - if newRoleID == ownerRole.ID { - return ownerRole.ID, nil - } - - isCurrentlyOwner := false - for _, p := range existing { - if p.RoleID == ownerRole.ID { - isCurrentlyOwner = true - break - } - } - if !isCurrentlyOwner { - return ownerRole.ID, nil - } - - ownerPolicies, err := s.policyService.List(ctx, policy.Filter{ - GroupID: groupID, - RoleID: ownerRole.ID, - }) - if err != nil { - return "", fmt.Errorf("list group owner policies: %w", err) - } - if len(ownerPolicies) <= 1 { - return "", ErrLastGroupOwnerRole - } - return ownerRole.ID, nil + return s.validateMinRoleConstraint(ctx, schema.GroupOwnerRole, policy.Filter{GroupID: groupID}, newRoleID, existing, ErrLastGroupOwnerRole) } diff --git a/core/membership/list.go b/core/membership/list.go index c0326de930..5bbf0da4fa 100644 --- a/core/membership/list.go +++ b/core/membership/list.go @@ -39,25 +39,14 @@ type Member struct { Roles []role.Role } -// resourcePolicyFilter builds the policy filter that scopes a listing to the -// given resource. Returns ErrInvalidResourceType for unsupported namespaces. -func resourcePolicyFilter(resourceID, resourceType string, filter MemberFilter) (policy.Filter, error) { - flt := policy.Filter{ - PrincipalType: filter.PrincipalType, - RoleIDs: filter.RoleIDs, - ResourceType: resourceType, - } - switch resourceType { - case schema.OrganizationNamespace: - flt.OrgID = resourceID - case schema.ProjectNamespace: - flt.ProjectID = resourceID - case schema.GroupNamespace: - flt.GroupID = resourceID - default: - return policy.Filter{}, ErrInvalidResourceType - } - return flt, nil +// principalKey identifies a principal across policy rows. +type principalKey struct { + Type string + ID string +} + +func policyPrincipalKey(pol policy.Policy) principalKey { + return principalKey{Type: pol.PrincipalType, ID: pol.PrincipalID} } // ListPrincipalsByResource returns the principals (users, service users, groups) @@ -65,10 +54,12 @@ func resourcePolicyFilter(resourceID, resourceType string, filter MemberFilter) // principal type and/or role, and optionally enriched with the full list of // roles each principal holds on the resource. func (s *Service) ListPrincipalsByResource(ctx context.Context, resourceID, resourceType string, filter MemberFilter) ([]Member, error) { - flt, err := resourcePolicyFilter(resourceID, resourceType, filter) + flt, err := resourcePolicyFilter(resourceID, resourceType) if err != nil { return nil, err } + flt.PrincipalType = filter.PrincipalType + flt.RoleIDs = filter.RoleIDs policies, err := s.policyService.List(ctx, flt) if err != nil { @@ -77,10 +68,10 @@ func (s *Service) ListPrincipalsByResource(ctx context.Context, resourceID, reso policies = excludePATAllProjects(policies, resourceType) // deduplicate by (principalID, principalType) preserving order - memberIndex := make(map[string]int, len(policies)) + memberIndex := make(map[principalKey]int, len(policies)) members := make([]Member, 0, len(policies)) for _, pol := range policies { - key := pol.PrincipalType + "\x00" + pol.PrincipalID + key := policyPrincipalKey(pol) if _, ok := memberIndex[key]; ok { continue } @@ -91,24 +82,34 @@ func (s *Service) ListPrincipalsByResource(ctx context.Context, resourceID, reso }) } + if err := s.enrichMemberRoles(ctx, flt, resourceType, memberIndex, members); err != nil { + return nil, err + } + return members, nil +} + +// enrichMemberRoles fills each member's Roles with every role the principal +// holds on the resource, ignoring the role filter used to select the members. +// flt is the resource-scoped filter the members were listed with (taken by +// value — the role filter is stripped on the local copy). +func (s *Service) enrichMemberRoles(ctx context.Context, flt policy.Filter, resourceType string, memberIndex map[principalKey]int, members []Member) error { // fetch all policies for the resource (without role filtering) to get // the complete set of roles per principal in a single query - roleFlt := flt - roleFlt.RoleIDs = nil - allPolicies, err := s.policyService.List(ctx, roleFlt) + flt.RoleIDs = nil + allPolicies, err := s.policyService.List(ctx, flt) if err != nil { - return nil, fmt.Errorf("list policies for role enrichment: %w", err) + return fmt.Errorf("list policies for role enrichment: %w", err) } allPolicies = excludePATAllProjects(allPolicies, resourceType) - principalRoleIDs := make(map[string][]string, len(members)) - roleSeen := make(map[string]map[string]struct{}, len(members)) + principalRoleIDs := make(map[principalKey][]string, len(members)) + roleSeen := make(map[principalKey]map[string]struct{}, len(members)) uniqueRoleIDs := make(map[string]struct{}) for _, pol := range allPolicies { if pol.RoleID == "" { continue } - key := pol.PrincipalType + "\x00" + pol.PrincipalID + key := policyPrincipalKey(pol) if _, ok := memberIndex[key]; !ok { continue } @@ -122,32 +123,32 @@ func (s *Service) ListPrincipalsByResource(ctx context.Context, resourceID, reso principalRoleIDs[key] = append(principalRoleIDs[key], pol.RoleID) uniqueRoleIDs[pol.RoleID] = struct{}{} } + if len(uniqueRoleIDs) == 0 { + return nil + } - if len(uniqueRoleIDs) > 0 { - ids := make([]string, 0, len(uniqueRoleIDs)) - for id := range uniqueRoleIDs { - ids = append(ids, id) - } - roles, err := s.roleService.List(ctx, role.Filter{IDs: ids}) - if err != nil { - return nil, fmt.Errorf("list roles: %w", err) - } - roleByID := make(map[string]role.Role, len(roles)) - for _, r := range roles { - roleByID[r.ID] = r - } - for key, idx := range memberIndex { - memberRoles := make([]role.Role, 0, len(principalRoleIDs[key])) - for _, rid := range principalRoleIDs[key] { - if r, ok := roleByID[rid]; ok { - memberRoles = append(memberRoles, r) - } + ids := make([]string, 0, len(uniqueRoleIDs)) + for id := range uniqueRoleIDs { + ids = append(ids, id) + } + roles, err := s.roleService.List(ctx, role.Filter{IDs: ids}) + if err != nil { + return fmt.Errorf("list roles: %w", err) + } + roleByID := make(map[string]role.Role, len(roles)) + for _, r := range roles { + roleByID[r.ID] = r + } + for key, idx := range memberIndex { + memberRoles := make([]role.Role, 0, len(principalRoleIDs[key])) + for _, rid := range principalRoleIDs[key] { + if r, ok := roleByID[rid]; ok { + memberRoles = append(memberRoles, r) } - members[idx].Roles = memberRoles } + members[idx].Roles = memberRoles } - - return members, nil + return nil } // ListPrincipalIDsByResource returns the IDs of principals of the given type @@ -157,10 +158,11 @@ func (s *Service) ListPrincipalsByResource(ctx context.Context, resourceID, reso // cannot import membership types without creating an import cycle // (e.g. core/serviceuser, which this package itself imports). func (s *Service) ListPrincipalIDsByResource(ctx context.Context, resourceID, resourceType, principalType string) ([]string, error) { - flt, err := resourcePolicyFilter(resourceID, resourceType, MemberFilter{PrincipalType: principalType}) + flt, err := resourcePolicyFilter(resourceID, resourceType) if err != nil { return nil, err } + flt.PrincipalType = principalType policies, err := s.policyService.List(ctx, flt) if err != nil { @@ -257,11 +259,7 @@ func (s *Service) listOrgsForPrincipal(ctx context.Context, principalID, princip if err != nil { return nil, fmt.Errorf("list org policies: %w", err) } - ids := make([]string, 0, len(policies)) - for _, pol := range policies { - ids = append(ids, pol.ResourceID) - } - return utils.Deduplicate(ids), nil + return policyResourceIDs(policies), nil } // listGroupsForPrincipal returns every group the principal has a policy on. @@ -275,11 +273,7 @@ func (s *Service) listGroupsForPrincipal(ctx context.Context, principalID, princ if err != nil { return nil, fmt.Errorf("list group policies: %w", err) } - ids := make([]string, 0, len(policies)) - for _, pol := range policies { - ids = append(ids, pol.ResourceID) - } - ids = utils.Deduplicate(ids) + ids := policyResourceIDs(policies) if filter.OrgID != "" && len(ids) > 0 { ids, err = s.narrowGroupsByOrg(ctx, ids, filter.OrgID) diff --git a/core/membership/org.go b/core/membership/org.go index 698298de40..9e547859e2 100644 --- a/core/membership/org.go +++ b/core/membership/org.go @@ -1,11 +1,9 @@ package membership import ( + "context" "errors" "fmt" - "slices" - - "context" "github.com/raystack/frontier/core/audit" "github.com/raystack/frontier/core/group" @@ -15,7 +13,6 @@ import ( "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/internal/bootstrap/schema" - "github.com/raystack/frontier/pkg/utils" ) // AddOrganizationMember adds a principal (user, service user, or PAT) to an organization @@ -115,7 +112,7 @@ func (s *Service) SetOrganizationMemberRole(ctx context.Context, orgID, principa } // skip if the user already has exactly this role - if len(existing) == 1 && existing[0].RoleID == resolvedRoleID { + if hasExactlyRole(existing, resolvedRoleID) { return nil } @@ -172,7 +169,7 @@ func (s *Service) SetPATAllProjectsRole(ctx context.Context, orgID, patID, roleI } } - if len(existing) == 1 && existing[0].RoleID == resolvedRoleID { + if hasExactlyRole(existing, resolvedRoleID) { return nil } @@ -405,40 +402,7 @@ func (s *Service) removeCustomResourceAccess(ctx context.Context, principalID, p // 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) { - ownerRole, err := s.roleService.Get(ctx, schema.RoleOrganizationOwner) - if err != nil { - return "", fmt.Errorf("get owner role: %w", err) - } - - // no constraint if promoting to owner - if newRoleID == ownerRole.ID { - return ownerRole.ID, nil - } - - // no constraint if user is not currently an owner - isCurrentlyOwner := false - for _, p := range existing { - if p.RoleID == ownerRole.ID { - isCurrentlyOwner = true - break - } - } - if !isCurrentlyOwner { - return ownerRole.ID, nil - } - - // user is owner, being demoted — make sure at least one other owner remains - ownerPolicies, err := s.policyService.List(ctx, policy.Filter{ - OrgID: orgID, - RoleID: ownerRole.ID, - }) - if err != nil { - return "", fmt.Errorf("list owner policies: %w", err) - } - if len(ownerPolicies) <= 1 { - return "", ErrLastOwnerRole - } - return ownerRole.ID, nil + return s.validateMinRoleConstraint(ctx, schema.RoleOrganizationOwner, policy.Filter{OrgID: orgID}, newRoleID, existing, ErrLastOwnerRole) } // validateOrgRole checks that the role is valid for organization scope and returns it. @@ -446,25 +410,5 @@ func (s *Service) validateMinOwnerConstraint(ctx context.Context, orgID, newRole // - a platform-wide role scoped to organizations, or // - a custom role created for this specific organization. func (s *Service) validateOrgRole(ctx context.Context, roleID, orgID string) (role.Role, error) { - fetchedRole, err := s.roleService.Get(ctx, roleID) - if err != nil { - return role.Role{}, err - } - - // role must be scoped to organization regardless of whether it's platform-wide or org-specific - if !slices.Contains(fetchedRole.Scopes, schema.OrganizationNamespace) { - return role.Role{}, ErrInvalidOrgRole - } - - // custom role belonging to this org - if fetchedRole.OrgID == orgID { - return fetchedRole, nil - } - - // platform-wide role (no org ownership) - if utils.IsNullUUID(fetchedRole.OrgID) { - return fetchedRole, nil - } - - return role.Role{}, ErrInvalidOrgRole + return s.validateRoleForScope(ctx, roleID, orgID, schema.OrganizationNamespace, ErrInvalidOrgRole) } diff --git a/core/membership/project.go b/core/membership/project.go index b222daaceb..2be87ff947 100644 --- a/core/membership/project.go +++ b/core/membership/project.go @@ -4,14 +4,12 @@ import ( "context" "errors" "fmt" - "slices" "github.com/raystack/frontier/core/policy" "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/internal/bootstrap/schema" pkgAuditRecord "github.com/raystack/frontier/pkg/auditrecord" - "github.com/raystack/frontier/pkg/utils" ) // SetProjectMemberRole sets or changes a principal's role in a project (upsert). @@ -43,7 +41,7 @@ func (s *Service) SetProjectMemberRole(ctx context.Context, projectID, principal } // skip if the principal already has exactly this role - if len(existing) == 1 && existing[0].RoleID == resolvedRoleID { + if hasExactlyRole(existing, resolvedRoleID) { return nil } @@ -131,23 +129,5 @@ func (s *Service) unlinkProjectFromOrg(ctx context.Context, projectID, orgID str // - a platform-wide role scoped to projects, or // - a custom role created for the project's parent organization. func (s *Service) validateProjectRole(ctx context.Context, roleID, orgID string) (role.Role, error) { - fetchedRole, err := s.roleService.Get(ctx, roleID) - if err != nil { - return role.Role{}, err - } - if !slices.Contains(fetchedRole.Scopes, schema.ProjectNamespace) { - return role.Role{}, ErrInvalidProjectRole - } - - // custom role belonging to the project's parent org - if fetchedRole.OrgID == orgID { - return fetchedRole, nil - } - - // platform-wide role (no org ownership) - if utils.IsNullUUID(fetchedRole.OrgID) { - return fetchedRole, nil - } - - return role.Role{}, ErrInvalidProjectRole + return s.validateRoleForScope(ctx, roleID, orgID, schema.ProjectNamespace, ErrInvalidProjectRole) } diff --git a/core/membership/project_test.go b/core/membership/project_test.go index 1d8cd7a345..767789e12a 100644 --- a/core/membership/project_test.go +++ b/core/membership/project_test.go @@ -188,7 +188,7 @@ func TestService_RemoveProjectMember(t *testing.T) { name: "should return error if not a member", setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, _ *mocks.AuditRecordRepository) { prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil) - policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{}, nil) }, principalID: userID, principalType: schema.UserPrincipal, @@ -198,7 +198,7 @@ func TestService_RemoveProjectMember(t *testing.T) { name: "should succeed removing a user", setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) { prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil) - policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "p1"}}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil) policySvc.EXPECT().Delete(ctx, "p1").Return(nil) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, @@ -209,7 +209,7 @@ func TestService_RemoveProjectMember(t *testing.T) { name: "should succeed removing a service user", setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) { prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil) - policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal}).Return([]policy.Policy{{ID: "p1"}}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil) policySvc.EXPECT().Delete(ctx, "p1").Return(nil) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, @@ -220,7 +220,7 @@ func TestService_RemoveProjectMember(t *testing.T) { name: "should succeed removing a PAT", setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) { prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil) - policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.PATPrincipal}).Return([]policy.Policy{{ID: "p1"}}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.PATPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil) policySvc.EXPECT().Delete(ctx, "p1").Return(nil) auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, diff --git a/core/membership/service.go b/core/membership/service.go index 279848aa43..5078f547f4 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "slices" "github.com/raystack/frontier/core/auditrecord" "github.com/raystack/frontier/core/group" @@ -17,6 +18,7 @@ import ( "github.com/raystack/frontier/core/user" patmodels "github.com/raystack/frontier/core/userpat/models" "github.com/raystack/frontier/internal/bootstrap/schema" + "github.com/raystack/frontier/pkg/utils" ) type PolicyService interface { @@ -210,34 +212,102 @@ func (s *Service) createRelation(ctx context.Context, resourceID, resourceType, // removeAllPolicies finds and deletes all policies for a principal on a resource. // Returns the number of policies deleted. func (s *Service) removeAllPolicies(ctx context.Context, resourceID, resourceType, principalID, principalType string) (int, error) { - f := policyFilterForResource(resourceID, resourceType, principalID, principalType) - existing, err := s.policyService.List(ctx, f) + flt, err := resourcePolicyFilter(resourceID, resourceType) if err != nil { - return 0, fmt.Errorf("list policies: %w", err) - } - for _, pol := range existing { - if err := s.policyService.Delete(ctx, pol.ID); err != nil { - return 0, fmt.Errorf("delete policy %s: %w", pol.ID, err) - } + return 0, err } - return len(existing), nil + flt.PrincipalID = principalID + flt.PrincipalType = principalType + return s.removePoliciesByFilter(ctx, flt) } -// policyFilterForResource builds a policy.Filter with the correct resource-type field set. -func policyFilterForResource(resourceID, resourceType, principalID, principalType string) policy.Filter { - f := policy.Filter{ - PrincipalID: principalID, - PrincipalType: principalType, - } +// resourcePolicyFilter builds a policy.Filter scoped to the given resource, +// with the correct per-type ID field set. Returns ErrInvalidResourceType for +// unsupported namespaces. Callers add principal or role fields on top. +func resourcePolicyFilter(resourceID, resourceType string) (policy.Filter, error) { + flt := policy.Filter{ResourceType: resourceType} switch resourceType { case schema.OrganizationNamespace: - f.OrgID = resourceID + flt.OrgID = resourceID case schema.ProjectNamespace: - f.ProjectID = resourceID + flt.ProjectID = resourceID case schema.GroupNamespace: - f.GroupID = resourceID + flt.GroupID = resourceID + default: + return policy.Filter{}, ErrInvalidResourceType + } + return flt, nil +} + +// hasExactlyRole reports whether the existing policies are a single policy +// holding the given role — the "nothing to change" case for role upserts. +func hasExactlyRole(existing []policy.Policy, roleID string) bool { + return len(existing) == 1 && existing[0].RoleID == roleID +} + +// validateRoleForScope checks that the role can be assigned on the given +// namespace and returns it. A role qualifies if it is either: +// - a platform-wide role (no org ownership) scoped to the namespace, or +// - a custom role created for the given organization. +// +// errInvalid is returned for any role that doesn't qualify. +func (s *Service) validateRoleForScope(ctx context.Context, roleID, orgID, namespace string, errInvalid error) (role.Role, error) { + fetchedRole, err := s.roleService.Get(ctx, roleID) + if err != nil { + return role.Role{}, err + } + if !slices.Contains(fetchedRole.Scopes, namespace) { + return role.Role{}, errInvalid + } + + // custom role belonging to this org + if fetchedRole.OrgID == orgID { + return fetchedRole, nil + } + + // platform-wide role (no org ownership) + if utils.IsNullUUID(fetchedRole.OrgID) { + return fetchedRole, nil + } + + return role.Role{}, errInvalid +} + +// validateMinRoleConstraint ensures at least one holder of guardRoleName +// remains on the resource after demoting or removing one. resourceFilter +// scopes the holder count to the resource (the role ID is filled in here); +// errLast is returned when the principal is the last holder. Returns the +// resolved guard role ID for reuse as an atomic delete guard. +func (s *Service) validateMinRoleConstraint(ctx context.Context, guardRoleName string, resourceFilter policy.Filter, newRoleID string, existing []policy.Policy, errLast error) (string, error) { + guardRole, err := s.roleService.Get(ctx, guardRoleName) + if err != nil { + return "", fmt.Errorf("get owner role: %w", err) + } + + // no constraint if promoting to the guarded role + if newRoleID == guardRole.ID { + return guardRole.ID, nil + } + + // no constraint if the principal doesn't currently hold it + holdsGuardRole := slices.ContainsFunc(existing, func(p policy.Policy) bool { + return p.RoleID == guardRole.ID + }) + if !holdsGuardRole { + return guardRole.ID, nil + } + + // principal holds the guarded role and is losing it — make sure at least + // one other holder remains + resourceFilter.RoleID = guardRole.ID + holderPolicies, err := s.policyService.List(ctx, resourceFilter) + if err != nil { + return "", fmt.Errorf("list owner policies: %w", err) + } + if len(holderPolicies) <= 1 { + return "", errLast } - return f + return guardRole.ID, nil } // excludePATAllProjects hides a PAT's all-projects grant from org member From f60c87982065a68828b26bdb551aefc873f1506b Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 31 Jul 2026 12:15:10 +0530 Subject: [PATCH 2/3] refactor(membership): skip duplicate policy query when no role filter is set Review follow-up (CodeRabbit): with no role filter, the enrichment query in ListPrincipalsByResource was identical to the member query, costing a second round trip on the common path. Reuse the first result; only a role-filtered listing issues the second, unfiltered query. enrichMemberRoles now takes the policy set instead of fetching it. Co-Authored-By: Claude Fable 5 --- core/membership/list.go | 31 +++++++++++++++++-------------- core/membership/list_test.go | 8 ++++---- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/core/membership/list.go b/core/membership/list.go index 5bbf0da4fa..26893ddf9a 100644 --- a/core/membership/list.go +++ b/core/membership/list.go @@ -82,26 +82,29 @@ func (s *Service) ListPrincipalsByResource(ctx context.Context, resourceID, reso }) } - if err := s.enrichMemberRoles(ctx, flt, resourceType, memberIndex, members); err != nil { + // role enrichment needs every policy on the resource, not just the ones + // matching the role filter. Without a role filter the first query already + // returned exactly that set — reuse it instead of querying again. + allPolicies := policies + if len(filter.RoleIDs) > 0 { + flt.RoleIDs = nil + allPolicies, err = s.policyService.List(ctx, flt) + if err != nil { + return nil, fmt.Errorf("list policies for role enrichment: %w", err) + } + allPolicies = excludePATAllProjects(allPolicies, resourceType) + } + + if err := s.enrichMemberRoles(ctx, allPolicies, memberIndex, members); err != nil { return nil, err } return members, nil } // enrichMemberRoles fills each member's Roles with every role the principal -// holds on the resource, ignoring the role filter used to select the members. -// flt is the resource-scoped filter the members were listed with (taken by -// value — the role filter is stripped on the local copy). -func (s *Service) enrichMemberRoles(ctx context.Context, flt policy.Filter, resourceType string, memberIndex map[principalKey]int, members []Member) error { - // fetch all policies for the resource (without role filtering) to get - // the complete set of roles per principal in a single query - flt.RoleIDs = nil - allPolicies, err := s.policyService.List(ctx, flt) - if err != nil { - return fmt.Errorf("list policies for role enrichment: %w", err) - } - allPolicies = excludePATAllProjects(allPolicies, resourceType) - +// holds on the resource. allPolicies must be the resource's full policy set +// (no role filter), already cleaned of PAT all-projects grants. +func (s *Service) enrichMemberRoles(ctx context.Context, allPolicies []policy.Policy, memberIndex map[principalKey]int, members []Member) error { principalRoleIDs := make(map[principalKey][]string, len(members)) roleSeen := make(map[principalKey]map[string]struct{}, len(members)) uniqueRoleIDs := make(map[string]struct{}) diff --git a/core/membership/list_test.go b/core/membership/list_test.go index 1ce8f0878f..9abc17e605 100644 --- a/core/membership/list_test.go +++ b/core/membership/list_test.go @@ -105,7 +105,7 @@ func TestService_ListPrincipalsByResource(t *testing.T) { OrgID: orgID, PrincipalType: schema.UserPrincipal, ResourceType: schema.OrganizationNamespace, - }).Return(orgPolicies, nil).Times(2) + }).Return(orgPolicies, nil).Once() rs.EXPECT().List(ctx, mock.MatchedBy(func(f role.Filter) bool { return len(f.IDs) == 2 })).Return([]role.Role{viewerRole, ownerRole}, nil) @@ -159,7 +159,7 @@ func TestService_ListPrincipalsByResource(t *testing.T) { ProjectID: projectID, PrincipalType: schema.UserPrincipal, ResourceType: schema.ProjectNamespace, - }).Return(projectPolicies, nil).Times(2) + }).Return(projectPolicies, nil).Once() rs.EXPECT().List(ctx, mock.MatchedBy(func(f role.Filter) bool { return len(f.IDs) == 2 })).Return([]role.Role{viewerRole, ownerRole}, nil) @@ -181,7 +181,7 @@ func TestService_ListPrincipalsByResource(t *testing.T) { ProjectID: projectID, PrincipalType: schema.ServiceUserPrincipal, ResourceType: schema.ProjectNamespace, - }).Return(suPolicies, nil).Times(2) + }).Return(suPolicies, nil).Once() rs.EXPECT().List(ctx, role.Filter{IDs: []string{roleViewerID}}).Return([]role.Role{viewerRole}, nil) }, want: []membership.Member{ @@ -201,7 +201,7 @@ func TestService_ListPrincipalsByResource(t *testing.T) { GroupID: groupID, PrincipalType: schema.UserPrincipal, ResourceType: schema.GroupNamespace, - }).Return(groupPolicies, nil).Times(2) + }).Return(groupPolicies, nil).Once() rs.EXPECT().List(ctx, role.Filter{IDs: []string{roleViewerID}}).Return([]role.Role{viewerRole}, nil) }, want: []membership.Member{ From 423e594e6e58f5e74e9b9e2fbf344b4f957959d9 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Fri, 31 Jul 2026 15:05:35 +0530 Subject: [PATCH 3/3] refactor(membership): address review nits - validateMinRoleConstraint wraps its errors with the guard role name, so org and group owner lookups stay distinguishable in logs - RemoveAllGroupMembers tracks principals in a set; the relation pass reads the identity from the key instead of an arbitrary policy Co-Authored-By: Claude Fable 5 --- core/membership/group.go | 10 +++++----- core/membership/service.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/membership/group.go b/core/membership/group.go index 227a432662..a1e025c088 100644 --- a/core/membership/group.go +++ b/core/membership/group.go @@ -174,12 +174,12 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err // First pass: delete every policy. Track which principals had any // delete failure so we don't strip their SpiceDB relations while a // surviving policy still references them. - principals := make(map[principalKey]policy.Policy, len(policies)) + principals := make(map[principalKey]struct{}, len(policies)) failed := make(map[principalKey]struct{}, len(policies)) var errs error for _, p := range policies { key := policyPrincipalKey(p) - principals[key] = p + principals[key] = struct{}{} if delErr := s.policyService.Delete(ctx, p.ID); delErr != nil { failed[key] = struct{}{} errs = errors.Join(errs, fmt.Errorf("delete policy %s: %w", p.ID, delErr)) @@ -189,12 +189,12 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err // Second pass: clean up direct relations only for principals whose // policies were all deleted successfully. The rest get retried on the // next attempt once their lingering policies are removed. - for key, p := range principals { + for key := range principals { if _, hadFailure := failed[key]; hadFailure { continue } - 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)) + if relErr := s.removeGroupMemberRelation(ctx, groupID, key.ID, key.Type); relErr != nil { + errs = errors.Join(errs, fmt.Errorf("remove relations for %s:%s: %w", key.Type, key.ID, relErr)) } } diff --git a/core/membership/service.go b/core/membership/service.go index 5078f547f4..d1c3b941d0 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -281,7 +281,7 @@ func (s *Service) validateRoleForScope(ctx context.Context, roleID, orgID, names func (s *Service) validateMinRoleConstraint(ctx context.Context, guardRoleName string, resourceFilter policy.Filter, newRoleID string, existing []policy.Policy, errLast error) (string, error) { guardRole, err := s.roleService.Get(ctx, guardRoleName) if err != nil { - return "", fmt.Errorf("get owner role: %w", err) + return "", fmt.Errorf("get role %s: %w", guardRoleName, err) } // no constraint if promoting to the guarded role @@ -302,7 +302,7 @@ func (s *Service) validateMinRoleConstraint(ctx context.Context, guardRoleName s resourceFilter.RoleID = guardRole.ID holderPolicies, err := s.policyService.List(ctx, resourceFilter) if err != nil { - return "", fmt.Errorf("list owner policies: %w", err) + return "", fmt.Errorf("list policies for role %s: %w", guardRoleName, err) } if len(holderPolicies) <= 1 { return "", errLast