-
Notifications
You must be signed in to change notification settings - Fork 678
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.
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2835 +/- ##
==========================================
- Coverage 27.81% 25.98% -1.84%
==========================================
Files 354 46 -308
Lines 56028 11447 -44581
==========================================
- Hits 15585 2974 -12611
+ Misses 39669 8286 -31383
+ Partials 774 187 -587 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@arvindh123 Please review. |
pkg/roles/provisionmanage.go
Outdated
|
|
||
| if _, ok := r.builtInRoles[BuiltInRoleName(ro.Name)]; ok { | ||
| page, err := r.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if err != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, err) | ||
| } | ||
| if page.Total <= uint64(len(members)) { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member", ro.Name)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be applicable only for domains
We can move this logic to domain service func (svc *service) RoleRemoveAllMembers(ctx context.Context, session authn.Session, entityID, roleID string) error
89e77af to
08ff1d1
Compare
domains/service.go
Outdated
| currentMembersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if listErr != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members of role '%s' for invitation cleanup: %w", ro.Name, listErr)) | ||
| } | ||
|
|
||
| if len(currentMembersPage.Members) > 0 { | ||
| for _, member := range currentMembersPage.Members { | ||
| memberID := string(member) | ||
| if err := svc.repo.DeleteInvitation(ctx, memberID, entityID); err != nil && err != repoerr.ErrNotFound { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("failed to delete invitation for member '%s' in domain '%s' from role '%s': %w", memberID, entityID, ro.Name, err)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is taken care by RoleRemoveAllMembers
Please remove this logic
domains/service.go
Outdated
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | ||
| isBuiltInRole = true | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simply have like below
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | |
| isBuiltInRole = true | |
| } | |
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | |
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member and cannot have all members removed", ro.Name)) | |
| } | |
domains/service.go
Outdated
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | ||
| isBuiltInRole = true | ||
| } | ||
|
|
||
| if isBuiltInRole { | ||
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if listErr != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | ||
| } | ||
|
|
||
| if membersPage.Total <= uint64(len(members)) { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member. Attempting to remove %d member(s) from %d would leave too few.", ro.Name, len(members), membersPage.Total)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | |
| isBuiltInRole = true | |
| } | |
| if isBuiltInRole { | |
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | |
| if listErr != nil { | |
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | |
| } | |
| if membersPage.Total <= uint64(len(members)) { | |
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member. Attempting to remove %d member(s) from %d would leave too few.", ro.Name, len(members), membersPage.Total)) | |
| } | |
| } | |
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | |
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | |
| if listErr != nil { | |
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | |
| } | |
| if membersPage.Total <= uint64(len(members)) { | |
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member. Attempting to remove %d member(s) from %d would leave too few.", ro.Name, len(members), membersPage.Total)) | |
| } | |
| } |
| } | ||
|
|
||
| for _, memberID := range members { | ||
| if err := svc.repo.DeleteInvitation(ctx, memberID, entityID); err != nil && err != repoerr.ErrNotFound { |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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
| ro, err := svc.repo.RetrieveEntityRole(ctx, entityID, roleID) | ||
| if err != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to retrieve role '%s' for entity '%s': %w", roleID, entityID, err)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| return errors.Wrap(svcerr.ErrViewEntity, err) |
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if listErr != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| return errors.Wrap(svcerr.ErrViewEntity, err) |
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if listErr != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| membersPage, err := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | |
| if err != nil { |
| } | ||
| if membersPage.Total <= uint64(len(members)) { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member. Attempting to remove %d member(s) from %d would leave too few.", ro.Name, len(members), membersPage.Total)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| 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
| ro, err := svc.repo.RetrieveEntityRole(ctx, entityID, roleID) | ||
| if err != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to retrieve role '%s' for entity '%s': %w", roleID, entityID, err)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| return errors.Wrap(svcerr.ErrViewEntity, err) |
domains/service.go
Outdated
| if _, ok := svc.ProvisionManageService.BuiltInRoles[roles.BuiltInRoleName(ro.Name)]; ok { | ||
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if listErr != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | |
| membersPage, err := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | |
| if err != nil { |
domains/service.go
Outdated
| membersPage, listErr := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if listErr != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| return errors.Wrap(svcerr.ErrViewEntity, err) |
domains/service.go
Outdated
| return errors.Wrap(svcerr.ErrViewEntity, fmt.Errorf("failed to list members for built-in role '%s' to check count: %w", ro.Name, listErr)) | ||
| } | ||
| if membersPage.Total > 0 { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, fmt.Errorf("built-in role '%s' must retain at least one member and cannot have all members removed", ro.Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need of details
Just follow the approach mentioned here #2835 (comment)
| return dom, nil | ||
| } | ||
|
|
||
| // Only SuperAdmin can freeze the domain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this comment is removed ?
|
|
||
| d.CreatedAt = time.Now().UTC() | ||
|
|
||
| // Domain is created in repo first, because Roles table have foreign key relation with Domain ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this comment is removed ?
| return nil | ||
| } | ||
|
|
||
| // Add addition removal of user from invitations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this comment is removed ?
domains/service.go
Outdated
| if membersPage.Total <= uint64(len(members)) { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, errAtLeastOneMemberRetain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve described the general approach in this comment, but it shouldn’t be taken as a literal implementation.
Also, as per our established coding conventions, all error constants must be defined at the top of the file. Please make sure to move it there.
domains/service.go
Outdated
| return errors.Wrap(svcerr.ErrViewEntity, err) | ||
| } | ||
| if membersPage.Total > 0 { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, errAtLeastOneMemberRetain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve described the general approach in this comment, but it shouldn’t be taken as a literal implementation.
Also, as per our established coding conventions, all error constants must be defined at the top of the file. Please make sure to move it there.
Please try to reuse the same variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arvindh123 yes, it is already implemented in the last commit
pkg/roles/provisionmanage.go
Outdated
| policy policies.Service | ||
| actions []Action | ||
| builtInRoles map[BuiltInRoleName][]Action | ||
| BuiltInRoles map[BuiltInRoleName][]Action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of exporting the field builtInRoles , please add function like func (pms ProvisionManageService) BuiltInRoleActions(name BuiltInRoleName)([]Action, error)
and in domains/service.go, we can have something like below
if _, err := svc.ProvisionManageService.BuiltInRoleActions; err == nil {
membersPage, err := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0)
if err != nil {
return errors.Wrap(svcerr.ErrViewEntity, err)
}
if membersPage.Total <= uint64(len(members)) {
return errors.Wrap(svcerr.ErrRemoveEntity, errAtLeastOneMemberRetain)
}
}
domains/service.go
Outdated
| return errors.Wrap(svcerr.ErrViewEntity, err) | ||
| } | ||
| if membersPage.Total <= uint64(len(members)) { | ||
| return errors.Wrap(svcerr.ErrRemoveEntity, errAtLeastOneMemberRetain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API response code is 400 and In response we have no details for failure
{
"error": "",
"message": "view entity failed"
}So please move the error errAtLeastOneMemberRetain to pkg/errors/service/types.go
and use that like svcerr.ErrAtLeastOneMemberRetain
Please add this svcerr.ErrAtLeastOneMemberRetain to EncodeError function at api/http/common.go
for switch case http.StatusBadRequest
Line 238 in b79fdb1
| w.WriteHeader(http.StatusBadRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated:
{
"error": "",
"message": "must retain at least one member"
}
arvindh123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
domains/service.go
Outdated
| return errors.Wrap(svcerr.ErrViewEntity, err) | ||
| } | ||
| if membersPage.Total <= uint64(len(members)) { | ||
| return errors.Wrap(svcerr.ErrRetainOneMember, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffMboya
Do you think here we need to wrap with err?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffMboya Do you think here we need to wrap with
err?
Not really. Let me update
domains/service.go
Outdated
| if len(members) == 0 { | ||
| return svcerr.ErrMalformedEntity | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffMboya
Validation is done in API layer, So we can remove validation here
domains/service.go
Outdated
| membersPage, err := svc.repo.RoleListMembers(ctx, ro.ID, 0, 0) | ||
| if err != nil { | ||
| return errors.Wrap(svcerr.ErrViewEntity, err) | ||
| } | ||
| if membersPage.Total > 0 { | ||
| return svcerr.ErrRetainOneMember | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here simply we can return svcerr.ErrRetainOneMember if the role is BuiltInRole
No need for checking the members total.
Because this function itself going to remove everyone.
domains/service.go
Outdated
|
|
||
| for _, memberID := range members { | ||
| if err := svc.repo.DeleteInvitation(ctx, memberID, entityID); err != nil && err != repoerr.ErrNotFound { | ||
| return svcerr.ErrRetainOneMember |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this error, Here we should send the error returned by svc.repo.DeleteInvitation
Signed-off-by: JeffMboya <jangina.mboya@gmail.com> feat(roles): prevent RoleRemoveAllMembers from acting on admin role Signed-off-by: JeffMboya <jangina.mboya@gmail.com> feat(roles): update implementation Signed-off-by: JeffMboya <jangina.mboya@gmail.com> feat(roles): fix 404 on RoleRemoveAllMembers Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix CI Signed-off-by: JeffMboya <jangina.mboya@gmail.com> replace hardcoded admin checks with builtInRoles map lookup Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Address comments Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Address comments Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Address comments Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Address comments Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor error Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix CI Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Update domains/service.go Co-authored-by: Arvindh <30824765+arvindh123@users.noreply.github.com> Fix CI Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Update error Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Update error Signed-off-by: JeffMboya <jangina.mboya@gmail.com> UFix CI Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix CI Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor Signed-off-by: JeffMboya <jangina.mboya@gmail.com>


What type of PR is this?
This is a feature
What does this do?
This PR introduces a safeguard that prevents all members from being removed from the built-in
adminrole on any domain. When an attempt is made to remove the last user from theadminrole, the system now rejects the request with:{ "error": "", "message": "failed to remove entity" }This ensures that at least one user always retains admin privileges, preventing domains from becoming orphaned.
Example behavior:
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Yes. Manual testing was performed by removing members via the
/domains/{domainID}/roles/{roleID}/members/deleteendpoint and verifying behavior with various admin user counts. Unit tests will follow up in a separate test refactor PR.Did you document any new/modified feature?
Yes. Documented at absmach/supermq-docs#223
Notes