Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 11: (8 comments) Mostly nits, and a couple questions. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/hms_itest-base.h@56 PS11, Line 56: boost::optional<const std::string&> user); nit: Document what this is used for. Also does it make sense to add a default of none? http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@543 PS11, Line 543: TestAuthzErrorHandling nit: it generally looks like we name our test classes as nouns and test methods as verbs, e.g. AuthzErrorHandlingTest.TestNonExistentTable. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@545 PS11, Line 545: TestNonExistentTable For CreateTable (not parameterized here) would it make sense to add test cases for some of those tricky ID/name mismatches? Also do we need to worry about error handling for GetTabletLocations? http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/authz_provider.cc File src/kudu/master/authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/authz_provider.cc@44 PS11, Line 44: vector<string> acls = strings::Split(FLAGS_trusted_user_acl, ",", strings::SkipEmpty()); : std::move(acls.begin(), acls.end(), std::inserter(trusted_users_, trusted_users_.end())); Do we have tests for this? If not, maybe it's worth separating this change out into a new CR anyway? http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1705 PS11, Line 1705: checks nit: "check", same elsewhere http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1804 PS11, Line 1804: : // If the request contains both a table ID and table name, ensure that : // both match the same table. : if (table_identifier.has_table_name() && : table.get() != FindPtrOrNull(normalized_table_names_map_, table_name).get()) { : mismatch_table = true; Just trying to grasp the specific scenario we're worried about here. Our request looks like: table ID: fake table ID table name: real table name user doesn't have privileges on real table name Before, this would result in TNF, which leaks the presence of "real table name" in 'normalized_table_names_map_'. Now, if "real table name" didn't exist, we would send back NOT_AUTHORIZED (if the user didn't have privileges on "real table name") and NOT_FOUND otherwise. Is that right? I was having a bit of trouble at first juggling the names and IDs and errors to expect in the different scenarios, so just want to make sure I understand. Also, would it be viable to authorize(table_name) at the front of this method? Then we could leave this block here through the `table_name` reset at L1834 the same as it was before, which I think is a simpler mental model because then we would know something earlier on about this request (i.e. that the user is authorized to operate on `table_name`). Would something like that work? http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1837 PS11, Line 1837: respectively nit: remove this since it's not clarifying the ordering of anything http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@4671 PS11, Line 4671: // Acquire the table lock. nit: Coming into this code fresh, one might think from this comment that we care about locking the table for this request, when really, we only care about authorizing the request which locks the table as a side effect. Mind changing the comment to clarify what's important? Or maybe remove it altogether? -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 18 Mar 2019 00:28:35 +0000 Gerrit-HasComments: Yes
