Skip to content

SMQ-2739 - Prevent removing of all users from Built in admin role #2835

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
dborovcanin merged 4 commits into from
Jun 10, 2025
Merged

SMQ-2739 - Prevent removing of all users from Built in admin role #2835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53e6b11
feat(roles): prevent removal of last member from admin role
JeffMboya Apr 22, 2025
9c06a89
Refactor: Move role guard to domain
JeffMboya Jun 5, 2025
e5b1fd3
Address comments
JeffMboya Jun 5, 2025
de78195
Refactor
JeffMboya Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/http/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ func EncodeError(_ context.Context, err error, w http.ResponseWriter) {
errors.Contains(err, apiutil.ErrMissingRoleMembers),
errors.Contains(err, apiutil.ErrMissingDescription),
errors.Contains(err, apiutil.ErrMissingEntityID),
errors.Contains(err, apiutil.ErrInvalidRouteFormat):
errors.Contains(err, apiutil.ErrInvalidRouteFormat),
errors.Contains(err, svcerr.ErrRetainOneMember):
err = unwrap(err)
w.WriteHeader(http.StatusBadRequest)

Expand Down
35 changes: 30 additions & 5 deletions domains/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,15 +351,40 @@ func (svc *service) RemoveEntityMembers(ctx context.Context, session authn.Sessi
return svc.ProvisionManageService.RemoveEntityMembers(ctx, session, entityID, members)
}

func (svc *service) RoleRemoveMembers(ctx context.Context, session authn.Session, entityID, roleID string, members []string) (err error) {
for _, member := range members {
if err := svc.repo.DeleteInvitation(ctx, member, entityID); err != nil && err != repoerr.ErrNotFound {
func (svc *service) RoleRemoveMembers(ctx context.Context, session authn.Session, entityID, roleID string, members []string) error {
ro, err := svc.repo.RetrieveEntityRole(ctx, entityID, roleID)
if err != nil {
return errors.Wrap(svcerr.ErrViewEntity, err)
}
Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

Suggested change
}
return errors.Wrap(svcerr.ErrViewEntity, err)


if _, err := svc.ProvisionManageService.BuiltInRoleActions(roles.BuiltInRoleName(ro.Name)); err == nil {
membersPage, err := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0)
if err != nil {
return errors.Wrap(svcerr.ErrViewEntity, err)
}
Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

Suggested change
}
return errors.Wrap(svcerr.ErrViewEntity, err)

Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

Suggested change
}
membersPage, err := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0)
if err != nil {

if membersPage.Total <= uint64(len(members)) {
return svcerr.ErrRetainOneMember
}
Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

Suggested change
}
errAtLeastOneMemberRetain = errors.New("at least one member should be in inbuild role")
return errors.Wrap(svcerr.ErrRemoveEntity, errAtLeastOneMemberRetain)

No need for details , like how many members request and role name

}

for _, memberID := range members {
if err := svc.repo.DeleteInvitation(ctx, memberID, entityID); err != nil && err != repoerr.ErrNotFound {
Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

We can have repo call where we can delete All the Invitation at once something like

svc.repo.DeleteUsersInvitation(ctx,entityID, memberIDs...)

This requires new function invitations repo like DeleteUsersInvitation(ctx,entityID, memberIDs...)

Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

Create ticket for this #2924
We can solve this later in different PR

return err
}
}

return svc.ProvisionManageService.RoleRemoveMembers(ctx, session, entityID, roleID, members)
}

func (svc *service) RoleRemoveAllMembers(ctx context.Context, session authn.Session, entityID, roleID string) (err error) {
return svcerr.ErrNotFound
func (svc *service) RoleRemoveAllMembers(ctx context.Context, session authn.Session, entityID, roleID string) error {
ro, err := svc.repo.RetrieveEntityRole(ctx, entityID, roleID)
if err != nil {
return errors.Wrap(svcerr.ErrViewEntity, err)
}
Copy link
Contributor

@arvindh123 arvindh123 Jun 5, 2025

Choose a reason for hiding this comment

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

Suggested change
}
return errors.Wrap(svcerr.ErrViewEntity, err)


if _, err := svc.ProvisionManageService.BuiltInRoleActions(roles.BuiltInRoleName(ro.Name)); err == nil {
return svcerr.ErrRetainOneMember
}

return svc.ProvisionManageService.RoleRemoveAllMembers(ctx, session, entityID, roleID)
}
3 changes: 3 additions & 0 deletions pkg/errors/service/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,7 @@ var (

// ErrUnauthorizedPAT indicates failure occurred while authorizing PAT.
ErrUnauthorizedPAT = errors.New("failed to authorize PAT")

// ErrRetainOneMember indicates that at least one owner must be retained in the entity.
ErrRetainOneMember = errors.New("must retain at least one member")
)
8 changes: 8 additions & 0 deletions pkg/roles/provisionmanage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ func NewProvisionManageService(entityType string, repo Repository, policy polici
return rm, nil
}

func (pms ProvisionManageService) BuiltInRoleActions(name BuiltInRoleName) ([]Action, error) {
actions, ok := pms.builtInRoles[name]
if !ok {
return nil, errors.Wrap(svcerr.ErrNotFound, fmt.Errorf("role %s not found", name))
}
return actions, nil
}

func toRolesActions(actions []string) []Action {
roActions := []Action{}
for _, action := range actions {
Expand Down
Loading