> On 一月 4, 2023, 7:01 a.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/db/XXPolicyDao.java > > Lines 316 (patched) > > <https://reviews.apache.org/r/74268/diff/4/?file=2273372#file2273372line316> > > > > adding filter zoneId=RangerSecurityZone.RANGER_UNZONED_SECURITY_ZONE_ID > > will not retrieve policies that are in security zones. Is zoneId filter > > necessary? > > Ramachandran Krishnan wrote: > Will it create any security imapct when we use guid alone without passing > zoneId or zoneName or serviceName ? > If no, then we no need to put > zoneId=RangerSecurityZone.RANGER_UNZONED_SECURITY_ZONE_ID will not retrieve > policies that are in security zones > > Kirby Zhou wrote: > it seems created an security impact without > RANGER_UNZONED_SECURITY_ZONE_ID when zoneName is blank. > > The old code line300 set RANGER_UNZONED_SECURITY_ZONE_ID too. > > But if zoneName is not black, you should pass it to SQL query. > > Ramachandran Krishnan wrote: > Kirby Zhou/Madhan, > > As part of the fix we added the RANGER_UNZONED_SECURITY_ZONE_ID for > zoneId when guid is not null and serviceName,zoneName is null > > select obj from XXPolicy obj where obj.guid = :guid and obj.zoneId = > :zoneId > > zoneId ---> RANGER_UNZONED_SECURITY_ZONE_ID(1L) > to avoid the security impact . > > Ramachandran Krishnan wrote: > if (StringUtils.isNotBlank(serviceName)) { > if (StringUtils.isBlank(zoneName)) { > // query with guid, serviceName and > RANGER_UNZONED_SECURITY_ZONE_ID > return > getEntityManager().createNamedQuery("XXPolicy.findPolicyByPolicyGUIDAndServiceName", > tClass) > .setParameter("guid", guid) > .setParameter("serviceName", > serviceName) > .setParameter("zoneId", > RangerSecurityZone.RANGER_UNZONED_SECURITY_ZONE_ID) > .getSingleResult(); > } else { > // query with guid, serviceName and zoneName > return getEntityManager() > > .createNamedQuery("XXPolicy.findPolicyByPolicyGUIDAndServiceNameAndZoneName", > tClass) > .setParameter("guid", guid) > .setParameter("serviceName", > serviceName) > .setParameter("zoneName", > zoneName) > .getSingleResult(); > } > } else { > if (StringUtils.isNotBlank(zoneName)) { > // query with guid and zoneName > > return getEntityManager() > > .createNamedQuery("XXPolicy.findPolicyByPolicyGUIDAndZoneName", tClass) > .setParameter("guid", guid) > .setParameter("zoneName", > zoneName) > .getSingleResult(); > } else { > // query with guid and RANGER_UNZONED_SECURITY_ZONE_ID > > return getEntityManager() > > .createNamedQuery("XXPolicy.findPolicyByPolicyGUID", tClass) > .setParameter("guid", guid) > .setParameter("zoneId", > RangerSecurityZone.RANGER_UNZONED_SECURITY_ZONE_ID) > .getSingleResult(); > } > } > > Kirby Zhou/Madhan, > > I hope this will cover all the cases
It seems work. But I suggest you do not use StringUtils.isNotBlank and StringUtils.isBlank together —— This makes reading a little difficult. - Kirby ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74268/#review225041 ----------------------------------------------------------- On 一月 4, 2023, 11:17 a.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74268/ > ----------------------------------------------------------- > > (Updated 一月 4, 2023, 11:17 a.m.) > > > Review request for ranger, Don Bosco Durai, Abhay Kulkarni, Madhan Neethiraj, > Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, > Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan Periasamy. > > > Bugs: RANGER-4031 > https://issues.apache.org/jira/browse/RANGER-4031 > > > Repository: ranger > > > Description > ------- > > Not able to fetch Policy details using guid /api/policy/guid/{guid} without > service name > > Request without servicename > > curl -s -L -X GET > "https://q************/service/public/v2/api/policy/guid/****-2f42-4451-9edf-****"; > -H "Content-Type: application/json" -H "Accept: application/json" -H > "Authorization: Basic *********DEyMw==" > Response : 404 > > Request with servicename > > curl -s -L -X GET > "https://****************/service/public/v2/api/policy/guid/*****-2f42-4451-9edf-****?serviceName=hive"; > -H "Content-Type: application/json" -H "Accept: application/json" -H > "Authorization: Basic ***************==" > Response Proper : 200 with proper details > > Code : > https://github.com/apache/ranger/blob/master/security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java#L505 > > @GET @Path("/api/policy/guid/{guid}") > @Produces({ "application/json", "application/xml" }) > public RangerPolicy > getPolicyByGUIDAndServiceNameAndZoneName(@PathParam("guid") String guid, > > @DefaultValue("") > @QueryParam("serviceName") String serviceName, > > @DefaultValue("") @QueryParam("ZoneName") String zoneName) { > return > serviceREST.getPolicyByGUIDAndServiceNameAndZoneName(guid, serviceName, > zoneName); } > As query parameters are optional it should give proper response > > Expected : User should be able to get policy details using only guid in path > params > > > As part of the current design, Ranger expects both serviceName,guid should be > mandatory and zoneName can be optional > Proposal: > Add the logic to fetch the RangerPolicy by guid from the backend > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > 6b9604817 > security-admin/src/main/java/org/apache/ranger/db/XXPolicyDao.java > 37d7561d4 > security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java > c7a6ea0a6 > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > e17494fa9 > security-admin/src/main/resources/META-INF/jpa_named_queries.xml 85c8b6213 > security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java > 7f1ec6d3e > security-admin/src/test/java/org/apache/ranger/rest/TestPublicAPIsv2.java > 2a123de93 > security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java > 7b15810e0 > > > Diff: https://reviews.apache.org/r/74268/diff/5/ > > > Testing > ------- > > > Thanks, > > Ramachandran Krishnan > >
