----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74057/#review224566 -----------------------------------------------------------
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. plugin-nestedstructure/CONTRIBUTING Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313284> Please review if this is needed in Apache Ranger repo. If not, please remove. plugin-nestedstructure/NOTICE Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313285> Ranger => Apache Ranger plugin-nestedstructure/conf/log4j.properties Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313290> License header is missing. Please add. plugin-nestedstructure/conf/nestedstructure_servicedef.json Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313286> 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. plugin-nestedstructure/pom.xml Lines 2 (patched) <https://reviews.apache.org/r/74057/#comment313289> License header is missing. Please add. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java Lines 2 (patched) <https://reviews.apache.org/r/74057/#comment313288> Please review Apache guidelines on source code headers at https://www.apache.org/legal/src-headers, specifically about Copyright in sources and update. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java Lines 26 (patched) <https://reviews.apache.org/r/74057/#comment313287> 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. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java Lines 30 (patched) <https://reviews.apache.org/r/74057/#comment313295> Instance members are assigned value only in the constructor. Consider marking these as final. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/AccessResult.java Lines 34 (patched) <https://reviews.apache.org/r/74057/#comment313296> Default constructor is unused. Please review and remove. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java Lines 53 (patched) <https://reviews.apache.org/r/74057/#comment313297> Consider marking SUPPORTED_DATE_FORMATS as final, as it is is not reassigned after initialization. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java Lines 120 (patched) <https://reviews.apache.org/r/74057/#comment313298> Consider moving customMaskValue to 'case MaskTypes.CUSTOM', as this is unused in other cases. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/DataMasker.java Lines 210 (patched) <https://reviews.apache.org/r/74057/#comment313299> 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; } plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/ExampleClient.java Lines 24 (patched) <https://reviews.apache.org/r/74057/#comment313300> Consider moving ExampleClient to test directory. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/ExampleClient.java Lines 26 (patched) <https://reviews.apache.org/r/74057/#comment313301> userRoles is unused. Please review and remove. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/FieldLevelAccess.java Lines 26 (patched) <https://reviews.apache.org/r/74057/#comment313302> Consider marking all instance members as final, as they are assigned value only during initialization. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java Lines 43 (patched) <https://reviews.apache.org/r/74057/#comment313303> Consider marking following instance members as private: - documentContext - fieldContextDocument - fields plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java Lines 70 (patched) <https://reviews.apache.org/r/74057/#comment313305> 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. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/JsonManipulator.java Lines 76 (patched) <https://reviews.apache.org/r/74057/#comment313304> JasonParser constructor is depricated. Consider replacing #76 - #78 with: try { JsonParser.parseString(jsonString); // throws JsonSyntaxException plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 35 (patched) <https://reviews.apache.org/r/74057/#comment313310> 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. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 66 (patched) <https://reviews.apache.org/r/74057/#comment313319> Given NestedStructureAccessType is an enum, can accessType be of invalid member? plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 73 (patched) <https://reviews.apache.org/r/74057/#comment313311> Consider using log level warn in #73. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 90 (patched) <https://reviews.apache.org/r/74057/#comment313321> Shouldn't userGroups is passed to hasAnyAccess(), for group based authorization to work? plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 131 (patched) <https://reviews.apache.org/r/74057/#comment313316> Consider initializing fldRequest with value in #132. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 140 (patched) <https://reviews.apache.org/r/74057/#comment313317> Consider changing log level to debug. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 155 (patched) <https://reviews.apache.org/r/74057/#comment313318> Consider changing log level to debug. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 179 (patched) <https://reviews.apache.org/r/74057/#comment313314> Consider initializing filterRequest with value in #180. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 188 (patched) <https://reviews.apache.org/r/74057/#comment313315> Consider changing log level to debug. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 207 (patched) <https://reviews.apache.org/r/74057/#comment313312> Consider initializing request with value in #209. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 209 (patched) <https://reviews.apache.org/r/74057/#comment313320> userGroups is passed as null here. Shouldn't this be passed from caller, for group based authorization to work? plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructureAuthorizer.java Lines 217 (patched) <https://reviews.apache.org/r/74057/#comment313313> Consider changing log level to debug. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Resource.java Lines 25 (patched) <https://reviews.apache.org/r/74057/#comment313306> To consistent with rest of Apache Ranger sources, I suggest to rename NestedStructure_Resource to NestedStructureResource (i.e., remove underscore). plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Resource.java Lines 29 (patched) <https://reviews.apache.org/r/74057/#comment313308> Default constructor is unused. Please review and remove. plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/NestedStructure_Service.java Lines 27 (patched) <https://reviews.apache.org/r/74057/#comment313307> To consistent with rest of Apache Ranger sources, I suggest to rename NestedStructure_Service to NestedStructureService (i.e., remove underscore). plugin-nestedstructure/src/main/java/org.apache.ranger/authorization.nestedstructure.authorizer/RecordFilterJavaScript.java Lines 84 (patched) <https://reviews.apache.org/r/74057/#comment313309> Consider changing log level to debug - in #84 and #94. plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestDataMasker.java Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313294> License header is missing. Please add. plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestJsonManipulator.java Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313293> License header is missing. Please add. plugin-nestedstructure/src/test/java/org/apache/ranger/authorization/nestedstructure/authorizer/TestRecordFilterJavaScript.java Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313291> License header is missing. Please add. pom.xml Lines 22 (patched) <https://reviews.apache.org/r/74057/#comment313283> 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 tagsync/src/test/java/org/apache/ranger/tagsync/nestedstructureplugin/ResourceTests.java Lines 1 (patched) <https://reviews.apache.org/r/74057/#comment313292> License header is missing. Please add. - Madhan Neethiraj 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 > >
