Skip to content

feat: add service user support to membership validatePrincipal #1562

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 1 commit into from
Apr 22, 2026
Merged

feat: add service user support to membership validatePrincipal #1562

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
3 changes: 2 additions & 1 deletion core/membership/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ var (
ErrNotMember = errors.New("principal is not a member of this resource")
ErrInvalidOrgRole = errors.New("role is not valid for organization scope")
ErrLastOwnerRole = errors.New("cannot change role: this is the last owner of the organization")
ErrInvalidPrincipal = errors.New("only user principals are supported")
ErrInvalidPrincipal = errors.New("invalid principal")
ErrPrincipalNotInOrg = errors.New("principal does not belong to this organization")
ErrInvalidPrincipalType = errors.New("unsupported principal type")
Comment thread
whoAbhishekSah marked this conversation as resolved.
ErrNotOrgMember = errors.New("principal is not a member of the organization")
ErrInvalidProjectRole = errors.New("role is not valid for project scope")
Expand Down
78 changes: 47 additions & 31 deletions core/membership/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,17 @@ func NewService(
}
}

// AddOrganizationMember adds a new user to an organization with an explicit role,
// bypassing the invitation flow. Only user principals are supported — this is a
// direct-add operation for superadmins.
// Returns ErrAlreadyMember if the user already has a policy on this org.
// AddOrganizationMember adds a principal (user or service user) to an organization
// with an explicit role, bypassing the invitation flow.
// Returns ErrAlreadyMember if the principal already has a policy on this org.
func (s *Service) AddOrganizationMember(ctx context.Context, orgID, principalID, principalType, roleID string) error {
// orgService.Get returns ErrDisabled for disabled orgs
org, err := s.orgService.Get(ctx, orgID)
if err != nil {
return err
}

principal, err := s.validatePrincipal(ctx, principalID, principalType)
principal, err := s.validatePrincipal(ctx, orgID, principalID, principalType)
if err != nil {
return err
}
Expand Down Expand Up @@ -163,16 +162,15 @@ func (s *Service) AddOrganizationMember(ctx context.Context, orgID, principalID,
}

// SetOrganizationMemberRole changes an existing member's role in an organization.
// SetOrganizationMemberRole skips the write if the member already has exactly the requested role.
// Currently only user principals are supported. May be extended to service users
// in the future to give them org-level roles (see #1544).
// Supports user and service user principals.
// Skips the write if the member already has exactly the requested role.
func (s *Service) SetOrganizationMemberRole(ctx context.Context, orgID, principalID, principalType, roleID string) error {
org, err := s.orgService.Get(ctx, orgID)
if err != nil {
return err
}

principal, err := s.validatePrincipal(ctx, principalID, principalType)
principal, err := s.validatePrincipal(ctx, orgID, principalID, principalType)
if err != nil {
return err
}
Comment thread
whoAbhishekSah marked this conversation as resolved.
Expand Down Expand Up @@ -512,10 +510,10 @@ type principalInfo struct {
Email string
}

// validatePrincipal checks that the principal exists and is active.
// To add support for a new principal type (e.g., service user), add a case here
// and add the corresponding service dependency to the Service struct.
func (s *Service) validatePrincipal(ctx context.Context, principalID, principalType string) (principalInfo, error) {
// validatePrincipal checks that the principal exists, is active, and belongs to
// the target org. For users, org membership is checked separately via policies.
// For service users, org ownership is validated here since they have a fixed OrgID.
func (s *Service) validatePrincipal(ctx context.Context, orgID, principalID, principalType string) (principalInfo, error) {
switch principalType {
case schema.UserPrincipal:
usr, err := s.userService.GetByID(ctx, principalID)
Expand All @@ -531,10 +529,22 @@ func (s *Service) validatePrincipal(ctx context.Context, principalID, principalT
Name: usr.Title,
Email: usr.Email,
}, nil
// To support service users in the future, add:
// case schema.ServiceUserPrincipal:
// su, err := s.serviceUserService.Get(ctx, principalID)
// ...
case schema.ServiceUserPrincipal:
su, err := s.serviceuserService.Get(ctx, principalID)
if err != nil {
return principalInfo{}, err
}
if su.OrgID != orgID {
return principalInfo{}, ErrPrincipalNotInOrg
}
Comment thread
whoAbhishekSah marked this conversation as resolved.
if su.State == string(serviceuser.Disabled) {
return principalInfo{}, serviceuser.ErrDisabled
}
return principalInfo{
ID: su.ID,
Type: schema.ServiceUserPrincipal,
Name: su.Title,
}, nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
whoAbhishekSah marked this conversation as resolved.
Comment thread
whoAbhishekSah marked this conversation as resolved.
default:
return principalInfo{}, ErrInvalidPrincipal
}
Expand Down Expand Up @@ -575,6 +585,12 @@ func (s *Service) createRelation(ctx context.Context, resourceID, resourceType,
}

func (s *Service) auditOrgMemberRoleChanged(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.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{
Event: pkgAuditRecord.OrganizationMemberRoleChangedEvent,
Resource: auditrecord.Resource{
Expand All @@ -583,13 +599,10 @@ func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organizatio
Name: org.Title,
},
Target: &auditrecord.Target{
ID: p.ID,
Type: pkgAuditRecord.UserType,
Name: p.Name,
Metadata: map[string]any{
"email": p.Email,
"role_id": roleID,
},
ID: p.ID,
Type: targetType,
Name: p.Name,
Metadata: meta,
},
OrgID: org.ID,
OccurredAt: time.Now(),
Expand All @@ -604,6 +617,12 @@ func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organizatio
}

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.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{
Event: pkgAuditRecord.OrganizationMemberAddedEvent,
Resource: auditrecord.Resource{
Expand All @@ -612,13 +631,10 @@ func (s *Service) auditOrgMemberAdded(ctx context.Context, org organization.Orga
Name: org.Title,
},
Target: &auditrecord.Target{
ID: p.ID,
Type: pkgAuditRecord.UserType,
Name: p.Name,
Metadata: map[string]any{
"email": p.Email,
"role_id": roleID,
},
ID: p.ID,
Type: targetType,
Name: p.Name,
Metadata: meta,
},
OrgID: org.ID,
OccurredAt: time.Now(),
Expand Down
121 changes: 117 additions & 4 deletions core/membership/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ func TestService_AddOrganizationMember(t *testing.T) {
wantErrContain string
}{
{
name: "should return error if principal type is not user",
name: "should return error if principal type is unsupported",
setup: func(_ *mocks.PolicyService, _ *mocks.RelationService, _ *mocks.RoleService, orgSvc *mocks.OrgService, _ *mocks.UserService, _ *mocks.AuditRecordRepository) {
orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
},
orgID: orgID,
userID: userID,
principalType: schema.ServiceUserPrincipal,
principalType: "app/unknown",
roleID: viewerRoleID,
wantErr: membership.ErrInvalidPrincipal,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
Expand Down Expand Up @@ -276,6 +276,60 @@ func TestService_AddOrganizationMember(t *testing.T) {
}
}

func TestService_AddOrganizationMember_ServiceUser(t *testing.T) {
ctx := context.Background()
orgID := uuid.New().String()
suID := uuid.New().String()
viewerRoleID := uuid.New().String()

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

t.Run("should succeed adding a service user", func(t *testing.T) {
mockPolicySvc := mocks.NewPolicyService(t)
mockRelSvc := mocks.NewRelationService(t)
mockRoleSvc := mocks.NewRoleService(t)
mockOrgSvc := mocks.NewOrgService(t)
mockSuSvc := mocks.NewServiceuserService(t)
mockAuditRepo := mocks.NewAuditRecordRepository(t)

mockOrgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
mockSuSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: orgID, Title: "test-su", State: string(serviceuser.Enabled)}, nil)
mockRoleSvc.EXPECT().Get(ctx, viewerRoleID).Return(role.Role{ID: viewerRoleID, Scopes: []string{schema.OrganizationNamespace}}, nil)
mockPolicySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal}).Return([]policy.Policy{}, nil)
mockPolicySvc.EXPECT().Create(ctx, mock.Anything).Return(policy.Policy{}, nil)
mockRelSvc.EXPECT().Create(ctx, mock.Anything).Return(relation.Relation{}, nil)
mockAuditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)

svc := membership.NewService(log.NewNoop(), mockPolicySvc, mockRelSvc, mockRoleSvc, mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mockAuditRepo)
err := svc.AddOrganizationMember(ctx, orgID, suID, schema.ServiceUserPrincipal, viewerRoleID)
assert.NoError(t, err)
})

t.Run("should reject service user from different org", func(t *testing.T) {
mockOrgSvc := mocks.NewOrgService(t)
mockSuSvc := mocks.NewServiceuserService(t)

mockOrgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
mockSuSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: "other-org", State: string(serviceuser.Enabled)}, nil)

svc := membership.NewService(log.NewNoop(), mocks.NewPolicyService(t), mocks.NewRelationService(t), mocks.NewRoleService(t), mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mocks.NewAuditRecordRepository(t))
err := svc.AddOrganizationMember(ctx, orgID, suID, schema.ServiceUserPrincipal, viewerRoleID)
assert.ErrorIs(t, err, membership.ErrPrincipalNotInOrg)
})

t.Run("should reject disabled service user", func(t *testing.T) {
mockOrgSvc := mocks.NewOrgService(t)
mockSuSvc := mocks.NewServiceuserService(t)

mockOrgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
mockSuSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: orgID, State: string(serviceuser.Disabled)}, nil)

svc := membership.NewService(log.NewNoop(), mocks.NewPolicyService(t), mocks.NewRelationService(t), mocks.NewRoleService(t), mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mocks.NewAuditRecordRepository(t))
err := svc.AddOrganizationMember(ctx, orgID, suID, schema.ServiceUserPrincipal, viewerRoleID)
assert.ErrorIs(t, err, serviceuser.ErrDisabled)
})
}

