----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70525/#review214856 -----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Lines 302 (patched) <https://reviews.apache.org/r/70525/#comment301092> Please review the error message. Should it be "unable to get Security Zone with id"? security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Lines 318 (patched) <https://reviews.apache.org/r/70525/#comment301093> Do you mean if (!serviceRest.ensureZoneAdmin(existingSecurityZone.getName())) { here? I understand that names of both zones is the same, but it is more readable using existingSecurityZone here. security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Lines 345 (patched) <https://reviews.apache.org/r/70525/#comment301094> Please review RangerSecurityZone.getTagServices() and RangerSecurityZone.setTagServices(). It seems that setTagServices() if called with null parameter, does not set tagServices to an empty hashmap. So, dbTagServices and/or uiTagServices may have a null value, and NPE will result at line 349. Please review. security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Lines 385 (patched) <https://reviews.apache.org/r/70525/#comment301095> Can existingSecurityZone.getServices() return null? Please review. Same for securityZone.getServices(). security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Lines 397 (patched) <https://reviews.apache.org/r/70525/#comment301096> Will existingRangerSecurityZoneService or newRangerSecurityService be null? Please see lines 383/384. Please review. security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java Lines 424 (patched) <https://reviews.apache.org/r/70525/#comment301097> existingRangerSecurityZoneService is not checked here for null (because it will not be null), however, it is checked for null elsewhere (line 397) security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Lines 3263 (patched) <https://reviews.apache.org/r/70525/#comment301100> This condition is difficult to understand. Please see if this can be simplified. security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Line 3277 (original), 3279 (patched) <https://reviews.apache.org/r/70525/#comment301098> Earlier name - isZoneAdmin - better represents the functionality. Please see if this may be named back to isZoneAdmin. security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Lines 3316 (patched) <https://reviews.apache.org/r/70525/#comment301101> Please see if this can be renamed to isZoneAuditor security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Lines 3317 (patched) <https://reviews.apache.org/r/70525/#comment301103> isZoneAdmin==>isZoneAuditor? security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Line 3293 (original), 3322 (patched) <https://reviews.apache.org/r/70525/#comment301106> In case of such errors, please log message with error level, and print the exception. LOG.error("Unexpected error when fetching security zone with name:[" + zoneName + "] from database", e); security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java Line 3410 (original), 3448 (patched) <https://reviews.apache.org/r/70525/#comment301108> Is this needed? Please review. - Abhay Kulkarni On April 23, 2019, 1:54 p.m., bhavik patel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70525/ > ----------------------------------------------------------- > > (Updated April 23, 2019, 1:54 p.m.) > > > Review request for ranger, Ankita Sinha, Don Bosco Durai, Gautam Borad, Abhay > Kulkarni, Madhan Neethiraj, Oliver Szabo, Pradeep Agrawal, Ramesh Mani, > Selvamohan Neethiraj, Sailaja Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-2408 > https://issues.apache.org/jira/browse/RANGER-2408 > > > Repository: ranger > > > Description > ------- > > Restriction on CRUD operation of Zone, Policy and service. > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java > 6ce5365 > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > 6ddb359 > security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java > 8f39607 > > > Diff: https://reviews.apache.org/r/70525/diff/1/ > > > Testing > ------- > > Verfied CRUD operation of Zone, Policy and service for role such as admin, > auditor, user, service admin, delegate admin, zone admin and zone auditor. > > > Thanks, > > bhavik patel > >
