Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
On Sept. 16, 2014, 6:42 a.m., Thejas Nair wrote: ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 741 https://reviews.apache.org/r/25616/diff/2/?file=690379#file690379line741 should we skip it from ReadEntity if none of the columns are being used ? Though, that case is not going to be common. eg a query like 'update table set j=null;' should not require select privileges on the table, as there is no columns in where clause or value expression. Note that this is a change that we can also make in future without breaking users. (making a change in future to require fewer privileges will not break users). Ie, it does not have to be addressed in this patch. I don't think that makes any sense. If I have delete permissions but not select permissions I can delete all rows from a table but not some rows? That definitely violates the law of least astonishment. On Sept. 16, 2014, 6:42 a.m., Thejas Nair wrote: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java, line 265 https://reviews.apache.org/r/25616/diff/2/?file=690378#file690378line265 can you also use a column in the i = expression to make sure that that also gets included in the input list. eg (add another column l to table definition) update ... set i = 5 + l where j = 3; Done. - Alan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53485 --- On Sept. 16, 2014, 3:35 a.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 16, 2014, 3:35 a.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessInfo.java a4df8b4 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 16, 2014, 7:37 p.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs (updated) - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessInfo.java a4df8b4 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53580 --- itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java https://reviews.apache.org/r/25616/#comment93294 A nit : Lines 292-299 could be written as - assertEquals(Columns used, Arrays.asList(i, “l”), getSortedList(tableObj.getColumns())); Same with lines 303-311 - Thejas Nair On Sept. 16, 2014, 7:37 p.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 16, 2014, 7:37 p.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessInfo.java a4df8b4 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53593 --- Ship it! Ship It! - Thejas Nair On Sept. 16, 2014, 7:37 p.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 16, 2014, 7:37 p.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessInfo.java a4df8b4 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
On Sept. 16, 2014, 6:42 a.m., Thejas Nair wrote: ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 741 https://reviews.apache.org/r/25616/diff/2/?file=690379#file690379line741 should we skip it from ReadEntity if none of the columns are being used ? Though, that case is not going to be common. eg a query like 'update table set j=null;' should not require select privileges on the table, as there is no columns in where clause or value expression. Note that this is a change that we can also make in future without breaking users. (making a change in future to require fewer privileges will not break users). Ie, it does not have to be addressed in this patch. Alan Gates wrote: I don't think that makes any sense. If I have delete permissions but not select permissions I can delete all rows from a table but not some rows? That definitely violates the law of least astonishment. You need select privileges only for use of columns in the value expression (in update) and where clause (in update/delete). In case of an authorization implementation that supports column level access control on table, this is the behavior you get. You will be able to delete all rows even if you don't have select privileges on the columns. For example, you could have an application that sets some fields with PII to null. It would not need select privileges. I am fine with current approach, as we have the ability to change it in future, based on user feedback. - Thejas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53485 --- On Sept. 16, 2014, 7:37 p.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 16, 2014, 7:37 p.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessInfo.java a4df8b4 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53316 --- itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java https://reviews.apache.org/r/25616/#comment92956 Wouldn't select permissions for using column j in where clause be needed ? In most databases, you get to know the number of rows getting updated. Using that information, with the query in the test, you could find number of columns where j = 3. I haven't verified what SQL spec says about this (privileges needed for including columns in where clause in update statement.) Postgres says it is needed : http://www.postgresql.org/docs/9.2/static/sql-update.html You must have the UPDATE privilege on the table, or at least on the column(s) that are listed to be updated. You must also have the SELECT privilege on any column whose values are read in the expressions or condition. - Thejas Nair On Sept. 14, 2014, 4:30 a.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 14, 2014, 4:30 a.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
On Sept. 15, 2014, 7:24 a.m., Thejas Nair wrote: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java, line 272 https://reviews.apache.org/r/25616/diff/1/?file=688987#file688987line272 Wouldn't select permissions for using column j in where clause be needed ? In most databases, you get to know the number of rows getting updated. Using that information, with the query in the test, you could find number of columns where j = 3. I haven't verified what SQL spec says about this (privileges needed for including columns in where clause in update statement.) Postgres says it is needed : http://www.postgresql.org/docs/9.2/static/sql-update.html You must have the UPDATE privilege on the table, or at least on the column(s) that are listed to be updated. You must also have the SELECT privilege on any column whose values are read in the expressions or condition. I looked through the SQL spec and couldn't figure it out one way or another. I chose this route because it seemed odd to require SELECT privileges for UPDATE and DELETE. I see what you're saying about being able to tease out information such as how many rows match a where clause. If we want to require SELECT privileges for these operations that's ok. I'll just need to rework a few pieces of the patch and some of the tests. - Alan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53316 --- On Sept. 14, 2014, 4:30 a.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 14, 2014, 4:30 a.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53277 --- itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java https://reviews.apache.org/r/25616/#comment92869 This line that sets concurrency false can be removed. itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java https://reviews.apache.org/r/25616/#comment92870 It would be good to also verify the input columns being passed here. itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java https://reviews.apache.org/r/25616/#comment92871 A similar test for delete would also be useful, specially for testing the input columns being passed. ql/src/java/org/apache/hadoop/hive/ql/Driver.java https://reviews.apache.org/r/25616/#comment92872 Can you also change the variable name of tab2cols to indicate that it is the table to input column mapping (since we have updateTab2Cols) ? maybe selectTab2Cols or inputTab2Cols ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java https://reviews.apache.org/r/25616/#comment92873 For insert-overwrite, both insert and delete was required earlier. This would be new PrivRequirement(arr(SQLPrivTypeGrant.INSERT_NOGRANT, SQLPrivTypeGrant.DELETE_NOGRANT), IOType.OUTPUT, HivePrivObjectActionType.INSERT_OVERWRITE), ql/src/test/queries/clientpositive/authorization_update.q https://reviews.apache.org/r/25616/#comment92874 yes, This is something we really need to fix in hive! I think comment before dfs,add/delete, compile etc also has same issue. Vikram had taken a stab at fixing this, but I believe the fix turned out to be more involved that expected. - Thejas Nair On Sept. 14, 2014, 4:30 a.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 14, 2014, 4:30 a.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
Re: Review Request 25616: HIVE-7790 Update privileges to check for update and delete
On Sept. 14, 2014, 7:13 a.m., Thejas Nair wrote: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java, line 272 https://reviews.apache.org/r/25616/diff/1/?file=688987#file688987line272 It would be good to also verify the input columns being passed here. But I don't put the input columns in the list. You don't need read permissions to update, so I'm not adding these to a list to be checked. On Sept. 14, 2014, 7:13 a.m., Thejas Nair wrote: itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java, line 273 https://reviews.apache.org/r/25616/diff/1/?file=688987#file688987line273 A similar test for delete would also be useful, specially for testing the input columns being passed. Same as above on update, I'm not checking read permissions, so there's no list of input columns. On Sept. 14, 2014, 7:13 a.m., Thejas Nair wrote: ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 506 https://reviews.apache.org/r/25616/diff/1/?file=688988#file688988line506 Can you also change the variable name of tab2cols to indicate that it is the table to input column mapping (since we have updateTab2Cols) ? maybe selectTab2Cols or inputTab2Cols Changed to selectTab2Cols - Alan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/#review53277 --- On Sept. 14, 2014, 4:30 a.m., Alan Gates wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25616/ --- (Updated Sept. 14, 2014, 4:30 a.m.) Review request for hive and Thejas Nair. Bugs: HIVE-7790 https://issues.apache.org/jira/browse/HIVE-7790 Repository: hive-git Description --- Adds update and delete as action and adds checks for authorization during update and delete. Also adds passing of updated columns in case authorizer wishes to check them. Diffs - itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/authorization/plugin/TestHiveAuthorizerCheckInvocation.java 53d88b0 ql/src/java/org/apache/hadoop/hive/ql/Driver.java 298f429 ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java b2f66e0 ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java 3aaa09c ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationUtils.java 93df9f4 ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java 093b4fd ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/Operation2Privilege.java 3236341 ql/src/test/queries/clientnegative/authorization_delete_nodeletepriv.q PRE-CREATION ql/src/test/queries/clientnegative/authorization_update_noupdatepriv.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_delete_own_table.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update.q PRE-CREATION ql/src/test/queries/clientpositive/authorization_update_own_table.q PRE-CREATION ql/src/test/results/clientnegative/authorization_delete_nodeletepriv.q.out PRE-CREATION ql/src/test/results/clientnegative/authorization_update_noupdatepriv.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_delete_own_table.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update.q.out PRE-CREATION ql/src/test/results/clientpositive/authorization_update_own_table.q.out PRE-CREATION Diff: https://reviews.apache.org/r/25616/diff/ Testing --- Added tests, both positive and negative, for update and delete, including ability to update and delete tables created by user. Also added tests for passing correct update columns. Thanks, Alan Gates
