CookieClearingLogoutHandler for different Paths

[j0hnc0yne]

Summary

I would like to be able to utilize the CookieClearingLogoutHandler to remove cookies without a path set. Example is we are using SSO to authenticate with internal OpenId connect server. Cookies are set without a path by the server after user successfully authenticates. Our application runs on a different path. When user logs out of our app, the cookie is still valid for and therefore if the URL is loaded in same session, when our app redirects to the OpenId connect server will validate the user is still active and redirect back to our app, meaning user does not have to log in

Actual Behavior

Cookies are cleared on the Context Path only.

Expected Behavior

Enable the capability to clear cookies with no path set

Configuration

n/a

Version

5.1.1.RELEASE

Sample

Could we overload the constructor to do something like this?

public final class CookieClearingLogoutHandler implements LogoutHandler {
	private final List<String> cookiesToClear;
        private final boolean usePath = true;

	public CookieClearingLogoutHandler(String... cookiesToClear) {
		Assert.notNull(cookiesToClear, "List of cookies cannot be null");
		this.cookiesToClear = Arrays.asList(cookiesToClear);
	}

	public CookieClearingLogoutHandler(boolean usePath, String... cookiesToClear) {
		Assert.notNull(cookiesToClear, "List of cookies cannot be null");
		this.cookiesToClear = Arrays.asList(cookiesToClear);
                this.usePath = usePath;
	}

	public void logout(HttpServletRequest request, HttpServletResponse response,
			Authentication authentication) {
		for (String cookieName : cookiesToClear) {
			Cookie cookie = new Cookie(cookieName, null);
                        if(usePath){
			     String cookiePath = request.getContextPath() + "/";
			     cookie.setPath(cookiePath);
                        }
			cookie.setMaxAge(0);
			response.addCookie(cookie);
		}
	}
}

Let me know if there are any thoughts and if this functionality would make sense, I could put up a PR

[rwinch]

@maxcoinage Thanks for the suggestion. I think the goal makes sense, but I would change the implementation a bit.

Rather than setting usePath is not as flexible as we would like. The reason for that is that a request to /foo can set a cookie for the path /foo/bar. Another thing to consider is that there are other properties that impact how it is deleted. For example, it might be on a sub domain. Finally, each cookie may have a different path or a different domain setting.

For these reasons, perhaps it is better to allow injecting a collection of Cookie instances. We would want to validate that the Cookie isntances injected all had a maxAge that was set to 0. The existing constructor would create the Cookie's that we would add to the response so that the logic is always consistently adding the specified cookies to the response.

How does that sound?

If you would be willing to submit a PR that would be great! I'd be willing to help you through the process.

[j0hnc0yne]

Hi @rwinch - that suggestion makes much more sense, thanks for considering my request. I would be willing to add the code and test cases required and open a PR. This would be my first contribution to an open source repo, so would appreciate some guidance.

Should I base from the 5.1.x branch?

[rwinch]

@maxcoinage

I would be willing to add the code and test cases required and open a PR.

That is great news! Thank you for volunteering your time to contribute to Spring Security!

This would be my first contribution to an open source repo, so would appreciate some guidance.

That is great! No problem. If you need anything just ping me in the comments here or on the PR you create.

Should I base from the 5.1.x branch?

Please base it off of master. Any features that need backported the security team will take care of. Since this is considered a new feature and not a bug, it is very unlikely to be backported.

Again, if you need any help please don't hesitate to reach out. If you care to here are the Contributor Guidelines. Please don't stress about getting it right the first time as we can work together to get to the final goal of merging the PR. Thanks again for volunteering to help out!

[rwinch]

Fixed via: #6116