> On April 24, 2019, 4:59 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java > > Lines 345 (patched) > > <https://reviews.apache.org/r/70525/diff/1/?file=2140742#file2140742line345> > > > > 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.
getTagServices() is type of List<String>. In case of blank or null it will return empty List array. > On April 24, 2019, 4:59 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java > > Lines 385 (patched) > > <https://reviews.apache.org/r/70525/diff/1/?file=2140742#file2140742line385> > > > > Can existingSecurityZone.getServices() return null? Please review. Same > > for securityZone.getServices(). Null is not possible. So i have removed such null check. > On April 24, 2019, 4:59 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java > > Lines 397 (patched) > > <https://reviews.apache.org/r/70525/diff/1/?file=2140742#file2140742line397> > > > > Will existingRangerSecurityZoneService or newRangerSecurityService be > > null? Please see lines 383/384. Please review. Null is not possible. So i have removed such null check. > On April 24, 2019, 4:59 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/rest/SecurityZoneREST.java > > Lines 424 (patched) > > <https://reviews.apache.org/r/70525/diff/1/?file=2140742#file2140742line424> > > > > existingRangerSecurityZoneService is not checked here for null (because > > it will not be null), however, it is checked for null elsewhere (line 397) Null is not possible. So i have removed such null check from other places. > On April 24, 2019, 4:59 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > > Lines 3263 (patched) > > <https://reviews.apache.org/r/70525/diff/1/?file=2140743#file2140743line3267> > > > > This condition is difficult to understand. Please see if this can be > > simplified. Policy needs to be added in case user is ZoneAdmin or zoneAuditor. Check !StringUtils.isEmpty(policy.getZoneName()) is require for unzoned policy so in that case we shoudn't be going further for checking role." > On April 24, 2019, 4:59 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > > Line 3410 (original), 3448 (patched) > > <https://reviews.apache.org/r/70525/diff/1/?file=2140743#file2140743line3457> > > > > Is this needed? Please review. I have to make it public in order to fix the attached test case. - bhavik ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70525/#review214856 ----------------------------------------------------------- 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 > >
