> On Feb. 18, 2016, 7:06 p.m., DIPAYAN BHOWMICK wrote: > > contrib/views/files/src/main/java/org/apache/ambari/view/filebrowser/DownloadService.java, > > line 88 > > <https://reviews.apache.org/r/43508/diff/1/?file=1240500#file1240500line88> > > > > The reason checkperm was introduced so that the file content is > > prevented from getting returned to the frontend when the permission check > > call is made. So, in this case the first call will be a xhr request and the > > response of it will be the content of the whole file. I ran this patch in > > my machine and verified this behavior. This is not desirable as then > > effectively we will be downloading the file twice and will be problematic > > in case of big files. > > Keta Patel wrote: > Hello Dipayan, > yes, the response returned will have the file content but the download > happens only once. Please correct my understanding on this. > > For the case of big files to be handled gracefully, do you propose we add > the "checkperm" condition even in the other 2 APIs with @Path("/zip") and > @Path("/concat") because these 2 APIs still return the actual file content in > the response. By adding the "checkperm" clause, we would make the behavior of > these 2 APIs similar to the API with @Path("/browse") and would be returning > "allowed=true" in the 1st server call. > > DIPAYAN BHOWMICK wrote: > Hi Keta, > > The /zip and /concat endpoints ignore the files for which the user > doesnot have permission to read. So, checkperm will not be required for this > case. > > What I propeose id to separate out the path for download of normal file > and directories in file > controller(https://github.com/apache/ambari/blob/trunk/contrib/views/files/src/main/resources/ui/app/controllers/file.js#L102) > and use the same logic as used in download in files > controller(https://github.com/apache/ambari/blob/trunk/contrib/views/files/src/main/resources/ui/app/controllers/files.js#L84). > This will ensure that the {allowed: true} will not be required for > downloading of directories. > > If you check, the other flow to download the directory works. If we > select a directory using the checkbox and click download zip from the top > dropdown(located at the header), the download works. This flows through the > download code in the files controller. > > Let me know your thoughts. > > Thanks.
Hello Dipayan, Thank you for your input. Your changes fix this issue. My patch is not required. Thanks! - Keta ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43508/#review119680 ----------------------------------------------------------- On Feb. 23, 2016, 5:05 p.m., Keta Patel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43508/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2016, 5:05 p.m.) > > > Review request for Ambari, Di Li, DIPAYAN BHOWMICK, Jaimin Jetly, Pallav > Kulshreshtha, and Srimanth Gunturi. > > > Bugs: AMBARI-14932 > https://issues.apache.org/jira/browse/AMBARI-14932 > > > Repository: ambari > > > Description > ------- > > Steps to Reproduce: > 1. go to "View" - "FILES" - " Create Instance" > 2. go to hdfs file view > 3. click "download zip" icon > The click doesn't download any file. The REST call returns 200 OK status. > > Detailed steps on configuring Views can be found in the link below: > http://docs.hortonworks.com/HDPDocuments/Ambari-2.1.0.0/bk_ambari_views_guide/bk_ambari_views_guide-20150721.pdf > > > Diffs > ----- > > > contrib/views/files/src/main/java/org/apache/ambari/view/filebrowser/DownloadService.java > 749174a > contrib/views/files/src/main/resources/ui/app/controllers/file.js 88fa5fb > > Diff: https://reviews.apache.org/r/43508/diff/ > > > Testing > ------- > > FIX: > The check for "allowed" in the controller, from the response of > zipByRequestId() ("/zip" API call for downloading zip file) resulted in no > action after clicking the download zip button. This was because there was no > "allowed" paramter in the response of zip download. This "allowed" property > was added in the response of browse() ("/browse" API call for downloading > individual file) to check if the user had permissions for the file or not. > This was verified by opening the file. If that operation didn't throw an > error, then it would mean that the user had the required permissions to > download the individual file. But the fact that the point of execution > reaches past the statement of opening the file verifies that the user has the > permission. The check with "checkperm" and setting the response with > "allowed" attribute in the response is not necessary. > So, for the fix I simply removed the check for "allowed" from the controller > and also removed the check for "checkperm" which sets the "allowed" attribute > in the response. > > TESTS: > There are already exisitng tests for DownloadService, so I haven't added any > new ones. > > > File Attachments > ---------------- > > AMBARI-14932_fix2 > > https://reviews.apache.org/media/uploaded/files/2016/02/23/8a296b75-37ed-442e-88cc-70319a7bdda5__AMBARI_14932_Feb23.patch > > > Thanks, > > Keta Patel > >
