New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KNOX-2155 - KnoxSSO should handle multiple cookies with the same name #227
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few initial comments. I need to add some simple tests for CookieUtils
and there is one area I don't understand for backwards compat.
/** | ||
* Encapsulate the acquisition of the JWT token from HTTP cookies within the | ||
* request. | ||
* | ||
* @param req servlet request to get the JWT token from | ||
* @return serialized JWT token | ||
*/ | ||
@Override | ||
protected String getJWTFromCookie(HttpServletRequest req) { | ||
return super.getJWTFromCookie(req); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this break backwards compat if someone upgrades? I don't quite understand how these adapter classes work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming no since there is no reason this method gets called anymore since its deleted everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not break backwards compatibility, looks good.
gateway-util-common/src/main/java/org/apache/knox/gateway/util/CookieUtils.java
Show resolved
Hide resolved
This commit moves getting cookies by name to a new utility class. It forces callers to look through multiple cookies returned and handle that case. Signed-off-by: Kevin Risden <krisden@apache.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good !
Merged in 1b01961 |
This commit moves getting cookies by name to a
new utility class. It forces callers to look
through multiple cookies returned and handle
that case.