pradeepagrawal8184 commented on code in PR #1290:
URL: https://github.com/apache/ranger/pull/1290#discussion_r4204167704
##########
security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java:
##########
@@ -213,6 +214,82 @@ public static boolean processRevokeRequest(RangerPolicy
existingRangerPolicy, Gr
return policyUpdated;
}
+ public static boolean isXPolicyUniqueConstraintViolation(Throwable excp) {
+ return isXPolicyServiceSignatureUniqueConstraintViolation(excp) ||
isXPolicyNameServiceZoneUniqueConstraintViolation(excp);
+ }
+
+ public static boolean
isXPolicyServiceSignatureUniqueConstraintViolation(Throwable excp) {
+ boolean ret = false;
+ Throwable current = excp;
+
+ while (current != null && !ret) {
+ String message = current.getMessage();
+
+ if (StringUtils.containsIgnoreCase(message,
"x_policy_UK_service_signature")) {
Review Comment:
UK / optimistic-lock detection is substring-based on
`Throwable.getMessage()` and the cause chain (see also ~245, ~263–264). That
matches the new unit tests but may miss other JDBC/JPA wrappers. Consider typed
cause handling where Ranger already has it, or document MySQL/EclipseLink
message shapes; otherwise concurrent grants may still surface generic errors
instead of 409.
##########
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java:
##########
@@ -4472,6 +4420,86 @@ private RangerService
hideCriticalServiceDetailsForRoleUser(RangerService ranger
return rangerService;
}
+ private RangerPolicy createRangerPolicy(String serviceName, String
userName, String zoneName, RangerAccessResource resource, GrantRevokeRequest
grantRequest) throws Exception {
+ RangerPolicy policy = new RangerPolicy();
+ String guid = guidUtil.genGUID();
+
+ policy.setService(serviceName);
+ policy.setGuid(guid);
+ policy.setName("grant-" + guid);
+ policy.setDescription("created by grant");
+ policy.setIsAuditEnabled(grantRequest.getEnableAudit());
+ policy.setCreatedBy(userName);
+
+ Map<String, RangerPolicyResource> policyResources = new HashMap<>();
+ Set<String> resourceNames = resource.getKeys();
+
+ if (!CollectionUtils.isEmpty(resourceNames)) {
+ for (String resourceName : resourceNames) {
+ policyResources.put(resourceName,
getPolicyResource(resource.getValue(resourceName), grantRequest));
+ }
+ }
+
+ policy.setResources(policyResources);
+
+ RangerPolicyItem policyItem = new RangerPolicyItem();
+
+ policyItem.setDelegateAdmin(grantRequest.getDelegateAdmin());
+ policyItem.addUsers(grantRequest.getUsers());
+ policyItem.addGroups(grantRequest.getGroups());
+ policyItem.addRoles(grantRequest.getRoles());
+
+ for (String accessType : grantRequest.getAccessTypes()) {
+ policyItem.addAccess(new RangerPolicyItemAccess(accessType,
Boolean.TRUE));
+ }
+
+ policy.addPolicyItem(policyItem);
+ policy.setZoneName(zoneName);
+
+ ensureAdminAccess(policy, userName);
+
+ try {
+ return svcStore.createPolicy(policy);
+ } catch (Exception excp) {
+ if
(ServiceRESTUtil.isXPolicyServiceSignatureUniqueConstraintViolation(excp)) {
+ LOG.debug("createRangerPolicy: signature conflict detected;
trying merge flow. serviceName={}, zoneName={}, policyName={}",
+ serviceName, zoneName, policy.getName());
+
+ String signature = new
RangerPolicyResourceSignature(policy).getSignature();
+ List<RangerPolicy> matchedPolicies =
svcStore.getPoliciesByResourceSignature(serviceName, signature, true);
+ RangerPolicy existingPolicy = null;
+
+ if (CollectionUtils.isNotEmpty(matchedPolicies)) {
+ if (matchedPolicies.size() > 1) {
+ LOG.warn("createRangerPolicy: multiple policies found
for signature conflict fallback. serviceName={}, signature={}, count={}",
+ serviceName, signature,
matchedPolicies.size());
+ }
+
+ existingPolicy = matchedPolicies.get(0);
Review Comment:
When `matchedPolicies.size() > 1`, we only warn and use `get(0)`. Other
create flows treat ambiguous signature matches as hard errors (~1624–1631 in
this class). Should concurrent grant fail closed here instead of merging into
an arbitrary policy?
##########
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java:
##########
@@ -1217,55 +1219,28 @@ public RESTResponse
grantAccess(@PathParam("serviceName") String serviceName, Gr
ensureAdminAccess(policy, userName);
+
ServiceRESTUtil.preserveExistingPolicyGuidForGrantUpdate(svcStore, policy);
svcStore.updatePolicy(policy);
} else {
LOG.error("processGrantRequest processing failed");
throw new Exception("processGrantRequest processing
failed");
}
} else {
- policy = new RangerPolicy();
-
- policy.setService(serviceName);
- policy.setName("grant-" + System.currentTimeMillis()); //
TODO: better policy name
- policy.setDescription("created by grant");
- policy.setIsAuditEnabled(grantRequest.getEnableAudit());
- policy.setCreatedBy(userName);
-
- Map<String, RangerPolicyResource> policyResources = new
HashMap<>();
- Set<String> resourceNames =
resource.getKeys();
-
- if (!CollectionUtils.isEmpty(resourceNames)) {
- for (String resourceName : resourceNames) {
- policyResources.put(resourceName,
getPolicyResource(resource.getValue(resourceName), grantRequest));
- }
- }
-
- policy.setResources(policyResources);
-
- RangerPolicyItem policyItem = new RangerPolicyItem();
-
-
policyItem.setDelegateAdmin(grantRequest.getDelegateAdmin());
- policyItem.addUsers(grantRequest.getUsers());
- policyItem.addGroups(grantRequest.getGroups());
- policyItem.addRoles(grantRequest.getRoles());
-
- for (String accessType : grantRequest.getAccessTypes()) {
- policyItem.addAccess(new
RangerPolicyItemAccess(accessType, Boolean.TRUE));
- }
-
- policy.addPolicyItem(policyItem);
- policy.setZoneName(zoneName);
-
- ensureAdminAccess(policy, userName);
-
- svcStore.createPolicy(policy);
+ createRangerPolicy(serviceName, userName, zoneName,
resource, grantRequest);
}
} catch (WebApplicationException excp) {
throw excp;
} catch (Throwable excp) {
LOG.error("grantAccess({}, {}) failed", serviceName,
grantRequest, excp);
+ if (ServiceRESTUtil.isXPolicyUniqueConstraintViolation(excp)) {
Review Comment:
`isXPolicyUniqueConstraintViolation` includes
`x_policy_UK_name_service_zone`, but this catch always returns
`ERR_CONCURRENT_GRANT_POLICY_CREATE`. With `grant-{guid}` names, name/zone
collisions on create are unlikely; if an update path ever hit name/zone UK, the
message would say "creation." Optional: distinguish CREATE vs UPDATE by
constraint or operation phase. (Same pattern at ~1334 in `secureGrantAccess`.)
##########
security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java:
##########
@@ -3943,6 +3943,104 @@ void
test156GrantBlockedWhenUnauthenticatedAndFlagFalse() throws Exception {
Assertions.assertThrows(WebApplicationException.class, () ->
serviceREST.grantAccess("HDFS_1", grantRequest, request));
}
+ @Test
+ void testGrantAccessMapsUniqueConstraintToConflict409() throws Exception {
+ HttpServletRequest request =
Mockito.mock(HttpServletRequest.class);
+ String serviceName = "HDFS_1";
+ GrantRevokeRequest grantRequestObj = new GrantRevokeRequest();
+
+ Mockito.when(serviceUtil.isValidateHttpsAuthentication(serviceName,
request)).thenReturn(true);
+ Mockito.doThrow(new RuntimeException("Duplicate key
x_policy_UK_service_signature")).when(bizUtil).failUnauthenticatedIfNotAllowed();
Review Comment:
409 tests inject failures via `failUnauthenticatedIfNotAllowed()` /
`findByName()`—they prove catch → 409 mapping, not full grant → `createPolicy`
concurrency. `testCreateRangerPolicyMergesOnSignatureConflict` (~4095) is
stronger. Consider: UK on `createPolicy` → merge succeeds → 200; merge fails →
409.
##########
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java:
##########
@@ -4472,6 +4420,86 @@ private RangerService
hideCriticalServiceDetailsForRoleUser(RangerService ranger
return rangerService;
}
+ private RangerPolicy createRangerPolicy(String serviceName, String
userName, String zoneName, RangerAccessResource resource, GrantRevokeRequest
grantRequest) throws Exception {
+ RangerPolicy policy = new RangerPolicy();
+ String guid = guidUtil.genGUID();
+
+ policy.setService(serviceName);
+ policy.setGuid(guid);
+ policy.setName("grant-" + guid);
+ policy.setDescription("created by grant");
+ policy.setIsAuditEnabled(grantRequest.getEnableAudit());
+ policy.setCreatedBy(userName);
+
+ Map<String, RangerPolicyResource> policyResources = new HashMap<>();
+ Set<String> resourceNames = resource.getKeys();
+
+ if (!CollectionUtils.isEmpty(resourceNames)) {
+ for (String resourceName : resourceNames) {
+ policyResources.put(resourceName,
getPolicyResource(resource.getValue(resourceName), grantRequest));
+ }
+ }
+
+ policy.setResources(policyResources);
+
+ RangerPolicyItem policyItem = new RangerPolicyItem();
+
+ policyItem.setDelegateAdmin(grantRequest.getDelegateAdmin());
+ policyItem.addUsers(grantRequest.getUsers());
+ policyItem.addGroups(grantRequest.getGroups());
+ policyItem.addRoles(grantRequest.getRoles());
+
+ for (String accessType : grantRequest.getAccessTypes()) {
+ policyItem.addAccess(new RangerPolicyItemAccess(accessType,
Boolean.TRUE));
+ }
+
+ policy.addPolicyItem(policyItem);
+ policy.setZoneName(zoneName);
+
+ ensureAdminAccess(policy, userName);
+
+ try {
+ return svcStore.createPolicy(policy);
+ } catch (Exception excp) {
+ if
(ServiceRESTUtil.isXPolicyServiceSignatureUniqueConstraintViolation(excp)) {
+ LOG.debug("createRangerPolicy: signature conflict detected;
trying merge flow. serviceName={}, zoneName={}, policyName={}",
+ serviceName, zoneName, policy.getName());
+
+ String signature = new
RangerPolicyResourceSignature(policy).getSignature();
+ List<RangerPolicy> matchedPolicies =
svcStore.getPoliciesByResourceSignature(serviceName, signature, true);
+ RangerPolicy existingPolicy = null;
+
+ if (CollectionUtils.isNotEmpty(matchedPolicies)) {
+ if (matchedPolicies.size() > 1) {
+ LOG.warn("createRangerPolicy: multiple policies found
for signature conflict fallback. serviceName={}, signature={}, count={}",
+ serviceName, signature,
matchedPolicies.size());
+ }
+
+ existingPolicy = matchedPolicies.get(0);
+ } else {
+ LOG.error("createRangerPolicy: DB signature lookup could
not find policy after signature conflict. serviceName={}, zoneName={},
signature={}",
+ serviceName, zoneName, signature);
+ }
+
+ if (existingPolicy != null &&
ServiceRESTUtil.processGrantRequest(existingPolicy, grantRequest)) {
+ existingPolicy.setZoneName(zoneName);
+ ensureAdminAccess(existingPolicy, userName);
+ RangerPolicy updatedPolicy =
svcStore.updatePolicy(existingPolicy);
Review Comment:
Grant update paths call `preserveExistingPolicyGuidForGrantUpdate` before
`updatePolicy` (~1222, ~1314). This merge-after-UK path calls
`updatePolicy(existingPolicy)` directly. DB-loaded policy likely has GUID
already; calling the same helper would keep behavior consistent if
`processGrantRequest` ever clears GUID.
##########
security-admin/src/test/java/org/apache/ranger/rest/TestServiceRESTUtil.java:
##########
@@ -39,10 +41,16 @@
import java.util.List;
import java.util.Map;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
/**
* @generated by Cursor
Review Comment:
Please remove `@generated by Cursor` before ASF review; keep the standard
license header only.
##########
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java:
##########
@@ -4472,6 +4420,86 @@ private RangerService
hideCriticalServiceDetailsForRoleUser(RangerService ranger
return rangerService;
}
+ private RangerPolicy createRangerPolicy(String serviceName, String
userName, String zoneName, RangerAccessResource resource, GrantRevokeRequest
grantRequest) throws Exception {
+ RangerPolicy policy = new RangerPolicy();
+ String guid = guidUtil.genGUID();
+
+ policy.setService(serviceName);
+ policy.setGuid(guid);
+ policy.setName("grant-" + guid);
+ policy.setDescription("created by grant");
+ policy.setIsAuditEnabled(grantRequest.getEnableAudit());
+ policy.setCreatedBy(userName);
+
+ Map<String, RangerPolicyResource> policyResources = new HashMap<>();
+ Set<String> resourceNames = resource.getKeys();
+
+ if (!CollectionUtils.isEmpty(resourceNames)) {
+ for (String resourceName : resourceNames) {
+ policyResources.put(resourceName,
getPolicyResource(resource.getValue(resourceName), grantRequest));
+ }
+ }
+
+ policy.setResources(policyResources);
+
+ RangerPolicyItem policyItem = new RangerPolicyItem();
+
+ policyItem.setDelegateAdmin(grantRequest.getDelegateAdmin());
+ policyItem.addUsers(grantRequest.getUsers());
+ policyItem.addGroups(grantRequest.getGroups());
+ policyItem.addRoles(grantRequest.getRoles());
+
+ for (String accessType : grantRequest.getAccessTypes()) {
+ policyItem.addAccess(new RangerPolicyItemAccess(accessType,
Boolean.TRUE));
+ }
+
+ policy.addPolicyItem(policyItem);
+ policy.setZoneName(zoneName);
+
+ ensureAdminAccess(policy, userName);
+
+ try {
+ return svcStore.createPolicy(policy);
+ } catch (Exception excp) {
+ if
(ServiceRESTUtil.isXPolicyServiceSignatureUniqueConstraintViolation(excp)) {
+ LOG.debug("createRangerPolicy: signature conflict detected;
trying merge flow. serviceName={}, zoneName={}, policyName={}",
+ serviceName, zoneName, policy.getName());
+
+ String signature = new
RangerPolicyResourceSignature(policy).getSignature();
+ List<RangerPolicy> matchedPolicies =
svcStore.getPoliciesByResourceSignature(serviceName, signature, true);
+ RangerPolicy existingPolicy = null;
+
+ if (CollectionUtils.isNotEmpty(matchedPolicies)) {
+ if (matchedPolicies.size() > 1) {
+ LOG.warn("createRangerPolicy: multiple policies found
for signature conflict fallback. serviceName={}, signature={}, count={}",
+ serviceName, signature,
matchedPolicies.size());
+ }
+
+ existingPolicy = matchedPolicies.get(0);
+ } else {
+ LOG.error("createRangerPolicy: DB signature lookup could
not find policy after signature conflict. serviceName={}, zoneName={},
signature={}",
+ serviceName, zoneName, signature);
+ }
+
+ if (existingPolicy != null &&
ServiceRESTUtil.processGrantRequest(existingPolicy, grantRequest)) {
+ existingPolicy.setZoneName(zoneName);
+ ensureAdminAccess(existingPolicy, userName);
+ RangerPolicy updatedPolicy =
svcStore.updatePolicy(existingPolicy);
+
+ LOG.debug("createRangerPolicy: merge succeeded after
signature conflict. serviceName={}, zoneName={}, policyId={}",
+ serviceName, zoneName, existingPolicy.getId());
+
+ return updatedPolicy;
+ } else if (existingPolicy != null) {
+ LOG.error("createRangerPolicy: processGrantRequest
returned false while merging conflicting create request. serviceName={},
zoneName={}, policyId={}",
+ serviceName, zoneName, existingPolicy.getId());
+ }
+ }
+
+ throw excp;
Review Comment:
If signature UK is caught but merge fails, we rethrow and the outer catch
maps any UK to CREATE 409. Retries may work; clients cannot tell merge was
attempted. Optional: clearer log/metric or message when merge fails after UK.
--
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]
