> On March 12, 2018, 12:51 p.m., Madhan Neethiraj wrote: > > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java > > Lines 328 (patched) > > <https://reviews.apache.org/r/65920/diff/2/?file=1973123#file1973123line329> > > > > when would 'response' not contain REST_URL_IMPORT_SERVICETAGS_RESOURCE? > > If there is such a case, can it not be covered by testing for > > response.getStatus()? > > > > Please review and update other such usage in this file.
this could happen in case of redirection to a different page such as login page for some reason and status code(response.getStatus) could still be 200/204 for that page (as that page is successfully loaded) ,therefore added a check that request URL in response should be same as requested. > On March 12, 2018, 12:51 p.m., Madhan Neethiraj wrote: > > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java > > Lines 367 (patched) > > <https://reviews.apache.org/r/65920/diff/2/?file=1973123#file1973123line368> > > > > It is not clean why this is a 'cachedWebResource'. Please review and > > rename appropriately. unlike createWebResource() method, createCachedWebResource() creates a webresource without credentials in order to use that webresource for cookie based authentication.renaming createCachedWebResource() to createWebResourceForCookieAuth(). > On March 12, 2018, 12:51 p.m., Madhan Neethiraj wrote: > > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java > > Lines 369 (patched) > > <https://reviews.apache.org/r/65920/diff/2/?file=1973123#file1973123line370> > > > > 'tagRESTClient.getClient()' can return null, if > > tagRESTClient.resetClient() was called. Please review and update to handle > > this case. If tagRESTClient.resetClient() was called tagRESTClient.getClient() will return new object of Client,it wont return null.Also, we are calling tagRESTClient.resetClient() only when we want a completely new session(during first login or at the time when cookie expired/invalidated). Following is the tagRESTClient.getClient() method for reference. public Client getClient() { // result saves on access time when client is built at the time of the call Client result = client; if(result == null) { synchronized(this) { result = client; if(result == null) { client = result = buildClient(); } } } return result; } - Nikhil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65920/#review199004 ----------------------------------------------------------- On March 12, 2018, 3:46 p.m., Nikhil P wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65920/ > ----------------------------------------------------------- > > (Updated March 12, 2018, 3:46 p.m.) > > > Review request for ranger, Ankita Sinha, Don Bosco Durai, Gautam Borad, Abhay > Kulkarni, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, Selvamohan > Neethiraj, Sailaja Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-2010 > https://issues.apache.org/jira/browse/RANGER-2010 > > > Repository: ranger > > > Description > ------- > > Ranger Tagsync should use cookie based authentication for subsequent requests > to Ranger admin. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTClient.java > 0d94edc > tagsync/src/main/java/org/apache/ranger/tagsync/process/TagSyncConfig.java > 697c7cc > > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java > a1dc8f5 > tagsync/src/main/resources/ranger-tagsync-site.xml 3542ae2 > > > Diff: https://reviews.apache.org/r/65920/diff/3/ > > > Testing > ------- > > 1)verified if ranger tagsync works as expected. > 2)verified if tagsync is not re-login for every notification if > "ranger.tagsync.cookie.enabled" is set as true. > > > File Attachments > ---------------- > > 0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch > > https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch > > > Thanks, > > Nikhil P > >
