Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 7: (10 comments) Haven't gone deep into the tests yet. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc File src/kudu/integration-tests/master_sentry_advance-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@37 PS7, Line 37: advanced What is "advanced" referring to? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/authz_provider.cc File src/kudu/master/authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/authz_provider.cc@35 PS7, Line 35: without being authorized. nit: perhaps "without being authorized against fine-grained permissions" or something? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/authz_provider.cc@50 PS7, Line 50: static std::once_flag once; : std::call_once(once, [] { : debug::ScopedLeakCheckDisabler d; : vector<string> acls = strings::Split(FLAGS_trusted_user_acl, ",", strings::SkipEmpty()); : g_trusted_users = new unordered_set<string>(acls.begin(), acls.end()); : }); We only expect there to be on AuthzProvider per master process anyway. Why not put this in the constructor for AuthzProvider and store trusted_users as a private member? Then it's on the stack, and we don't have to worry about this extra leak-check-disabling/once stuff. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.h@730 PS7, Line 730: If 'user' is provided, : // checks that the user is authorized to delete the table. Mind adding a sentence explaining why 'user' might not be provided? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.h@865 PS7, Line 865: authorized to operate on table, nit: "authorized to operate on the table" http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1713 PS7, Line 1713: optional<const string&> user = rpc ? : make_optional<const string&>(rpc->remote_user().username()) : none; The past couple times I've read this, I've been confused why it's possible for IsCreateTableDone to not have an `rpc`. Is it only for tests? If so, I'm not a huge fan of it cluttering production code, and I think the additional lines of defining a dummy RpcContext in test code are worth it to avoid these extra checks on `rpc` and `user`. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1769 PS7, Line 1769: string table_name; : : auto nit: extra line http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1784 PS7, Line 1784: error nit: maybe rename this something more descriptive, like `sanitized_error` or `authorized_error` or something? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1785 PS7, Line 1785: table is authorized to : // avoid leaking whether the table exists nit: I'm having trouble parsing "authorized to avoid leaking", mind rephrasing it? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1786 PS7, Line 1786: empty table name will : // also trigger a NOT_AUTHORIZED error Could we just check for this up front and return Status::InvalidArgument or something? -- 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: 7 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: Sat, 09 Feb 2019 09:23:00 +0000 Gerrit-HasComments: Yes
