You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by "kumaab (via GitHub)" <gi...@apache.org> on 2025/04/02 21:12:25 UTC
[PR] RANGER-5178: Update callers of executeOnTransactionCommit to check for existence of objects [ranger]
kumaab opened a new pull request, #554:
URL: https://github.com/apache/ranger/pull/554
## What changes were proposed in this pull request?
`executeOnTransactionCommit` in `RangerTransactionSynchronizationAdapter.java` is called multiple places for doing post transaction commit tasks in a subsequent transaction following the parent transaction.
The PR adds checks for appropriate objects referenced in classes `PolicyRefUpdater` and `RoleRefUpdater`.
## How was this patch tested?
Pending.
--
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: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5178: Update callers of executeOnTransactionCommit to check for existence of objects [ranger]
Posted by "mneethiraj (via GitHub)" <gi...@apache.org>.
mneethiraj commented on code in PR #554:
URL: https://github.com/apache/ranger/pull/554#discussion_r2025945263
##########
security-admin/src/main/java/org/apache/ranger/biz/PolicyRefUpdater.java:
##########
@@ -516,42 +516,50 @@ private Long createPrincipal(String user) {
return ret;
}
+ private boolean doesPolicyExist(XXPolicy xPolicy) {
+ return daoMgr.getXXPolicy().getById(xPolicy.getId()) != null;
+ }
+
private void createPolicyAssociation(Long id, String name) {
LOG.debug("===> PolicyPrincipalAssociator.createPolicyAssociation(policyId={}, type={}, name={}, id={})", xPolicy.getId(), type.name(), name, id);
- switch (type) {
- case USER: {
- XXPolicyRefUser xPolUser = rangerAuditFields.populateAuditFields(new XXPolicyRefUser(), xPolicy);
+ if (doesPolicyExist(xPolicy)) {
Review Comment:
In addition to checking for existence of policy, shouldn't existence of user/group/role be checked as well?
```
if (doesPolicyExist(xPolicy) && doesPrincipalExist(name, type)) {
```
##########
security-admin/src/main/java/org/apache/ranger/biz/RoleRefUpdater.java:
##########
@@ -345,45 +345,63 @@ private Long createPrincipal(String user) {
return ret;
}
+ private boolean doesPrincipalExist(String name, PolicyRefUpdater.PRINCIPAL_TYPE type) {
+ switch (type) {
+ case USER:
+ return daoMgr.getXXUser().findByUserName(name) != null;
+ case GROUP:
+ return daoMgr.getXXGroup().findByGroupName(name) != null;
+ case ROLE:
+ return daoMgr.getXXRole().findByRoleName(name) != null;
+ default:
+ break;
+ }
+ return false;
+ }
+
private void createRoleAssociation(Long id, String name) {
LOG.debug("===> RolePrincipalAssociator.createRoleAssociation(roleId={}, type={}, name={}, id={})", roleId, type.name(), name, id);
- switch (type) {
- case USER: {
- XXRoleRefUser xRoleRefUser = rangerAuditFields.populateAuditFieldsForCreate(new XXRoleRefUser());
+ if (doesPrincipalExist(name, type)) {
Review Comment:
In addition to checking for existence of principal (user/group/role), shouldn't existence of role be checked as well?
```
if (doesRoleExist(id) && doesPrincipalExist(name, type)) {
```
--
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: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5178: Update callers of executeOnTransactionCommit to check for existence of objects [ranger]
Posted by "kumaab (via GitHub)" <gi...@apache.org>.
kumaab commented on code in PR #554:
URL: https://github.com/apache/ranger/pull/554#discussion_r2029195488
##########
security-admin/src/main/java/org/apache/ranger/biz/RoleRefUpdater.java:
##########
@@ -345,45 +345,63 @@ private Long createPrincipal(String user) {
return ret;
}
+ private boolean doesPrincipalExist(String name, PolicyRefUpdater.PRINCIPAL_TYPE type) {
+ switch (type) {
+ case USER:
+ return daoMgr.getXXUser().findByUserName(name) != null;
+ case GROUP:
+ return daoMgr.getXXGroup().findByGroupName(name) != null;
+ case ROLE:
+ return daoMgr.getXXRole().findByRoleName(name) != null;
+ default:
+ break;
+ }
+ return false;
+ }
+
private void createRoleAssociation(Long id, String name) {
LOG.debug("===> RolePrincipalAssociator.createRoleAssociation(roleId={}, type={}, name={}, id={})", roleId, type.name(), name, id);
- switch (type) {
- case USER: {
- XXRoleRefUser xRoleRefUser = rangerAuditFields.populateAuditFieldsForCreate(new XXRoleRefUser());
+ if (doesPrincipalExist(name, type)) {
Review Comment:
Actually figured out `doesPrincipalExist(name, type)` is redundant as it happens in the caller `Long id = createOrGetPrincipal(isAdmin);` in `boolean doAssociate(boolean isAdmin) {`, check for `roleId` should be done instead, will update the PR.
--
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: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RANGER-5178: Update callers of executeOnTransactionCommit to check for existence of objects [ranger]
Posted by "kumaab (via GitHub)" <gi...@apache.org>.
kumaab commented on code in PR #554:
URL: https://github.com/apache/ranger/pull/554#discussion_r2029172587
##########
security-admin/src/main/java/org/apache/ranger/biz/PolicyRefUpdater.java:
##########
@@ -516,42 +516,50 @@ private Long createPrincipal(String user) {
return ret;
}
+ private boolean doesPolicyExist(XXPolicy xPolicy) {
+ return daoMgr.getXXPolicy().getById(xPolicy.getId()) != null;
+ }
+
private void createPolicyAssociation(Long id, String name) {
LOG.debug("===> PolicyPrincipalAssociator.createPolicyAssociation(policyId={}, type={}, name={}, id={})", xPolicy.getId(), type.name(), name, id);
- switch (type) {
- case USER: {
- XXPolicyRefUser xPolUser = rangerAuditFields.populateAuditFields(new XXPolicyRefUser(), xPolicy);
+ if (doesPolicyExist(xPolicy)) {
Review Comment:
Check for principal is done as a pre-check from the caller of this function in the same thread, for reference:
`Long id = createOrGetPrincipal(isAdmin);` in `boolean doAssociate(boolean isAdmin) {`
--
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: dev-unsubscribe@ranger.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org