Repository: incubator-ranger Updated Branches: refs/heads/master ee9d78bbb -> 8a9b57ed5
RANGER-359: If policy update does not change the resource then resource signature check should exclude itself from policies with matched resources Signed-off-by: Madhan Neethiraj <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-ranger/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-ranger/commit/8a9b57ed Tree: http://git-wip-us.apache.org/repos/asf/incubator-ranger/tree/8a9b57ed Diff: http://git-wip-us.apache.org/repos/asf/incubator-ranger/diff/8a9b57ed Branch: refs/heads/master Commit: 8a9b57ed59ddd1d20b5529b9a8336d8e179c5165 Parents: ee9d78b Author: Alok Lal <[email protected]> Authored: Tue May 5 10:06:59 2015 -0700 Committer: Madhan Neethiraj <[email protected]> Committed: Tue May 5 11:04:31 2015 -0700 ---------------------------------------------------------------------- .../model/validation/RangerPolicyValidator.java | 27 +++++++++-------- .../validation/TestRangerPolicyValidator.java | 31 +++++++++++++++----- 2 files changed, 39 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/8a9b57ed/agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java ---------------------------------------------------------------------- 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 0092aaf..7f0318f 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 @@ -242,7 +242,7 @@ public class RangerPolicyValidator extends RangerValidator { boolean valid = true; Map<String, RangerPolicyResource> resourceMap = policy.getResources(); if (resourceMap != null) { // following checks can't be done meaningfully otherwise - valid = isPolicyResourceUnique(policy, failures) && valid; + valid = isPolicyResourceUnique(policy, failures, action) && valid; if (serviceDef != null) { // following checks can't be done meaningfully otherwise valid = isValidResourceNames(policy, failures, serviceDef) && valid; valid = isValidResourceValues(resourceMap, failures, serviceDef) && valid; @@ -256,10 +256,10 @@ public class RangerPolicyValidator extends RangerValidator { return valid; } - boolean isPolicyResourceUnique(RangerPolicy policy, final List<ValidationFailureDetails> failures) { + boolean isPolicyResourceUnique(RangerPolicy policy, final List<ValidationFailureDetails> failures, Action action) { if(LOG.isDebugEnabled()) { - LOG.debug(String.format("==> RangerPolicyValidator.isPolicyResourceUnique(%s, %s)", policy, failures)); + LOG.debug(String.format("==> RangerPolicyValidator.isPolicyResourceUnique(%s, %s, %s)", policy, failures, action)); } boolean valid = true; @@ -267,21 +267,24 @@ public class RangerPolicyValidator extends RangerValidator { String signature = policySignature.getSignature(); List<RangerPolicy> policies = getPoliciesForResourceSignature(signature); if (CollectionUtils.isNotEmpty(policies)) { - RangerPolicy otherPolicy = policies.iterator().next(); - valid = false; - failures.add(new ValidationFailureDetailsBuilder() - .field("resources") - .isSemanticallyIncorrect() - .becauseOf("found another policy[" + otherPolicy.getName() + "] with matching resources[" + otherPolicy.getResources() + "]!") - .build()); + 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("found another policy[" + matchedPolicy.getName() + "] with matching resources[" + matchedPolicy.getResources() + "]!") + .build()); + valid = false; + } } if(LOG.isDebugEnabled()) { - LOG.debug(String.format("<== RangerPolicyValidator.isPolicyResourceUnique(%s, %s): %s", policy, failures, valid)); + LOG.debug(String.format("<== RangerPolicyValidator.isPolicyResourceUnique(%s, %s, %s): %s", policy, failures, action, valid)); } return valid; } - + boolean isValidResourceNames(final RangerPolicy policy, final List<ValidationFailureDetails> failures, final RangerServiceDef serviceDef) { if(LOG.isDebugEnabled()) { http://git-wip-us.apache.org/repos/asf/incubator-ranger/blob/8a9b57ed/agents-common/src/test/java/org/apache/ranger/plugin/model/validation/TestRangerPolicyValidator.java ---------------------------------------------------------------------- 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 1a4f366..d55cdc5 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 @@ -677,15 +677,32 @@ public class TestRangerPolicyValidator { when(_factory.createPolicyResourceSignature(_policy)).thenReturn(signature); List<RangerPolicy> policies = null; when(_store.getPoliciesByResourceSignature(hash)).thenReturn(policies); - assertTrue(_validator.isPolicyResourceUnique(_policy, _failures)); policies = new ArrayList<RangerPolicy>(); - assertTrue(_validator.isPolicyResourceUnique(_policy, _failures)); - - // if store does have any policy then test should fail with appropriate error message. - RangerPolicy policy1 = mock(RangerPolicy.class); policies.add(policy1); - RangerPolicy policy2 = mock(RangerPolicy.class); policies.add(policy2); + for (Action action : cu) { + assertTrue(_validator.isPolicyResourceUnique(_policy, _failures, action)); + assertTrue(_validator.isPolicyResourceUnique(_policy, _failures, action)); + } + /* + * If store does have any policy then test should fail with appropriate error message. + * For create any match is a problem + */ + RangerPolicy policy1 = mock(RangerPolicy.class); policies.add(policy1); when(_store.getPoliciesByResourceSignature(hash)).thenReturn(policies); - assertFalse(_validator.isPolicyResourceUnique(_policy, _failures)); + 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(policy1.getId()).thenReturn(103L); + when(_policy.getId()).thenReturn(103L); + 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); + 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) + policies.add(policy2); + assertFalse(_validator.isPolicyResourceUnique(_policy, _failures, Action.UPDATE)); _utils.checkFailureForSemanticError(_failures, "resources"); }
