Repository navigation
Conversation
…ursive role expansion in RoleREST
vyommani
requested review from
dineshkumar-yadav,
mneethiraj,
pradeepagrawal8184 and
rameeshm
October 21, 2026 08:04
mneethiraj
reviewed
Oct 22, 2026
| } | ||
|
|
||
| while (!pending.isEmpty()) { | ||
| String nestedRoleName = pending.poll(); |
Contributor
There was a problem hiding this comment.
nestedRoleName => memberRoleName
| continue; | ||
| } | ||
|
|
||
| XXRole xxNestedRole = daoMgr.getXXRole().findByRoleName(nestedRoleName); |
Contributor
There was a problem hiding this comment.
Consider replacing 493 to 508 with a new method in XXRoleDao to retrieve the list of refereced roles:
XXRoleDao roleDao = daoMgr.getXXRole();
while (!pending.isEmpty()) {
...
Collection<String> refRoleNames = roleDao.getRoleMemberNames(memberRoleName);
pending.add(refRoleNames);
}
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Roles can contain other roles, but nothing stops a role from ending up inside itself, directly or through other roles. Several RoleREST helpers also expand nested roles recursively, without tracking roles already visited. When a cycle is present, requests that expand it fail.
Validation on save (RoleDBStore)
Create: reject a role whose nested roles contain it. The check walks the stored definitions of the nested roles, loading each role once, so it also catches references that are only written after commit (createNonExistUserGroupRole, for example within an import).
Update: check only the nested roles the update adds. Reject the update if one of them is the role itself or one of its ancestors, found by walking x_role_ref_role ids upward with a visited set. An update that adds no nested role does no extra work.
Grant: relies on updateRole for this check, including granting a role into itself. The separate recursive check in addUsersGroupsAndRoles is removed.
Role expansion (RoleREST)
getRoleMemberNames and getRoleMembers are now iterative, with a visited set. Each nested role is loaded once, and a cycle already present in the data terminates.
ensureRoleAccess reuses the roles loaded during expansion instead of fetching them again. The access decision is unchanged.
getUserRoles shares one visited set across all of the user’s roles, so a nested role common to several of them is loaded once.
A nested role named in a role definition that doesn’t exist (yet) is skipped instead of failing the request.
Behaviour changes
Saving a role that would contain itself now fails with: “a role can not contain itself, directly or through nested roles.”
Existing data with cycles can still be read, and those roles can still be edited.
How was this patch tested?
TestRoleDBStore:
self-nesting rejected on create and on update;
a create rejected when an existing nested role already contains the new role;
adding an ancestor as a nested role rejected;
an update with unchanged nested roles doing no extra lookups;
an existing cycle above the role terminating and the update succeeding.
TestRoleREST:
GET /roles/{id} on a cyclic role returning a denial, with each role loaded at most once;
getUserRoles on a cyclic role terminating;
a missing nested role skipped on read.