Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry ...................................................................... Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; > I'm not sure why privilege should imply authorizable, but action should not I don't feel strongly, but I also don't understand your response. My understanding of the Sentry authorization model is this: - There are several statically defined action types (e.g. ALL, METADATA, READ, CREATE, etc.). There's a mild hierarchy here in that ALL implies a bunch of other actions and METADATA is implied by other actions. - An "action" is entirely derived from an action type. This is obvious so why bother stating it? Because I'm trying to draw a distinction with how authorizables behave; see below. - There are several statically defined authorizable types (e.g. SERVER, DATABASE, TABLE, etc.). There's a hierarchy here as well. - Unlike an action, an "authorizable" is derived from both an authorizable type _and_ a string that names the authorizable. For example, to describe an authorizable with type DATABASE we need a string that both names the database as well as its parent SERVER. - A privilege is comprised of a pair of action and authorizable. - Privileges may be granted to users. Or roles or groups (can't quite remember if this last part is true, but it's not really material to the argument I'm making here). I called out this variable name because I felt that it was conflating the nouns of "privilege" and "action". To me, the fact that it's a set of actions suggests that its name ought to reflect that. But ignoring that for a minute, where did I suggest that a privilege implies an authorizable but not an action? If anything, I said that a privilege implies the _combination_ of an action and an authorizable. So, to reiterate, I don't feel strongly about the name, but I do feel strongly about making sure we're understanding one another. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102 PS11, Line 102: the : // given table. > As far as I see this is enforced in SentryAuthzProvider::GetSentryPrivilege Yeah, as I commented in the other review, I think _encapsulating_ the enforcement into SentryPrivilegesBranch (as a constructor or something) would further clarify the invariant. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431 PS11, Line 431: !boost::iequals(privilege.tableName, requested_authorizable.table))) { > I didn't understand the 'kServer' part of your comment. Ugh, I un-typoed the typo. SentryAuthorizableScope defines 'kSever' (note the missing 'r'). -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Apr 2019 19:20:58 +0000 Gerrit-HasComments: Yes
