Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: IMPALA-10496: SAML implementation in Impala ...................................................................... Patch Set 24: (10 comments) http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.h File be/src/rpc/authentication.h: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.h@58 PS24, Line 58: AuthProvider* GetExternalHttpAuthProvider(); brief comment http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@125 PS24, Line 125: allow nit: allows http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@573 PS24, Line 573: bool ParseParams(std::map<string, string*>& params_to_check, Brief comment about the parameters here. http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@629 PS24, Line 629: NULL nit: we prefer nullptr, here and elsewhere http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/rpc/authentication.cc@683 PS24, Line 683: TODO probably worth explicitly saying what still needs to be done here http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.h@85 PS24, Line 85: porpuses typo: purposes http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.cpp@228 PS24, Line 228: nit: unnecessary whitespace http://gerrit.cloudera.org:8080/#/c/16833/24/be/src/transport/THttpServer.cpp@252 PS24, Line 252: saml_port_ = -1; : auth_value_ = ""; : cookie_value_ = ""; might be nice to put this logic in some sort of resetConnection() helper function that could be called here, below in the 'if (!authorized && !fallback_to_other_auths)' block and at the very end of headersDone() when we've succeeded. Could help avoid bugs, eg. if I follow correctly we're currently not resetting saml_port to -1 when SAML succeeds. Possibly that doesn't matter, but better to be safe. http://gerrit.cloudera.org:8080/#/c/16833/24/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/16833/24/common/thrift/metrics.json@1315 PS24, Line 1315: webserver This maybe gets a little confusing, since usually when we talk about the "webserver" we're referring to the webui. I think you can just leave this word off and it'll be clear since it says the "HiveServer2 HTTP API", here and below http://gerrit.cloudera.org:8080/#/c/16833/24/common/thrift/metrics.json@1329 PS24, Line 1329: IC typo: space -- To view, visit http://gerrit.cloudera.org:8080/16833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0c026cba1b90e7ff6ec5ae49be78b0d1edd8dfa Gerrit-Change-Number: 16833 Gerrit-PatchSet: 24 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Feb 2021 01:07:36 +0000 Gerrit-HasComments: Yes
