diff --git a/core/membership/errors.go b/core/membership/errors.go index 0e01732ea..22654b4e5 100644 --- a/core/membership/errors.go +++ b/core/membership/errors.go @@ -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") ErrNotOrgMember = errors.New("principal is not a member of the organization") ErrInvalidProjectRole = errors.New("role is not valid for project scope") diff --git a/core/membership/service.go b/core/membership/service.go index 4c900c2cf..5ea895269 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -103,10 +103,9 @@ 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) @@ -114,7 +113,7 @@ func (s *Service) AddOrganizationMember(ctx context.Context, orgID, principalID, return err } - principal, err := s.validatePrincipal(ctx, principalID, principalType) + principal, err := s.validatePrincipal(ctx, orgID, principalID, principalType) if err != nil { return err } @@ -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 } @@ -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) @@ -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 + } + if su.State == string(serviceuser.Disabled) { + return principalInfo{}, serviceuser.ErrDisabled + } + return principalInfo{ + ID: su.ID, + Type: schema.ServiceUserPrincipal, + Name: su.Title, + }, nil default: return principalInfo{}, ErrInvalidPrincipal } @@ -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{ @@ -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(), @@ -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{ @@ -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(), diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 1779cde17..56b1a49c8 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -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, }, @@ -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() @@ -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, }, @@ -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()