Re: Review Request 43508: AMBARI-14932: "download zip" does not work

[DIPAYAN BHOWMICK <di...@gmail.com>]

> 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.

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.


- DIPAYAN


-----------------------------------------------------------
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
> 
>