Skip to content

refactor(membership): deduplicate repeated patterns #1827

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 4 commits into from
Jul 31, 2026
Merged

refactor(membership): deduplicate repeated patterns #1827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cc7cae
refactor(membership): deduplicate repeated patterns
whoAbhishekSah Jul 30, 2026
f60c879
refactor(membership): skip duplicate policy query when no role filter…
whoAbhishekSah Jul 31, 2026
423e594
refactor(membership): address review nits
whoAbhishekSah Jul 31, 2026
6176c6f
Merge branch 'main' into refactor/membership-dedupe
whoAbhishekSah Jul 31, 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
222 changes: 57 additions & 165 deletions core/membership/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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
}
}
65 changes: 10 additions & 55 deletions core/membership/group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -176,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[string]policy.Policy, len(policies))
failed := make(map[string]struct{}, len(policies))
principals := make(map[principalKey]struct{}, len(policies))
failed := make(map[principalKey]struct{}, len(policies))
var errs error
for _, p := range policies {
key := p.PrincipalType + "\x00" + p.PrincipalID
principals[key] = p
key := policyPrincipalKey(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))
Expand All @@ -191,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))
}
}

Expand Down Expand Up @@ -311,56 +309,13 @@ 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
// after the role change. Returns the resolved group owner role ID so the
// 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)
}
Loading
Loading