[ 
https://issues.apache.org/jira/browse/HADOOP-14935?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16198647#comment-16198647
 ] 

Steve Loughran commented on HADOOP-14935:
-----------------------------------------

As noted, I'm not going to consider this patch for commit until Yetus is happy 
and you confirm you've tested everything yourself. Here's a provisional review.


This patch includes a new option for 
getFileStatus"fs.azure.enable.authorization.getfilestatus". What is this, why 
is it needed, and why isn't it is own patch?

And, given that HADOOP-14845 only added this code last week, how stable is all 
of this? 

h4. NativeAzureFileSystems

* javadocs to Access to explictly declare that AccessControlException is raised 
on access control, FNFE if the file is not present; same as the superclass. 
Maybe just use the {@inheritDoc} tag to do this.

* if it's just formatting changes, let's leave the changes to getFileStatus 
out: keeps the merge complexity down.

h4. tests


* {{testAccessFileDoesNotExist}} doesn't reset permissions. Does this matter?
* add test to verify that execute isn't validated (i.e. valid even if caller 
has no permissions)
* add test to create a file under a directory which does not exist. Check 
access permissions of created parent dir
* test for file without permissions to access and doesn't exist: need to verify 
that FNFE is raised.
* I'd like to see the codepath for all possible access permissions checked, 
including NONE. These can all be done within existing tests; it's needed to 
stop regressions in future.


h4. minor style issues

* No need to prefix things with "this.". it's only done when there is a 
parameter to a method with the same name as a field, something only done in 
constructors and setters
* space after a // and its comment




> Azure: POSIX permissions are taking effect in access() method even when 
> authorization is enabled
> ------------------------------------------------------------------------------------------------
>
>                 Key: HADOOP-14935
>                 URL: https://issues.apache.org/jira/browse/HADOOP-14935
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: fs/azure
>    Affects Versions: 2.9.0
>            Reporter: Santhosh G Nayak
>            Assignee: Santhosh G Nayak
>         Attachments: HADOOP-14935.1.patch
>
>
> FileSystem implementation class for azure i.e. {{NativeAzureFileSystem}} does 
> not override {{access(path,mode)}} method and uses the default implementation 
> from the base class. This base implementaion uses the POSIX permissions to 
> check if the requested user has access to given path or not even when 
> authorization is enabled, which is incorrect.
> {{NativeAzureFileSystem.access()}} in authorization enabled mode should use 
> the authorization mechanism provided instead of relying on the POSIX 
> permission ons. So the proposal is to override {{FileSystem.access()}} method 
> in {{NativeAzureFileSystem}} such that it honors the authorization mechanism 
> configured in authorization enabled mode and falls back to POSIX permissions 
> otherwise.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to