Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider ...................................................................... Patch Set 7: (7 comments) Mostly just a few nits at this point. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270 PS5, Line 270: > I thought wrapped function has 4 space indent? That's what we do for wrapping long lines. >From the GSG >(https://google.github.io/styleguide/cppguide.html#Formatting_Lambda_Expressions): "Format parameters and bodies as for any other function" http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@55 PS7, Line 55: This following methods will fail if the operation is not authorized, or the : // Sentry service is unreachable, or 'kudu' (Kudu server name) is a non-admin : // user in Sentry, or Sentry fails to resolve the group mapping of : // the user. How about we split these out so it's a bit easier to read. The following authorizing methods will fail if: - the operation is not authorized - the Sentry service is unreachable - the specified '--kudu_service_name' is a non-admin user in Sentry - Sentry fails to resolve the group mapping of the user http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@90 PS7, Line 90: // : // This method will fail if the Sentry service is unreachable, or 'kudu' : // (Kudu server name) is a non admin user in Sentry, or Sentry fail to : // resolve the group mapping of the user. nit: not needed given the comment up top? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259 PS5, Line 259: return Status::OK(); : } : > For example, if user Bob has ALL privilege on table B and he wants to check Ah, thanks for the explanation. So based on where the Authorize() fails, a malicious user might be able to discover something they shouldn't? http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@115 PS7, Line 115: on super nit: elsewhere we seem to favor capitalizing the whole of "ALL ON DATABASE" http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@209 PS7, Line 209: nit: spacing here too http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@235 PS7, Line 235: Status SentryAuthzProvider::GetAuthorizable(const string& table_name, I don't think this needs to be a part of the SentryAuthzProvider class. Maybe stick it in an anonymous namespace? -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <hao....@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: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 06:04:03 +0000 Gerrit-HasComments: Yes
