> On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > Barbara - thank you for your contribution to Apache Ranger. This plugin to > > authorize access to Json records/fields, including row-filter and > > field-masking will be useful widely in any application serving complex Json > > records. > > > > Here are my initial comments on reviewing the patch. I will review the > > patch further and add further comments if necessary.
Madhan, thanks for your very careful review! > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/CONTRIBUTING > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268948#file2268948line1> > > > > Please review if this is needed in Apache Ranger repo. If not, please > > remove. removed > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/NOTICE > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268950#file2268950line1> > > > > Ranger => Apache Ranger done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/conf/log4j.properties > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268952#file2268952line1> > > > > License header is missing. Please add. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/conf/nestedstructure_servicedef.json > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268953#file2268953line1> > > > > Consider moving service-def to > > agents-common/src/main/resources/service-defs, alog with rest of > > service-defs. This will also enable Ranger to load this service-def at > > startup automatically in EmbeddedServiceDefsUtil. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/pom.xml > > Lines 2 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268957#file2268957line2> > > > > License header is missing. Please add. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java > > Lines 2 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268958#file2268958line2> > > > > Please review Apache guidelines on source code headers at > > https://www.apache.org/legal/src-headers, specifically about Copyright in > > sources and update. done on all .java files > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java > > Lines 26 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268958#file2268958line26> > > > > Apache strongly recommends to avoid author tags in source code - see > > https://www.apache.org/foundation/records/minutes/2004/board_minutes_2004_09_22.txt > > (Recommend strongly that @author is avoided; but leave it to each PMC to > > make the final call with their respective communities). > > > > To be consistent with rest of Apache Ranger source code, I suggest to > > remove @author tag from sources. removed all @author tags > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java > > Lines 30 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268958#file2268958line30> > > > > Instance members are assigned value only in the constructor. Consider > > marking these as final. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java > > Lines 34 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268958#file2268958line34> > > > > Default constructor is unused. Please review and remove. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java > > Lines 53 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268959#file2268959line53> > > > > Consider marking SUPPORTED_DATE_FORMATS as final, as it is is not > > reassigned after initialization. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java > > Lines 120 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268959#file2268959line120> > > > > Consider moving customMaskValue to 'case MaskTypes.CUSTOM', as this is > > unused in other cases. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java > > Lines 210 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268959#file2268959line210> > > > > For better readability, consider replacing #210 and #211 with the > > following: > > > > int maskedValueLen = StringUtils.length(value); > > > > if (maskedValueLen < MaskTypes.MIN_MASK_LENGTH) { > > maskedValueLen = MaskTypes.MIN_MASK_LENGTH; > > } else if (maskedValueLen > MaskTypes.MAX_MASK_LENGTH) { > > maskedValueLen = MaskTypes.MAX_MASK_LENGTH; > > } done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/ExampleClient.java > > Lines 24 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268960#file2268960line24> > > > > Consider moving ExampleClient to test directory. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/ExampleClient.java > > Lines 26 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268960#file2268960line26> > > > > userRoles is unused. Please review and remove. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/FieldLevelAccess.java > > Lines 26 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268961#file2268961line26> > > > > Consider marking all instance members as final, as they are assigned > > value only during initialization. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java > > Lines 43 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268962#file2268962line43> > > > > Consider marking following instance members as private: > > - documentContext > > - fieldContextDocument > > - fields done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java > > Lines 70 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268962#file2268962line70> > > > > Consider moving private methods after public and protected methods, to > > be consistent with rest of Apache Ranger sources. Please see coding > > guidelines at > > https://cwiki.apache.org/confluence/display/RANGER/Coding+guidelines. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java > > Lines 76 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268962#file2268962line76> > > > > JasonParser constructor is depricated. Consider replacing #76 - #78 > > with: > > try { > > JsonParser.parseString(jsonString); // throws JsonSyntaxException done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 35 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line35> > > > > Consider marking plugin as final, along with replacing static block #42 > > - #52 with: > > static { > > plugin = new RangerBasePlugin(RANGER_CMT_SERVICETYPE, > > RANGER_CMT_APPID); > > > > plugin.setResultProcessor(new > > RangerDefaultAuditHandler(plugin.getConfig())); > > plugin.init(); > > } > > > > Also, consider making plugin a non-static member of > > NestedStructureAuthorizer, along with all the methods as well. The callers > > can get access to the instance with a static method > > NestedStructureAuthorizer.instance(). This will enable keeping necessary > > states in NestedStructureAuthorizer instance. Replaced static block as recommended. marking plugin final causes an error in the first line of the static block, so I didn't do that. > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 66 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line66> > > > > Given NestedStructureAccessType is an enum, can accessType be of > > invalid member? validateAccessType deleted > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 73 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line73> > > > > Consider using log level warn in #73. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 90 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line90> > > > > Shouldn't userGroups is passed to hasAnyAccess(), for group based > > authorization to work? done. > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 131 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line131> > > > > Consider initializing fldRequest with value in #132. done. > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 140 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line140> > > > > Consider changing log level to debug. I like seeing the policy number in info, since masking policy decisions aren't logged, even though the policy is configured to log them. Is that a bug in our code, or wider Ranger code? > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 155 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line155> > > > > Consider changing log level to debug. I like seeing the policy number in info, since masking policy decisions aren't logged, even though the policy is configured to log them. Is that a bug in our code, or wider Ranger code? > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 179 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line179> > > > > Consider initializing filterRequest with value in #180. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 188 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line188> > > > > Consider changing log level to debug. I like seeing row filter in info. :-) > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 207 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line207> > > > > Consider initializing request with value in #209. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 209 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line209> > > > > userGroups is passed as null here. Shouldn't this be passed from > > caller, for group based authorization to work? done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > > Lines 217 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268966#file2268966line217> > > > > Consider changing log level to debug. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Resource.java > > Lines 25 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268967#file2268967line25> > > > > To consistent with rest of Apache Ranger sources, I suggest to rename > > NestedStructure_Resource to NestedStructureResource (i.e., remove > > underscore). done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Resource.java > > Lines 29 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268967#file2268967line29> > > > > Default constructor is unused. Please review and remove. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Service.java > > Lines 27 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268968#file2268968line27> > > > > To consistent with rest of Apache Ranger sources, I suggest to rename > > NestedStructure_Service to NestedStructureService (i.e., remove underscore). done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/RecordFilterJavaScript.java > > Lines 84 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268969#file2268969line84> > > > > Consider changing log level to debug - in #84 and #94. consolidated 84 into 94. But I like to see filter conditions in info. > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestDataMasker.java > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268970#file2268970line1> > > > > License header is missing. Please add. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestJsonManipulator.java > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268971#file2268971line1> > > > > License header is missing. Please add. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestRecordFilterJavaScript.java > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268972#file2268972line1> > > > > License header is missing. Please add. done > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > pom.xml > > Lines 22 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268973#file2268973line22> > > > > Instead of introducing a global <modules> section, please add > > plugin-nestedstructure along with other modules, for example in: > > profiles.profile.all > > profiles.profile.ranger-jdk11 done. Sorry, I think that was some default that didn't get removed. > On July 16, 2022, 6:09 a.m., Madhan Neethiraj wrote: > > tagsync/src/test/java/org/apache/ranger/tagsync/nestedstructureplugin/ResourceTests.java > > Lines 1 (patched) > > <https://reviews.apache.org/r/74057/diff/1/?file=2268975#file2268975line1> > > > > License header is missing. Please add. done - Barbara ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74057/#review224566 ----------------------------------------------------------- On July 13, 2022, 11:03 p.m., Barbara Eckman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74057/ > ----------------------------------------------------------- > > (Updated July 13, 2022, 11:03 p.m.) > > > Review request for ranger and Madhan Neethiraj. > > > Repository: ranger > > > Description > ------- > > It would be nice to be able to do fine-grained access control (FGA) over > nested structures, e.g., the JSON responses of API calls. This requires the > individual attributes in a JSON object to be first-class metadata objects > which can be tagged and on which policies can be written. We have built a > plugin and the corresponding Apache Atlas metadata structures and > tagsync-mapper to support TBAC/RBAC/ABAC FGA over JSON structures. Our > instigating use case was FGA over the JSON responses of API calls, but this > plugin has potential value anywhere FGA over the individual attributes of > nested structures is needed, eg JSON messages read from Kafka topics. > > > Diffs > ----- > > plugin-nestedstructure/CONTRIBUTING PRE-CREATION > plugin-nestedstructure/LICENSE PRE-CREATION > plugin-nestedstructure/NOTICE PRE-CREATION > plugin-nestedstructure/README.md PRE-CREATION > plugin-nestedstructure/conf/log4j.properties PRE-CREATION > plugin-nestedstructure/conf/nestedstructure_servicedef.json PRE-CREATION > plugin-nestedstructure/conf/ranger-nestedstructure-audit.xml PRE-CREATION > plugin-nestedstructure/conf/ranger-nestedstructure-policymgr-ssl.xml > PRE-CREATION > plugin-nestedstructure/conf/ranger-nestedstructure-security.xml > PRE-CREATION > plugin-nestedstructure/pom.xml PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/ExampleClient.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/FieldLevelAccess.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/MaskTypes.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/MaskingException.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAccessType.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Resource.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Service.java > PRE-CREATION > > plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/RecordFilterJavaScript.java > PRE-CREATION > > plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestDataMasker.java > PRE-CREATION > > plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestJsonManipulator.java > PRE-CREATION > > plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestRecordFilterJavaScript.java > PRE-CREATION > pom.xml 0945f4b1d > > tagsync/src/main/java/org/apache/ranger/tagsync/nestedstructureplugin/AtlasNestedStructureResourceMapper.java > PRE-CREATION > > tagsync/src/test/java/org/apache/ranger/tagsync/nestedstructureplugin/ResourceTests.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/74057/diff/1/ > > > Testing > ------- > > > Thanks, > > Barbara Eckman > >
