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 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@186 PS7, Line 186: authorizable->__set_db(database.ToString()); What if 'database' is empty at this point? If that should not happen, add corresponding DCHECK here. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@192 PS7, Line 192: LOG(FATAL) << "unsupported SentryAuthorizableScope"; nit: consider outputting the 'scope' parameter here as well -- might be useful for troubleshooting if things go awry and this FATAL is hit. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@211 PS7, Line 211: user == owner Might be not the part of this changelist, but just to clarify. Are usernames case-sensitive in all this API? http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h File src/kudu/sentry/sentry-test-base.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h@82 PS7, Line 82: virtual bool KerberosEnabled() = 0; nit: add 'const' ? http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc File src/kudu/sentry/sentry_action.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc@20 PS7, Line 20: #include <stdint.h> nit: use <cstdint> instead http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc File src/kudu/sentry/sentry_authorizable_scope-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@61 PS7, Line 61: SentryAuthorizableScope scope; : for (const auto& valid_scope : valid_scopes) { : ASSERT_OK(SentryAuthorizableScope::FromString(valid_scope, &scope)); Does it make sense to check for the result scope? From the black-box testing perspective nothing guarantees that "column", being a valid scope, might map into 'Scope::SERVER'. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@66 PS7, Line 66: throws returns http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@68 PS7, Line 68: "UNINITIALIZED" maybe, add 'tablecolumn' into the list of invalid scopes to verify SentryAuthorizableScope::FromString() behavior. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@49 PS7, Line 49: SentryAuthorizableScope(); Could you add a comment justifying the existence of this constructor? Ideally, only the SentryAuthorizableScope(Scope scope) should be there since and it's not obvious why UNINITIALIZED even exists. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc File src/kudu/sentry/sentry_authorizable_scope.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@20 PS7, Line 20: #include <stdint.h> nit: use #include <cstdint> instead Yes, IWYU is confusing with its suggestions and hopefully we can fix that with the newer version of the IWYU tool. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@39 PS7, Line 39: "SERVER" a tiny nit for this and other string literals that are shared between this function and SentryAuthorizableScope::FromString() as well: consider declaring 'static constexpr char const kServer[] = "SERVER";' and using those constants instead. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@96 PS7, Line 96: } nit: I guess gcc would report a warning here because the function returns nothing reaching the end of the scope. Consider adding 'return false;' as the last line of the SentryAuthorizableScope::Implies() method. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc@56 PS7, Line 56: bool KerberosEnabled() { nit: add 'const' ? http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h File src/kudu/sentry/sentry_client.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h@99 PS7, Line 99: // If the user is granted both 'ALL on SERVER server1' and 'SELECT on TABLE db1.a' Does that mean that if there is a request for a particular column C in a table and 1) the user is granted 'GRANT ALL AT SERVER "S" TO USER "U"' 2) for some reason, somebody explicitly specified 'GRANT ALL TO COLUMN "C" OF TABLE "T"' at server "S" then the response will always contain the _whole_ contents of the Sentry's DB up to the very top level (i.e. server "S")? -- 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: 7 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: Tue, 12 Mar 2019 19:28:32 +0000 Gerrit-HasComments: Yes
