You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Thejas Nair <th...@hortonworks.com> on 2014/05/21 03:49:41 UTC
Review Request 21749: HIVE-7061 - sql std auth - insert queries without
overwrite should not require delete privileges
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21749/
-----------------------------------------------------------
Review request for hive, Ashutosh Chauhan and Thejas Nair.
Bugs: HIVE-7061
https://issues.apache.org/jira/browse/HIVE-7061
Repository: hive-git
Description
-------
See bug
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 9040d9b
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java a3a689d
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java b0a804c
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java 229c063
ql/src/test/queries/clientnegative/authorization_insertoverwrite_nodel.q PRE-CREATION
ql/src/test/queries/clientpositive/authorization_insert.q PRE-CREATION
ql/src/test/results/clientnegative/authorization_insert_noinspriv.q.out ee8d49e
ql/src/test/results/clientnegative/authorization_insertoverwrite_nodel.q.out PRE-CREATION
ql/src/test/results/clientpositive/authorization_insert.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/21749/diff/
Testing
-------
Test included.
Thanks,
Thejas Nair
Re: Review Request 21749: HIVE-7061 - sql std auth - insert queries without
overwrite should not require delete privileges
Posted by Thejas Nair <th...@hortonworks.com>.
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java, line 51
> > <https://reviews.apache.org/r/21749/diff/1/?file=585933#file585933line51>
> >
> > Better name : HivePrivType?
This is an object type and not a privilege type.
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 763
> > <https://reviews.apache.org/r/21749/diff/1/?file=585932#file585932line763>
> >
> > What other writeType is legal here? This should throw unsupported writeType instead I think.
For purposes of authorization other write types are not considered. I have renamed DEFAULT to OTHER .
I will clarify in updated comment.
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java, line 55
> > <https://reviews.apache.org/r/21749/diff/1/?file=585933#file585933line55>
> >
> > Better name : HivePrivActionType ? Object in there sounds redundant ?
> >
This type is applicable only within an object. There is a higher level HiveOperation for the high level operation (eg DROP_TABLE, QUERY, LOAD ..), but each object in that operation might have different actions happening on it. I think the word object in it gives better indication of where its used/applicable.
Do you want me to shorten it to HiveObjectActionType ?
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java, line 56
> > <https://reviews.apache.org/r/21749/diff/1/?file=585933#file585933line56>
> >
> > Why do we need DEFAULT here? Its good to document what actions it covers. Else, I think UNKNOWN is better name.
Changed to OTHER
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java, line 40
> > <https://reviews.apache.org/r/21749/diff/1/?file=585934#file585934line40>
> >
> > Better name : RequiredPrivs?
This is a private class that has privilege requirement and the conditions under which the privilege requirement is applicable.
Operation2Privilege.getRequiredPrivs returns a RequiredPrivileges object after applying the conditions in it.
How about PrivRequirementCondition ?
- Thejas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21749/#review43659
-----------------------------------------------------------
On May 21, 2014, 1:49 a.m., Thejas Nair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21749/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 1:49 a.m.)
>
>
> Review request for hive, Ashutosh Chauhan and Thejas Nair.
>
>
> Bugs: HIVE-7061
> https://issues.apache.org/jira/browse/HIVE-7061
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See bug
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 9040d9b
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java a3a689d
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java b0a804c
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java 229c063
> ql/src/test/queries/clientnegative/authorization_insertoverwrite_nodel.q PRE-CREATION
> ql/src/test/queries/clientpositive/authorization_insert.q PRE-CREATION
> ql/src/test/results/clientnegative/authorization_insert_noinspriv.q.out ee8d49e
> ql/src/test/results/clientnegative/authorization_insertoverwrite_nodel.q.out PRE-CREATION
> ql/src/test/results/clientpositive/authorization_insert.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21749/diff/
>
>
> Testing
> -------
>
> Test included.
>
>
> Thanks,
>
> Thejas Nair
>
>
Re: Review Request 21749: HIVE-7061 - sql std auth - insert queries without
overwrite should not require delete privileges
Posted by Ashutosh Chauhan <ha...@apache.org>.
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java, line 55
> > <https://reviews.apache.org/r/21749/diff/1/?file=585933#file585933line55>
> >
> > Better name : HivePrivActionType ? Object in there sounds redundant ?
> >
>
> Thejas Nair wrote:
> This type is applicable only within an object. There is a higher level HiveOperation for the high level operation (eg DROP_TABLE, QUERY, LOAD ..), but each object in that operation might have different actions happening on it. I think the word object in it gives better indication of where its used/applicable.
> Do you want me to shorten it to HiveObjectActionType ?
>
In that case, HivePrivObjectActionType seems better to me.
> On May 21, 2014, 10:22 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java, line 40
> > <https://reviews.apache.org/r/21749/diff/1/?file=585934#file585934line40>
> >
> > Better name : RequiredPrivs?
>
> Thejas Nair wrote:
> This is a private class that has privilege requirement and the conditions under which the privilege requirement is applicable.
>
> Operation2Privilege.getRequiredPrivs returns a RequiredPrivileges object after applying the conditions in it.
>
> How about PrivRequirementCondition ?
>
PrivRequirement is better in that case.
- Ashutosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21749/#review43659
-----------------------------------------------------------
On May 21, 2014, 1:49 a.m., Thejas Nair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21749/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 1:49 a.m.)
>
>
> Review request for hive, Ashutosh Chauhan and Thejas Nair.
>
>
> Bugs: HIVE-7061
> https://issues.apache.org/jira/browse/HIVE-7061
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See bug
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 9040d9b
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java a3a689d
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java b0a804c
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java 229c063
> ql/src/test/queries/clientnegative/authorization_insertoverwrite_nodel.q PRE-CREATION
> ql/src/test/queries/clientpositive/authorization_insert.q PRE-CREATION
> ql/src/test/results/clientnegative/authorization_insert_noinspriv.q.out ee8d49e
> ql/src/test/results/clientnegative/authorization_insertoverwrite_nodel.q.out PRE-CREATION
> ql/src/test/results/clientpositive/authorization_insert.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21749/diff/
>
>
> Testing
> -------
>
> Test included.
>
>
> Thanks,
>
> Thejas Nair
>
>
Re: Review Request 21749: HIVE-7061 - sql std auth - insert queries without
overwrite should not require delete privileges
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21749/#review43659
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/Driver.java
<https://reviews.apache.org/r/21749/#comment77941>
To keep Driver file size in control, shall we move this to AuthorizationUtils class ?
ql/src/java/org/apache/hadoop/hive/ql/Driver.java
<https://reviews.apache.org/r/21749/#comment77943>
This kind of null check may result in DEFAULT being returned which will be undesirable. I think instead of that we should make sure that writeType can never be null here and don't do null check here.
ql/src/java/org/apache/hadoop/hive/ql/Driver.java
<https://reviews.apache.org/r/21749/#comment77944>
What other writeType is legal here? This should throw unsupported writeType instead I think.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java
<https://reviews.apache.org/r/21749/#comment77948>
Better name : HivePrivType?
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java
<https://reviews.apache.org/r/21749/#comment77947>
Better name : HivePrivActionType ? Object in there sounds redundant ?
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java
<https://reviews.apache.org/r/21749/#comment77942>
Why do we need DEFAULT here? Its good to document what actions it covers. Else, I think UNKNOWN is better name.
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java
<https://reviews.apache.org/r/21749/#comment77949>
Better name : RequiredPrivs?
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java
<https://reviews.apache.org/r/21749/#comment77950>
In what case, actionType could be null ? Good to document here.
- Ashutosh Chauhan
On May 21, 2014, 1:49 a.m., Thejas Nair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21749/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 1:49 a.m.)
>
>
> Review request for hive, Ashutosh Chauhan and Thejas Nair.
>
>
> Bugs: HIVE-7061
> https://issues.apache.org/jira/browse/HIVE-7061
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> See bug
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 9040d9b
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java a3a689d
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java b0a804c
> ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java 229c063
> ql/src/test/queries/clientnegative/authorization_insertoverwrite_nodel.q PRE-CREATION
> ql/src/test/queries/clientpositive/authorization_insert.q PRE-CREATION
> ql/src/test/results/clientnegative/authorization_insert_noinspriv.q.out ee8d49e
> ql/src/test/results/clientnegative/authorization_insertoverwrite_nodel.q.out PRE-CREATION
> ql/src/test/results/clientpositive/authorization_insert.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/21749/diff/
>
>
> Testing
> -------
>
> Test included.
>
>
> Thanks,
>
> Thejas Nair
>
>
Re: Review Request 21749: HIVE-7061 - sql std auth - insert queries without
overwrite should not require delete privileges
Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21749/
-----------------------------------------------------------
(Updated May 22, 2014, 6:35 p.m.)
Review request for hive, Ashutosh Chauhan and Thejas Nair.
Changes
-------
Addressing review comments.
Fixed the one related failed test case (user name was not set, so the test output had my name in 'show grants' output).
Bugs: HIVE-7061
https://issues.apache.org/jira/browse/HIVE-7061
Repository: hive-git
Description
-------
See bug
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 9040d9b
ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java 4276b3f
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java a95d784
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java a3a689d
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java b0a804c
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java 229c063
ql/src/test/queries/clientnegative/authorization_insertoverwrite_nodel.q PRE-CREATION
ql/src/test/queries/clientpositive/authorization_insert.q PRE-CREATION
ql/src/test/results/clientnegative/authorization_insert_noinspriv.q.out ee8d49e
ql/src/test/results/clientnegative/authorization_insertoverwrite_nodel.q.out PRE-CREATION
ql/src/test/results/clientpositive/authorization_insert.q.out PRE-CREATION
Diff: https://reviews.apache.org/r/21749/diff/
Testing
-------
Test included.
Thanks,
Thejas Nair