Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 6: (15 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 TSentryGrantOption::type& grant_option) { May want to consider defaulting to ::DISABLED here (and in the other get privilege methods) since that's the case so often. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@363 PS6, Line 363: TEST_P(TestAuthzHierarchy, TestAuthorizableScope) { Do you want this coverage for the "need grant option" case too? http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@378 PS6, Line 378: case SentryAuthorizableScope::Scope::TABLE: Shouldn't there be an entry for COLUMN? Oh, we can't do GetAuthorizable() for COLUMN. So why is COLUMN in the list of tested authorizable scopes at all? http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@397 PS6, Line 397: break; Shouldn't this be FALLTHROUGH_INTENDED? http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@412 PS6, Line 412: false)); This is the default value. On L423 too. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@413 PS6, Line 413: ASSERT_OK(DropRole(kRoleName)); If you used slightly different role names (and user names I guess), you wouldn't need to drop roles and the test may be a bit faster (by eliminating a few RPCs). Maybe not worth it though. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider-test.cc@430 PS6, Line 430: ::testing::Values(true, : false) Use ::testing::Bool() here. 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: const std::string& table_ident, AFAICT this is just a table name; could we name it table_name to be less confusing? http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92 PS6, Line 92: bool grant_option = false); This might be clearer as "require_grant_option" or "need_grant_option". 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@282 PS6, Line 282: Semantics Nit: semantics http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@283 PS6, Line 283: is Nit: are http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@314 PS6, Line 314: WARN_NOT_OK(s, Substitute("failed to construct sentry action from $0", : privilege.action)); If we just WARN here (and below), won't the Implies() calls crash? Seems like we should WARN and either fail to authorize, or WARN and ignore the privilege. Oh, I see there's an s.ok() check on L324. That'll ensure that SentryAuthorizableScope::FromString passed, but not SentryAction::FromString, whose status was since overwritten. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.cc@334 PS6, Line 334: LOG(ERROR) << "Action <" << action << "> on authorizable <" : << authorizable << "> with authorizable scope <" : << scope << "> is not permitted for user <" : << user << ">"; Nit: would be clearer using strings::Substitute. 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 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 the LOG(FATAL) and the return of nullptr here. -- 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: 6 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: Fri, 08 Mar 2019 00:49:07 +0000 Gerrit-HasComments: Yes
