Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 13: (12 comments) Just more nits and a couple test suggestions. http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/integration-tests/master_sentry-itest.cc@109 PS13, Line 109: table nit: rename to table_name http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/integration-tests/master_sentry-itest.cc@373 PS13, Line 373: // Listing tables only shows the tables which user has proper privileges on. : ASSERT_OK(GrantGetMetadataTablePrivilege(kDatabaseName, kTableName)); : tables.clear(); : ASSERT_OK(client_->ListTables(&tables)); : ASSERT_EQ(vector<string>({ table_name }), tables); : : ASSERT_OK(GrantGetMetadataTablePrivilege(kDatabaseName, kSecondTable)); : tables.clear(); : ASSERT_OK(client_->ListTables(&tables)); : unordered_set<string> tables_set(tables.begin(), tables.end()); : ASSERT_EQ(unordered_set<string>({ table_name, sec_table_name }), tables_set); Maybe consider adding a case where ALL ON DATABASE is granted? Or is that covered somewhere? http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.h@853 PS13, Line 853: Sentry uses table name to identify the protected resource nit: Maybe make this agnostic to Sentry? e.g. "We expect that our external authorization metadata store uses table name to identify the protected resource." http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1333 PS13, Line 1333: name != none nit: should be able to just do: if (name) { if you want. http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1410 PS13, Line 1410: string user = rpc->remote_user().username(); : string owner nit: cref these? http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1763 PS13, Line 1763: and nit: add a comma "Looking up, locking, and authorizing" http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1790 PS13, Line 1790: "the table does not exist", SecureShortDebugString(table_identifier)), nit: spacing http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1845 PS13, Line 1845: Substitute("the table ID refers to a different table '$0' than '$1'", : table_name, table_identifier.table_name()), : SecureShortDebugString(table_identifier)), nit: spacing http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@2425 PS13, Line 2425: string nit: cref http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@2757 PS13, Line 2757: string nit: cref http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@4663 PS13, Line 4663: // It's Ok to return NOT_FOUND error back to the client, even when authorization : // is enabled. Because tablet id is a randomly generated string which doesn't : // carry any user facing information. nit reword: "It's OK to return NOT_FOUND back to the client, even with authorization enabled, because tablet IDs are randomly generated and don't carry user data." http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/sentry_authz_provider-test.cc@67 PS13, Line 67: ASSERT_TRUE(authz_provider.IsTrustedUser("impala")); : ASSERT_TRUE(authz_provider.IsTrustedUser("hive")); : ASSERT_TRUE(authz_provider.IsTrustedUser("hdfs")); Do we have tests that trusted users can actually sidestep authorization? -- 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: 13 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: Fri, 22 Mar 2019 03:01:43 +0000 Gerrit-HasComments: Yes
