Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 7: (17 comments) http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@137 PS6, Line 137: const string& action, > May want to consider defaulting to ::DISABLED here (and in the other get pr Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@363 PS6, Line 363: Status s = sentry_authz_provider_->AuthorizeCreateTable("DB.table", kTestUser, kTestUser); > Do you want this coverage for the "need grant option" case too? I don't think this test needs to cover grant option as well, which is independently validated from authorizable scope. Though it should happen in the test for table creation with a different owner than the user. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@378 PS6, Line 378: public ::testing::WithParamInterface< > Shouldn't there be an entry for COLUMN? Hmm, right. I was thinking to add more coverage, but on a second thought this is really not add anything. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@397 PS6, Line 397: TSentryPrivilege server_priv = GetServerPrivilege(action); > Shouldn't this be FALLTHROUGH_INTENDED? Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@412 PS6, Line 412: > This is the default value. On L423 too. Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@413 PS6, Line 413: switch (scope) { > If you used slightly different role names (and user names I guess), you wou Yeah, I thought about this, but mini sentry uses the fixed predefined user-group mapping https://github.com/apache/kudu/blob/master/src/kudu/sentry/mini_sentry.cc#L294. So yes, I don't think it worth it. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@430 PS6, Line 430: eAndAddToGroups(kRoleName, kUserGroup)); : ASSERT_OK(AlterRoleGrantPrivilege(kRoleName > Use ::testing::Bool() here. Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@90 PS6, Line 90: sentry::SentryAction::Action action, > AFAICT this is just a table name; could we name it table_name to be less co Yeah, I will add a comment as Andrew suggested. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@90 PS6, Line 90: sentry::SentryAction::Action action, > Ah, I suggested this change since I recall there being a concept of a table Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92 PS6, Line 92: const std::string& user, > This might be clearer as "require_grant_option" or "need_grant_option". Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@178 PS6, Line 178: case SentryAuthorizableScope::Scope::TABLE: : RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, > This is a contract with developers, not clients -- it'd be a bug if we got Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@282 PS6, Line 282: Therefor > Nit: semantics Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@283 PS6, Line 283: in > Nit: are Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@314 PS6, Line 314: privilege.action)); : if (!s.ok()) { > If we just WARN here (and below), won't the Implies() calls crash? Seems li Ah right, thanks for catching that. Added a regression test. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@334 PS6, Line 334: // e.g. 'whether table A exists'. : LOG(ERROR) << Substitute("Action <$0> on table <$1> with authorizable scope " : "<$2> is not permitted for user <$3>", : s > Nit: would be clearer using strings::Substitute. Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.h@57 PS6, Line 57: a > Nit: an Done http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.cc File src/kudu/sentry/sentry_authorizable_scope.cc: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/sentry/sentry_authorizable_scope.cc@36 PS6, Line 36: const char* ScopeToString(SentryAuthorizableScope::Scope scope) { > Nit: could you make this consistent w.r.t. ActionToString? I'm referring to Done -- To view, visit http://gerrit.cloudera.org:8080/12500 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a Gerrit-Change-Number: 12500 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: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 12 Mar 2019 06:37:51 +0000 Gerrit-HasComments: Yes
