[
https://issues.apache.org/jira/browse/HADOOP-14899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16180997#comment-16180997
]
Steve Loughran commented on HADOOP-14899:
-----------------------------------------
Privilege is one of those words I can never spell right, so I had to check its
spelling before reviewing this.
h3. {{NativeAzureFileSystem}}
* L698: you can't change the name of properties which users may have been
using. If you do want to move to a new property, you need to retain the old one
& add it to the list of deprecated properties through
{{Configuration.addDeprecations}}. Given that {{NativeAzureFileSystem}} is
tagged @Public, @Stable, you'll have to do the same there, with a new constant,
@deprecate the previous one. Or just leave the name of the option alone. That's
simpler, works with existing test and docs.
* L2916: multiline javadocs need to start on the second line of the comment;
all javadocs must have a "." at the end for the javadoc compiler to be happy.
* L2980: if that line is > 80 chars, you'll need to split it. Yes, even if it
existed before: this is our chance to cleanup.
* L7971. The original code used actualUser, falling back to getCurrentuser when
actualUser == null. The new chane appears to only go off getCurrentUser. This
is a major change and the bit that worries me the most. why the change? Bear in
mind nobody ever fully understands UGI internals, so I'm not sure it's wrong,
just need to understand why the change, what the implications are. Are you
confident that getCurrentUser never returns null, or does that need to be
handled too? (FWIW, I don't see that it can return null from my quick look at
the getCurrentUser-> getLoginUser() -> loginUserFromSubject sequence.
* L3055. Better to convert the array to a list early on. And, as the conf is
unlikely to change during the life of the client, do it on FS init & cache it.
I think you could consider making the allowed user logic something out of the
class, so that you can test it in more easily. But, given you've got those
tests already written
h3. Tests.
tests look OK, though the doAs() code complicates reading. Moving to pure-Java
8 will fix that in future.
But: its pretty much the same codepath followed: create rule, create mock user,
create test path, attempt to manipulate permissions, check outcome (positive vs
negative). Which makes me think: you could factor out all the tests into one or
two methods with common behaviour
* test lists to include, [], [""], ["*"], ["user", "*"]; ["","user"], ["*",
"*"]. I think you've got most of those covered.
> Restrict Access to setPermission operation when authorization is enabled in
> WASB
> --------------------------------------------------------------------------------
>
> Key: HADOOP-14899
> URL: https://issues.apache.org/jira/browse/HADOOP-14899
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Reporter: Kannapiran Srinivasan
> Assignee: Kannapiran Srinivasan
> Labels: fs, secure, wasb
> Attachments: HADOOP-14899-001.patch, HADOOP-14899-002.patch
>
>
> In case of authorization enabled Wasb clusters, we need to restrict setting
> permissions on files or folders to owner or list of privileged users.
> Currently in the WASB implementation even when authorization is enabled there
> is no check happens while doing setPermission call. In this JIRA we would
> like to add the check on the setPermission call in NativeAzureFileSystem
> implementation so that only owner or the privileged list of users or daemon
> users can change the permissions of files/folders
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
