You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@ambari.apache.org by Keta Patel <pa...@us.ibm.com> on 2016/02/12 01:38:38 UTC

Review Request 43508: "download zip" does not work

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43508/
-----------------------------------------------------------

Review request for Ambari, Di Li 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

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

Posted by Keta Patel <pa...@us.ibm.com>.


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

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

Posted by Di Li <di...@ca.ibm.com>.


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

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

Posted by Di Li <di...@ca.ibm.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43508/#review119469
-----------------------------------------------------------



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?

- Di Li


On Feb. 12, 2016, 5:34 p.m., Keta Patel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43508/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2016, 5:34 p.m.)
> 
> 
> Review request for Ambari, Di Li, DIPAYAN BHOWMICK, 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
> 
>

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

Posted by Keta Patel <pa...@us.ibm.com>.

(Updated Feb. 12, 2016, 5:34 p.m.)

Review request for Ambari, Di Li, DIPAYAN BHOWMICK, Pallav Kulshreshtha, and Srimanth Gunturi.

Summary (updated)
-----------------

AMBARI-14932: "download zip" does not work

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

TESTS:
There are already exisitng tests for DownloadService, so I haven't added any new ones.

Thanks,

Keta Patel