> 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?
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!
- Keta
-----------------------------------------------------------
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
>
>
