Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11750 )
Change subject: WIP KUDU-2542: add initial authorization token impl ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc File src/kudu/security/token-test.cc: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token-test.cc@358 PS1, Line 358: TEST_F(TokenTest, TestGenerateAuthzToken) { Add a test case covering different validity periods for authn and authz, basically to check that the max() call is working. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto File src/kudu/security/token.proto: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@37 PS1, Line 37: optional bool all_privileges = 2; I'm weakly in favor of not having an 'all_privileges' flag, and instead translating all into setting the scan/insert/update/delete privileges flags explicitly. My reasoning is primarily that it removes a special case from every place that has to check permissions. We're fundamentally going to have more places in the code which check privileges than which translate Sentry privileges into a scan token, so I think this will pay off. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h File src/kudu/security/token_signer.h: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@31 PS1, Line 31: namespace boost { > instead use: although this looks like it may not even be needed; I don't see optional used anywhere below. http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc File src/kudu/security/token_signer.cc: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.cc@69 PS1, Line 69: {authn_token_validity_seconds_, authz_token_validity_seconds} The braces shouldn't be necessary here. -- To view, visit http://gerrit.cloudera.org:8080/11750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id28747ec38675abdf50dce1e7c176d29213e370f Gerrit-Change-Number: 11750 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <aw...@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: Mon, 22 Oct 2018 23:32:34 +0000 Gerrit-HasComments: Yes
