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

2016-03-03 Thread Keta Patel 


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

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

2016-02-29 Thread DIPAYAN BHOWMICK 


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

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

2016-02-23 Thread Keta Patel 

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


Changes
---

I have attached a new patch "AMBARI-14932_fix2" which retains the changes from 
AMBARI-14228 by updating the condition in the controller "file.js" where it 
checks for "data.allowed" only if option="browse". No changes are required in 
the server-code.

Please provide your feedback on the new fix.


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 (updated)


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

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

2016-02-18 Thread Keta Patel 


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

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.


- Keta


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


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

2016-02-18 Thread DIPAYAN BHOWMICK 

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




contrib/views/files/src/main/java/org/apache/ambari/view/filebrowser/DownloadService.java
 


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.


- DIPAYAN BHOWMICK


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

2016-02-18 Thread Di Li 

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


Ship it!




Ship It!

- Di Li


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

2016-02-18 Thread Di Li 


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

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

2016-02-17 Thread Keta Patel 


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

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

2016-02-17 Thread Di Li 

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

2016-02-12 Thread Keta Patel 

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


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

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