Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16072 )
Change subject: KUDU-3090 Add ownership privileges ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/16072/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/16072/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@113 PS3, Line 113: .setAllowed(fals > nit: seems we typically have wrapped lines space at 4 spaces, or at an alig Done http://gerrit.cloudera.org:8080/#/c/16072/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@157 PS3, Line 157: * : * @param requests the given RangerR > nit: update this Done http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/client/client.h@884 PS3, Line 884: KuduTableCreator& set_owner(const std::string& owner); > Hrm, this seems like it belongs in the patch that introduces table ownershi Done http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/integration-tests/master_authz-itest.cc@866 PS3, Line 866: SERT_OK(desc.funcs.grant_privileges(this, privilege_params)); : ASSERT_OK(desc.funcs.do_action(this, action_params)); > What if the CreateTable statement requires ALL and delegate admin, e.g. if 'impala' would be a trusted user, so I don't think it applies there. Regardless, owner is not a privilege, but a user owning an existing table, so to create tables with a different owner a user is required to have delegate admin + ALL on the database. http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/master/default_authz_provider.h File src/kudu/master/default_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/master/default_authz_provider.h@51 PS3, Line 51: bool /*is_owner*/) override > It seems confusing for this to have the same API as AuthorizeCreateTable(), Done http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client-test.cc@176 PS3, Line 176: bool authorized; : ASSERT_OK(client_.AuthorizeAction("jdoe", ActionPB::CREATE, "bar", "baz", : > Why these changes? Done http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client-test.cc@179 PS3, Line 179: > nit: could you annotate these with comments, e.g. "/*is_owner*/false"? Done http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client.h@88 PS3, Line 88: is_ow > nit: IMO call-sites would be clearer if this were called 'is_owner' or some Done -- To view, visit http://gerrit.cloudera.org:8080/16072 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109 Gerrit-Change-Number: 16072 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 25 Jun 2020 15:15:59 +0000 Gerrit-HasComments: Yes
