brumi1024 commented on code in PR #8273:
URL: https://github.com/apache/hadoop/pull/8273#discussion_r2852879109
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/WebAppProxyServlet.java:
##########
@@ -311,11 +321,19 @@ private void proxyLink(final HttpServletRequest req,
}
}
+ Map<String, String> cookies = new HashMap<>();
String user = req.getRemoteUser();
- if (user != null && !user.isEmpty()) {
- base.setHeader("Cookie",
- PROXY_USER_COOKIE_NAME + "=" + URLEncoder.encode(user, "ASCII"));
+ if (StringUtils.hasLength(user)) {
+ LOG.debug("Cookie {} will be set", PROXY_USER_COOKIE_NAME);
+ cookies.put(PROXY_USER_COOKIE_NAME, URLEncoder.encode(user, "ASCII"));
+ }
+ String jwtCookie = getCookie(req, jwtCookieName);
+ if (StringUtils.hasLength(jwtCookie)) {
+ LOG.debug("Cookie {} will be set", jwtCookieName);
+ cookies.put(jwtCookieName, jwtCookie);
}
+ setCookies(base, cookies);
Review Comment:
Small regression: previously we only set the cookies if the user wasn't
empty or null. Now if the cookies map is an empty HashMap we simply write an
empty string to the Cookies header. Probably a similar empty check should be
called here as well.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/WebAppProxyServlet.java:
##########
@@ -311,11 +321,19 @@ private void proxyLink(final HttpServletRequest req,
}
}
+ Map<String, String> cookies = new HashMap<>();
String user = req.getRemoteUser();
- if (user != null && !user.isEmpty()) {
- base.setHeader("Cookie",
- PROXY_USER_COOKIE_NAME + "=" + URLEncoder.encode(user, "ASCII"));
+ if (StringUtils.hasLength(user)) {
+ LOG.debug("Cookie {} will be set", PROXY_USER_COOKIE_NAME);
+ cookies.put(PROXY_USER_COOKIE_NAME, URLEncoder.encode(user, "ASCII"));
Review Comment:
Nit: StandardCharsets.US_ASCII could be used instead of the literal.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/ProxyUtils.java:
##########
@@ -122,4 +127,40 @@ public static void rejectNonHttpRequests(ServletRequest
req) throws
throw new ServletException(E_HTTP_HTTPS_ONLY);
}
}
+
+ /**
+ * Returns the value of a cookie with the given name from the HTTP servlet
request.
+ * Cookie name comparison is case-insensitive. If the request contains no
cookies
+ * or the specified cookie is not present, this method returns {@code null}.
+ *
+ * @param req the HTTP servlet request containing cookies
+ * @param cookieName the name of the cookie to retrieve
+ * @return the cookie value, or {@code null} if not found
+ */
+ public static String getCookie(HttpServletRequest req, String cookieName) {
+ Cookie[] cookies = req.getCookies();
+ if (cookies != null) {
+ for (Cookie cookie : cookies) {
+ if (cookieName.equalsIgnoreCase(cookie.getName())) {
Review Comment:
Nit: why equalsIgnoreCase? Cookie names are case-sensitive, and
JWTRedirectAuthenticationHandler.getJWTFromCookie uses
`cookieName.equals(cookie.getName()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
