----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75047/#review226540 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/gds/GdsPolicyEngine.java Line 81 (original), 81 (patched) <https://reviews.apache.org/r/75047/#comment314782> No need to record in the context that anyAccess is asked for, with the following line? Similarly, reset in the finally block? RangerAccessRequestUtil.setIsAnyAccessInContext(request.getContext(), Boolean.TRUE); agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 833 (patched) <https://reviews.apache.org/r/75047/#comment314794> It seems this method is looking if access is allowed for at least one of the accesses in accessesInGroup. If yes, consider changing getAccessACLForOneGroup() to: // returns: // ACCESS_ALLOWED: if access is allowed for any one entry in accessTypes // ACCESS_DENIED: if access is denied for all entries in accessTypes // ACCESS_CONDITIONAL: if access is conditional for all entries in accessTypes // ACCESSES_UNDETERMINED: no of the above cases Integer deriveAccessResultFromGroup(RangerAccessRquest request, Set<String> accessTypes) { ... } agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 834 (patched) <https://reviews.apache.org/r/75047/#comment314793> Consider replacing tabs with whitespaces. agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 850 (patched) <https://reviews.apache.org/r/75047/#comment314796> It might be intuitive to replace use of isAccessDetermined and deniedAccessResult with the following. int allowedCount = 0; int deniedCount = 0; int conditionalCount = 0; for (String accessType : accessesInGroup) { Integer accessResult = accessTypeResults.get(accessType); if (accessResult == null) { continue; } switch (accessResult) { case ACCESS_ALLOWED: allowedCount++; break; case ACCESS_DENIED: deniedCount++; break; case ACCESS_CONDITIONAL: conditionalCount++; break; } if (allowedCount > 0) { break; } } final int ret; if (allowedCount > 0) { ret = ACCESS_ALLOWED; } else if (accessesInGroup.size() == 0) { ret = ACCESS_UNDETERMINED; } else if (deniedCount == accessTypes.size()) { ret = ACCESS_DENIED; } else if (conditionalCount == accessTypes.size()) { ret = ACCESS_CONDITIONAL; } else { ret = ACCESS_UNDETERMINED; } return ret; agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 868 (patched) <https://reviews.apache.org/r/75047/#comment314795> Consider moving #868 to inside the if block at #870 as allAccessTypes is used only within this block. agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 1062 (patched) <https://reviews.apache.org/r/75047/#comment314800> Consider renaming getAccessResultForOneGroup() to deriveAccessResultFromGroup(). agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 1073 (patched) <https://reviews.apache.org/r/75047/#comment314801> Is it not necessary to check if accessResult.isAccessDetermined == true? RangerAccessResult ret = null; for (String accessType : accesssesInGroup) { RangerAccessResult accessResult = accessTypeResults.get(accessType); if (accessResult == null) { continue; } else if (!accessResult.getIsAccessDetermined()) { if (ret == null) { ret = accessResult; } } else if (accesResult.getIsAllowed()) { ret = accessResult; break; } else { if (ret == null || !ret.getIsAccessDetermined()) { ret = accessResult; } } } return ret; agents-common/src/main/java/org/apache/ranger/plugin/util/RangerAccessRequestUtil.java Lines 255 (patched) <https://reviews.apache.org/r/75047/#comment314799> It should be safe to assume that the value was stored with a call to setAllRequestedAccessTypeGroups(). So, consider replacing the call to convertToSetofSets() with a cast: return (Set<Set<String>>) val; hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java Line 885 (original), 887 (patched) <https://reviews.apache.org/r/75047/#comment314784> Consider using the stream way: Set<Set<String>> accessTypeGroups = accessTypes.stream().map(e -> Collections.singleton(e)).collect(toSet()); hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java Lines 892 (patched) <https://reviews.apache.org/r/75047/#comment314798> Consider avoiding creation of TreeSet<> here. Instead update the caller to creae TreeSet<>. Also, replace HashSet<> in #887 with TreeSet<>? hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java Line 962 (original), 969 (patched) <https://reviews.apache.org/r/75047/#comment314783> Consider using the stream way: Set<Set<String>> accessTypeGroups = accessTypes.stream().map(e -> Collections.singleton(e)).collect(toSet()); hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java Lines 974 (patched) <https://reviews.apache.org/r/75047/#comment314797> Consider avoiding creation of TreeSet<> here. Instead update the caller to creae TreeSet<>. Also, replace HashSet<> in #969 with TreeSet<>? - Madhan Neethiraj On June 12, 2024, 10:56 p.m., Abhay Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75047/ > ----------------------------------------------------------- > > (Updated June 12, 2024, 10:56 p.m.) > > > Review request for ranger, Dineshkumar Yadav, madhan, Madhan Neethiraj, > Pradeep Agrawal, Ramesh Mani, and Velmurugan Periasamy. > > > Bugs: RANGER-4820 > https://issues.apache.org/jira/browse/RANGER-4820 > > > Repository: ranger > > > Description > ------- > > Currently, Ranger policy engine supports authorization of multiple accesses > for a given resource in a single call to the Ranger plugin's > isAccessAllowed() API. However, it has some limitations which are addressed > by this JIRA. > > Limitation: If multiple accesses are to be authorized, then the current > authorization logic in Ranger policy engine is designed to allow the request > to succeed (that is, grant access) only if all requested accesses are granted. > > This Jira supports organizing accesses in groups where each group is granted > access if any access in the group is allowed, and the request is successful > (that is, user is allowed access) only if all groups are granted access. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java > b0dc7a461 > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/gds/GdsPolicyEngine.java > 6a6709254 > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java > c43ec4c2f > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerAccessRequestUtil.java > df0352ca9 > > agents-common/src/test/resources/policyengine/test_policyengine_hdfs_multiple_accesses.json > 8962c5a3f > > hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java > c892bced3 > > > Diff: https://reviews.apache.org/r/75047/diff/1/ > > > Testing > ------- > > Updated the unit tests for muliple access > (agents-common/src/test/resources/policyengine/test_policyengine_hdfs_multiple_accesses.json). > > Ran all unit tests successfully. > > > Thanks, > > Abhay Kulkarni > >
