Review Request 68273: SENTRY-2313: alter database set owner command can be executed only by user with proper privilege

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68273/
---

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2313
https://issues.apache.org/jira/browse/sentry-2313


Repository: sentry


Description
---

only user with "all with grant option" on database or its parent issue the 
command "alter database set owner". Since HIVE-18031 is not in the hive version 
integrated with Sentry, some code is commented out with "TODO: Enable once 
HIVE-18031 is available" or "Enable the test once HIVE-18031 is in the hiver 
version integrated with Sentry"


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
 09bd9b5 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
 d976512 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
 0dd8bf1 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java
 080eda8 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 2ecf8fe 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivilegesWithGrantOption.java
 4de7475 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 f5e4db8 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestOperationsPart1.java
 f3edae2 


Diff: https://reviews.apache.org/r/68273/diff/1/


Testing
---

tested the ignored test cases on private build that has HIVE-18031 ported.


Thanks,

Na Li

Re: Review Request 68271: SENTRY-2337: [REVERT] SENTRY-2295: Owner privileges should not be granted to sentry admin users

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68271/#review207007
---


Ship it!




Ship It!

- Na Li


On Aug. 8, 2018, 9:37 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68271/
> ---
> 
> (Updated Aug. 8, 2018, 9:37 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2337
> https://issues.apache.org/jira/browse/SENTRY-2337
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This is not correct behavior as sentry admin users have privileges to 
> grant/revoke privileges need not have privileges to view/update the HMS data. 
> They need explicit privileges to do so.
> 
>  
> 
> user belonging to sentry admin groups need owner privileges for that objects 
> that they own.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  5644181af6a631120bdcd097e4e390a652106dc0 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  164188bc56e2d0332b8e9608ed163b22ec3bb08e 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  e2e8c399cae125c263ac4310f6bc3f6b42da21d9 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  2ecf8feb05d9f7807681508cd4fbe1b0982760e4 
> 
> 
> Diff: https://reviews.apache.org/r/68271/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests accordingly.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review207006
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
Lines 181 (patched)


It would be better to throw exception for other value, so people calls this 
function with other value can add meaningful operation text.


- Na Li


On Aug. 8, 2018, 10:36 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 10:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/Constants.java
>  6e91f8b9ead009629d6bccd206122b2071e8fd64 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/3/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review207004
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
Line 179 (original), 179 (patched)


should you throw exception for other value? So if someone calls this 
function for other reason, they can update this function to add meaningful 
operation text


- Na Li


On Aug. 8, 2018, 10:36 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 10:36 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/Constants.java
>  6e91f8b9ead009629d6bccd206122b2071e8fd64 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/3/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/
---

(Updated Aug. 8, 2018, 10:36 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Bugs: sentry-2157
https://issues.apache.org/jira/browse/sentry-2157


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
 61becceac881443b02182e6ab1012add4c046499 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
 6479a6055e8c7087f0e484080ec9d46a9c146212 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/Constants.java
 6e91f8b9ead009629d6bccd206122b2071e8fd64 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
 307f38eadb65bf12dc6225cfe43a5d590657d055 


Diff: https://reviews.apache.org/r/68268/diff/3/

Changes: https://reviews.apache.org/r/68268/diff/2-3/


Testing
---

I run the patch in a cluster and the audit logs is displaying the correct 
messages.

{"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
 privilege granted to USER: 
sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}


Thanks,

Sergio Pena

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/
---

(Updated Aug. 8, 2018, 10:10 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Changes
---

The new output shows:

{"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_OWNER_PRIVILEGE","eventTime":"1533765994895","operationText":"OWNER
 privilege on  table 'db2.t1'
is granted to USER: 
sergio","allowed":"true","databaseName":"db2","tableName":"t1","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}

If a transferred happens, then the message will display 'OWNER PRIVILEGE on 
table db2.t1 is transffered to USER: sergio'


Bugs: sentry-2157
https://issues.apache.org/jira/browse/sentry-2157


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java
 ef63a34df1c90a08fc397a22454b6be176bae6cf 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java
 e261fc6f66e29174c13940c70bdcf0caaa5290fa 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java
 ee4fce56ec18092dca49036263e7f2590ba9fb66 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 c1beaed0a6c76ae6cc60bde241b1aa2deae030a8 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeChanges.java
 abcf3ca5d813c8ae04413111d3decdbf587c693d 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeEntity.java
 85f81475192c58535700a0ca078bfbe25ac1fac4 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeEntityType.java
 ac44c1f91396f96d6dec52912f3fade089eeb845 
  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java
 5691933fe85688f4f064c8c25e181c2857b1e6f3 
  sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift 
61582cd0eb9a8c096d84fa97e0d7639605f6265e 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPermissionUpdate.java
 8bd9d439bcca7bdb6b94375a21b98388e2dcdb99 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
 c162ec19cbc40fc2a8dc6192f5af2db82e90336e 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java
 761c760f2f982e2ce580a1e0af0295d735d9fb5b 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
 f0ca787bb0c98c0ecbfebe4f8bba1a0fb499eec5 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java
 d47b9f73ee92de49bd785e9587ce47df5b2d3a80 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
 2a675041811ece7147fdd3c1bef767ac27113a2c 
  
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
 6cb787bb7f61f0ee3e536d073757ba8758ae8fa6 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java
 d7bc748345638c49f341404b356d8a1944c2612f 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
 b86136d6fe857aa1a87214550276f7b6b0800de9 
  
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
 845c1377d3756fe2df794d79a5fcb3d0e9f2eb31 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TListSentryPrivilegesRequest.java
 01e52300e2eba72aa4f7fa3e9ecab77aa0737d4b 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryHmsEventNotification.java
 fe891252eec81b28651106ea73a9480d42d5a0b7 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryObjectOwnerType.java
 6b540b88241abf4cb87246082b481f2b4c6fdca2 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java
 1a8034b10f381395ca93c81cd432505d5fdcb5c8 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 07da2ba3caf9f5e5f1191ab7d3843ab4fd5dde33 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 f238748ccc5c1f1b6157ec04a0b1d3211f997ccf 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java
 

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 7:25 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1577 (patched)
> > 
> >
> > should this be audit.onUpdateOwnerPrivilege()?
> 
> Sergio Pena wrote:
> Update implies a grant, so I'd rather use grant to avoid having two 
> methods that do the same.
> 
> Na Li wrote:
> update implies a grant and also removing. It is alter, not just grant. It 
> is better to be accurate, so we can differentiate different scenarios. In 
> this way, it is easier to debug when something goes wrong. To do this, you 
> need to add more functions for update message such as "OWNER privilege is 
> transferred to USER" OR "OWNER privilege is transferred to ROLE" base on the 
> new owner type, not "OWNER privilege granted to USER"

Aaa true. I changed it to audit.onTransferOwnerPrivilege()


- Sergio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206991
---


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 9:12 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
> > Lines 169 (patched)
> > 
> >
> > This function calls createCmdForImplicitGrantOwnerPrivilege without 
> > checking anything specific to owner privilege. Should this function's name 
> > reflect the fact that it is specific for owner privilege? Otherwise, it is 
> > easy to introduce bug that some other developer could call this function 
> > for privilege other than owner privilege. Then audit log is wrong

I modify the flags to avoid confusions.


- Sergio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206999
---


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68271: SENTRY-2337: [REVERT] SENTRY-2295: Owner privileges should not be granted to sentry admin users

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68271/
---

Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Bugs: SENTRY-2337
https://issues.apache.org/jira/browse/SENTRY-2337


Repository: sentry


Description
---

This is not correct behavior as sentry admin users have privileges to 
grant/revoke privileges need not have privileges to view/update the HMS data. 
They need explicit privileges to do so.

 

user belonging to sentry admin groups need owner privileges for that objects 
that they own.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 5644181af6a631120bdcd097e4e390a652106dc0 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 164188bc56e2d0332b8e9608ed163b22ec3bb08e 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 e2e8c399cae125c263ac4310f6bc3f6b42da21d9 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
 2ecf8feb05d9f7807681508cd4fbe1b0982760e4 


Diff: https://reviews.apache.org/r/68271/diff/1/


Testing
---

Updated the tests accordingly.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Na Li via Review Board]

> On Aug. 8, 2018, 7:25 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1577 (patched)
> > 
> >
> > should this be audit.onUpdateOwnerPrivilege()?
> 
> Sergio Pena wrote:
> Update implies a grant, so I'd rather use grant to avoid having two 
> methods that do the same.

update implies a grant and also removing. It is alter, not just grant. It is 
better to be accurate, so we can differentiate different scenarios. In this 
way, it is easier to debug when something goes wrong. To do this, you need to 
add more functions for update message such as "OWNER privilege is transferred 
to USER" OR "OWNER privilege is transferred to ROLE" base on the new owner 
type, not "OWNER privilege granted to USER"


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206991
---


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206999
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
Lines 169 (patched)


This function calls createCmdForImplicitGrantOwnerPrivilege without 
checking anything specific to owner privilege. Should this function's name 
reflect the fact that it is specific for owner privilege? Otherwise, it is easy 
to introduce bug that some other developer could call this function for 
privilege other than owner privilege. Then audit log is wrong


- Na Li


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68264: SENTRY-2335: Allow sentry to have multiple handlers for a Signal

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68264/#review206998
---


Ship it!




Ship It!

- Na Li


On Aug. 8, 2018, 8:25 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68264/
> ---
> 
> (Updated Aug. 8, 2018, 8:25 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2335
> https://issues.apache.org/jira/browse/SENTRY-2335
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This changes the SigUtil class to allow for multiple signal handler functions 
> to be defined and executed per signal.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
>  1f16e1435dde850d651b9b9f5ad8e6253269d106 
> 
> 
> Diff: https://reviews.apache.org/r/68264/diff/2/
> 
> 
> Testing
> ---
> 
> Tested with unit tests and manual testing
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 68264: SENTRY-2335: Allow sentry to have multiple handlers for a Signal

[Brian Towles via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68264/
---

(Updated Aug. 8, 2018, 3:25 p.m.)


Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Changes
---

Added try/catch for signal handler running


Bugs: SENTRY-2335
https://issues.apache.org/jira/browse/SENTRY-2335


Repository: sentry


Description
---

This changes the SigUtil class to allow for multiple signal handler functions 
to be defined and executed per signal.


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
 1f16e1435dde850d651b9b9f5ad8e6253269d106 


Diff: https://reviews.apache.org/r/68264/diff/2/

Changes: https://reviews.apache.org/r/68264/diff/1-2/


Testing
---

Tested with unit tests and manual testing


Thanks,

Brian Towles

Re: Review Request 68264: SENTRY-2335: Allow sentry to have multiple handlers for a Signal

[Brian Towles via Review Board]

> On Aug. 8, 2018, 2:48 p.m., Na Li wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
> > Line 66 (original), 72 (patched)
> > 
> >
> > should we use try/catch here? So if one handler fails to handle the 
> > signal, the next handlers can still process the singnal?

Yeah this makes sense.


- Brian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68264/#review206992
---


On Aug. 7, 2018, 4 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68264/
> ---
> 
> (Updated Aug. 7, 2018, 4 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2335
> https://issues.apache.org/jira/browse/SENTRY-2335
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This changes the SigUtil class to allow for multiple signal handler functions 
> to be defined and executed per signal.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
>  1f16e1435dde850d651b9b9f5ad8e6253269d106 
> 
> 
> Diff: https://reviews.apache.org/r/68264/diff/1/
> 
> 
> Testing
> ---
> 
> Tested with unit tests and manual testing
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 67888: SENTRY-2308: Create privilege on table has no use case

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67888/#review206994
---



can you add test case for this change?

- Na Li


On July 13, 2018, 6:45 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67888/
> ---
> 
> (Updated July 13, 2018, 6:45 p.m.)
> 
> 
> Review request for sentry and Arjun Mishra.
> 
> 
> Bugs: sentry-2308
> https://issues.apache.org/jira/browse/sentry-2308
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Do not allow CREATE privileges on a table. There is no use case for it.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java
>  2c662e55382adc36abd18b6b6b82aa7a0b2f210b 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java
>  beca2f8f3fd03953d5652337dee7e69313f098a3 
> 
> 
> Diff: https://reviews.apache.org/r/67888/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68264: SENTRY-2335: Allow sentry to have multiple handlers for a Signal

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68264/#review206992
---




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
Line 66 (original), 72 (patched)


should we use try/catch here? So if one handler fails to handle the signal, 
the next handlers can still process the singnal?


- Na Li


On Aug. 7, 2018, 9 p.m., Brian Towles wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68264/
> ---
> 
> (Updated Aug. 7, 2018, 9 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2335
> https://issues.apache.org/jira/browse/SENTRY-2335
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This changes the SigUtil class to allow for multiple signal handler functions 
> to be defined and executed per signal.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SigUtils.java
>  1f16e1435dde850d651b9b9f5ad8e6253269d106 
> 
> 
> Diff: https://reviews.apache.org/r/68264/diff/1/
> 
> 
> Testing
> ---
> 
> Tested with unit tests and manual testing
> 
> 
> Thanks,
> 
> Brian Towles
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206991
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Line 1396 (original), 1336 (patched)


do we generate audit message when owner privilege is removed in 
notification processor?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1577 (patched)


should this be audit.onUpdateOwnerPrivilege()?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1582 (patched)


should this be audit.onUpdateOwnerPrivilege()?


- Na Li


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206989
---


Ship it!




Ship It!

- Arjun Mishra


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Line 1619 (original), 1571 (patched)
> > 
> >
> > Why don't we have an audit log for revoking owner privileges?
> 
> Sergio Pena wrote:
> Because there is not information about what is being revoked. The revoke 
> happens by the revokeOwnerPrivileges() call.
> 
> I initially though on breaking the code to allow auditing the revoke, but 
> something special of the OWNER privilege is that only one role or user can 
> have it and it cannot be revoked. So, if you grant the OWNER to a user or 
> role, it immediatly means nobody else has that privilege, so in this case, it 
> does not make sense to log that as the OWNER privilege is assigned to one 
> user or role.
> 
> Arjun Mishra wrote:
> So are you saying we can never revoke owner privileges? If yes then I am 
> ok with the change. I would think the owner privilege would get revoked by 
> deleting the hive object. And I know Sentry gets notified of it

The owner and any other privilege is deleted by Sentry when an object is 
deleted on HMS. That's (for some reason) hasn't been audited. We might want to 
audit that too.
And yes, the OWNER privilege cannot be revoked, it can only be transferred to 
another user.


- Sergio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206980
---


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68249: SENTRY-2255: alter table set owner command can be executed only by user with proper privilege

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68249/#review206985
---


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Aug. 7, 2018, 2:14 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68249/
> ---
> 
> (Updated Aug. 7, 2018, 2:14 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2255
> https://issues.apache.org/jira/browse/sentry-2255
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> 1) support grant option for DDL option. This is a new feature. Many places 
> have to be changed at sentry server and client to make it happen
> 2) Add privilege map in HiveAuthzPrivilegesMap, which is commentted out as 
> hive version integrated with Sentry does not support "ALTER TABLE SET OWNER"
> 3) Add more testing cases and verify owner privilege, which is disabled
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
>  f1cbbb6 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivileges.java
>  f164b30 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java
>  9350af0 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java
>  ab55609 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java
>  73fcda8 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoAuthorizationProvider.java
>  be0830d 
>   
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
>  a9b98f3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  621ce89 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
>  8a10214 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  4588963 
> 
> 
> Diff: https://reviews.apache.org/r/68249/diff/1/
> 
> 
> Testing
> ---
> 
> existing test cases in TestOwnerPrivileges passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Arjun Mishra via Review Board]

> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Line 1619 (original), 1571 (patched)
> > 
> >
> > Why don't we have an audit log for revoking owner privileges?
> 
> Sergio Pena wrote:
> Because there is not information about what is being revoked. The revoke 
> happens by the revokeOwnerPrivileges() call.
> 
> I initially though on breaking the code to allow auditing the revoke, but 
> something special of the OWNER privilege is that only one role or user can 
> have it and it cannot be revoked. So, if you grant the OWNER to a user or 
> role, it immediatly means nobody else has that privilege, so in this case, it 
> does not make sense to log that as the OWNER privilege is assigned to one 
> user or role.

So are you saying we can never revoke owner privileges? If yes then I am ok 
with the change. I would think the owner privilege would get revoked by 
deleting the hive object. And I know Sentry gets notified of it


- Arjun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206980
---


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1503 (patched)
> > 
> >
> > Can we maintain the same consistency with other audit methods?Maybe 
> > just pass in the request object?
> > 
> > Also, Can you explain why onGrantOwnerPrivileges needs the Status 
> > parameter to be passed in?

I cannot keep the consistency unless I create a thrift object for user 
privileges. Because the owner privilege is granted implicitly by Sentry, then 
we don't need the Thrift object, so I had to have this method with those 
parameters.

The Status is used by the createJsonLogEntity() method to append the status.


> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Line 1619 (original), 1571 (patched)
> > 
> >
> > Why don't we have an audit log for revoking owner privileges?

Because there is not information about what is being revoked. The revoke 
happens by the revokeOwnerPrivileges() call.

I initially though on breaking the code to allow auditing the revoke, but 
something special of the OWNER privilege is that only one role or user can have 
it and it cannot be revoked. So, if you grant the OWNER to a user or role, it 
immediatly means nobody else has that privilege, so in this case, it does not 
make sense to log that as the OWNER privilege is assigned to one user or role.


> On Aug. 8, 2018, 3:46 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
> > Lines 215 (patched)
> > 
> >
> > I like this. Was curious since this should return the command that the 
> > user ran. 
> > Is there some way to display that?

I thought that too, but how to know what happend was executed? We know it could 
be a create database or table, or alter database or table; but that's a command 
from Hive. This audit log is for Sentry (no Hive). The only way I thought so is 
to write that message.


- Sergio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206980
---


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/#review206981
---


Ship it!




Overall looks good. I had a question about audit message for revoking owner 
privleges and a few clarifications.

- Arjun Mishra


On Aug. 8, 2018, 2:29 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68268/
> ---
> 
> (Updated Aug. 8, 2018, 2:29 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.
> 
> 
> Bugs: sentry-2157
> https://issues.apache.org/jira/browse/sentry-2157
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This patch logs owner privileges grants and revokes.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  61f9168b1970144dbf0b7a7378f2d25e70f1761d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
>  61becceac881443b02182e6ab1012add4c046499 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
>  6479a6055e8c7087f0e484080ec9d46a9c146212 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
>  307f38eadb65bf12dc6225cfe43a5d590657d055 
> 
> 
> Diff: https://reviews.apache.org/r/68268/diff/1/
> 
> 
> Testing
> ---
> 
> I run the patch in a cluster and the audit logs is displaying the correct 
> messages.
> 
> {"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
>  privilege granted to USER: 
> sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/
---

(Updated Aug. 8, 2018, 2:29 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Bugs: sentry-2157
https://issues.apache.org/jira/browse/sentry-2157


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
 61becceac881443b02182e6ab1012add4c046499 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
 6479a6055e8c7087f0e484080ec9d46a9c146212 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
 307f38eadb65bf12dc6225cfe43a5d590657d055 


Diff: https://reviews.apache.org/r/68268/diff/1/


Testing (updated)
---

I run the patch in a cluster and the audit logs is displaying the correct 
messages.

{"serviceName":"Sentry-Service","userName":"ubuntu","impersonator":"","ipAddress":"/127.0.0.1","operation":"GRANT_PRIVILEGE","eventTime":"1533738512795","operationText":"OWNER
 privilege granted to USER: 
sergio","allowed":"true","databaseName":"default","tableName":"t2","column":null,"resourcePath":null,"objectType":"PRINCIPAL"}


Thanks,

Sergio Pena

Review Request 68268: SENTRY-2157: Update audit log to grant/revoke owner privileges

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68268/
---

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li.


Bugs: sentry-2157
https://issues.apache.org/jira/browse/sentry-2157


Repository: sentry


Description
---

This patch logs owner privileges grants and revokes.


Diffs
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 61f9168b1970144dbf0b7a7378f2d25e70f1761d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/audit/SentryAuditLogger.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java
 61becceac881443b02182e6ab1012add4c046499 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java
 6479a6055e8c7087f0e484080ec9d46a9c146212 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java
 307f38eadb65bf12dc6225cfe43a5d590657d055 


Diff: https://reviews.apache.org/r/68268/diff/1/


Testing
---

I run the patch in a cluster and the audit logs is displaying the correct 
messages.


Thanks,

Sergio Pena