Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@10 PS4, Line 10: on the following rules, : : a privilege implies another when: nit: merge the lines as "based on the rules that a privilege implies another when:" http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc@349 PS4, Line 349: TestPrivilegeScope AuthorizableScope http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider-test.cc@381 PS4, Line 381: default Why don't we need to add column privileges for Scope::TABLE? http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.h@85 PS4, Line 85: sentry::SentryAuthorizableScope::Scope scope, : sentry::SentryAction::Action action, : const std::string& table_name, : const std::string& user, : bool grant_option = false); Document the fields. E.g. how is 'scope' used? What if 'scope' isn't TABLE? e.g. COLUMN? How is grant_option used? After looking elsewhere, I think "table_name" might be more descriptive as "table_ident" since that implies it also has to do with the database? http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/master/sentry_authz_provider.cc@175 PS4, Line 175: case SentryAuthorizableScope::Scope::COLUMN: The authorizable doesn't have any table info in it for a column? Can you add a comment explaining why this is a no-op? http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope-test.cc File src/kudu/sentry/sentry_authorizable_scope-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope-test.cc@59 PS4, Line 59: TEST(SentryAuthorizableScopeTest, TestFromString) { : SentryAuthorizableScope column; : ASSERT_OK(SentryAuthorizableScope::FromString("column", &column)); Should this test the other scopes too? http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12500/4/src/kudu/sentry/sentry_authorizable_scope.h@61 PS4, Line 61: a an -- 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: 4 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: Wed, 06 Mar 2019 01:27:04 +0000 Gerrit-HasComments: Yes
