Adds cookie based RequestCache #8653
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
eleftherias
added
in: web
There was a problem hiding this comment.
Thanks for the PR @zeeshanadnan!
I have left some feedback inline.
| * | ||
| * @param cookieName is the new name of the cookie. | ||
| */ | ||
| public void setCookieName(String cookieName) { |
There was a problem hiding this comment.
I don't think we need to make the cookie name configurable.
We can always add this functionality later if a user requests it.
Similarly, I don't think we need setCookieMaxAge and setCookiePath for now.
|
|
||
|
|
||
| /** | ||
| * {@code RequestCache} which stores the {@code SavedRequest} in a cookie. |
There was a problem hiding this comment.
We should let users know that this cache will not store the complete request, but just the request URI.
Unlike HttpSessionRequestCache, it does not store headers etc.
| @Override | ||
| public SavedRequest getRequest(HttpServletRequest request, HttpServletResponse response) { | ||
| Cookie savedRequestCookie = WebUtils.getCookie(request, cookieName); | ||
| return savedRequestCookie != null ? new SimpleSavedRequest(savedRequestCookie.getValue()) : null; |
There was a problem hiding this comment.
This should use DefaultSavedRequest instead of SimpleSavedRequest
There was a problem hiding this comment.
@eleftherias i did not find a suitable constructor to do it in an easy way. Also in the getMatchingRequest method its always going to return null. Is it going to make a difference or i am missing something? Any suggestion on a better approach will be very helpful. Thanks.
There was a problem hiding this comment.
@zeeshanadnan It can use the builder,
new DefaultSavedRequest.Builder().setRequestURI(savedRequestCookie.getValue()).build()
web/src/main/java/org/springframework/security/web/savedrequest/CookieRequestCache.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the updates @zeeshanadnan!
I have left some more feedback inline.
| request.setServerName("abc.com"); | ||
| request.setRequestURI("/destination"); | ||
|
|
||
| String encodedRedirectUrl = Base64.getEncoder().encodeToString("http://abc.com/destination".getBytes()); |
There was a problem hiding this comment.
Please use "https://" here, or a relative URL. We avoid using "http://" in our code and use the nohttp library to find occurrences. You can read more about it here https://github.com/spring-io/nohttp.
| public void matchingRequestMethodSetsAnExpiredCookieOnRequestWithSavedRequestCookie() { | ||
| CookieRequestCache cookieRequestCache = new CookieRequestCache(); | ||
| MockHttpServletRequest request = new MockHttpServletRequest(); | ||
| String redirectUrl = "http://abc.com/destination?param1=a¶m2=b¶m3=1122"; |
There was a problem hiding this comment.
Please use "https://" here, or a relative path. We avoid using "http://" in our code and use the nohttp library to find occurrences. You can read more about it here https://github.com/spring-io/nohttp.
| private RequestMatcher requestMatcher = AnyRequestMatcher.INSTANCE; | ||
| protected final Log logger = LogFactory.getLog(this.getClass()); | ||
|
|
||
| private static final String COOKIE_NAME = "saved_request"; |
There was a problem hiding this comment.
In order to be consistent with the server implementation, the cookie should be named "REDIRECT_URI".
| protected final Log logger = LogFactory.getLog(this.getClass()); | ||
|
|
||
| private static final String COOKIE_NAME = "saved_request"; | ||
| private static final String COOKIE_PATH = null; |
There was a problem hiding this comment.
Is there a reason why the path is hardcoded to null?
I would expect it to change based on the request.
There was a problem hiding this comment.
Initially my thought was user should be able to control the cookie path so i included a property and setter method. But yes you are right the default value should be request's context path.
| private static final String DEFAULT_COOKIE_NAME = "saved_request"; | ||
|
|
||
| @Test | ||
| public void originalRequestSetsSavedRequestInACookieOnResponse() { |
There was a problem hiding this comment.
In an effort to have test names be consistent across the codebase, we would like all new tests to follow the naming convention methodWhenConditionThenResult.
For example, this test could be renamed to something like saveRequestWhenMatchesThenSavedRequestInACookieOnResponse.
|
Sorry @eleftherias for the late updates. |
|
Thanks for the PR @zeeshanadnan! |
fixes gh-8034