func TestService_SetOrganizationMemberRole(t *testing.T) {
ctx := context.Background()
orgID := uuid.New().String()
Expand Down Expand Up @@ -304,11 +358,11 @@ func TestService_SetOrganizationMemberRole(t *testing.T) {
wantErrContain string
}{
{
name: "should return error if principal type is not user",
name: "should return error if principal type is unsupported",
setup: func(_ *mocks.PolicyService, _ *mocks.RelationService, _ *mocks.RoleService, orgSvc *mocks.OrgService, _ *mocks.UserService, _ *mocks.AuditRecordRepository) {
orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
},
principalType: schema.ServiceUserPrincipal,
principalType: "app/unknown",
roleID: viewerRoleID,
wantErr: membership.ErrInvalidPrincipal,
},
Expand Down Expand Up @@ -467,6 +521,65 @@ func TestService_SetOrganizationMemberRole(t *testing.T) {
}
}

func TestService_SetOrganizationMemberRole_ServiceUser(t *testing.T) {
ctx := context.Background()
orgID := uuid.New().String()
suID := uuid.New().String()
viewerRoleID := uuid.New().String()
ownerRoleID := uuid.New().String()

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

t.Run("should succeed changing service user role", func(t *testing.T) {
mockPolicySvc := mocks.NewPolicyService(t)
mockRelSvc := mocks.NewRelationService(t)
mockRoleSvc := mocks.NewRoleService(t)
mockOrgSvc := mocks.NewOrgService(t)
mockSuSvc := mocks.NewServiceuserService(t)
mockAuditRepo := mocks.NewAuditRecordRepository(t)

mockOrgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
mockSuSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: orgID, Title: "test-su", State: string(serviceuser.Enabled)}, nil)
mockRoleSvc.EXPECT().Get(ctx, viewerRoleID).Return(role.Role{ID: viewerRoleID, Scopes: []string{schema.OrganizationNamespace}}, nil)
mockPolicySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal}).Return([]policy.Policy{{ID: "p1", RoleID: ownerRoleID}}, nil)
mockRoleSvc.EXPECT().Get(ctx, schema.RoleOrganizationOwner).Return(role.Role{ID: ownerRoleID, Name: schema.RoleOrganizationOwner}, nil)
mockPolicySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, RoleID: ownerRoleID}).Return([]policy.Policy{{ID: "p1"}, {ID: "p2"}}, 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)
mockAuditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)

