mneethiraj commented on code in PR #962:
URL: https://github.com/apache/ranger/pull/962#discussion_r3284092749
##########
security-admin/src/main/java/org/apache/ranger/db/XXResourceDefDao.java:
##########
@@ -101,4 +110,29 @@ public List<XXResourceDef> findByParentResId(Long
parentId) {
return new ArrayList<>();
}
}
+
+ public Map<String, XXResourceDef>
findXXResourceDefsByNameAndPolicyId(Set<String> names, Long policyId) {
Review Comment:
The caller of `findXXResourceDefsByNameAndPolicyId()` only needs resourceId
(`XXResourceDef.id`) for each resource name. Retrieving only `id` from the
database will help improve the performance.
1. I suggest updating & renaming the method from:
```
public Map<String, XXResourceDef>
findXXResourceDefsByNameAndPolicyId(Set<String> names, Long policyId)
```
to:
```
public Map<String, Long> findResourceDefIdsByNameAndPolicyId(Set<String>
names, Long policyId)
```
2. Update the named query `XXResourceDef.findByNamesAndPolicyId` to return
`id` instead of `XXResourceDef`.
3. Review similar updates to following named queries and corresponding Dao
methods that use them:
- `XXAccessTypeDef.findByNamesAndServiceId`
- `XXPolicyConditionDef.findByServiceDefIdAndNames`
- `XXDataMaskTypeDef.findByNamesAndServiceId`
##########
security-admin/src/main/java/org/apache/ranger/db/XXPolicyRefRoleDao.java:
##########
@@ -133,4 +135,34 @@ public void deleteByPolicyId(Long policyId) {
batchDeleteByIds("XXPolicyRefRole.deleteByIds", ids, "ids");
}
+
+ public Map<String, Long> findRoleNameIdByPolicyId(Long policyId) {
+ if (policyId == null) {
+ return Collections.emptyMap();
+ }
+ try {
+ List<Object[]> results = getEntityManager()
+
.createNamedQuery("XXPolicyRefRole.findRoleNameIdByPolicyId", Object[].class)
+ .setParameter("policyId", policyId)
+ .getResultList();
+
+ Map<String, Long> roleNameIdMap = new HashMap<>();
Review Comment:
To be consistent, consider using streams to convert `results` ito
`roleNameIdMap` - similar to `XXRoleDao.getIdsByRoleNames()`.
##########
security-admin/src/main/java/org/apache/ranger/db/XXPolicyRefUserDao.java:
##########
@@ -133,4 +135,34 @@ public void deleteByPolicyId(Long policyId) {
batchDeleteByIds("XXPolicyRefUser.deleteByIds", ids, "ids");
}
+
+ public Map<String, Long> findUserNameIdByPolicyId(Long policyId) {
+ if (policyId == null) {
+ return Collections.emptyMap();
+ }
+ try {
+ List<Object[]> results = getEntityManager()
+
.createNamedQuery("XXPolicyRefUser.findUserNameIdByPolicyId", Object[].class)
+ .setParameter("policyId", policyId)
+ .getResultList();
+
+ Map<String, Long> userNameIdMap = new HashMap<>();
Review Comment:
To be consistent, consider using streams to convert `results` ito
`userNameIdMap` - similar to `XXRoleDao.getIdsByRoleNames()`.
##########
security-admin/src/main/java/org/apache/ranger/db/XXPolicyRefGroupDao.java:
##########
@@ -120,4 +122,34 @@ public void deleteByPolicyId(Long policyId) {
batchDeleteByIds("XXPolicyRefGroup.deleteByIds", ids, "ids");
}
+
+ public Map<String, Long> findGroupNameByPolicyId(Long policyId) {
+ if (policyId == null) {
+ return Collections.emptyMap();
+ }
+ try {
+ List<Object[]> results = getEntityManager()
+
.createNamedQuery("XXPolicyRefGroup.findGroupNameByPolicyId", Object[].class)
+ .setParameter("policyId", policyId)
+ .getResultList();
+
+ Map<String, Long> groupNameIdMap = new HashMap<>();
Review Comment:
To be consistent, consider using streams to convert `results` ito
`groupNameIdMap` - similar to `XXRoleDao.getIdsByRoleNames()`.
##########
security-admin/src/main/java/org/apache/ranger/biz/PolicyRefUpdater.java:
##########
@@ -175,167 +181,171 @@ public void createNewPolMappingForRefTable(RangerPolicy
policy, XXPolicy xPolicy
}
}
- List<XXPolicyRefResource> xPolResources = new ArrayList<>();
-
- for (String resource : resourceNames) {
- XXResourceDef xResDef =
daoMgr.getXXResourceDef().findByNameAndPolicyId(resource, policy.getId());
+ if (isCleanupRefTablesNeeded) {
+ cleanupRefTablesForUpdate(policyId, userNames, roleNames,
groupNames);
+ }
- if (xResDef == null) {
- throw new Exception(resource + ": is not a valid
resource-type. policy='" + policy.getName() + "' service='" +
policy.getService() + "'");
+ if (CollectionUtils.isNotEmpty(resourceNames)) {
+ List<XXPolicyRefResource> xPolResources = new ArrayList<>();
+ Map<String, XXResourceDef> nameToResDefMap =
daoMgr.getXXResourceDef().findXXResourceDefsByNameAndPolicyId(resourceNames,
policy.getId());
+ for (String resource : resourceNames) {
+ XXResourceDef xResDef = nameToResDefMap.get(resource);
+ if (xResDef == null) {
+ throw new Exception(resource + ": is not a valid
resource-type. policy='" + policy.getName() + "' service='" +
policy.getService() + "'");
+ }
+ XXPolicyRefResource xPolRes = new XXPolicyRefResource();
+ xPolRes.setPolicyId(policy.getId());
+ xPolRes.setResourceDefId(xResDef.getId());
+ xPolRes.setResourceName(resource);
+ xPolResources.add(xPolRes);
}
-
- XXPolicyRefResource xPolRes = new XXPolicyRefResource();
-
- xPolRes.setPolicyId(policy.getId());
- xPolRes.setResourceDefId(xResDef.getId());
- xPolRes.setResourceName(resource);
-
- xPolResources.add(xPolRes);
+ batchInsert(xPolResources, daoMgr.getXXPolicyRefResource(),
oldBulkMode);
}
- daoMgr.getXXPolicyRefResource().batchCreate(xPolResources);
-
if (createPrincipalsIfAbsent && !rangerBizUtil.checkAdminAccess()) {
LOG.warn("policy={}: createPrincipalIfAbsent=true, but current
user does not have admin privileges!", policy.getName());
createPrincipalsIfAbsent = false;
}
List<XXPolicyRefRole> xPolRoles = new ArrayList<>();
+ if (CollectionUtils.isNotEmpty(roleNames)) {
+ LOG.debug("x_policy_ref_role - New role entries to insert for
policy ID {}: {}", policyId, roleNames);
+ Set<String> filteredRoleNames = roleNames.stream()
+ .filter(str -> !StringUtils.isBlank(str))
+ .collect(Collectors.toSet());
+ Map<String, Long> roleNameIdMap =
daoMgr.getXXRole().getIdsByRoleNames(filteredRoleNames);
+ for (String roleName : filteredRoleNames) {
+ PolicyPrincipalAssociator associator = new
PolicyPrincipalAssociator(PRINCIPAL_TYPE.ROLE, roleName, xPolicy,
roleNameIdMap.get(roleName), null, null, xPolRoles);
Review Comment:
Instead of having `PolicyPrincipalAssociator` take 3 collection parameters
and populating one of them in `doAssociate()` call, consider:
- creating 3 classes `PolicyRoleAssociator`, `PolicyGroupAssociator` and
`PolicyUserAssociator`
- add method `getPolicyRef()` to return
XXPolicyRefRole/XXPolicyRefGroup/XXPolicyRefUser when the policy exists
- add method `createPolicyRef()` to create
XXPolicyRefRole/XXPolicyRefGroup/XXPolicyRefUser when the policy exists
```
Long roleId = roleNameIdMap.get(roleName);
PolicyRoleAssociator associator = new PolicyRoleAssociator(roleName, roleId,
xPolicy);
if (roleId != null) {
XXPolicyRoleRef roleRef = associator.getPolicyRef();
if (roleRef != null) {
xPolRoles.add(roleRef);
}
} else if (createPrincipalsIfAbsent) {
rangerTransactionSynchronizationAdapter.executeOnTransactionCommit(associator);
} else {
VXResponse gjResponse = new VXResponse();
...
throw restErrorUtil.generateRESTException(gjResponse);
}
```
Review handling of groups and users references as well - line 243 and 268.
##########
security-admin/src/main/java/org/apache/ranger/db/XXResourceDefDao.java:
##########
@@ -101,4 +110,29 @@ public List<XXResourceDef> findByParentResId(Long
parentId) {
return new ArrayList<>();
}
}
+
+ public Map<String, XXResourceDef>
findXXResourceDefsByNameAndPolicyId(Set<String> names, Long policyId) {
+ Map<String, XXResourceDef> ret = Collections.emptyMap();
+ if (policyId == null || CollectionUtils.isEmpty(names)) {
+ return ret;
Review Comment:
Consider having a single `return` statement in a method.
##########
security-admin/src/main/java/org/apache/ranger/biz/PolicyRefUpdater.java:
##########
@@ -356,17 +366,151 @@ public Boolean cleanupRefTables(RangerPolicy policy) {
return true;
}
+ /**
+ * Cleans up reference tables for a given policy before updating it.
+ * This includes handling policy reference users, roles, and other
+ * associated reference entities like resources, groups, access types,
+ * conditions, and data mask types.
+ *
+ * @param policyId ID of the policy being updated.
+ * @param policyUsers Set of usernames associated with the updated
policy.
+ * @param policyRoles Set of role names associated with the updated
policy.
+ * @param policyGroups Set of group names associated with the updated
policy.
+ * @return true if cleanup was successful.
+ */
+ public Boolean cleanupRefTablesForUpdate(Long policyId, Set<String>
policyUsers, Set<String> policyRoles, Set<String> policyGroups) {
+ XXPolicyRefUserDao xPolicyRefUserDao = daoMgr.getXXPolicyRefUser();
+ XXPolicyRefRoleDao xxPolicyRefRoleDao = daoMgr.getXXPolicyRefRole();
+ XXPolicyRefGroupDao xxPolicyRefGroupDao = daoMgr.getXXPolicyRefGroup();
+
+ Map<String, Long> policyUserNameIdMap =
xPolicyRefUserDao.findUserNameIdByPolicyId(policyId);
Review Comment:
Consider moving lines 386, 387 and 388 into cleanupPolicyRefUsers(),
cleanupPolicyRefRoles() and cleanupPolicyRefGroups() respectively - to improve
readability.
--
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]
