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/#review207005
---


Ship it!




Ship It!

- 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
 c7145848a9e280d9267fbd

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
> 
>

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 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
> > Line 1396 (original), 1336 (patched)
> > 
> >
> > do we generate audit message when owner privilege is removed in 
> > notification processor?

There is not audit process in the NotificationProcessor. That would require 
different parameters in the Audit loger and how the JSON are generated as well.
Currently all privileges are cleaned-up when a DROP command is found and are 
not audited. We should consider investigat that issue in another patch as 
cleaning an unexistent object of privileges is different from revoking 
privileges.


> 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()?

Update implies a grant, so I'd rather use grant to avoid having two methods 
that do the same.


> 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 1582 (patched)
> > 
> >
> > should this be audit.onUpdateOwnerPrivilege()?

Update implies a grant, so I'd rather use grant to avoid having two methods 
that do the same.


- 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

[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 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

[Arjun Mishra via Review Board]

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




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?



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?



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?


- 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