Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/23237 )
Change subject: IMPALA-14285: Add SAML2 authentication support for Impala Web UI ...................................................................... Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/23237/9/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/23237/9/be/src/transport/THttpServer.h@231 PS9, Line 231: he/she expects to receive the bearer token. nit: "where bearing token is expected"? http://gerrit.cloudera.org:8080/#/c/23237/9/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/23237/9/be/src/util/backend-gflag-util.cc@193 PS9, Line 193: ws_saml2_sp_callback_url optional: webserver_saml2_sp_callback_url would be clearer http://gerrit.cloudera.org:8080/#/c/23237/9/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/23237/9/common/thrift/metrics.json@3944 PS9, Line 3944: an invalid SAML2 This sounds unfinished - a similar description could be used as for hs2 saml failures. http://gerrit.cloudera.org:8080/#/c/23237/9/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateStoreBase.java File fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateStoreBase.java: http://gerrit.cloudera.org:8080/#/c/23237/9/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateStoreBase.java@38 PS9, Line 38: public abstract class HiveSamlRelayStateStoreBase<T> implements ValueGenerator { I don't see the original class removed - is that still needed? https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateStore.java http://gerrit.cloudera.org:8080/#/c/23237/9/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientWS.java File fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientWS.java: http://gerrit.cloudera.org:8080/#/c/23237/9/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClientWS.java@80 PS9, Line 80: @Override : public void validateAuthnResponse(WrappedWebContext webContext) : throws InternalException { : String nameId; : String relayState; : String uri; : try { : relayState = HiveSamlRelayStateStoreWS.get().getRelayStateInfo(webContext); : uri = HiveSamlRelayStateStoreWS.get().getRelayStateInfo(relayState).getUri(); : } catch (HttpSamlAuthenticationException e) { : LOG.error("Invalid relay state", e); : webContext.setResponseStatusCode(HttpStatus.SC_UNAUTHORIZED); : return; : } : try { : LOG.debug("RelayState uri is " + uri); : nameId = validateAuthnResponseInner(webContext); : } catch (HttpSamlAuthenticationException e) { : if (e instanceof HttpSamlNoGroupsMatchedException) { : LOG.error("Could not authenticate user since the groups didn't match", e); : } else { : LOG.error("SAML response could not be validated", e); : } : webContext.setResponseStatusCode(401); : webContext.setResponseContent("", : "SAML assertion could not be validated. Check server logs for more details."); : return; : } : Preconditions.checkState(nameId != null); This seems nearly the same as HiveSamlHttpServlet.doPost() HiveSamlHttpServlet was created to keep Hive and Impala code as similar as possible, but this seems less important now as both projects have moved. My idea would be to move most of validateAuthnResponse to ImpalaSamlClientBase and delete HiveSamlHttpServlet. There are tiny differences like uri vs port, but a common relay object could be used instead. http://gerrit.cloudera.org:8080/#/c/23237/9/tests/custom_cluster/test_saml2_sso.py File tests/custom_cluster/test_saml2_sso.py: http://gerrit.cloudera.org:8080/#/c/23237/9/tests/custom_cluster/test_saml2_sso.py@383 PS9, Line 383: Inherits from both TestHS2Saml and TestWebserverSaml to reuse their methods. I find the inheritance with tests quite unusual. Also, I don't see a good reason, though maybe I miss something - can't all _* utility functions go to base and tests only to lead classes? -- To view, visit http://gerrit.cloudera.org:8080/23237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12540300529f9c240abf7196141ecb0ae6e37995 Gerrit-Change-Number: 23237 Gerrit-PatchSet: 9 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 16 Dec 2025 16:10:18 +0000 Gerrit-HasComments: Yes
