Copilot commented on code in PR #980:
URL: https://github.com/apache/ranger/pull/980#discussion_r3307506582
##########
security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java:
##########
@@ -115,15 +115,15 @@ public RangerRole createRole(RangerRole role, Boolean
createNonExistUserGroupRol
throw new Exception("Cannot create role:[" + role + "]");
}
- roleRefUpdater.createNewRoleMappingForRefTable(createdRole,
createNonExistUserGroupRole);
+ roleRefUpdater.createNewRoleMappingForRefTable(createdRole,
createNonExistUserGroupRole, false);
Review Comment:
`isRefTableCleanupRequired` is ignored here: the call hard-codes `false`, so
the new method parameter has no effect. Please pass through
`isRefTableCleanupRequired` (or remove the parameter if create should always
skip cleanup).
##########
security-admin/src/main/java/org/apache/ranger/biz/RoleDBStore.java:
##########
@@ -150,7 +150,7 @@ public RangerRole updateRole(RangerRole role, Boolean
createNonExistUserGroupRol
throw new Exception("Cannot update role:[" + role + "]");
}
- roleRefUpdater.createNewRoleMappingForRefTable(updatedRole,
createNonExistUserGroupRole);
+ roleRefUpdater.createNewRoleMappingForRefTable(updatedRole,
createNonExistUserGroupRole, true);
Review Comment:
`isRefTableCleanupRequired` is ignored here: the call hard-codes `true`, so
callers cannot disable selective cleanup if needed. Please pass the
`isRefTableCleanupRequired` argument through to RoleRefUpdater.
##########
security-admin/src/main/java/org/apache/ranger/db/XXRoleRefUserDao.java:
##########
@@ -65,6 +68,25 @@ public List<Long> findIdsByRoleId(Long roleId) {
return ret;
}
+ public Map<String, Long> findUserNameIdMapByRoleId(Long roleId) {
+ Map<String, Long> ret = Collections.emptyMap();
+ if (roleId != null) {
+ try {
+ Collection<Object[]> results = getEntityManager()
+
.createNamedQuery("XXRoleRefUser.findUserNameIdByRoleId", Object[].class)
+ .setParameter("roleId", roleId)
+ .getResultList();
+ ret = results.stream().collect(
+ Collectors.toMap(
+ object -> (String) object[0],
+ object -> (Long) object[1]));
Review Comment:
`Collectors.toMap(...)` is used here without a merge function. Since
`x_role_ref_user` does not enforce uniqueness on (role_id, user_name),
duplicate rows can exist and will cause `toMap()` to throw
`IllegalStateException` during role update. Please add a merge function (e.g.,
keep the first ID) or otherwise handle duplicates defensively.
##########
security-admin/src/main/java/org/apache/ranger/db/XXRoleRefGroupDao.java:
##########
@@ -92,6 +95,25 @@ public List<XXRoleRefGroup> findByGroupName(String
groupName) {
}
}
+ public Map<String, Long> findGroupNameIdByRoleId(Long roleId) {
+ Map<String, Long> ret = Collections.emptyMap();
+ if (roleId != null) {
+ try {
+ Collection<Object[]> results = getEntityManager()
+
.createNamedQuery("XXRoleRefGroup.findGroupNameIdByRoleId", Object[].class)
+ .setParameter("roleId", roleId)
+ .getResultList();
+ ret = results.stream().collect(
+ Collectors.toMap(
+ object -> (String) object[0],
+ object -> (Long) object[1]));
Review Comment:
`Collectors.toMap(...)` is used here without a merge function. Because
`x_role_ref_group` does not enforce uniqueness on (role_id, group_name),
duplicate rows can cause `toMap()` to throw `IllegalStateException` and break
role updates. Please add a merge function (e.g., keep the first ID) or
otherwise handle duplicates defensively.
##########
security-admin/src/main/java/org/apache/ranger/biz/RoleRefUpdater.java:
##########
@@ -186,203 +231,459 @@ public Boolean cleanupRefTables(RangerRole rangerRole) {
return true;
}
- private class RolePrincipalAssociator implements Runnable {
- final PolicyRefUpdater.PRINCIPAL_TYPE type;
- final String name;
- final Long roleId;
+ public Boolean cleanupRefTablesForUpdate(RangerRole rangerRole,
Set<String> roleUsers, Set<String> roleGroups, Set<String> roleRoles) {
+ final Long roleId = rangerRole.getId();
+ if (roleId == null) {
+ return false;
+ }
+
+ cleanupRoleUsers(roleUsers, roleId, daoMgr.getXXRoleRefUser());
+ cleanupRoleGroups(roleGroups, roleId, daoMgr.getXXRoleRefGroup());
+ cleanupRoleRoles(roleRoles, roleId, daoMgr.getXXRoleRefRole());
+
+ return true;
+ }
+
+ /**
+ * Synchronizes the role-user associations for a given role by identifying
and removing
+ * users who are no longer associated with the role in the incoming data.
+ * <p>
+ * This method compares the current set of users assigned to the role
(`roleUsers`)
+ * with the existing role-user mappings stored in the database
(`userNameIdMap`).
+ * It determines which users should be deleted (present in the DB but not
in `roleUsers`)
+ * and removes their associations using the DAO.
+ * <p>
+ * After execution:
+ * <ul>
+ * <li>Users present in both the database and `roleUsers` (i.e., common
users) are left unchanged.</li>
+ * <li>Users present in the database but not in `roleUsers` are
deleted.</li>
+ * <li>The `roleUsers` set is modified to remove users that are already
associated, leaving only new users to be inserted (if needed later).</li>
+ * </ul>
+ *
+ * @param roleUsers Set of usernames that should be currently
associated with the role.
+ * This is the source of truth, typically coming
from an external request.
+ * @param roleId ID of the role for which the cleanup is being
performed.
+ * @param dao DAO for role reference users.
+ */
+ private void cleanupRoleUsers(Set<String> roleUsers, Long roleId,
XXRoleRefUserDao dao) {
+ Map<String, Long> userNameIdMap =
dao.findUserNameIdMapByRoleId(roleId);
+ if (userNameIdMap != null && !userNameIdMap.isEmpty()) {
+ Set<String> existingUsers = userNameIdMap.keySet();
+ Set<String> toDeleteUsers = new HashSet<>(existingUsers);
+ Set<String> commonUsers = new HashSet<>(roleUsers);
+
+ // Identify users present in both new and existing sets
+ commonUsers.retainAll(toDeleteUsers);
+
+ // Identify users to delete (in DB but not in new set)
+ toDeleteUsers.removeAll(commonUsers);
+
+ // Remove already existing users from the new set (they don’t need
to be inserted again)
+ roleUsers.removeAll(commonUsers);
+ if (CollectionUtils.isNotEmpty(toDeleteUsers)) {
+ List<Long> idsToDelete = toDeleteUsers.stream()
+ .map(userNameIdMap::get)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+
+ LOG.debug("Deleting user IDs from x_role_ref_user table: {}
for role ID: {}", idsToDelete, roleId);
+ dao.deleteRoleRefUserByIds(idsToDelete);
+ }
+ }
+ }
+
+ /**
+ * Synchronizes role-group associations for a given role by identifying
and removing
+ * groups that are no longer associated with the role in the incoming data.
+ * <p>
+ * This method compares the current set of groups that should be
associated with a role
+ * (`roleGroups`) against the existing group-role mappings from the
database (`groupNameIdMap`).
+ * It determines which group associations should be deleted (present in DB
but not in input set),
+ * and deletes those associations using the DAO.
+ * <p>
+ * After execution:
+ * <ul>
+ * <li>Groups present in both the input and DB (i.e., common groups) are
preserved.</li>
+ * <li>Groups present in the DB but not in the input set are
deleted.</li>
+ * <li>The `roleGroups` input set is modified to only include new groups
to be inserted later (if any).</li>
+ * </ul>
+ *
+ * @param roleGroups Set of group names that should be associated
with the role.
+ * This is the latest source of truth (e.g., from
an API or external input).
+ * @param roleId The ID of the role for which associations are
being synced.
+ * @param dao DAO for role reference groups.
+ */
+ private void cleanupRoleGroups(Set<String> roleGroups, Long roleId,
XXRoleRefGroupDao dao) {
+ Map<String, Long> groupNameIdMap = dao.findGroupNameIdByRoleId(roleId);
+ if (groupNameIdMap != null && !groupNameIdMap.isEmpty()) {
+ // All groups currently associated in the DB
+ Set<String> existingGroups = groupNameIdMap.keySet();
+
+ // Create a mutable set to track deletions
+ Set<String> toDeleteGroups = new HashSet<>(existingGroups);
+
+ // Identify common groups between input and existing
+ Set<String> commonGroups = new HashSet<>(roleGroups);
+ commonGroups.retainAll(toDeleteGroups);
+
+ // Remove common groups from both sets to isolate only new and
obsolete entries
+ toDeleteGroups.removeAll(commonGroups); // only those to be deleted
+ roleGroups.removeAll(commonGroups); // only those to be newly
inserted
+
+ if (CollectionUtils.isNotEmpty(toDeleteGroups)) {
+ List<Long> idsToDelete = toDeleteGroups.stream()
+ .map(groupNameIdMap::get)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+
+ LOG.debug("Deleting group IDs from x_role_ref_group table: {}
for role ID: {}", idsToDelete, roleId);
+ dao.deleteRoleRefGroupByIds(idsToDelete);
+ }
+ }
+ }
+
+ /**
+ * Synchronizes sub-role associations for a given parent role by
identifying and removing
+ * sub-roles that are no longer associated with the parent role in the
incoming data.
+ * <p>
+ * This method compares the current set of sub-roles that should be
associated with a role
+ * (`roleRoles`) against the existing role-role mappings stored in the
database (`subRoleNameIdMap`).
+ * It determines which sub-role associations should be removed (those
present in the DB but not in the new set)
+ * and deletes them using the provided DAO.
+ * <p>
+ * After execution:
+ * <ul>
+ * <li>Sub-roles that are present in both the input set and DB (common
roles) are preserved.</li>
+ * <li>Sub-roles that are present in the DB but missing from the input
set are deleted.</li>
+ * <li>The input set `roleRoles` is modified to retain only new
sub-roles that may be inserted later.</li>
+ * </ul>
+ *
+ * @param roleRoles A set of sub-role names that should currently
be associated with the parent role.
+ * This represents the latest state (e.g., from
an external request).
+ * @param roleId The ID of the parent role for which sub-role
associations are being updated.
+ * @param dao DAO for role reference roles.
+ */
+ private void cleanupRoleRoles(Set<String> roleRoles, Long roleId,
XXRoleRefRoleDao dao) {
+ Map<String, Long> subRoleNameIdMap =
dao.findSubRoleNameIdByRoleId(roleId);
+ if (subRoleNameIdMap != null && !subRoleNameIdMap.isEmpty()) {
+ // Get the current set of associated sub-roles from DB
+ Set<String> existingRoles = subRoleNameIdMap.keySet();
+
+ // Prepare to identify obsolete sub-roles
+ Set<String> toDeleteRoles = new HashSet<>(existingRoles);
+
+ // Identify roles that are common in both new and existing sets
+ Set<String> commonRoles = new HashSet<>(roleRoles);
+ commonRoles.retainAll(toDeleteRoles);
+
+ // Remove common roles from the deletion list and the new input set
+ toDeleteRoles.removeAll(commonRoles);
+ roleRoles.removeAll(commonRoles);
+
+ // Delete obsolete sub-role associations
+ if (!toDeleteRoles.isEmpty()) {
+ List<Long> idsToDelete = toDeleteRoles.stream()
+ .map(subRoleNameIdMap::get)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+
+ LOG.debug("Deleting role IDs from x_role_ref_role table: {}
for role ID: {}", idsToDelete, roleId);
+ dao.deleteRoleRefRoleByIds(idsToDelete);
+ }
+ }
+ }
+
+ private boolean doesRoleExist(Long roleId) {
+ return roleId != null && daoMgr.getXXRole().findByRoleId(roleId) !=
null;
Review Comment:
`doesRoleExist()` uses `XXRoleDao.findByRoleId()`, which runs a named-query
(`getSingleResult()`) and can hit the DB each time. Since `doesRoleExist()` is
called for every principal in `getRoleRef()/createRoleRef()`, this can
reintroduce N+1 queries on large roles. Consider avoiding the per-principal
existence check (or caching the result once), and/or using
`daoMgr.getXXRole().getById(roleId)` to leverage the persistence context.
##########
security-admin/src/main/java/org/apache/ranger/db/XXRoleRefRoleDao.java:
##########
@@ -124,6 +127,25 @@ public Set<Long> getContainingRoles(Long subRoleId) {
return ret;
}
+ public Map<String, Long> findSubRoleNameIdByRoleId(Long roleId) {
+ Map<String, Long> ret = Collections.emptyMap();
+ if (roleId != null) {
+ try {
+ Collection<Object[]> results = getEntityManager()
+
.createNamedQuery("XXRoleRefRole.findSubRoleNameIdByRoleId", Object[].class)
+ .setParameter("roleId", roleId)
+ .getResultList();
+ ret = results.stream().collect(
+ Collectors.toMap(
+ object -> (String) object[0],
+ object -> (Long) object[1]));
Review Comment:
`Collectors.toMap(...)` is used here without a merge function. Because
`x_role_ref_role` does not enforce uniqueness on (role_id, sub_role_name),
duplicate rows can cause `toMap()` to throw `IllegalStateException` and break
role updates. Please add a merge function (e.g., keep the first ID) or
otherwise handle duplicates defensively.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
