----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74825/#review226124 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 394 (patched) <https://reviews.apache.org/r/74825/#comment314403> If there are many resource-evaluators, this will return false. Is that expected? Please review. agents-common/src/main/java/org/apache/ranger/plugin/policyresourcematcher/RangerDefaultPolicyResourceMatcher.java Lines 403 (patched) <https://reviews.apache.org/r/74825/#comment314404> Is the null check for the resourceValue needed here? In both cases, it is executing the same logic (lines 404 and 406. Please review. Please consider if line 406 needs to be ret = matcher == null && matcher.isSomeMatch( resourceValue, evalContext); agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerAbstractResourceMatcher.java Line 22 (original), 22 (patched) <https://reviews.apache.org/r/74825/#comment314405> Please consider importing packages one-by-one. agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerAbstractResourceMatcher.java Lines 276 (patched) <https://reviews.apache.org/r/74825/#comment314406> Please review if this condition is correct. The first part of the condition may not be needed. Can this condition be replaced with ret = policyValues.containsAny(resValues); ? security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdminImpl.java Lines 435 (patched) <https://reviews.apache.org/r/74825/#comment314407> This loop iterates over all resource policies. This may be expensive. Is there any way to limit the set of policies to search? Please review. security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java Line 29 (original), 30 (patched) <https://reviews.apache.org/r/74825/#comment314408> Please consider importing package individually. security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java Lines 144 (patched) <https://reviews.apache.org/r/74825/#comment314410> Should this be (!CollectionUtils.isEmpty(rangerPolicyItems[POLICYITEM_TYPE.ALLOW_EXCEPTIONS.ordinal()].getAccesses())) { Please review this and similar code in this function. security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java Lines 249 (patched) <https://reviews.apache.org/r/74825/#comment314409> RangerFactory.getResourceSignature() uses policy-resources and conditions to compute signature. It does not consider users/groups/roles. So, this function may return true when the policy matches signature but does not match users/groups/roles. Please review. security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java Lines 964 (patched) <https://reviews.apache.org/r/74825/#comment314412> revokeRequest->grantRequest security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java Lines 1009 (patched) <https://reviews.apache.org/r/74825/#comment314413> Even if policyResources are less then revokedResources, revokedResources should be removed from policyResources. Please review. security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java Lines 1045 (patched) <https://reviews.apache.org/r/74825/#comment314414> resourceMap is extracted from existingPolicy. In that case, is it necessary to set it back into existingPolicy? Please review. - Abhay Kulkarni On Jan. 9, 2024, 6:48 a.m., Ramesh Mani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74825/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2024, 6:48 a.m.) > > > Review request for ranger, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, > Pradeep Agrawal, Selvamohan Neethiraj, Sailaja Polavarapu, and Velmurugan > Periasamy. > > > Bugs: RANGER-4638 > https://issues.apache.org/jira/browse/RANGER-4638 > > > Repository: ranger > > > Description > ------- > > RANGER-4638:Multiple Columns Revoke not generating policies with correct > number of columns > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java > 7fe2a2eb3 > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerPolicyEvaluator.java > 0a14b387a > > agents-common/src/main/java/org/apache/ranger/plugin/policyresourcematcher/RangerDefaultPolicyResourceMatcher.java > f16157ce6 > > agents-common/src/main/java/org/apache/ranger/plugin/policyresourcematcher/RangerPolicyResourceMatcher.java > e1cd89b70 > > agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerAbstractResourceMatcher.java > 5eee8d11a > > agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/RangerResourceMatcher.java > ec22e01bf > > agents-common/src/test/java/org/apache/ranger/plugin/resourcematcher/TestDefaultPolicyResourceisCompleteOrSomeMatchMatcher.java > PRE-CREATION > > agents-common/src/test/resources/resourcematcher/test_defaultpolicyresource_isCompleteOrSomeMatch_matcher.json > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdmin.java > 15a1e7118 > > security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdminImpl.java > 84ee31ba2 > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > cc9df27d6 > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java > 60e34c0c7 > security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java > a630e575b > > > Diff: https://reviews.apache.org/r/74825/diff/1/ > > > Testing > ------- > > Impala / Hive beeline. > > 1) "grant select(col1, col2, col3) on table demo.test to role Role1" => > Create a Grant Policy for the given resource in Hadoop Sql > > > 2) "grant select(col1, col2, col3, col4) on table demo.test to role Role1" > => updates the policy created in #1 with new col4 resource > > if "revoke select(col1, col2, col3, col4) on table demo.test from role > Role1" is done => Since all the columns are revoked for Select, we delete the > policy created in #1 > if "revoke select(col1, col2, col3) on table demo.test from role Role1" > is done => policy created in #1 will be updated to remove col1,col2,col3 from > the policy to revoke the access. > > HBASE shell > > grant 'nifi', 'RWXCA', 'test' => create policy with 'RWXCA' access for user > nifi on table 'test'. > > > revoke 'nifi', 'test' => revoke access for user "nifi" on hbase table 'test'. > Here policy will be removed. > > > Thanks, > > Ramesh Mani > >
