Skip to content

feat(projects): When adding members check all memberships #2368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
javirln wants to merge 1 commit into from
Closed

feat(projects): When adding members check all memberships #2368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions app/controlplane/pkg/biz/orginvitation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,13 +411,13 @@ func (uc *OrgInvitationUseCase) processProjectMembership(ctx context.Context, in
role = authz.RoleProjectViewer
}

// Check if the user is already a member of the project
existingMembership, err := uc.projectRepo.FindProjectMembershipByProjectAndID(ctx, orgUUID, *projectID, userUUID, authz.MembershipTypeUser)
if err != nil && !IsNotFound(err) {
// Check if the user already has membership in the project (consider inherited memberships)
membershipExists, err := uc.projectRepo.ExistsProjectMembershipEffective(ctx, orgUUID, *projectID, userUUID, authz.MembershipTypeUser)
if err != nil {
return fmt.Errorf("error checking project membership for user %s: %w", userUUID, err)
}

if existingMembership != nil {
if membershipExists {
// User is already a member of the project, nothing to do
uc.logger.Infow("msg", "User already in project", "invitation_id", invitation.ID.String(), "org_id", invitation.Org.ID, "user_id", userUUID.String(), "project_id", projectID.String())
return nil
Expand Down
21 changes: 13 additions & 8 deletions app/controlplane/pkg/biz/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ type ProjectsRepo interface {
UpdateMemberRoleInProject(ctx context.Context, orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType, newRole authz.Role) (*ProjectMembership, error)
// FindProjectMembershipByProjectAndID finds a project membership by project ID and member ID (user or group).
FindProjectMembershipByProjectAndID(ctx context.Context, orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType) (*ProjectMembership, error)
// ExistsProjectMembershipEffective checks if a project membership by project ID and member ID (user or group).
// considering inherited (effective) memberships as well.
ExistsProjectMembershipEffective(ctx context.Context, orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType) (bool, error)
// ListPendingInvitationsByProject retrieves a list of pending invitations for a project.
ListPendingInvitationsByProject(ctx context.Context, orgID uuid.UUID, projectID uuid.UUID, paginationOpts *pagination.OffsetPaginationOpts) ([]*OrgInvitation, int, error)
}
Expand Down Expand Up @@ -342,12 +345,13 @@ func (uc *ProjectUseCase) addUserToProject(ctx context.Context, orgID uuid.UUID,

userUUID := uuid.MustParse(userMembership.User.ID)

// Check if the user is already a member of the project
existingMembership, err := uc.projectsRepository.FindProjectMembershipByProjectAndID(ctx, orgID, projectID, userUUID, authz.MembershipTypeUser)
if err != nil && !IsNotFound(err) {
// Check if the user is already a member of the project (including inherited memberships)
membershipExists, err := uc.projectsRepository.ExistsProjectMembershipEffective(ctx, orgID, projectID, userUUID, authz.MembershipTypeUser)
Copy link
Member

@jiparis jiparis Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this? This will prevent setting a custom role at the project level (inherited as viewer, local as Admin). This was one of our main use cases if I'm not wrong.

Copy link
Member Author

@javirln javirln Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we take into account the role when checking if the membership exists, we could have both, inherited as viewer and local as admin, that's an easy change.

if err != nil {
return nil, fmt.Errorf("failed to check existing membership: %w", err)
}
if existingMembership != nil {

if membershipExists {
return nil, NewErrAlreadyExistsStr("user is already a member of this project")
}

Expand Down Expand Up @@ -428,12 +432,13 @@ func (uc *ProjectUseCase) addGroupToProject(ctx context.Context, orgID uuid.UUID
return nil, fmt.Errorf("failed to validate group reference: %w", err)
}

// Check if the group already has membership in the project
existingMembership, err := uc.projectsRepository.FindProjectMembershipByProjectAndID(ctx, orgID, projectID, resolvedGroupID, authz.MembershipTypeGroup)
if err != nil && !IsNotFound(err) {
// Check if the group is already a member of the project (including inherited memberships)
membershipExists, err := uc.projectsRepository.ExistsProjectMembershipEffective(ctx, orgID, projectID, resolvedGroupID, authz.MembershipTypeGroup)
if err != nil {
return nil, fmt.Errorf("failed to check existing group membership: %w", err)
}
if existingMembership != nil {

if membershipExists {
return nil, NewErrAlreadyExistsStr("group is already a member of this project")
}

Expand Down
52 changes: 30 additions & 22 deletions app/controlplane/pkg/data/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func (r *ProjectRepo) RemoveMemberFromProject(ctx context.Context, orgID uuid.UU
}

// Find the membership to delete
m, err := r.queryMembership(orgID, projectID, memberID, membershipType).Only(ctx)
m, err := r.buildMembershipQuery(orgID, projectID, memberID, membershipType, true).Only(ctx)

if err != nil {
if ent.IsNotFound(err) {
Expand All @@ -248,8 +248,8 @@ func (r *ProjectRepo) RemoveMemberFromProject(ctx context.Context, orgID uuid.UU

// FindProjectMembershipByProjectAndID finds a project membership by project ID and member ID (user or group)
func (r *ProjectRepo) FindProjectMembershipByProjectAndID(ctx context.Context, orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType) (*biz.ProjectMembership, error) {
// Find the membership
m, err := r.queryMembership(orgID, projectID, memberID, membershipType).Only(ctx)
// Find the membership (direct only)
m, err := r.buildMembershipQuery(orgID, projectID, memberID, membershipType, true).Only(ctx)

if err != nil {
if ent.IsNotFound(err) {
Expand All @@ -258,38 +258,28 @@ func (r *ProjectRepo) FindProjectMembershipByProjectAndID(ctx context.Context, o
return nil, fmt.Errorf("failed to find membership: %w", err)
}

// Build the membership response based on the membership type
projectMembership := &biz.ProjectMembership{
MembershipType: m.MembershipType,
Role: m.Role,
CreatedAt: &m.CreatedAt,
UpdatedAt: &m.UpdatedAt,
}

switch membershipType {
case authz.MembershipTypeUser:
// Fetch the user details for user memberships
u, err := r.data.DB.User.Get(ctx, memberID)
if err != nil {
if ent.IsNotFound(err) {
return nil, biz.NewErrNotFound("user")
}
return nil, fmt.Errorf("failed to find user: %w", err)
}
projectMembership.User = entUserToBizUser(u)
return entProjectMembershipToBiz(m, u, nil), nil
case authz.MembershipTypeGroup:
// Fetch the group details for group memberships
g, err := r.data.DB.Group.Query().Where(group.ID(memberID), group.DeletedAtIsNil()).Only(ctx)
if err != nil {
if ent.IsNotFound(err) {
return nil, biz.NewErrNotFound("group")
}
return nil, fmt.Errorf("failed to find group: %w", err)
}
projectMembership.Group = entGroupToBiz(g)
return entProjectMembershipToBiz(m, nil, g), nil
default:
return entProjectMembershipToBiz(m, nil, nil), nil
}

return projectMembership, nil
}

// UpdateMemberRoleInProject updates the role of a member in a project
Expand All @@ -308,7 +298,7 @@ func (r *ProjectRepo) UpdateMemberRoleInProject(ctx context.Context, orgID uuid.
}

// Find the membership to update
m, err := r.queryMembership(orgID, projectID, memberID, membershipType).Only(ctx)
m, err := r.buildMembershipQuery(orgID, projectID, memberID, membershipType, true).Only(ctx)

if err != nil {
if ent.IsNotFound(err) {
Expand All @@ -326,9 +316,10 @@ func (r *ProjectRepo) UpdateMemberRoleInProject(ctx context.Context, orgID uuid.
return entProjectMembershipToBiz(m, nil, nil), nil
}

// queryMembership is a helper function to build a common membership query
func (r *ProjectRepo) queryMembership(orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType) *ent.MembershipQuery {
return r.data.DB.Membership.Query().
// buildMembershipQuery constructs a query that can find both direct and inherited memberships
func (r *ProjectRepo) buildMembershipQuery(orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType, directMembershipOnly bool) *ent.MembershipQuery {
// By default, all memberships are considered, both direct and inherited.
baseQuery := r.data.DB.Membership.Query().
Where(
membership.HasOrganizationWith(
organization.ID(orgID),
Expand All @@ -337,8 +328,25 @@ func (r *ProjectRepo) queryMembership(orgID uuid.UUID, projectID uuid.UUID, memb
membership.MemberID(memberID),
membership.ResourceTypeEQ(authz.ResourceTypeProject),
membership.ResourceID(projectID),
membership.ParentIDIsNil(), // Only top-level memberships
).WithOrganization()

// Only consider direct memberships (parent is nil)
if directMembershipOnly {
baseQuery = baseQuery.Where(membership.ParentIDIsNil())
}

return baseQuery
}

// ExistsProjectMembershipEffective checks if a project membership exists considering both direct and inherited
// memberships for the member on the project.
func (r *ProjectRepo) ExistsProjectMembershipEffective(ctx context.Context, orgID uuid.UUID, projectID uuid.UUID, memberID uuid.UUID, membershipType authz.MembershipType) (bool, error) {
exists, err := r.buildMembershipQuery(orgID, projectID, memberID, membershipType, false).Exist(ctx)
if err != nil {
return exists, fmt.Errorf("failed to find membership: %w", err)
}

return exists, nil
}

// entProjectToBiz converts an ent.Project to a biz.Project
Expand Down
Loading