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

Phabricator commented on HIVE-3705:
-----------------------------------

ashutoshc has requested changes to the revision "HIVE-3705 [jira] Adding 
authorization capability to the metastore".

  Patch is getting close. I added few more comments, mostly around documenting 
semantics used in the patch.

INLINE COMMENTS
  
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256
 This warrants a java-doc comment that in case of partition for which location 
is not available yet, table's location is used. This will be the case when some 
one does alter table add partition.
  Actually, this will be true even in case of create table, where dir will get 
created only after the authorization has happened, so will it check permissions 
of parent ?

  I think it will be good to add java-doc of all the scenarios, which dir will 
be used to base authentication. e.g., in case if user does create table and 
provides location and dir already exists, then that dir is used, otherwise 
parent dir.

  Semantics of which dir is used in which use-case need to be called out for 
explicitly. I assume your semantics model is based on 
http://wiki.apache.org/pig/Howl/HowlAuthorizationProposal and 
http://wiki.apache.org/pig/Howl/AuthorizationImplNotes

  If so, please document various pieces of code of this patch with all this 
information.

  
ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27
 You don't store the handler, just use its conf. So, I don't see how this 
interface and its impl is useful as it stands today.
  On the other hand, I agree that, this is an important interface and is good 
to have this interface here because there could be alternative impl possible 
here. So, lets have the interface.
  
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122
 As I noted above, these kind of semantic decision need to be documented in the 
code.
  
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168
 If they are not used currently than instead of defining semantics now, we 
should let it fail in default branch and have the exception thrown, such that 
when someone later does add support for these, we can formalize the semantics.
  
ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44
 In the trunk, this is done via  MetaStoreUtils::startMetaStore() . See, 
https://issues.apache.org/jira/browse/HIVE-3724 for recommended usage pattern 
for it.

REVISION DETAIL
  https://reviews.facebook.net/D6681

BRANCH
  HIVE-3705

To: JIRA, ashutoshc, khorgath

                
> Adding authorization capability to the metastore
> ------------------------------------------------
>
>                 Key: HIVE-3705
>                 URL: https://issues.apache.org/jira/browse/HIVE-3705
>             Project: Hive
>          Issue Type: New Feature
>          Components: Authorization, Metastore
>            Reporter: Sushanth Sowmyan
>            Assignee: Sushanth Sowmyan
>         Attachments: HIVE-3705.D6681.1.patch, HIVE-3705.D6681.2.patch, 
> HIVE-3705.D6681.3.patch, HIVE-3705.D6681.4.patch, 
> hive-backend-auth.2.git.patch, hive-backend-auth.git.patch, 
> hivesec_investigation.pdf
>
>
> In an environment where multiple clients access a single metastore, and we 
> want to evolve hive security to a point where it's no longer simply 
> preventing users from shooting their own foot, we need to be able to 
> authorize metastore calls as well, instead of simply performing every 
> metastore api call that's made.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to