diff --git a/cmd/serve.go b/cmd/serve.go index f4d4031b5..109656239 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -432,6 +432,9 @@ func buildAPIDependencies( authnService, policyService, preferenceService, auditRecordRepository, roleService) membershipService := membership.NewService(logger, policyService, relationService, roleService, organizationService, userService, auditRecordRepository) + // Setter injection: org → membership is circular (membership needs org for validation, + // org needs membership for Create/AdminCreate). Break the cycle with a post-init setter. + organizationService.SetMembershipService(membershipService) orgKycRepository := postgres.NewOrgKycRepository(dbc) orgKycService := kyc.NewService(orgKycRepository) @@ -467,7 +470,7 @@ func buildAPIDependencies( userProjectsService := userprojects.NewService(userProjectsRepository) domainRepository := postgres.NewDomainRepository(logger, dbc) - domainService := domain.NewService(logger, domainRepository, userService, organizationService) + domainService := domain.NewService(logger, domainRepository, userService, organizationService, membershipService) metaschemaRepository := postgres.NewMetaSchemaRepository(logger, dbc) metaschemaService := metaschema.NewService(metaschemaRepository) @@ -495,8 +498,8 @@ func buildAPIDependencies( ) invitationService := invitation.NewService(mailDialer, postgres.NewInvitationRepository(logger, dbc), - organizationService, groupService, userService, relationService, policyService, preferenceService, - auditRecordRepository) + organizationService, groupService, userService, relationService, preferenceService, + auditRecordRepository, membershipService) if GetStripeClientFunc == nil { // allow to override the stripe client creation function in tests diff --git a/core/domain/service.go b/core/domain/service.go index 2880761ed..17b3140b1 100644 --- a/core/domain/service.go +++ b/core/domain/service.go @@ -4,11 +4,13 @@ import ( "context" "crypto/rand" "encoding/base64" + "errors" "fmt" "net" "strings" "time" + "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/pkg/utils" @@ -25,17 +27,21 @@ type UserService interface { } type OrgService interface { - ListByUser(ctx context.Context, principal authenticate.Principal, filter organization.Filter) ([]organization.Organization, error) - AddMember(ctx context.Context, orgID, relationName string, principal authenticate.Principal) error Get(ctx context.Context, id string) (organization.Organization, error) + ListByUser(ctx context.Context, principal authenticate.Principal, filter organization.Filter) ([]organization.Organization, error) +} + +type MembershipService interface { + AddOrganizationMember(ctx context.Context, orgID, principalID, principalType, roleID string) error } type Service struct { - repository Repository - userService UserService - orgService OrgService - cron *cron.Cron - log log.Logger + repository Repository + userService UserService + orgService OrgService + membershipService MembershipService + cron *cron.Cron + log log.Logger } const ( @@ -45,13 +51,14 @@ const ( refreshTime = "0 0 * * *" // Once a day at midnight (UTC) ) -func NewService(logger log.Logger, repository Repository, userService UserService, orgService OrgService) *Service { +func NewService(logger log.Logger, repository Repository, userService UserService, orgService OrgService, membershipService MembershipService) *Service { return &Service{ - repository: repository, - userService: userService, - orgService: orgService, - cron: cron.New(), - log: logger, + repository: repository, + userService: userService, + orgService: orgService, + membershipService: membershipService, + cron: cron.New(), + log: logger, } } @@ -133,21 +140,6 @@ func (s Service) Join(ctx context.Context, orgID string, userId string) error { return err } - // check if user is already a member of the organization. if yes, do nothing and return nil - userOrgs, err := s.orgService.ListByUser(ctx, authenticate.Principal{ - ID: currUser.ID, - Type: schema.UserPrincipal, - }, organization.Filter{}) - if err != nil { - return err - } - - for _, org := range userOrgs { - if org.ID == orgResp.ID { - return nil - } - } - userDomain := utils.ExtractDomainFromEmail(currUser.Email) if userDomain == "" { return user.ErrInvalidEmail @@ -164,10 +156,10 @@ func (s Service) Join(ctx context.Context, orgID string, userId string) error { for _, dmn := range orgTrustedDomains { if userDomain == dmn.Name { - if err = s.orgService.AddMember(ctx, orgResp.ID, schema.MemberRelationName, authenticate.Principal{ - ID: currUser.ID, - Type: schema.UserPrincipal, - }); err != nil { + // AddOrganizationMember checks if user is already a member and + // returns ErrAlreadyMember — treat that as success (nothing to do) + err = s.membershipService.AddOrganizationMember(ctx, orgResp.ID, currUser.ID, schema.UserPrincipal, schema.RoleOrganizationViewer) + if err != nil && !errors.Is(err, membership.ErrAlreadyMember) { return err } return nil diff --git a/core/invitation/mocks/membership_service.go b/core/invitation/mocks/membership_service.go new file mode 100644 index 000000000..7a81278b0 --- /dev/null +++ b/core/invitation/mocks/membership_service.go @@ -0,0 +1,86 @@ +// Code generated by mockery v2.53.5. DO NOT EDIT. + +package mocks + +import ( + context "context" + + mock "github.com/stretchr/testify/mock" +) + +// MembershipService is an autogenerated mock type for the MembershipService type +type MembershipService struct { + mock.Mock +} + +type MembershipService_Expecter struct { + mock *mock.Mock +} + +func (_m *MembershipService) EXPECT() *MembershipService_Expecter { + return &MembershipService_Expecter{mock: &_m.Mock} +} + +// AddOrganizationMember provides a mock function with given fields: ctx, orgID, principalID, principalType, roleID +func (_m *MembershipService) AddOrganizationMember(ctx context.Context, orgID string, principalID string, principalType string, roleID string) error { + ret := _m.Called(ctx, orgID, principalID, principalType, roleID) + + if len(ret) == 0 { + panic("no return value specified for AddOrganizationMember") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, orgID, principalID, principalType, roleID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_AddOrganizationMember_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'AddOrganizationMember' +type MembershipService_AddOrganizationMember_Call struct { + *mock.Call +} + +// AddOrganizationMember is a helper method to define mock.On call +// - ctx context.Context +// - orgID string +// - principalID string +// - principalType string +// - roleID string +func (_e *MembershipService_Expecter) AddOrganizationMember(ctx interface{}, orgID interface{}, principalID interface{}, principalType interface{}, roleID interface{}) *MembershipService_AddOrganizationMember_Call { + return &MembershipService_AddOrganizationMember_Call{Call: _e.mock.On("AddOrganizationMember", ctx, orgID, principalID, principalType, roleID)} +} + +func (_c *MembershipService_AddOrganizationMember_Call) Run(run func(ctx context.Context, orgID string, principalID string, principalType string, roleID string)) *MembershipService_AddOrganizationMember_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string), args[4].(string)) + }) + return _c +} + +func (_c *MembershipService_AddOrganizationMember_Call) Return(_a0 error) *MembershipService_AddOrganizationMember_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_AddOrganizationMember_Call) RunAndReturn(run func(context.Context, string, string, string, string) error) *MembershipService_AddOrganizationMember_Call { + _c.Call.Return(run) + return _c +} + +// NewMembershipService creates a new instance of MembershipService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMembershipService(t interface { + mock.TestingT + Cleanup(func()) +}) *MembershipService { + mock := &MembershipService{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/core/invitation/mocks/organization_service.go b/core/invitation/mocks/organization_service.go index 3eeedd106..a7fd17551 100644 --- a/core/invitation/mocks/organization_service.go +++ b/core/invitation/mocks/organization_service.go @@ -25,55 +25,6 @@ func (_m *OrganizationService) EXPECT() *OrganizationService_Expecter { return &OrganizationService_Expecter{mock: &_m.Mock} } -// AddMember provides a mock function with given fields: ctx, orgID, relationName, principal -func (_m *OrganizationService) AddMember(ctx context.Context, orgID string, relationName string, principal authenticate.Principal) error { - ret := _m.Called(ctx, orgID, relationName, principal) - - if len(ret) == 0 { - panic("no return value specified for AddMember") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, authenticate.Principal) error); ok { - r0 = rf(ctx, orgID, relationName, principal) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// OrganizationService_AddMember_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'AddMember' -type OrganizationService_AddMember_Call struct { - *mock.Call -} - -// AddMember is a helper method to define mock.On call -// - ctx context.Context -// - orgID string -// - relationName string -// - principal authenticate.Principal -func (_e *OrganizationService_Expecter) AddMember(ctx interface{}, orgID interface{}, relationName interface{}, principal interface{}) *OrganizationService_AddMember_Call { - return &OrganizationService_AddMember_Call{Call: _e.mock.On("AddMember", ctx, orgID, relationName, principal)} -} - -func (_c *OrganizationService_AddMember_Call) Run(run func(ctx context.Context, orgID string, relationName string, principal authenticate.Principal)) *OrganizationService_AddMember_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(authenticate.Principal)) - }) - return _c -} - -func (_c *OrganizationService_AddMember_Call) Return(_a0 error) *OrganizationService_AddMember_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *OrganizationService_AddMember_Call) RunAndReturn(run func(context.Context, string, string, authenticate.Principal) error) *OrganizationService_AddMember_Call { - _c.Call.Return(run) - return _c -} - // Get provides a mock function with given fields: ctx, id func (_m *OrganizationService) Get(ctx context.Context, id string) (organization.Organization, error) { ret := _m.Called(ctx, id) diff --git a/core/invitation/service.go b/core/invitation/service.go index bf59ff17d..4b8a87d84 100644 --- a/core/invitation/service.go +++ b/core/invitation/service.go @@ -17,7 +17,7 @@ import ( "github.com/mcuadros/go-defaults" "github.com/raystack/frontier/core/auditrecord/models" "github.com/raystack/frontier/core/authenticate" - "github.com/raystack/frontier/core/policy" + "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/preference" pkgAuditRecord "github.com/raystack/frontier/pkg/auditrecord" "gopkg.in/mail.v2" @@ -46,10 +46,13 @@ type UserService interface { type OrganizationService interface { Get(ctx context.Context, id string) (organization.Organization, error) - AddMember(ctx context.Context, orgID, relationName string, principal authenticate.Principal) error ListByUser(ctx context.Context, p authenticate.Principal, f organization.Filter) ([]organization.Organization, error) } +type MembershipService interface { + AddOrganizationMember(ctx context.Context, orgID, principalID, principalType, roleID string) error +} + type GroupService interface { Get(ctx context.Context, id string) (group.Group, error) AddMember(ctx context.Context, groupID string, principal authenticate.Principal) error @@ -61,10 +64,6 @@ type RelationService interface { Delete(ctx context.Context, rel relation.Relation) error } -type PolicyService interface { - Create(ctx context.Context, policy policy.Policy) (policy.Policy, error) -} - type PreferencesService interface { LoadPlatformPreferences(ctx context.Context) (map[string]string, error) } @@ -80,16 +79,17 @@ type Service struct { groupSvc GroupService userService UserService relationService RelationService - policyService PolicyService prefService PreferencesService auditRecordRepository AuditRecordRepository + membershipSvc MembershipService } func NewService(dialer mailer.Dialer, repo Repository, orgSvc OrganizationService, grpSvc GroupService, userService UserService, relService RelationService, - policyService PolicyService, prefService PreferencesService, - auditRecordRepository AuditRecordRepository) *Service { + prefService PreferencesService, + auditRecordRepository AuditRecordRepository, + membershipSvc MembershipService) *Service { return &Service{ dialer: dialer, repo: repo, @@ -97,9 +97,9 @@ func NewService(dialer mailer.Dialer, repo Repository, groupSvc: grpSvc, userService: userService, relationService: relService, - policyService: policyService, prefService: prefService, auditRecordRepository: auditRecordRepository, + membershipSvc: membershipSvc, } } @@ -297,18 +297,24 @@ func (s Service) Accept(ctx context.Context, id uuid.UUID) error { return ErrInviteExpired } - // check if user is already a member of the organization - // if yes, check if any other part of the invitation applies like group membership userOb, userOrgMember, err := s.isUserOrgMember(ctx, invite.OrgID, invite.UserEmailID) if err != nil { return err } + + // Use the first role ID from the invitation, fall back to viewer. + orgRoleID := schema.RoleOrganizationViewer + conf := s.getConfig(ctx) + if conf.WithRoles && len(invite.RoleIDs) > 0 { + orgRoleID = invite.RoleIDs[0] + } + if !userOrgMember { - // if not, add user to the organization - if err = s.orgSvc.AddMember(ctx, invite.OrgID, schema.MemberRelationName, authenticate.Principal{ - ID: userOb.ID, - Type: schema.UserPrincipal, - }); err != nil { + // User is not yet a member — add with the invitation's role. + // ErrAlreadyMember is possible in a race (user added between invite creation + // and acceptance) — treat as success since the user is already in the org. + err = s.membershipSvc.AddOrganizationMember(ctx, invite.OrgID, userOb.ID, schema.UserPrincipal, orgRoleID) + if err != nil && !errors.Is(err, membership.ErrAlreadyMember) { return err } } @@ -322,14 +328,11 @@ func (s Service) Accept(ctx context.Context, id uuid.UUID) error { return err } for _, groupID := range invite.GroupIDs { - // check if group id is valid grp, err := s.groupSvc.Get(ctx, groupID) if err != nil { return err } - // check if user is already a member of the group - // if yes, skip alreadyGroupMember := false for _, g := range userGroups { if g.ID == grp.ID { @@ -348,28 +351,6 @@ func (s Service) Accept(ctx context.Context, id uuid.UUID) error { } } - // check if invitation has a list of roles which we want to assign to the user at org level - var roleErr error - if len(invite.RoleIDs) > 0 { - conf := s.getConfig(ctx) - if conf.WithRoles { - for _, inviteRoleID := range invite.RoleIDs { - if _, err := s.policyService.Create(ctx, policy.Policy{ - RoleID: inviteRoleID, - ResourceID: invite.OrgID, - ResourceType: schema.OrganizationNamespace, - PrincipalID: userOb.ID, - PrincipalType: schema.UserPrincipal, - }); err != nil { - roleErr = errors.Join(roleErr, err) - } - } - if roleErr != nil { - return roleErr - } - } - } - // fetch organization details for audit record org, err := s.orgSvc.Get(ctx, invite.OrgID) if err != nil { diff --git a/core/invitation/service_test.go b/core/invitation/service_test.go index 9565ab969..b44625a02 100644 --- a/core/invitation/service_test.go +++ b/core/invitation/service_test.go @@ -20,7 +20,7 @@ import ( ) func mockService(t *testing.T) (*mocks2.Dialer, *mocks.Repository, *mocks.OrganizationService, *mocks.GroupService, - *mocks.UserService, *mocks.RelationService, *mocks.PolicyService, *mocks.PreferencesService, *auditMocks.Repository) { + *mocks.UserService, *mocks.RelationService, *mocks.PreferencesService, *auditMocks.Repository) { t.Helper() dialer := mocks2.NewDialer(t) repo := mocks.NewRepository(t) @@ -28,10 +28,9 @@ func mockService(t *testing.T) (*mocks2.Dialer, *mocks.Repository, *mocks.Organi orgService := mocks.NewOrganizationService(t) groupService := mocks.NewGroupService(t) relationService := mocks.NewRelationService(t) - policyService := mocks.NewPolicyService(t) prefService := mocks.NewPreferencesService(t) auditRecordRepo := auditMocks.NewRepository(t) - return dialer, repo, orgService, groupService, userService, relationService, policyService, prefService, auditRecordRepo + return dialer, repo, orgService, groupService, userService, relationService, prefService, auditRecordRepo } func TestService_Create(t *testing.T) { @@ -50,7 +49,7 @@ func TestService_Create(t *testing.T) { }, err: invitation.ErrAlreadyMember, setup: func() *invitation.Service { - dialer, repo, orgService, groupService, userService, relationService, policyService, prefService, auditRecordRepo := mockService(t) + dialer, repo, orgService, groupService, userService, relationService, prefService, auditRecordRepo := mockService(t) prefService.EXPECT().LoadPlatformPreferences(mock.Anything).Return(map[string]string{}, nil) orgService.EXPECT().Get(mock.Anything, "org-id").Return(organization.Organization{ @@ -70,8 +69,9 @@ func TestService_Create(t *testing.T) { Email: "test@example.com", }, nil) + membershipSvc := mocks.NewMembershipService(t) return invitation.NewService(dialer, repo, orgService, groupService, - userService, relationService, policyService, prefService, auditRecordRepo) + userService, relationService, prefService, auditRecordRepo, membershipSvc) }, }, } diff --git a/core/organization/errors.go b/core/organization/errors.go index 07d544e52..5cbfe14f3 100644 --- a/core/organization/errors.go +++ b/core/organization/errors.go @@ -3,13 +3,14 @@ package organization import "errors" var ( - ErrNotExist = errors.New("org doesn't exist") - ErrInvalidUUID = errors.New("invalid syntax of uuid") - ErrInvalidID = errors.New("org id is invalid") - ErrConflict = errors.New("org already exist") - ErrInvalidDetail = errors.New("invalid org detail") - ErrDisabled = errors.New("org is disabled") - ErrLastOwnerRole = errors.New("cannot remove the last owner role") - ErrNotMember = errors.New("user is not a member of the organization") - ErrInvalidOrgRole = errors.New("role is not valid for organization scope") + ErrNotExist = errors.New("org doesn't exist") + ErrInvalidUUID = errors.New("invalid syntax of uuid") + ErrInvalidID = errors.New("org id is invalid") + ErrConflict = errors.New("org already exist") + ErrInvalidDetail = errors.New("invalid org detail") + ErrDisabled = errors.New("org is disabled") + ErrLastOwnerRole = errors.New("cannot remove the last owner role") + ErrNotMember = errors.New("user is not a member of the organization") + ErrInvalidOrgRole = errors.New("role is not valid for organization scope") + ErrUserPrincipalOnly = errors.New("only user principals can create organizations") ) diff --git a/core/organization/mocks/membership_service.go b/core/organization/mocks/membership_service.go new file mode 100644 index 000000000..7a81278b0 --- /dev/null +++ b/core/organization/mocks/membership_service.go @@ -0,0 +1,86 @@ +// Code generated by mockery v2.53.5. DO NOT EDIT. + +package mocks + +import ( + context "context" + + mock "github.com/stretchr/testify/mock" +) + +// MembershipService is an autogenerated mock type for the MembershipService type +type MembershipService struct { + mock.Mock +} + +type MembershipService_Expecter struct { + mock *mock.Mock +} + +func (_m *MembershipService) EXPECT() *MembershipService_Expecter { + return &MembershipService_Expecter{mock: &_m.Mock} +} + +// AddOrganizationMember provides a mock function with given fields: ctx, orgID, principalID, principalType, roleID +func (_m *MembershipService) AddOrganizationMember(ctx context.Context, orgID string, principalID string, principalType string, roleID string) error { + ret := _m.Called(ctx, orgID, principalID, principalType, roleID) + + if len(ret) == 0 { + panic("no return value specified for AddOrganizationMember") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, orgID, principalID, principalType, roleID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_AddOrganizationMember_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'AddOrganizationMember' +type MembershipService_AddOrganizationMember_Call struct { + *mock.Call +} + +// AddOrganizationMember is a helper method to define mock.On call +// - ctx context.Context +// - orgID string +// - principalID string +// - principalType string +// - roleID string +func (_e *MembershipService_Expecter) AddOrganizationMember(ctx interface{}, orgID interface{}, principalID interface{}, principalType interface{}, roleID interface{}) *MembershipService_AddOrganizationMember_Call { + return &MembershipService_AddOrganizationMember_Call{Call: _e.mock.On("AddOrganizationMember", ctx, orgID, principalID, principalType, roleID)} +} + +func (_c *MembershipService_AddOrganizationMember_Call) Run(run func(ctx context.Context, orgID string, principalID string, principalType string, roleID string)) *MembershipService_AddOrganizationMember_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string), args[4].(string)) + }) + return _c +} + +func (_c *MembershipService_AddOrganizationMember_Call) Return(_a0 error) *MembershipService_AddOrganizationMember_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_AddOrganizationMember_Call) RunAndReturn(run func(context.Context, string, string, string, string) error) *MembershipService_AddOrganizationMember_Call { + _c.Call.Return(run) + return _c +} + +// NewMembershipService creates a new instance of MembershipService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMembershipService(t interface { + mock.TestingT + Cleanup(func()) +}) *MembershipService { + mock := &MembershipService{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/core/organization/service.go b/core/organization/service.go index b1591be45..b09be7207 100644 --- a/core/organization/service.go +++ b/core/organization/service.go @@ -73,6 +73,10 @@ type RoleService interface { Get(ctx context.Context, idOrName string) (role.Role, error) } +type MembershipService interface { + AddOrganizationMember(ctx context.Context, orgID, principalID, principalType, roleID string) error +} + type Service struct { repository Repository relationService RelationService @@ -82,6 +86,7 @@ type Service struct { prefService PreferencesService auditRecordRepository AuditRecordRepository roleService RoleService + membershipService MembershipService } func NewService(repository Repository, relationService RelationService, @@ -100,6 +105,12 @@ func NewService(repository Repository, relationService RelationService, } } +// SetMembershipService sets the membership dependency after construction to break +// the circular init order between organization and membership services. +func (s *Service) SetMembershipService(ms MembershipService) { + s.membershipService = ms +} + // extractPrincipalInfo extracts display name and metadata from principal func extractPrincipalInfo(principal authenticate.Principal) (string, map[string]any) { metadata := make(map[string]any) @@ -125,18 +136,6 @@ func extractPrincipalInfo(principal authenticate.Principal) (string, map[string] return name, metadata } -// mapPrincipalTypeToAuditType maps schema principal types to audit record type constants -func mapPrincipalTypeToAuditType(principalType string) pkgAuditRecord.EntityType { - switch principalType { - case schema.ServiceUserPrincipal: - return pkgAuditRecord.ServiceUserType - case schema.PATPrincipal: - return pkgAuditRecord.PATType - default: - return pkgAuditRecord.UserType - } -} - // Get returns an enabled organization by id or name. Will return `org is disabled` error if the organization is disabled func (s Service) Get(ctx context.Context, idOrName string) (Organization, error) { if utils.IsValidUUID(idOrName) { @@ -185,25 +184,32 @@ func (s Service) Create(ctx context.Context, org Organization) (Organization, er if err != nil { return Organization{}, fmt.Errorf("%w: %s", user.ErrNotExist, err.Error()) } + if principal.Type != schema.UserPrincipal { + return Organization{}, ErrUserPrincipalOnly + } defaultState, err := s.GetDefaultOrgStateOnCreate(ctx) if err != nil { return Organization{}, err } + // Always create as Enabled so that AddOrganizationMember (which validates + // the org is enabled) succeeds. Disable afterwards if the platform preference + // requires it — this avoids leaving an ownerless org row on failure. newOrg, err := s.repository.Create(ctx, Organization{ Name: org.Name, Title: org.Title, Avatar: org.Avatar, Metadata: org.Metadata, - State: defaultState, + State: Enabled, }) if err != nil { return Organization{}, err } - // attach user as owner - if err = s.AddMember(ctx, newOrg.ID, schema.OwnerRelationName, principal); err != nil { + // Use AddOrganizationMember (not SetOrganizationMemberRole) because the user + // is not yet a member — the org was just created. Set requires existing membership. + if err = s.membershipService.AddOrganizationMember(ctx, newOrg.ID, principal.ID, principal.Type, AdminRole); err != nil { return newOrg, err } @@ -212,78 +218,14 @@ func (s Service) Create(ctx context.Context, org Organization) (Organization, er return newOrg, err } - return newOrg, nil -} - -func (s Service) AddMember(ctx context.Context, orgID, relationName string, principal authenticate.Principal) error { - roleID := MemberRole - if relationName == schema.OwnerRelationName { - roleID = AdminRole - } - if _, err := s.policyService.Create(ctx, policy.Policy{ - RoleID: roleID, - ResourceID: orgID, - ResourceType: schema.OrganizationNamespace, - PrincipalID: principal.ID, - PrincipalType: principal.Type, - }); err != nil { - return err - } - if _, err := s.relationService.Create(ctx, relation.Relation{ - Object: relation.Object{ - ID: orgID, - Namespace: schema.OrganizationNamespace, - }, - Subject: relation.Subject{ - ID: principal.ID, - Namespace: principal.Type, - }, - RelationName: relationName, - }); err != nil { - return err - } - - // Get organization details for audit - org, err := s.repository.GetByID(ctx, orgID) - if err != nil { - return err - } - - principalName, principalMetadata := extractPrincipalInfo(principal) - targetMetadata := map[string]any{ - "relation": relationName, - "role": roleID, - } - // Merge principal metadata into target metadata - for k, v := range principalMetadata { - targetMetadata[k] = v - } - - _, err = s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.OrganizationMemberAddedEvent, - Resource: auditrecord.Resource{ - ID: orgID, - Type: pkgAuditRecord.OrganizationType, - Name: org.Title, - }, - Target: &auditrecord.Target{ - ID: principal.ID, - Type: mapPrincipalTypeToAuditType(principal.Type), - Name: principalName, - Metadata: targetMetadata, - }, - OrgID: orgID, - OccurredAt: time.Now(), - }) - if err != nil { - return err + if defaultState == Disabled { + if err = s.repository.SetState(ctx, newOrg.ID, Disabled); err != nil { + return newOrg, err + } + newOrg.State = Disabled } - audit.GetAuditor(ctx, orgID).Log(audit.OrgMemberCreatedEvent, audit.Target{ - ID: principal.ID, - Type: principal.Type, - }) - return nil + return newOrg, nil } func (s Service) AttachToPlatform(ctx context.Context, orgID string) error { @@ -356,19 +298,6 @@ func (s Service) ListByUser(ctx context.Context, principal authenticate.Principa return s.repository.List(ctx, filter) } -func (s Service) AddUsers(ctx context.Context, orgID string, userIDs []string) error { - var err error - for _, userID := range userIDs { - if currentErr := s.AddMember(ctx, orgID, schema.MemberRelationName, authenticate.Principal{ - ID: userID, - Type: schema.UserPrincipal, - }); currentErr != nil { - err = errors.Join(err, currentErr) - } - } - return err -} - // RemoveUsers removes users from an organization as members // it doesn't remove user access to projects or other resources provided // by policies, don't call directly, use cascade deleter @@ -520,23 +449,23 @@ func (s Service) AdminCreate(ctx context.Context, org Organization, ownerEmail s return Organization{}, err } - // Create organization + // Always create as Enabled so that AddOrganizationMember (which validates + // the org is enabled) succeeds. Disable afterwards if the platform preference + // requires it — this avoids leaving an ownerless org row on failure. newOrg, err := s.repository.Create(ctx, Organization{ Name: org.Name, Title: org.Title, Avatar: org.Avatar, Metadata: org.Metadata, - State: defaultState, + State: Enabled, }) if err != nil { return Organization{}, err } - // Add user as organization owner - if err = s.AddMember(ctx, newOrg.ID, schema.OwnerRelationName, authenticate.Principal{ - ID: usr.ID, - Type: schema.UserPrincipal, - }); err != nil { + // Use AddOrganizationMember (not SetOrganizationMemberRole) because the user + // is not yet a member — the org was just created. Set requires existing membership. + if err = s.membershipService.AddOrganizationMember(ctx, newOrg.ID, usr.ID, schema.UserPrincipal, AdminRole); err != nil { return newOrg, fmt.Errorf("failed to add user as owner: %w", err) } @@ -545,5 +474,12 @@ func (s Service) AdminCreate(ctx context.Context, org Organization, ownerEmail s return newOrg, fmt.Errorf("failed to attach to platform: %w", err) } + if defaultState == Disabled { + if err = s.repository.SetState(ctx, newOrg.ID, Disabled); err != nil { + return newOrg, fmt.Errorf("failed to set org state: %w", err) + } + newOrg.State = Disabled + } + return newOrg, nil } diff --git a/core/organization/service_test.go b/core/organization/service_test.go index 769e7f9e6..89d484208 100644 --- a/core/organization/service_test.go +++ b/core/organization/service_test.go @@ -6,11 +6,9 @@ import ( "testing" "github.com/google/uuid" - "github.com/raystack/frontier/core/auditrecord" "github.com/raystack/frontier/core/authenticate" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/organization/mocks" - "github.com/raystack/frontier/core/policy" "github.com/raystack/frontier/core/preference" "github.com/raystack/frontier/core/relation" pat "github.com/raystack/frontier/core/userpat/models" @@ -168,63 +166,6 @@ func TestService_GetDefaultOrgStateOnCreate(t *testing.T) { }) } -func TestService_AddMember(t *testing.T) { - mockRepo := mocks.NewRepository(t) - mockRelationSvc := mocks.NewRelationService(t) - mockUserSvc := mocks.NewUserService(t) - mockAuthnSvc := mocks.NewAuthnService(t) - mockPolicySvc := mocks.NewPolicyService(t) - mockPrefSvc := mocks.NewPreferencesService(t) - mockAuditRecordRepo := mocks.NewAuditRecordRepository(t) - - mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockAuditRecordRepo, mockRoleSvc) - - t.Run("should create policy and relation for member as per role", func(t *testing.T) { - inputOrgID := "test-id" - inputRelationName := schema.MemberRelationName - inputPrincipal := authenticate.Principal{ - ID: "test-principal-id", - Type: schema.UserPrincipal, - } - - expectedOrg := organization.Organization{ - ID: inputOrgID, - Name: "test-org", - Title: "Test Organization", - State: organization.Enabled, - } - - policyToBeCreated := policy.Policy{ - RoleID: organization.MemberRole, - ResourceID: inputOrgID, - ResourceType: schema.OrganizationNamespace, - PrincipalID: inputPrincipal.ID, - PrincipalType: inputPrincipal.Type, - } - - relationToBeCreated := relation.Relation{ - Object: relation.Object{ - ID: inputOrgID, - Namespace: schema.OrganizationNamespace, - }, - Subject: relation.Subject{ - ID: inputPrincipal.ID, - Namespace: inputPrincipal.Type, - }, - RelationName: schema.MemberRelationName, - } - - mockPolicySvc.On("Create", mock.Anything, policyToBeCreated).Return(policy.Policy{}, nil) - mockRelationSvc.On("Create", mock.Anything, relationToBeCreated).Return(relation.Relation{}, nil) - mockRepo.On("GetByID", mock.Anything, inputOrgID).Return(expectedOrg, nil).Once() - mockAuditRecordRepo.On("Create", mock.Anything, mock.AnythingOfType("models.AuditRecord")).Return(auditrecord.AuditRecord{}, nil).Once() - - err := svc.AddMember(context.Background(), inputOrgID, inputRelationName, inputPrincipal) - assert.Nil(t, err) - }) -} - func TestService_AttachToPlatform(t *testing.T) { mockRepo := mocks.NewRepository(t) mockRelationSvc := mocks.NewRelationService(t) diff --git a/internal/api/v1beta1connect/domain.go b/internal/api/v1beta1connect/domain.go index fb5a35600..b3b34ea4d 100644 --- a/internal/api/v1beta1connect/domain.go +++ b/internal/api/v1beta1connect/domain.go @@ -5,7 +5,9 @@ import ( "connectrpc.com/connect" "github.com/raystack/frontier/core/domain" + "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" + "github.com/raystack/frontier/core/user" "github.com/raystack/frontier/pkg/errors" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "go.uber.org/zap" @@ -118,21 +120,6 @@ func (h *ConnectHandler) GetOrganizationDomain(ctx context.Context, request *con func (h *ConnectHandler) JoinOrganization(ctx context.Context, request *connect.Request[frontierv1beta1.JoinOrganizationRequest]) (*connect.Response[frontierv1beta1.JoinOrganizationResponse], error) { errorLogger := NewErrorLogger() - orgResp, err := h.orgService.Get(ctx, request.Msg.GetOrgId()) - if err != nil { - switch { - case errors.Is(err, organization.ErrDisabled): - return nil, connect.NewError(connect.CodeNotFound, ErrOrgDisabled) - case errors.Is(err, organization.ErrNotExist): - return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) - default: - errorLogger.LogServiceError(ctx, request, "JoinOrganization.Get", err, - zap.String("org_id", request.Msg.GetOrgId())) - return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) - } - } - - // get current user principal, err := h.GetLoggedInPrincipal(ctx) if err != nil { errorLogger.LogServiceError(ctx, request, "JoinOrganization.GetLoggedInPrincipal", err, @@ -140,10 +127,22 @@ func (h *ConnectHandler) JoinOrganization(ctx context.Context, request *connect. return nil, connect.NewError(connect.CodeUnauthenticated, ErrUnauthenticated) } - if err := h.domainService.Join(ctx, orgResp.ID, principal.ID); err != nil { - switch err { - case domain.ErrDomainsMisMatch: + if err := h.domainService.Join(ctx, request.Msg.GetOrgId(), principal.ID); err != nil { + switch { + case errors.Is(err, domain.ErrDomainsMisMatch): return nil, connect.NewError(connect.CodeInvalidArgument, ErrDomainMismatch) + case errors.Is(err, user.ErrNotExist): + return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist) + case errors.Is(err, user.ErrDisabled): + return nil, connect.NewError(connect.CodeFailedPrecondition, err) + case errors.Is(err, organization.ErrDisabled): + return nil, connect.NewError(connect.CodeNotFound, ErrOrgDisabled) + case errors.Is(err, organization.ErrNotExist): + return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) + case errors.Is(err, membership.ErrInvalidOrgRole): + return nil, connect.NewError(connect.CodeInvalidArgument, err) + case errors.Is(err, membership.ErrAlreadyMember): + return nil, connect.NewError(connect.CodeAlreadyExists, err) default: errorLogger.LogServiceError(ctx, request, "JoinOrganization.Join", err, zap.String("org_id", request.Msg.GetOrgId()), diff --git a/internal/api/v1beta1connect/domain_test.go b/internal/api/v1beta1connect/domain_test.go index e5f8d1e5f..cc5a6adf3 100644 --- a/internal/api/v1beta1connect/domain_test.go +++ b/internal/api/v1beta1connect/domain_test.go @@ -387,23 +387,11 @@ func TestHandler_JoinOrganization(t *testing.T) { want *connect.Response[frontierv1beta1.JoinOrganizationResponse] wantErr error }{ - { - name: "should return internal error if org service return some error", - setup: func(os *mocks.OrganizationService, ds *mocks.DomainService, as *mocks.AuthnService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(organization.Organization{}, errors.New("test error")) - as.EXPECT().GetPrincipal(mock.AnythingOfType("context.backgroundCtx")).Return(authenticate.Principal{ID: testUserID}, nil) - }, - request: connect.NewRequest(&frontierv1beta1.JoinOrganizationRequest{ - OrgId: testOrgID, - }), - want: nil, - wantErr: connect.NewError(connect.CodeInternal, ErrInternalServerError), - }, { name: "should return not found error if org is disabled", setup: func(os *mocks.OrganizationService, ds *mocks.DomainService, as *mocks.AuthnService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(organization.Organization{}, organization.ErrDisabled) as.EXPECT().GetPrincipal(mock.AnythingOfType("context.backgroundCtx")).Return(authenticate.Principal{ID: testUserID}, nil) + ds.EXPECT().Join(mock.AnythingOfType("context.backgroundCtx"), testOrgID, testUserID).Return(organization.ErrDisabled) }, request: connect.NewRequest(&frontierv1beta1.JoinOrganizationRequest{ OrgId: testOrgID, @@ -414,8 +402,8 @@ func TestHandler_JoinOrganization(t *testing.T) { { name: "should return not found error if org does not exist", setup: func(os *mocks.OrganizationService, ds *mocks.DomainService, as *mocks.AuthnService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(organization.Organization{}, organization.ErrNotExist) as.EXPECT().GetPrincipal(mock.AnythingOfType("context.backgroundCtx")).Return(authenticate.Principal{ID: testUserID}, nil) + ds.EXPECT().Join(mock.AnythingOfType("context.backgroundCtx"), testOrgID, testUserID).Return(organization.ErrNotExist) }, request: connect.NewRequest(&frontierv1beta1.JoinOrganizationRequest{ OrgId: testOrgID, @@ -426,7 +414,6 @@ func TestHandler_JoinOrganization(t *testing.T) { { name: "should return invalid argument error if domains mismatch", setup: func(os *mocks.OrganizationService, ds *mocks.DomainService, as *mocks.AuthnService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) as.EXPECT().GetPrincipal(mock.AnythingOfType("context.backgroundCtx")).Return(authenticate.Principal{ID: testUserID}, nil) ds.EXPECT().Join(mock.AnythingOfType("context.backgroundCtx"), testOrgID, testUserID).Return(domain.ErrDomainsMisMatch) }, @@ -439,7 +426,6 @@ func TestHandler_JoinOrganization(t *testing.T) { { name: "should return internal error if domain service fails", setup: func(os *mocks.OrganizationService, ds *mocks.DomainService, as *mocks.AuthnService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) as.EXPECT().GetPrincipal(mock.AnythingOfType("context.backgroundCtx")).Return(authenticate.Principal{ID: testUserID}, nil) ds.EXPECT().Join(mock.AnythingOfType("context.backgroundCtx"), testOrgID, testUserID).Return(errors.New("domain service error")) }, @@ -452,7 +438,6 @@ func TestHandler_JoinOrganization(t *testing.T) { { name: "should join organization successfully", setup: func(os *mocks.OrganizationService, ds *mocks.DomainService, as *mocks.AuthnService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) as.EXPECT().GetPrincipal(mock.AnythingOfType("context.backgroundCtx")).Return(authenticate.Principal{ID: testUserID}, nil) ds.EXPECT().Join(mock.AnythingOfType("context.backgroundCtx"), testOrgID, testUserID).Return(nil) }, diff --git a/internal/api/v1beta1connect/interfaces.go b/internal/api/v1beta1connect/interfaces.go index c8c537679..38f8c2e7e 100644 --- a/internal/api/v1beta1connect/interfaces.go +++ b/internal/api/v1beta1connect/interfaces.go @@ -124,7 +124,6 @@ type OrganizationService interface { List(ctx context.Context, f organization.Filter) ([]organization.Organization, error) Update(ctx context.Context, toUpdate organization.Organization) (organization.Organization, error) ListByUser(ctx context.Context, principal authenticate.Principal, flt organization.Filter) ([]organization.Organization, error) - AddUsers(ctx context.Context, orgID string, userID []string) error Enable(ctx context.Context, id string) error Disable(ctx context.Context, id string) error } diff --git a/internal/api/v1beta1connect/invitations.go b/internal/api/v1beta1connect/invitations.go index 24273a2f9..7ab8cd1a7 100644 --- a/internal/api/v1beta1connect/invitations.go +++ b/internal/api/v1beta1connect/invitations.go @@ -8,6 +8,7 @@ import ( "connectrpc.com/connect" "github.com/google/uuid" "github.com/raystack/frontier/core/invitation" + "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/user" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" @@ -239,20 +240,6 @@ func (h *ConnectHandler) GetOrganizationInvitation(ctx context.Context, request func (h *ConnectHandler) AcceptOrganizationInvitation(ctx context.Context, request *connect.Request[frontierv1beta1.AcceptOrganizationInvitationRequest]) (*connect.Response[frontierv1beta1.AcceptOrganizationInvitationResponse], error) { errorLogger := NewErrorLogger() - _, err := h.orgService.Get(ctx, request.Msg.GetOrgId()) - if err != nil { - switch { - case errors.Is(err, organization.ErrDisabled): - return nil, connect.NewError(connect.CodeNotFound, ErrOrgDisabled) - case errors.Is(err, organization.ErrNotExist): - return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) - default: - errorLogger.LogServiceError(ctx, request, "AcceptOrganizationInvitation.Get", err, - zap.String("org_id", request.Msg.GetOrgId())) - return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) - } - } - inviteID, err := uuid.Parse(request.Msg.GetId()) if err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) @@ -266,6 +253,16 @@ func (h *ConnectHandler) AcceptOrganizationInvitation(ctx context.Context, reque return nil, connect.NewError(connect.CodeNotFound, ErrInvitationNotFound) case errors.Is(err, user.ErrNotExist): return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist) + case errors.Is(err, user.ErrDisabled): + return nil, connect.NewError(connect.CodeFailedPrecondition, err) + case errors.Is(err, organization.ErrDisabled): + return nil, connect.NewError(connect.CodeNotFound, ErrOrgDisabled) + case errors.Is(err, organization.ErrNotExist): + return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) + case errors.Is(err, membership.ErrInvalidOrgRole): + return nil, connect.NewError(connect.CodeInvalidArgument, err) + case errors.Is(err, membership.ErrAlreadyMember): + return nil, connect.NewError(connect.CodeAlreadyExists, err) default: errorLogger.LogServiceError(ctx, request, "AcceptOrganizationInvitation.Accept", err, zap.String("invitation_id", request.Msg.GetId()), diff --git a/internal/api/v1beta1connect/invitations_test.go b/internal/api/v1beta1connect/invitations_test.go index 4ddf10b8f..c82902f90 100644 --- a/internal/api/v1beta1connect/invitations_test.go +++ b/internal/api/v1beta1connect/invitations_test.go @@ -533,7 +533,6 @@ func TestHandler_AcceptOrganizationInvitation(t *testing.T) { { name: "should return an error if invite not found", setup: func(is *mocks.InvitationService, us *mocks.UserService, gs *mocks.GroupService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) is.EXPECT().Accept(mock.AnythingOfType("context.backgroundCtx"), testInvitation1ID).Return(invitation.ErrNotFound) }, request: connect.NewRequest(&frontierv1beta1.AcceptOrganizationInvitationRequest{ @@ -546,7 +545,6 @@ func TestHandler_AcceptOrganizationInvitation(t *testing.T) { { name: "should return an error if unable to get user by id", setup: func(is *mocks.InvitationService, us *mocks.UserService, gs *mocks.GroupService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) is.EXPECT().Accept(mock.AnythingOfType("context.backgroundCtx"), testInvitation1ID).Return(user.ErrNotExist) }, request: connect.NewRequest(&frontierv1beta1.AcceptOrganizationInvitationRequest{ @@ -559,7 +557,6 @@ func TestHandler_AcceptOrganizationInvitation(t *testing.T) { { name: "should return an internal error if unable to accept invitation", setup: func(is *mocks.InvitationService, us *mocks.UserService, gs *mocks.GroupService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) is.EXPECT().Accept(mock.AnythingOfType("context.backgroundCtx"), testInvitation1ID).Return(errors.New("test error")) }, request: connect.NewRequest(&frontierv1beta1.AcceptOrganizationInvitationRequest{ @@ -572,7 +569,6 @@ func TestHandler_AcceptOrganizationInvitation(t *testing.T) { { name: "should return error if invitation is expired", setup: func(is *mocks.InvitationService, us *mocks.UserService, gs *mocks.GroupService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) is.EXPECT().Accept(mock.AnythingOfType("context.backgroundCtx"), testInvitation3ID).Return(invitation.ErrInviteExpired) }, request: connect.NewRequest(&frontierv1beta1.AcceptOrganizationInvitationRequest{ @@ -585,7 +581,6 @@ func TestHandler_AcceptOrganizationInvitation(t *testing.T) { { name: "should accept an invitation on success", setup: func(is *mocks.InvitationService, us *mocks.UserService, gs *mocks.GroupService, os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) is.EXPECT().Accept(mock.AnythingOfType("context.backgroundCtx"), testInvitation1ID).Return(nil) }, request: connect.NewRequest(&frontierv1beta1.AcceptOrganizationInvitationRequest{ diff --git a/internal/api/v1beta1connect/mocks/organization_service.go b/internal/api/v1beta1connect/mocks/organization_service.go index 9ca0fae15..fac005cc6 100644 --- a/internal/api/v1beta1connect/mocks/organization_service.go +++ b/internal/api/v1beta1connect/mocks/organization_service.go @@ -25,54 +25,6 @@ func (_m *OrganizationService) EXPECT() *OrganizationService_Expecter { return &OrganizationService_Expecter{mock: &_m.Mock} } -// AddUsers provides a mock function with given fields: ctx, orgID, userID -func (_m *OrganizationService) AddUsers(ctx context.Context, orgID string, userID []string) error { - ret := _m.Called(ctx, orgID, userID) - - if len(ret) == 0 { - panic("no return value specified for AddUsers") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, []string) error); ok { - r0 = rf(ctx, orgID, userID) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// OrganizationService_AddUsers_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'AddUsers' -type OrganizationService_AddUsers_Call struct { - *mock.Call -} - -// AddUsers is a helper method to define mock.On call -// - ctx context.Context -// - orgID string -// - userID []string -func (_e *OrganizationService_Expecter) AddUsers(ctx interface{}, orgID interface{}, userID interface{}) *OrganizationService_AddUsers_Call { - return &OrganizationService_AddUsers_Call{Call: _e.mock.On("AddUsers", ctx, orgID, userID)} -} - -func (_c *OrganizationService_AddUsers_Call) Run(run func(ctx context.Context, orgID string, userID []string)) *OrganizationService_AddUsers_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].([]string)) - }) - return _c -} - -func (_c *OrganizationService_AddUsers_Call) Return(_a0 error) *OrganizationService_AddUsers_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *OrganizationService_AddUsers_Call) RunAndReturn(run func(context.Context, string, []string) error) *OrganizationService_AddUsers_Call { - _c.Call.Return(run) - return _c -} - // AdminCreate provides a mock function with given fields: ctx, org, ownerEmail func (_m *OrganizationService) AdminCreate(ctx context.Context, org organization.Organization, ownerEmail string) (organization.Organization, error) { ret := _m.Called(ctx, org, ownerEmail) diff --git a/internal/api/v1beta1connect/organization.go b/internal/api/v1beta1connect/organization.go index 51410872c..98233c787 100644 --- a/internal/api/v1beta1connect/organization.go +++ b/internal/api/v1beta1connect/organization.go @@ -142,6 +142,8 @@ func (h *ConnectHandler) CreateOrganization(ctx context.Context, request *connec switch { case errors.Is(err, user.ErrInvalidEmail): return nil, connect.NewError(connect.CodeUnauthenticated, ErrUnauthenticated) + case errors.Is(err, organization.ErrUserPrincipalOnly): + return nil, connect.NewError(connect.CodePermissionDenied, err) case errors.Is(err, organization.ErrInvalidDetail): return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) case errors.Is(err, organization.ErrConflict): @@ -469,33 +471,6 @@ func (h *ConnectHandler) ListOrganizationUsers(ctx context.Context, request *con }), nil } -func (h *ConnectHandler) AddOrganizationUsers(ctx context.Context, request *connect.Request[frontierv1beta1.AddOrganizationUsersRequest]) (*connect.Response[frontierv1beta1.AddOrganizationUsersResponse], error) { - errorLogger := NewErrorLogger() - - orgResp, err := h.orgService.Get(ctx, request.Msg.GetId()) - if err != nil { - switch { - case errors.Is(err, organization.ErrDisabled): - return nil, connect.NewError(connect.CodeNotFound, ErrOrgDisabled) - case errors.Is(err, organization.ErrNotExist): - return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) - default: - errorLogger.LogServiceError(ctx, request, "AddOrganizationUsers.Get", err, - zap.String("org_id", request.Msg.GetId())) - return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) - } - } - - if err := h.orgService.AddUsers(ctx, orgResp.ID, request.Msg.GetUserIds()); err != nil { - errorLogger.LogServiceError(ctx, request, "AddOrganizationUsers.AddUsers", err, - zap.String("org_id", orgResp.ID), - zap.Strings("user_ids", request.Msg.GetUserIds())) - return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError) - } - - return connect.NewResponse(&frontierv1beta1.AddOrganizationUsersResponse{}), nil -} - func (h *ConnectHandler) RemoveOrganizationUser(ctx context.Context, request *connect.Request[frontierv1beta1.RemoveOrganizationUserRequest]) (*connect.Response[frontierv1beta1.RemoveOrganizationUserResponse], error) { errorLogger := NewErrorLogger() diff --git a/internal/api/v1beta1connect/organization_test.go b/internal/api/v1beta1connect/organization_test.go index 9c24cc446..d5f9aab31 100644 --- a/internal/api/v1beta1connect/organization_test.go +++ b/internal/api/v1beta1connect/organization_test.go @@ -921,91 +921,6 @@ func TestHandler_ListOrganizationUsers(t *testing.T) { } } -func TestHandler_AddOrganizationUsers(t *testing.T) { - tests := []struct { - name string - setup func(os *mocks.OrganizationService) - request *connect.Request[frontierv1beta1.AddOrganizationUsersRequest] - want *connect.Response[frontierv1beta1.AddOrganizationUsersResponse] - wantErr error - }{ - { - name: "should return internal error if org service return some error", - setup: func(os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(organization.Organization{}, errors.New("test error")) - }, - request: connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: testOrgID, - UserIds: []string{"some-user-id"}, - }), - want: nil, - wantErr: connect.NewError(connect.CodeInternal, ErrInternalServerError), - }, - { - name: "should return not found error if org does not exist", - setup: func(os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(organization.Organization{}, organization.ErrNotExist) - }, - request: connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: testOrgID, - UserIds: []string{"some-user-id"}, - }), - want: nil, - wantErr: connect.NewError(connect.CodeNotFound, ErrNotFound), - }, - { - name: "should return not found error if org is disabled", - setup: func(os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(organization.Organization{}, organization.ErrDisabled) - }, - request: connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: testOrgID, - UserIds: []string{"some-user-id"}, - }), - want: nil, - wantErr: connect.NewError(connect.CodeNotFound, ErrOrgDisabled), - }, - { - name: "should return internal error if AddUsers fails", - setup: func(os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) - os.EXPECT().AddUsers(mock.AnythingOfType("context.backgroundCtx"), testOrgID, []string{"some-user-id"}).Return(errors.New("add users error")) - }, - request: connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: testOrgID, - UserIds: []string{"some-user-id"}, - }), - want: nil, - wantErr: connect.NewError(connect.CodeInternal, ErrInternalServerError), - }, - { - name: "should add user to org successfully", - setup: func(os *mocks.OrganizationService) { - os.EXPECT().Get(mock.AnythingOfType("context.backgroundCtx"), testOrgID).Return(testOrgMap[testOrgID], nil) - os.EXPECT().AddUsers(mock.AnythingOfType("context.backgroundCtx"), testOrgID, []string{"some-user-id"}).Return(nil) - }, - request: connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: testOrgID, - UserIds: []string{"some-user-id"}, - }), - want: connect.NewResponse(&frontierv1beta1.AddOrganizationUsersResponse{}), - wantErr: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - mockOrgService := new(mocks.OrganizationService) - if tt.setup != nil { - tt.setup(mockOrgService) - } - mockDep := &ConnectHandler{orgService: mockOrgService} - resp, err := mockDep.AddOrganizationUsers(context.Background(), tt.request) - assert.Equal(t, tt.want, resp) - assert.Equal(t, tt.wantErr, err) - }) - } -} - func TestHandler_RemoveOrganizationUser(t *testing.T) { tests := []struct { name string diff --git a/pkg/server/connect_interceptors/authorization.go b/pkg/server/connect_interceptors/authorization.go index 4b8c692c0..02e32c156 100644 --- a/pkg/server/connect_interceptors/authorization.go +++ b/pkg/server/connect_interceptors/authorization.go @@ -353,10 +353,6 @@ var authorizationValidationMap = map[string]func(ctx context.Context, handler *v pbreq := req.(*connect.Request[frontierv1beta1.ListOrganizationProjectsRequest]) return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetId()}, schema.ProjectListPermission, req) }, - "/raystack.frontier.v1beta1.FrontierService/AddOrganizationUsers": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error { - pbreq := req.(*connect.Request[frontierv1beta1.AddOrganizationUsersRequest]) - return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetId()}, schema.UpdatePermission, req) - }, "/raystack.frontier.v1beta1.FrontierService/RemoveOrganizationUser": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error { pbreq := req.(*connect.Request[frontierv1beta1.RemoveOrganizationUserRequest]) return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetId()}, schema.UpdatePermission, req) diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index e7ae9b3a6..a7f117678 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -36,6 +36,7 @@ import ( "github.com/raystack/frontier/pkg/logger" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "github.com/raystack/frontier/test/e2e/testbench" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "google.golang.org/protobuf/types/known/structpb" ) @@ -48,10 +49,21 @@ const ( computeManagerRoleName = "compute_order_manager" ) +func requireAddOrgMembersSuccess(t require.TestingT, resp *connect.Response[frontierv1beta1.AddOrganizationMembersResponse], err error) { + require.NoError(t, err) + require.NotNil(t, resp) + for _, result := range resp.Msg.GetResults() { + require.Truef(t, result.GetSuccess(), + "failed to add org member user_id=%s role_id=%s: %s", + result.GetUserId(), result.GetRoleId(), result.GetError()) + } +} + type APIRegressionTestSuite struct { suite.Suite - testBench *testbench.TestBench - adminCookie string + testBench *testbench.TestBench + adminCookie string + orgViewerRole string } func (s *APIRegressionTestSuite) SetupSuite() { @@ -100,6 +112,21 @@ func (s *APIRegressionTestSuite) SetupSuite() { s.Require().NoError(testbench.BootstrapOrganizations(ctx, s.testBench.Client, adminCookie)) s.Require().NoError(testbench.BootstrapProject(ctx, s.testBench.Client, adminCookie)) s.Require().NoError(testbench.BootstrapGroup(ctx, s.testBench.Client, adminCookie)) + + // look up org viewer role UUID for AddOrganizationMembers calls + ctxAdmin := testbench.ContextWithAuth(ctx, adminCookie) + rolesResp, err := s.testBench.Client.ListRoles(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListRolesRequest{ + State: "enabled", + Scopes: []string{"app/organization"}, + })) + s.Require().NoError(err) + for _, r := range rolesResp.Msg.GetRoles() { + if r.GetName() == schema.RoleOrganizationViewer { + s.orgViewerRole = r.GetId() + break + } + } + s.Require().NotEmpty(s.orgViewerRole, "org viewer role not found") } func (s *APIRegressionTestSuite) TearDownSuite() { @@ -159,11 +186,14 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { }})) s.Assert().NoError(err) - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: createOrgResp.Msg.GetOrganization().GetId(), - UserIds: []string{userResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: userResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) orgUsersResp, err := s.testBench.Client.ListOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationUsersRequest{ Id: createOrgResp.Msg.GetOrganization().GetId(), @@ -190,11 +220,14 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { s.Assert().NoError(err) // attach user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: createOrgResp.Msg.GetOrganization().GetId(), - UserIds: []string{createUserResponse.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResponse.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) createProjResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ Body: &frontierv1beta1.ProjectRequestBody{ @@ -271,11 +304,14 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { s.Assert().NoError(err) // attach user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: createOrgResp.Msg.GetOrganization().GetId(), - UserIds: []string{createUserResponse.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResponse.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) // check users listUsersBeforeDelete, err := s.testBench.Client.ListUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListUsersRequest{ @@ -302,7 +338,7 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { return u.GetId() }), createUserResponse.Msg.GetUser().GetId()) }) - s.Run("5. a user should successfully create a new org and list it even if it's disabled", func() { + s.Run("5. creating an org when PlatformDisableOrgsOnCreate is true should succeed but org is disabled", func() { // enable disable_org_on_create preference disabledOrgs, err := s.testBench.AdminClient.CreatePreferences(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePreferencesRequest{ Preferences: []*frontierv1beta1.PreferenceRequestBody{ @@ -318,6 +354,7 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { normalUserCookie, err := testbench.AuthenticateUser(context.Background(), s.testBench.Client, "normaluser@raystack.org") s.Require().NoError(err) ctxOrgUserAuth := testbench.ContextWithAuth(context.Background(), normalUserCookie) + // org creation succeeds — org is created enabled, membership setup completes, then disabled createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgUserAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ Body: &frontierv1beta1.OrganizationRequestBody{ Title: "org acme 5", @@ -327,22 +364,6 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { s.Assert().NoError(err) s.Assert().Equal(organization.Disabled.String(), createOrgResp.Msg.GetOrganization().GetState()) - // should not list org if it's disabled by default - userEnabledOrgs, err := s.testBench.Client.ListOrganizationsByCurrentUser(ctxOrgUserAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationsByCurrentUserRequest{})) - s.Assert().NoError(err) - s.Assert().False(slices.Contains(utils.Map(userEnabledOrgs.Msg.GetOrganizations(), func(o *frontierv1beta1.Organization) string { - return o.GetName() - }), createOrgResp.Msg.GetOrganization().GetName())) - - // should list org even if it's disabled - userDisabledOrgs, err := s.testBench.Client.ListOrganizationsByCurrentUser(ctxOrgUserAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationsByCurrentUserRequest{ - State: organization.Disabled.String(), - })) - s.Assert().NoError(err) - s.Assert().True(slices.Contains(utils.Map(userDisabledOrgs.Msg.GetOrganizations(), func(o *frontierv1beta1.Organization) string { - return o.GetName() - }), createOrgResp.Msg.GetOrganization().GetName())) - // reset disable_org_on_create preference _, err = s.testBench.AdminClient.CreatePreferences(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePreferencesRequest{ Preferences: []*frontierv1beta1.PreferenceRequestBody{ @@ -373,11 +394,14 @@ func (s *APIRegressionTestSuite) TestOrganizationAPI() { s.Assert().NotNil(createUser1Resp) // add user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: createOrgResp.Msg.GetOrganization().GetId(), - UserIds: []string{createUser1Resp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUser1Resp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) orgUsersResp, err := s.testBench.Client.ListOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationUsersRequest{ Id: createOrgResp.Msg.GetOrganization().GetId(), @@ -842,11 +866,14 @@ func (s *APIRegressionTestSuite) TestGroupAPI() { s.Assert().NotNil(createUserResp) // add basic user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: myOrg.GetId(), - UserIds: []string{createUserResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: myOrg.GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) // give it access to create group _, err = s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ @@ -869,11 +896,14 @@ func (s *APIRegressionTestSuite) TestGroupAPI() { s.Assert().NotNil(createOwnerUserResp) // add owner user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: myOrg.GetId(), - UserIds: []string{createOwnerUserResp.Msg.GetUser().GetId()}, + addMembersResp, err = s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: myOrg.GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createOwnerUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) // give it access to create everything _, err = s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ @@ -1179,11 +1209,14 @@ func (s *APIRegressionTestSuite) TestUserAPI() { })) s.Assert().NoError(err) - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: existingOrg.Msg.GetOrganization().GetId(), - UserIds: []string{createUserResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: existingOrg.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) _, err = s.testBench.Client.AddGroupUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddGroupUsersRequest{ Id: existingGroup.GetId(), OrgId: existingGroup.GetOrgId(), @@ -1262,11 +1295,14 @@ func (s *APIRegressionTestSuite) TestUserAPI() { })) s.Assert().NoError(err) - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: existingOrg.Msg.GetOrganization().GetId(), - UserIds: []string{createUserResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: existingOrg.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) orgUsersRespAfterRelation, err := s.testBench.Client.ListOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationUsersRequest{ Id: existingOrg.Msg.GetOrganization().GetId(), PermissionFilter: organization.MemberRole, @@ -1334,11 +1370,14 @@ func (s *APIRegressionTestSuite) TestUserAPI() { s.Assert().NoError(err) s.Assert().Equal(1, len(listExistingUsers.Msg.GetUsers())) - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: existingOrg.Msg.GetOrganization().GetId(), - UserIds: []string{createUserResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: existingOrg.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) listNewUsers, err := s.testBench.Client.ListUsers(ctxCurrentUser, connect.NewRequest(&frontierv1beta1.ListUsersRequest{ OrgId: existingOrg.Msg.GetOrganization().GetId(), @@ -1513,11 +1552,14 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { s.Assert().NoError(err) // attach user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: createOrgResp.Msg.GetOrganization().GetId(), - UserIds: []string{userResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: userResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) createProjResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ Body: &frontierv1beta1.ProjectRequestBody{ @@ -1588,12 +1630,15 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { }})) s.Assert().NoError(err) - // attach user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: createOrgResp.Msg.GetOrganization().GetId(), - UserIds: []string{user1Resp.Msg.GetUser().GetId(), user2Resp.Msg.GetUser().GetId()}, + // attach users to org + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: createOrgResp.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{ + {UserId: user1Resp.Msg.GetUser().GetId(), RoleId: s.orgViewerRole}, + {UserId: user2Resp.Msg.GetUser().GetId(), RoleId: s.orgViewerRole}, + }, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) createProjResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ Body: &frontierv1beta1.ProjectRequestBody{ @@ -2028,8 +2073,9 @@ func (s *APIRegressionTestSuite) TestInvitationAPI() { createRoleResp, err := s.testBench.Client.CreateOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRoleRequest{ OrgId: existingOrg.Msg.GetOrganization().GetId(), Body: &frontierv1beta1.RoleRequestBody{ - Title: "invitation role 1", - Name: "invitation_role_1", + Title: "invitation role 1", + Name: "invitation_role_1", + Scopes: []string{"app/organization"}, Permissions: []string{ "app.organization.groupcreate", "app.organization.grouplist", @@ -2163,11 +2209,14 @@ func (s *APIRegressionTestSuite) TestInvitationAPI() { s.Assert().NoError(err) // attach user to org - _, err = s.testBench.Client.AddOrganizationUsers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: existingOrg.Msg.GetOrganization().GetId(), - UserIds: []string{createUserResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: existingOrg.Msg.GetOrganization().GetId(), + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) _, err = s.testBench.Client.CreateOrganizationInvitation(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationInvitationRequest{ OrgId: existingOrg.Msg.GetOrganization().GetId(), @@ -2433,7 +2482,7 @@ func (s *APIRegressionTestSuite) TestPreferencesAPI() { return p.GetName() == preference.PlatformDisableOrgsOnCreate })[0].GetValue()) }) - s.Run("4. PlatformDisableOrgsOnCreate if set to true should disable all orgs when created", func() { + s.Run("4. PlatformDisableOrgsOnCreate if set to true should create org in disabled state", func() { createPrefResp, err := s.testBench.AdminClient.CreatePreferences(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePreferencesRequest{ Preferences: []*frontierv1beta1.PreferenceRequestBody{ { @@ -2446,7 +2495,7 @@ func (s *APIRegressionTestSuite) TestPreferencesAPI() { s.Assert().NotNil(createPrefResp) s.Assert().True(len(createPrefResp.Msg.GetPreference()) > 0) - // create a new org + // org creation succeeds — org is created enabled, membership setup completes, then disabled createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ Body: &frontierv1beta1.OrganizationRequestBody{ Title: "org 2", @@ -2454,7 +2503,6 @@ func (s *APIRegressionTestSuite) TestPreferencesAPI() { }, })) s.Assert().NoError(err) - s.Assert().NotNil(createOrgResp) s.Assert().Equal(organization.Disabled.String(), createOrgResp.Msg.GetOrganization().GetState()) // reset it back to false diff --git a/test/e2e/regression/onboarding_test.go b/test/e2e/regression/onboarding_test.go index f39c180b6..e4713d2d4 100644 --- a/test/e2e/regression/onboarding_test.go +++ b/test/e2e/regression/onboarding_test.go @@ -24,8 +24,9 @@ import ( type OnboardingRegressionTestSuite struct { suite.Suite - testBench *testbench.TestBench - adminCookie string + testBench *testbench.TestBench + adminCookie string + orgViewerRole string } func (s *OnboardingRegressionTestSuite) SetupSuite() { @@ -73,6 +74,20 @@ func (s *OnboardingRegressionTestSuite) SetupSuite() { s.Require().NoError(testbench.BootstrapOrganizations(ctx, s.testBench.Client, adminCookie)) s.Require().NoError(testbench.BootstrapProject(ctx, s.testBench.Client, adminCookie)) s.Require().NoError(testbench.BootstrapGroup(ctx, s.testBench.Client, adminCookie)) + + ctxAdmin := testbench.ContextWithAuth(ctx, adminCookie) + rolesResp, err := s.testBench.Client.ListRoles(ctxAdmin, connect.NewRequest(&frontierv1beta1.ListRolesRequest{ + State: "enabled", + Scopes: []string{"app/organization"}, + })) + s.Require().NoError(err) + for _, r := range rolesResp.Msg.GetRoles() { + if r.GetName() == schema.RoleOrganizationViewer { + s.orgViewerRole = r.GetId() + break + } + } + s.Require().NotEmpty(s.orgViewerRole, "org viewer role not found") } func (s *OnboardingRegressionTestSuite) TearDownSuite() { @@ -208,11 +223,14 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { newUserID = createUserResp.Msg.GetUser().GetId() // make user member of the org - _, err = s.testBench.Client.AddOrganizationUsers(ctx, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: orgID, - UserIds: []string{newUserID}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctx, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: orgID, + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: newUserID, + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) // assign new user as project admin createPolicyResp, err := s.testBench.Client.CreatePolicy(ctx, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{Body: &frontierv1beta1.PolicyRequestBody{ @@ -273,11 +291,14 @@ func (s *OnboardingRegressionTestSuite) TestOnboardOrganizationWithUser() { s.Assert().NotNil(createUserResp) // make user member of the org - _, err = s.testBench.Client.AddOrganizationUsers(ctx, connect.NewRequest(&frontierv1beta1.AddOrganizationUsersRequest{ - Id: orgID, - UserIds: []string{createUserResp.Msg.GetUser().GetId()}, + addMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctx, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: orgID, + Members: []*frontierv1beta1.OrgMemberEntry{{ + UserId: createUserResp.Msg.GetUser().GetId(), + RoleId: s.orgViewerRole, + }}, })) - s.Assert().NoError(err) + requireAddOrgMembersSuccess(s.T(), addMembersResp, err) resourceViewerRole := "" listRolesResp, err := s.testBench.Client.ListRoles(ctx, connect.NewRequest(&frontierv1beta1.ListRolesRequest{})) diff --git a/test/e2e/regression/serviceusers_test.go b/test/e2e/regression/serviceusers_test.go index ae072681b..c4a74d609 100644 --- a/test/e2e/regression/serviceusers_test.go +++ b/test/e2e/regression/serviceusers_test.go @@ -182,7 +182,7 @@ func (s *ServiceUsersRegressionTestSuite) TestServiceUserWithKey() { s.Assert().NoError(err) s.Assert().NotNil(currentUserResp.Body) }) - s.Run("6. service user should be able to create an organization with full permission", func() { + s.Run("6. service user should not be able to create an organization", func() { _, err := s.testBench.Client.CreateOrganization(context.Background(), connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ Body: &frontierv1beta1.OrganizationRequestBody{ Name: "org-su-test-1", @@ -193,21 +193,12 @@ func (s *ServiceUsersRegressionTestSuite) TestServiceUserWithKey() { ctxWithKey := testbench.ContextWithHeaders(context.Background(), map[string]string{ "Authorization": "Bearer " + string(svKeyToken), }) - createOrgResp, err := s.testBench.Client.CreateOrganization(ctxWithKey, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + _, err = s.testBench.Client.CreateOrganization(ctxWithKey, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ Body: &frontierv1beta1.OrganizationRequestBody{ Name: "org-su-test-1", }, })) - s.Assert().NoError(err) - s.Assert().NotNil(createOrgResp) - - checkPermResp, err := s.testBench.Client.CheckResourcePermission(ctxWithKey, connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ - ObjectId: createOrgResp.Msg.GetOrganization().GetId(), - ObjectNamespace: "organization", - Permission: schema.UpdatePermission, - })) - s.Assert().NoError(err) - s.Assert().True(checkPermResp.Msg.GetStatus()) + s.Assert().Error(err) }) s.Run("7. service user should be allowed to assign role", func() { ctxWithKey := testbench.ContextWithHeaders(context.Background(), map[string]string{