svc := membership.NewService(log.NewNoop(), mockPolicySvc, mockRelSvc, mockRoleSvc, mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mockAuditRepo)
err := svc.SetOrganizationMemberRole(ctx, orgID, suID, schema.ServiceUserPrincipal, viewerRoleID)
assert.NoError(t, err)
})

t.Run("should reject service user from different org", func(t *testing.T) {
mockOrgSvc := mocks.NewOrgService(t)
mockSuSvc := mocks.NewServiceuserService(t)

mockOrgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
mockSuSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: "other-org", State: string(serviceuser.Enabled)}, nil)

svc := membership.NewService(log.NewNoop(), mocks.NewPolicyService(t), mocks.NewRelationService(t), mocks.NewRoleService(t), mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mocks.NewAuditRecordRepository(t))
err := svc.SetOrganizationMemberRole(ctx, orgID, suID, schema.ServiceUserPrincipal, viewerRoleID)
assert.ErrorIs(t, err, membership.ErrPrincipalNotInOrg)
})

t.Run("should reject disabled service user", func(t *testing.T) {
mockOrgSvc := mocks.NewOrgService(t)
mockSuSvc := mocks.NewServiceuserService(t)

mockOrgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil)
mockSuSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: orgID, State: string(serviceuser.Disabled)}, nil)

svc := membership.NewService(log.NewNoop(), mocks.NewPolicyService(t), mocks.NewRelationService(t), mocks.NewRoleService(t), mockOrgSvc, mocks.NewUserService(t), mocks.NewProjectService(t), mocks.NewGroupService(t), mockSuSvc, mocks.NewAuditRecordRepository(t))
err := svc.SetOrganizationMemberRole(ctx, orgID, suID, schema.ServiceUserPrincipal, viewerRoleID)
assert.ErrorIs(t, err, serviceuser.ErrDisabled)
})
}

func TestService_RemoveOrganizationMember(t *testing.T) {
ctx := context.Background()
orgID := uuid.New().String()
Expand Down
Loading