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 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@9 PS4, Line 9: base > based Done 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 anothe Done http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@16 PS4, Line 16: 3. grant option from the former implies the grant option from the latter. > What are these, and how do they fit into the overall Sentry authorization m The design doc briefly touched the grant option when talking about the required privilege for 'CreateTable'. In short, grant option is allowing a user to grant the privilege to others. http://gerrit.cloudera.org:8080/#/c/12500/4//COMMIT_MSG@40 PS4, Line 40: filering > filtering Done 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 Done 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? Done 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? Done http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/3/src/kudu/master/sentry_authz_provider.cc@284 PS3, Line 284: list > nit: lists Done 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 ad Done 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? Done 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 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: 5 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: Thu, 07 Mar 2019 10:03:38 +0000 Gerrit-HasComments: Yes
