Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 2: (6 comments) Took a quick look at this. I'm not sure I understand the essence of the Sentry scoping, it seems I need to revisit this once more. http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12 PS2, Line 12: a privilege implies another when, : 1. the authorizable from the former implies the authorizable from the : latter, : 2. the action from the former implies the action from the latter, : 3. and grant option from the former implies the grant option from the : latter. Is there a document about that? A link to Sentry's docs would be helpful. http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@33 PS2, Line 33: sentry API will return anything that matches > Understanding this requires an understanding of the Sentry API, what it ret +1 So, in this case the list of returned privileges will contain all ALTER, DROP and RENAME even if we are not interested in that? http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39 PS2, Line 39: This patch adds privilge scope validation to SentryAuthzProvider to : ensure only authorizable with a higher privilege scope on the hierarchy : can imply authorizables with a lower scope on the hierarchy. What would be the problem if not having this patch? Could you elaborate on details? An example would be great. http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc@425 PS2, Line 425: AuthzOptions { : true, : SentryPrivilegeScope::Scope::TABLE, : }, : AuthzOptions { : false, : SentryPrivilegeScope::Scope::TABLE, : }, : AuthzOptions { : true, : SentryPrivilegeScope::Scope::DATABASE, : }, : AuthzOptions { : false, : SentryPrivilegeScope::Scope::DATABASE, : }, : AuthzOptions { : true, : SentryPrivilegeScope::Scope::SERVER, : }, : AuthzOptions { : false, : SentryPrivilegeScope::Scope::SERVER, : } Did you want just the Cartesian product? If so, maybe use separate parameters and use ::testing::Combine(); there is an example in https://github.com/apache/kudu/blob/2be008763cda5eb28c0dfa8863e396575fce91ab/src/kudu/tools/kudu-admin-test.cc#L406 http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h File src/kudu/sentry/sentry_privilege_scope.h: http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h@58 PS2, Line 58: imply implies http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc File src/kudu/sentry/sentry_privilege_scope.cc: http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc@42 PS2, Line 42: return "<cannot reach here>"; maybe: LOG(FATAL) << static_cast<uint16_t>(scope) << ": unknown scope"; return nullptr; -- 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: 2 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: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 18:24:09 +0000 Gerrit-HasComments: Yes
