Re: Review Request 21749: HIVE-7061 - sql std auth - insert queries without overwrite should not require delete privileges
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
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
--- 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
Re: 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/#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
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
