> On Feb. 17, 2016, 4:25 p.m., Di Li wrote:
> > Hello Keta,
> >
> > Why did you remove code change introduced via AMBARI-14228? Have you tested
> > if that'd introduce regression?
> >
> > If you look at DownloadService.java, methods mapped to @Path("/zip")
> > neither is setting allowed to true. as the "allowed" is only set when url
> > is /browse, have you tried distinguishing when to and not to check the
> > "data.allowed" at the front end?
> >
> > Please also add Jaimin Jetly as a reviewer?
>
> Keta Patel wrote:
> Hello Di,
> I have added Jaimin Jetly as per your suggestion.
>
> Regarding your question on when the front end distinguishes between
> checking "data.allowed" in the response, I have understood the following from
> the code:
> 1. For the API with @Path("/browse"), the following happens on the
> server-side:
> - To check if the user has permission to read the file, a file-open
> operation is performed. If the user has required permission, then the
> operation goes through successfully.
> But if the user does not have the permission, then a
> "ServiceFormattedException" error is thrown. This error is caught in the
> front-end by the controller and an error message is displayed.
> - If the user is able to open the file without throwing the error:
> - then the next statement checks if "checkperm=true", and returns a
> response with "allowed=true"
> - else if "checkperm=false", the response returned only contains the
> file to be downloaded.
>
> 2. For teh API with @Path("/concat") and @Path("/zip"), the following
> happens on server-side:
> - "checkperm" is not used at all.
> - The response contains only the data of the file(s) to be downloaded.
>
> 3. Front-end controller:
> - It always calls the function "downloadFile" for any download action
> in the FILES view.
> - This function always makes the server call with "checkperm=true"
> - It then checks if the response contained "allowed=true" to proceed
> with the download.
> - If the response doesn't contain "allowed=true", nothing is done.
> - If the response contains "allowed=true", a 2nd server call is made
> with "checkperm=false" which returns the response with the file(s) to be
> downloaded.
>
>
> The above code works fine for the API with @Path("/browse"), but fails to
> download for the other 2 APIs.
> The proposed FIX in the patch does the following for the above 3 points:
> 1. For the API with @Path("/browse"):
> - In order to see if the user has permission for the file, if the file
> open operation does not throw an error, we can conclude that the user has the
> required permission.
> We do not need to set "allowed=true" just to indicate that the user
> has the permission. In the proposed fix, I have removed the clause checking
> if "checkperm=true"
> and hence, response no longer contains "allowed=true"
> - If the user is able to open the file without throwing the error:
> - the response returned only contains the file to be downloaded.
>
> 2. For teh API with @Path("/concat") and @Path("/zip"):
> - No changes are made.
>
> 3. Front-end controller:
> - I have removed the check for "allowed=true" in the response.
> - If an error was thrown, it gets caught in the failureCallback
> function.
> - Else, the file is downloaded.
>
> Thank you!
I see. Thanks for the explanation.
- Di
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43508/#review119469
-----------------------------------------------------------
On Feb. 17, 2016, 7:22 p.m., Keta Patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43508/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2016, 7:22 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.
>
>
> Thanks,
>
> Keta Patel
>
>
