This is an automated email from the ASF dual-hosted git repository.
abhay pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ranger.git
The following commit(s) were added to refs/heads/master by this push:
new 856571c RANGER-3490: Make policy resource signature is unique in a
service
856571c is described below
commit 856571c4348e31725498c0922338339c76ebba02
Author: Abhay Kulkarni <ab...@apache.org>
AuthorDate: Wed Nov 24 07:38:20 2021 -0800
RANGER-3490: Make policy resource signature is unique in a service
---
.../model/RangerPolicyResourceSignature.java | 5 ++++
.../model/validation/RangerPolicyValidator.java | 35 ++++++++++------------
.../plugin/model/validation/RangerValidator.java | 21 +++++++++----
.../model/TestRangerPolicyResourceSignature.java | 18 +++++++++++
.../validation/TestRangerPolicyValidator.java | 24 +++++++++------
.../model/validation/TestRangerValidator.java | 4 +--
.../java/org/apache/ranger/biz/ServiceDBStore.java | 30 ++++++++++++-------
.../org/apache/ranger/biz/TestServiceDBStore.java | 4 +++
8 files changed, 95 insertions(+), 46 deletions(-)
diff --git
a/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
b/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
index 312005e..c84d0bc 100644
---
a/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
+++
b/agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java
@@ -121,6 +121,8 @@ public class RangerPolicyResourceSignature {
LOG.debug("isPolicyValidForResourceSignatureComputation: resources collection
on policy was null!");
} else if (_policy.getResources().containsKey(null)) {
LOG.debug("isPolicyValidForResourceSignatureComputation: resources collection
has resource with null name!");
+ } else if (StringUtils.isEmpty(_policy.getGuid())) {
+
LOG.debug("isPolicyValidForResourceSignatureComputation: policy GUID is
empty!");
} else {
valid = true;
}
@@ -163,6 +165,9 @@ public class RangerPolicyResourceSignature {
CustomConditionSerialiser
customConditionSerialiser = new
CustomConditionSerialiser(_policy.getConditions());
resource +=
customConditionSerialiser.toString();
}
+ if (!_policy.getIsEnabled()) {
+ resource += _policy.getGuid();
+ }
String result =
String.format("{version=%d,type=%d,resource=%s}", _SignatureVersion, type,
resource);
return result;
diff --git
a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
index 0ba1fb9..0519227 100644
---
a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
+++
b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java
@@ -539,25 +539,22 @@ public class RangerPolicyValidator extends
RangerValidator {
}
boolean valid = true;
- if (!Boolean.TRUE.equals(policy.getIsEnabled())) {
- LOG.debug("Policy is disabled. Skipping resource
uniqueness validation.");
- } else {
- RangerPolicyResourceSignature policySignature =
_factory.createPolicyResourceSignature(policy);
- String signature = policySignature.getSignature();
- List<RangerPolicy> policies =
getPoliciesForResourceSignature(policy.getService(), signature);
- if (CollectionUtils.isNotEmpty(policies)) {
- ValidationErrorCode error =
ValidationErrorCode.POLICY_VALIDATION_ERR_DUPLICATE_POLICY_RESOURCE;
- RangerPolicy matchedPolicy =
policies.iterator().next();
- // there shouldn't be a matching policy for
create. During update only match should be to itself
- if (action == Action.CREATE || (action ==
Action.UPDATE && (policies.size() > 1 ||
!matchedPolicy.getId().equals(policy.getId())))) {
- failures.add(new
ValidationFailureDetailsBuilder()
- .field("resources")
-
.isSemanticallyIncorrect()
-
.becauseOf(error.getMessage(matchedPolicy.getName(), policy.getService()))
-
.errorCode(error.getErrorCode())
- .build());
- valid = false;
- }
+
+ RangerPolicyResourceSignature policySignature =
_factory.createPolicyResourceSignature(policy);
+ String signature = policySignature.getSignature();
+ List<RangerPolicy> policies =
getPoliciesForResourceSignature(policy.getService(), signature);
+ if (CollectionUtils.isNotEmpty(policies)) {
+ ValidationErrorCode error =
ValidationErrorCode.POLICY_VALIDATION_ERR_DUPLICATE_POLICY_RESOURCE;
+ RangerPolicy matchedPolicy = policies.iterator().next();
+ // there shouldn't be a matching policy for create.
During update only match should be to itself
+ if (action == Action.CREATE || (action == Action.UPDATE
&& !matchedPolicy.getId().equals(policy.getId()))) {
+ failures.add(new
ValidationFailureDetailsBuilder()
+ .field("resources")
+ .isSemanticallyIncorrect()
+
.becauseOf(error.getMessage(matchedPolicy.getName(), policy.getService()))
+ .errorCode(error.getErrorCode())
+ .build());
+ valid = false;
}
}
diff --git
a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
index b02090b..e99598d 100644
---
a/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
+++
b/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java
@@ -346,19 +346,30 @@ public abstract class RangerValidator {
if(LOG.isDebugEnabled()) {
LOG.debug(String.format("==>
RangerValidator.getPoliciesForResourceSignature(%s, %s)", serviceName,
policySignature));
}
+ final List<RangerPolicy> ret;
- List<RangerPolicy> policies = null;
+ List<RangerPolicy> enabledPolicies = new ArrayList<>();
+ List<RangerPolicy> disabledPolicies = new ArrayList<>();
try {
- policies =
_store.getPoliciesByResourceSignature(serviceName, policySignature, true); //
only look for enabled policies
+ enabledPolicies =
_store.getPoliciesByResourceSignature(serviceName, policySignature, true);
+ disabledPolicies =
_store.getPoliciesByResourceSignature(serviceName, policySignature, false);
} catch (Exception e) {
LOG.debug("Encountred exception while retrieving
policies from service store!", e);
}
+ if (CollectionUtils.isEmpty(enabledPolicies)) {
+ ret = disabledPolicies;
+ } else if (CollectionUtils.isEmpty(disabledPolicies)) {
+ ret = enabledPolicies;
+ } else {
+ ret = enabledPolicies;
+ ret.addAll(disabledPolicies);
+ }
if(LOG.isDebugEnabled()) {
- int count = policies == null ? 0 : policies.size();
- LOG.debug(String.format("<==
RangerValidator.getPoliciesForResourceSignature(%s, %s): count[%d], %s",
serviceName, policySignature, count, policies));
+ int count = ret == null ? 0 : ret.size();
+ LOG.debug(String.format("<==
RangerValidator.getPoliciesForResourceSignature(%s, %s): count[%d], %s",
serviceName, policySignature, count, ret));
}
- return policies;
+ return ret;
}
RangerSecurityZone getSecurityZone(Long id) {
diff --git
a/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
b/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
index bc9df31..87075ec 100644
---
a/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
+++
b/agents-common/src/test/java/org/apache/ranger/plugin/model/TestRangerPolicyResourceSignature.java
@@ -89,6 +89,7 @@ public class TestRangerPolicyResourceSignature {
// empty resources map is ok!
Map<String, RangerPolicyResource> policyResources = new
HashMap<>();
when(rangerPolicy.getResources()).thenReturn(policyResources);
+ when(rangerPolicy.getGuid()).thenReturn("TEST_GUID");
policySerializer = new PolicySerializer(rangerPolicy);
Assert.assertTrue("policy.getResources().isEmpty()",
policySerializer.isPolicyValidForResourceSignatureComputation());
@@ -159,6 +160,8 @@ public class TestRangerPolicyResourceSignature {
Map<String, RangerPolicyResource> policyResources =
_utils.createPolicyResourceMap(first);
when(policy.getResources()).thenReturn(policyResources);
when(policy.getZoneName()).thenReturn(null);
+ when(policy.getGuid()).thenReturn("TEST_GUID");
+ when(policy.getIsEnabled()).thenReturn(true);
serializer = new PolicySerializer(policy);
String expectedVersion = "version=1";
String expectedType = "type=0";
@@ -190,6 +193,9 @@ public class TestRangerPolicyResourceSignature {
RangerPolicy policy2 =
createPolicy(first_recursive_null_or_false);
RangerPolicyResourceSignature signature1 = new
RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new
RangerPolicyResourceSignature(policy2);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
+
Assert.assertEquals("Recursive flag: null is same as false",
signature1.toString(), signature2.toString());
}
@@ -198,6 +204,8 @@ public class TestRangerPolicyResourceSignature {
// create two policies with resources that differ only in the
recursive flag, i.e. null/false in one and true in another
RangerPolicy policy1 = createPolicy(first);
RangerPolicy policy2 =
createPolicy(first_recursive_flag_different);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
RangerPolicyResourceSignature signature1 = new
RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new
RangerPolicyResourceSignature(policy2);
Assert.assertFalse("Resources differ only by recursive flag
true vs false/null", signature1.toString().equals(signature2.toString()));
@@ -208,6 +216,8 @@ public class TestRangerPolicyResourceSignature {
// create two policies with resources that differ only in the
excludes flag such that flags are null in one and false in another
RangerPolicy policy1 = createPolicy(first);
RangerPolicy policy2 =
createPolicy(first_excludes_null_or_false);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
RangerPolicyResourceSignature signature1 = new
RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new
RangerPolicyResourceSignature(policy2);
Assert.assertEquals("Excludes flag: null is same as false",
signature1.toString(), signature2.toString());
@@ -218,6 +228,8 @@ public class TestRangerPolicyResourceSignature {
// create two policies with resources that differ only in the
excludes flag, i.e. null/false in one and true in another
RangerPolicy policy1 = createPolicy(first);
RangerPolicy policy2 =
createPolicy(first_excludes_flag_different);
+ when(policy1.getGuid()).thenReturn("TEST_GUID-1");
+ when(policy2.getGuid()).thenReturn("TEST_GUID-2");
RangerPolicyResourceSignature signature1 = new
RangerPolicyResourceSignature(policy1);
RangerPolicyResourceSignature signature2 = new
RangerPolicyResourceSignature(policy2);
Assert.assertFalse("Resources differ only by excludes flag true
vs false/null", signature1.toString().equals(signature2.toString()));
@@ -227,6 +239,7 @@ public class TestRangerPolicyResourceSignature {
RangerPolicy policy = mock(RangerPolicy.class);
Map<String, RangerPolicyResource> resources =
_utils.createPolicyResourceMap(data);
when(policy.getResources()).thenReturn(resources);
+ when(policy.getIsEnabled()).thenReturn(true);
return policy;
}
@@ -236,11 +249,16 @@ public class TestRangerPolicyResourceSignature {
RangerPolicy aPolicy = mock(RangerPolicy.class);
Map<String, RangerPolicyResource> resources =
_utils.createPolicyResourceMap(first);
when(aPolicy.getResources()).thenReturn(resources);
+ when(aPolicy.getIsEnabled()).thenReturn(true);
+ when(aPolicy.getGuid()).thenReturn("TEST_GUID-1");
RangerPolicyResourceSignature signature = new
RangerPolicyResourceSignature(aPolicy);
RangerPolicy anotherPolicy = mock(RangerPolicy.class);
resources = _utils.createPolicyResourceMap(data_second);
when(anotherPolicy.getResources()).thenReturn(resources);
+ when(anotherPolicy.getIsEnabled()).thenReturn(true);
+ when(anotherPolicy.getGuid()).thenReturn("TEST_GUID-2");
+
RangerPolicyResourceSignature anotherSignature = new
RangerPolicyResourceSignature(anotherPolicy);
Assert.assertTrue(signature.equals(anotherSignature));
Assert.assertTrue(anotherSignature.equals(signature));
diff --git
a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
index f0e2d07..0c6c4c2 100644
---
a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
+++
b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java
@@ -205,6 +205,12 @@ public class TestRangerPolicyValidator {
when(_store.getServiceByName("service-name2")).thenReturn(service2);
when(_policy.getService()).thenReturn("service-name2");
+
+ RangerPolicyResourceSignature policySignature =
mock(RangerPolicyResourceSignature.class);
+ when(policySignature.getSignature()).thenReturn("hash-1");
+
+
when(_factory.createPolicyResourceSignature(_policy)).thenReturn(policySignature);
+
when(_store.getServiceByName("service-name2")).thenReturn(service2);
action = Action.UPDATE;
@@ -730,26 +736,26 @@ public class TestRangerPolicyValidator {
when(_policy.getIsEnabled()).thenReturn(true); // ensure policy
is enabled
_failures.clear();
Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures,
Action.CREATE));
_utils.checkFailureForSemanticError(_failures, "resources");
- // same check should pass if the policy is disabled
+ // same check should fail even if the policy is disabled
when(_policy.getIsEnabled()).thenReturn(false);
- _failures.clear();
Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures,
Action.CREATE));
- Assert.assertTrue("failures collection wasn't empty!",
_failures.isEmpty());
+ _failures.clear();
Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures,
Action.CREATE));
+ _utils.checkFailureForSemanticError(_failures, "resources");
// For Update match with itself is not a problem as long as it
isn't itself, i.e. same id.
when(_policy.getIsEnabled()).thenReturn(true); // ensure policy
is enabled
when(policy1.getId()).thenReturn(103L);
when(_policy.getId()).thenReturn(103L);
Assert.assertTrue(_validator.isPolicyResourceUnique(_policy,
_failures, Action.UPDATE));
-
+
// matching policy can't be some other policy (i.e. different
id) because that implies a conflict.
when(policy1.getId()).thenReturn(104L);
Assert.assertFalse(_validator.isPolicyResourceUnique(_policy,
_failures, Action.UPDATE));
_utils.checkFailureForSemanticError(_failures, "resources");
// same check should pass if the policy is disabled
when(_policy.getIsEnabled()).thenReturn(false);
- _failures.clear();
Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures,
Action.UPDATE));
- Assert.assertTrue("failures collection wasn't empty!",
_failures.isEmpty());
-
+ _failures.clear();
Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures,
Action.UPDATE));
+ _utils.checkFailureForSemanticError(_failures, "resources");
+
// And validation should never pass if there are more than one
policies with matching signature, regardless of their ID!!
RangerPolicy policy2 = mock(RangerPolicy.class);
when(policy2.getId()).thenReturn(103L); // has same id as the
policy being tested (_policy)
@@ -759,8 +765,8 @@ public class TestRangerPolicyValidator {
_utils.checkFailureForSemanticError(_failures, "resources");
// same check should pass if the policy is disabled
when(_policy.getIsEnabled()).thenReturn(false);
- _failures.clear();
Assert.assertTrue(_validator.isPolicyResourceUnique(_policy, _failures,
Action.UPDATE));
- Assert.assertTrue("failures collection wasn't empty!",
_failures.isEmpty());
+ _failures.clear();
Assert.assertFalse(_validator.isPolicyResourceUnique(_policy, _failures,
Action.UPDATE));
+ _utils.checkFailureForSemanticError(_failures, "resources");
}
@Test
diff --git
a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
index 6de590b..6114225 100644
---
a/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
+++
b/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerValidator.java
@@ -186,9 +186,9 @@ public class TestRangerValidator {
String serviceName = "service-name";
boolean isPolicyEnabled = true;
when(_store.getPoliciesByResourceSignature(serviceName,
hexSignature, isPolicyEnabled)).thenReturn(null);
-
Assert.assertNull(_validator.getPoliciesForResourceSignature(serviceName,
hexSignature));
+
Assert.assertNotNull(_validator.getPoliciesForResourceSignature(serviceName,
hexSignature));
when(_store.getPoliciesByResourceSignature(serviceName,
hexSignature, isPolicyEnabled)).thenThrow(new Exception());
-
Assert.assertNull(_validator.getPoliciesForResourceSignature(serviceName,
hexSignature));
+
Assert.assertNotNull(_validator.getPoliciesForResourceSignature(serviceName,
hexSignature));
// what ever store returns should come back
hexSignature = "anotherSignature";
diff --git
a/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
b/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
index f13cef7..cb57b99 100644
--- a/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
+++ b/security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
@@ -65,6 +65,7 @@ import org.apache.ranger.authorization.utils.JsonUtils;
import org.apache.ranger.biz.ServiceDBStore.METRIC_TYPE;
import org.apache.ranger.common.AppConstants;
import org.apache.ranger.common.ContextUtil;
+import org.apache.ranger.common.GUIDUtil;
import org.apache.ranger.common.MessageEnums;
import org.apache.ranger.common.RangerCommonEnums;
import org.apache.ranger.common.db.RangerTransactionSynchronizationAdapter;
@@ -347,6 +348,9 @@ public class ServiceDBStore extends AbstractServiceStore {
@Autowired
SecurityZoneDBStore securityZoneStore;
+ @Autowired
+ GUIDUtil guidUtil;
+
private static volatile boolean legacyServiceDefsInitDone = false;
private Boolean populateExistingBaseFields = false;
@@ -2029,11 +2033,6 @@ public class ServiceDBStore extends AbstractServiceStore
{
policy.setVersion(Long.valueOf(1));
updatePolicySignature(policy);
- boolean updateServiceInfoRoleVersion = false;
- if (isSupportsRolesDownloadByService()) {
- updateServiceInfoRoleVersion =
isRoleDownloadRequired(policy, service);
- }
-
if(populateExistingBaseFields) {
assignedIdPolicyService.setPopulateExistingBaseFields(true);
daoMgr.getXXPolicy().setIdentityInsert(true);
@@ -2050,8 +2049,12 @@ public class ServiceDBStore extends AbstractServiceStore
{
XXPolicy xCreatedPolicy =
daoMgr.getXXPolicy().getById(policy.getId());
policyRefUpdater.createNewPolMappingForRefTable(policy,
xCreatedPolicy, xServiceDef);
createOrMapLabels(xCreatedPolicy, uniquePolicyLabels);
- RangerPolicy createdPolicy =
policyService.getPopulatedViewObject(xCreatedPolicy);
+ RangerPolicy createdPolicy =
policyService.getPopulatedViewObject(xCreatedPolicy);
+ boolean updateServiceInfoRoleVersion = false;
+ if (isSupportsRolesDownloadByService()) {
+ updateServiceInfoRoleVersion =
isRoleDownloadRequired(createdPolicy, service);
+ }
handlePolicyUpdate(service,
RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE, createdPolicy,
updateServiceInfoRoleVersion);
dataHistService.createObjectDataHistory(createdPolicy,
RangerDataHistService.ACTION_CREATE);
@@ -2175,11 +2178,6 @@ public class ServiceDBStore extends AbstractServiceStore
{
updatePolicySignature(policy);
- boolean updateServiceInfoRoleVersion = false;
- if (isSupportsRolesDownloadByService()) {
- updateServiceInfoRoleVersion =
isRoleDownloadRequired(policy, service);
- }
-
policy = policyService.update(policy);
XXPolicy newUpdPolicy =
daoMgr.getXXPolicy().getById(policy.getId());
@@ -2189,6 +2187,11 @@ public class ServiceDBStore extends AbstractServiceStore
{
policyRefUpdater.createNewPolMappingForRefTable(policy,
newUpdPolicy, xServiceDef);
createOrMapLabels(newUpdPolicy, uniquePolicyLabels);
RangerPolicy updPolicy =
policyService.getPopulatedViewObject(newUpdPolicy);
+
+ boolean updateServiceInfoRoleVersion = false;
+ if (isSupportsRolesDownloadByService()) {
+ updateServiceInfoRoleVersion =
isRoleDownloadRequired(updPolicy, service);
+ }
handlePolicyUpdate(service,
RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE, updPolicy,
updateServiceInfoRoleVersion);
dataHistService.createObjectDataHistory(updPolicy,
RangerDataHistService.ACTION_UPDATE);
@@ -3831,6 +3834,11 @@ public class ServiceDBStore extends AbstractServiceStore
{
}
void updatePolicySignature(RangerPolicy policy) {
+ String guid = policy.getGuid();
+ if (StringUtils.isEmpty(guid)) {
+ guid = guidUtil.genGUID();
+ policy.setGuid(guid);
+ }
RangerPolicyResourceSignature policySignature =
factory.createPolicyResourceSignature(policy);
String signature = policySignature.getSignature();
policy.setResourceSignature(signature);
diff --git
a/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
b/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
index ef0d3b4..dfb5814 100644
--- a/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
+++ b/security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java
@@ -25,6 +25,7 @@ import java.util.Map;
import org.apache.commons.collections.ListUtils;
import org.apache.ranger.common.ContextUtil;
+import org.apache.ranger.common.GUIDUtil;
import org.apache.ranger.common.JSONUtil;
import org.apache.ranger.common.RESTErrorUtil;
import org.apache.ranger.common.RangerFactory;
@@ -150,6 +151,9 @@ public class TestServiceDBStore {
@Mock
JSONUtil jsonUtil;
+ @Mock
+ GUIDUtil guidUtil;
+
@Rule
public ExpectedException thrown = ExpectedException.none();
