Skip to content
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

Add support for partitioned cookies #42316

Closed
wants to merge 1 commit into from
Closed

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Sep 15, 2024

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 15, 2024
@philwebb
Copy link
Member

Thanks @nosan. I think we should also update AbstractServletWebServerFactory.configureSessionCookie and WebSessionIdResolverAutoConfiguration.initializeCookie as well to support the new property. If you have time to update your PR, that would be great. Otherwise we can take it on when we merge.

@philwebb philwebb added type: enhancement A general enhancement for: merge-with-amendments Needs some changes when we merge and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 15, 2024
@philwebb philwebb added this to the 3.4.x milestone Sep 15, 2024
@nosan nosan force-pushed the gh-42307 branch 2 times, most recently from 3eca7a4 to 8a04295 Compare September 15, 2024 22:01
@nosan
Copy link
Contributor Author

nosan commented Sep 15, 2024

Thanks for the feedback @philwebb.

I did update my PR but unfortunately, I have not found a straightforward way to update AbstractServletWebServerFactory.configureSessionCookie because jakarta.servlet.SessionCookieConfig does not have a method to set the partitioned property.

The only way as I understand is:

	map.from(cookie::getPartitioned)
				.to(partitioned -> config.setAttribute(PARTITIONED_ATTRIBUTE_NAME, Boolean.toString(partitioned)));

https://jakarta.ee/specifications/servlet/6.0/apidocs/jakarta.servlet/jakarta/servlet/sessioncookieconfig#setAttribute(java.lang.String,java.lang.String)

@bclozel
Copy link
Member

bclozel commented Sep 16, 2024

I think this is the only way to support partitioned cookies, see Tomcat for reference:

https://github.com/apache/tomcat/blob/d6e5a088433af43a35293f9a2b19cff359efb79b/java/org/apache/catalina/core/ApplicationSessionCookieConfig.java#L224

@nosan
Copy link
Contributor Author

nosan commented Sep 16, 2024

@bclozel
Copy link
Member

bclozel commented Sep 16, 2024

@nosan you can raise this as a Tomcat issue if you think this is a bug.

@nosan
Copy link
Contributor Author

nosan commented Sep 16, 2024

@bclozel
For me, it does not sound like a bug.
As I understood you can set a value for partitioned through Context.setUsePartitioned and then if you specify
sessionCookieConfig.setAttribute(PARTITIONED_ATTRIBUTE_NAME, Boolean.toString(partitioned) this value will be overridden.

// NOTE: The priority order for session cookie configuration is:
// 1. Context level configuration
// 2. Values from SessionCookieConfig
// 3. Defaults

so, this line which I added in this PR should work:

	map.from(cookie::getPartitioned)
   			.to(partitioned -> config.setAttribute(PARTITIONED_ATTRIBUTE_NAME, Boolean.toString(partitioned)));

@bclozel
Copy link
Member

bclozel commented Sep 16, 2024

That was as well my assessment in the first place.

@mhalbritter mhalbritter self-assigned this Sep 20, 2024
mhalbritter pushed a commit that referenced this pull request Sep 25, 2024
mhalbritter added a commit that referenced this pull request Sep 25, 2024
@mhalbritter mhalbritter removed the for: merge-with-amendments Needs some changes when we merge label Sep 25, 2024
@mhalbritter
Copy link
Contributor

Thanks @nosan !

@mhalbritter mhalbritter modified the milestones: 3.4.x, 3.4.0-RC1 Sep 25, 2024
@nosan nosan deleted the gh-42307 branch September 25, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants