Skip to content

RANGER-5850: Validate nested role membership for cycles and avoid rec… #1324

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

Open
vyommani wants to merge 1 commit into
base: master
Choose a base branch
from
Open

RANGER-5850: Validate nested role membership for cycles and avoid rec… #1324

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
118 changes: 118 additions & 0 deletions security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,12 @@
import org.springframework.stereotype.Component;

import javax.annotation.PostConstruct;
import javax.ws.rs.WebApplicationException;

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Deque;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -113,6 +116,8 @@ public RangerRole createRole(RangerRole role, Boolean createNonExistUserGroupRol
throw restErrorUtil.createRESTException("role with name: " + role.getName() + " already exists", MessageEnums.ERROR_DUPLICATE_OBJECT);
}

ensureRoleNotNestedInItself(role);

Runnable roleVersionUpdater = new RoleVersionUpdater(daoMgr);
transactionSynchronizationAdapter.executeOnTransactionCommit(roleVersionUpdater);

Expand Down Expand Up @@ -149,6 +154,8 @@ public RangerRole updateRole(RangerRole role, Boolean createNonExistUserGroupRol
oldRole = JsonUtils.jsonToObject(xxRole.getRoleText(), RangerRole.class);
}

ensureNoRoleNestingCycle(role, xxRole, oldRole);

Runnable roleVersionUpdater = new RoleVersionUpdater(daoMgr);

transactionSynchronizationAdapter.executeOnTransactionCommit(roleVersionUpdater);
Expand Down Expand Up @@ -454,6 +461,117 @@ private void ensureRoleNameUpdateAllowed(String roleName) throws Exception {
}
}

/**
* Rejects a new role whose nested roles contain it, directly or transitively.
*
* The ref table can't be used here: when an earlier save referenced a not-yet-existing role with
* createNonExistUserGroupRole=true (e.g. earlier in the same import), its x_role_ref_role row is only written
* after commit, so that edge exists only in the stored role JSON. So walk the new role's nested roles downwards
* using their stored definitions. Each role is loaded at most once; a role with no nested roles costs nothing.
*/
private void ensureRoleNotNestedInItself(RangerRole role) {
Set<String> visited = new HashSet<>();
Deque<String> pending = new ArrayDeque<>();

for (RangerRole.RoleMember member : role.getRoles()) {
if (member != null && StringUtils.isNotEmpty(member.getName())) {
pending.add(member.getName());
}
}

while (!pending.isEmpty()) {
String nestedRoleName = pending.poll();

@mneethiraj mneethiraj Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nestedRoleName => memberRoleName


if (StringUtils.equals(nestedRoleName, role.getName())) {
throw createNestingCycleException(role.getName());
}

if (!visited.add(nestedRoleName)) {
continue;
}

XXRole xxNestedRole = daoMgr.getXXRole().findByRoleName(nestedRoleName);

@mneethiraj mneethiraj Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
  }


if (xxNestedRole == null) { // will be created empty
continue;
}

RangerRole nestedRole = roleService.read(xxNestedRole.getId());

if (nestedRole != null) {
for (RangerRole.RoleMember member : nestedRole.getRoles()) {
if (member != null && StringUtils.isNotEmpty(member.getName())) {
pending.add(member.getName());
}
}
}
}
}

/**
* Rejects an update that would make the role contain itself, directly or through nested roles.
*
* Only nested roles added by this update are checked: an edge role -> N closes a cycle iff N is the role
* itself or one of its ancestors. Ancestors are found by walking x_role_ref_role upwards by id (the same walk
* updatePolicyVersions() already performs on every update), so no role JSON is loaded. Updates that do not add
* a nested role (e.g. user/group membership changes) do no extra DB work.
*/
private void ensureNoRoleNestingCycle(RangerRole role, XXRole xxRole, RangerRole oldRole) {
Set<String> addedRoleNames = new HashSet<>();

for (RangerRole.RoleMember member : role.getRoles()) {
if (member != null && StringUtils.isNotEmpty(member.getName())) {
addedRoleNames.add(member.getName());
}
}

if (oldRole != null) {
for (RangerRole.RoleMember member : oldRole.getRoles()) {
if (member != null) {
addedRoleNames.remove(member.getName());
}
}
}

if (addedRoleNames.isEmpty()) {
return;
}

if (addedRoleNames.contains(role.getName()) || addedRoleNames.contains(xxRole.getName())) {
throw createNestingCycleException(role.getName());
}

// roles that don't exist yet are created empty, so they can't close a cycle
Set<Long> addedRoleIds = new HashSet<>(daoMgr.getXXRole().getIdsByRoleNames(addedRoleNames).values());

if (addedRoleIds.isEmpty()) {
return;
}

Set<Long> visited = new HashSet<>();
Deque<Long> pending = new ArrayDeque<>();

pending.add(xxRole.getId());

while (!pending.isEmpty()) {
Long roleId = pending.poll();

if (addedRoleIds.contains(roleId)) {
throw createNestingCycleException(role.getName());
}

if (visited.add(roleId)) {
pending.addAll(daoMgr.getXXRoleRefRole().getContainingRoles(roleId));
}
}
}

private WebApplicationException createNestingCycleException(String roleName) {
LOG.error("Role {} can not be saved: nested roles would make it contain itself", roleName);

return restErrorUtil.createRESTException("Invalid role members for role " + roleName + ": a role can not contain itself, directly or through nested roles", MessageEnums.INVALID_INPUT_DATA);
}

private void ensureRoleDeleteAllowed(String roleName) throws Exception {
boolean roleNotInPolicy = ensureRoleNotInPolicy(roleName);

Expand Down
108 changes: 80 additions & 28 deletions security-admin/src/main/java/org/apache/ranger/rest/RoleREST.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,16 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;

@Path("roles")
Expand Down Expand Up @@ -1007,15 +1011,16 @@ public List<String> getUserRoles(@PathParam("user") String userName, @Context Ht

Set<RangerRole> roleList = roleStore.getRoleNames(userName, userMgr.getGroupsForUser(userName));

// one visited set across all of the user's roles: a nested role shared by several of them is loaded once
Set<String> nestedRoleNames = new HashSet<>();

for (RangerRole role : roleList) {
ret.add(role.getName());

Set<String> roleMembers = new HashSet<>();

getRoleMemberNames(roleMembers, role);

ret.addAll(roleMembers);
getRoleMemberNames(nestedRoleNames, role);
}

ret.addAll(nestedRoleNames);
} catch (WebApplicationException excp) {
throw excp;
} catch (Throwable excp) {
Expand Down Expand Up @@ -1407,15 +1412,18 @@ private boolean ensureRoleAccess(String username, Set<String> userGroups, Ranger
}

Set<RangerRole.RoleMember> roleMemberList = new HashSet<>();

getRoleMembers(roleMemberList, role);
Map<String, RangerRole> loadedRoles = getRoleMembers(roleMemberList, role);

for (RangerRole.RoleMember roleMember : roleMemberList) {
if (!roleMember.getIsAdmin()) {
continue;
}

RangerRole roleMemberObj = roleStore.getRole(roleMember.getName());
RangerRole roleMemberObj = loadedRoles.get(roleMember.getName()); // already loaded while expanding

if (roleMemberObj == null) { // nested role doesn't exist (yet)
continue;
}

if (getUserNames(roleMemberObj).contains(username)) {
isAccessible = true;
Expand Down Expand Up @@ -1447,19 +1455,7 @@ private RangerRole addUsersGroupsAndRoles(RangerRole role, Set<String> users, Se
}

try {
// Check for any cycling relationship between roles
for (String newRole : roles) {
//get members recursively and check if the grantor role is already a member
Set<String> roleMembers = new HashSet<>();

getRoleMemberNames(roleMembers, roleStore.getRole(newRole));

LOG.debug("Role members for {} = {}", newRole, roleMembers);

if (roleMembers.contains(role.getName())) {
throw new Exception("Invalid role grant");
}
}
// nesting cycles, including granting a role into itself, are rejected by RoleDBStore.updateRole()

Set<RangerRole.RoleMember> roleUsers = new HashSet<>();
Set<RangerRole.RoleMember> roleGroups = new HashSet<>();
Expand Down Expand Up @@ -1678,19 +1674,75 @@ private Set<String> getRoleNames(RangerRole role) {
return rolenames;
}

/*
* Adds names of all roles nested, directly or transitively, under the given role. Iterative with a visited
* set: each role is loaded at most once and a cyclic role graph terminates instead of overflowing the stack.
*/
private void getRoleMemberNames(Set<String> roleMembers, RangerRole role) throws Exception {
for (RangerRole.RoleMember roleMember : role.getRoles()) {
roleMembers.add(roleMember.getName());
Deque<RangerRole> pending = new ArrayDeque<>();

pending.add(role);

while (!pending.isEmpty()) {
RangerRole current = pending.poll();

for (RangerRole.RoleMember roleMember : current.getRoles()) {
if (roleMembers.add(roleMember.getName())) {
RangerRole memberRole = getNestedRoleIfExists(roleMember.getName());

if (memberRole != null) {
pending.add(memberRole);
}
}
}
}
}

/*
* Adds all role members nested, directly or transitively, under the given role, and returns the nested roles
* loaded along the way (by name). Iterative: each nested role is loaded at most once, so a cyclic role graph
* terminates instead of overflowing the stack.
*/
private Map<String, RangerRole> getRoleMembers(Set<RangerRole.RoleMember> roleMembers, RangerRole role) throws Exception {
Map<String, RangerRole> loaded = new HashMap<>();
Deque<RangerRole> pending = new ArrayDeque<>();

pending.add(role);

while (!pending.isEmpty()) {
RangerRole current = pending.poll();

for (RangerRole.RoleMember roleMember : current.getRoles()) {
roleMembers.add(roleMember);

if (!loaded.containsKey(roleMember.getName())) {
RangerRole memberRole = getNestedRoleIfExists(roleMember.getName());

getRoleMemberNames(roleMembers, roleStore.getRole(roleMember.getName()));
loaded.put(roleMember.getName(), memberRole);

if (memberRole != null) {
pending.add(memberRole);
}
}
}
}

return loaded;
}

private void getRoleMembers(Set<RangerRole.RoleMember> roleMembers, RangerRole role) throws Exception {
for (RangerRole.RoleMember roleMember : role.getRoles()) {
roleMembers.add(roleMember);
/*
* A nested role named in a stored role definition may not exist yet, e.g. when it is created after commit by
* createNonExistUserGroupRole. Skip it instead of failing the whole read.
* RoleDBStore.getRole(String) throws WebApplicationException only when the role is not found (by name, or by id
* in roleService.read() if deleted in between); DB failures surface as persistence exceptions and still propagate.
*/
private RangerRole getNestedRoleIfExists(String roleName) {
try {
return roleStore.getRole(roleName);
} catch (WebApplicationException excp) {
LOG.warn("Nested role {} does not exist; skipping it", roleName);

getRoleMembers(roleMembers, roleStore.getRole(roleMember.getName()));
return null;
}
}

Expand Down
Loading