Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )
Change subject: KUDU-3090 Add delegate admin privilege ...................................................................... Patch Set 4: (8 comments) We can probably break this out of the relation chain so it doesn't depend on the owner patch. The behavior we're adding hinges on ownership in CreateTable RPCs (which already exists), rather than storing ownership under the hood. http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@414 PS4, Line 414: GrantAllWithGrantPrivilege Let's follow suit with naming and call this GrantAllWithGrantDatabasePrivilege http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@419 PS4, Line 419: AuthorizationPolicy tbl_policy; : tbl_policy.databases.emplace_back(p.db_name); : tbl_policy.tables.emplace_back("*"); : tbl_policy.items.emplace_back(PolicyItem({kTestUser}, {ActionPB::ALL}, true)); : RETURN_NOT_OK(ranger_->AddPolicy(move(tbl_policy))); Why do we need this too? http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@660 PS4, Line 660: TEST_P(MasterAuthzITest, TestCreateTableDifferentOwner) { Consider adding some or all of the following test cases: - what if the user is a delegate admin for the table? - what if the user also has ALL ON TABLE (but not on database) - what if the user has ALL ON DATABASE but isn't a delegate admin? - what if the user is a trusted user? http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc@95 PS4, Line 95: admin nit: same here -- this naming makes it sound like we should be passing the name of an admin around. Perhaps consider something like 'requires_admin' or 'requires_delegate_admin' http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger.proto@71 PS4, Line 71: adminisitrative privileges Given the vast number of admins that appear to exist in Ranger[1], let's be specific and refer to this in comments and in variable name as "delegate admin". [1] https://cwiki.apache.org/confluence/display/RANGER/Introduction+of+Security+Zones+in+Ranger Also I mentioned something similar in another review -- IMO it's a confusing practice to refer to booleans as things that sound like they should be strings (e.g. admin/owner instead of requires_admin/is_owner). consider naming this requires_delegate_admin or somesuch http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: PS4: Not totally related to this patch, but we should really consider migrating this to use MiniRanger. The fact that we can plug in arbitrary privileges and allowances seems odd to me. http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@182 PS4, Line 182: false nit: comment annotations here too? http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@398 PS4, Line 398: true Why do any of these need to be true? -- To view, visit http://gerrit.cloudera.org:8080/16071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c Gerrit-Change-Number: 16071 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 16:33:04 +0000 Gerrit-HasComments: Yes
