-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add Jackson 3 support and deprecate Jackson 2 one #17832
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
base: main
Are you sure you want to change the base?
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.
Thanks for the pull request. Generally, I think this is looking good. I've provided some feedback inline.
It also appears that there are still some classes that need migrated. Searching for jackson on main should give you a complete list of classes that are using jackson.
For example, webauthn has jackson support. I'm guessing this was missed because it is inconsistent with the rest of the jackson support in that it is in a jackson package rather than jackson2. It is also automatically registered because it does not use default types which would make it insecure.
There are also some tests that still use ObjectMapper
or deprecated Spring Framework Jackson 2 classes (e.g. JwtDecodersTests.java#L388). Unless the test is specifically for the Jackson 2 support, we should update these to use non-deprecated classes (e.g. JsonMapper
).
There are various Spring framework jackson based classes that have been deprecated that we should migrate away from. For example, MappingJackson2HttpMessageConverter
usage (e.g. WebAuthnAuthenticationFilter.converter) should be migrated to JacksonJsonHttpMessageConverter
if jackson 3 is on the classpath. Likely there could be a static factory method used that Spring Security uses to obtain the correct default json converter instance.
cas/src/main/java/org/springframework/security/cas/jackson/CasJacksonModule.java
Show resolved
Hide resolved
...in/java/org/springframework/security/jackson/AbstractUnmodifiableCollectionDeserializer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/springframework/security/jackson/AllowlistTypeResolverBuilder.java
Show resolved
Hide resolved
* @author Rob Winch | ||
* @since 7.0 | ||
*/ | ||
public class AllowlistTypeResolverBuilder extends DefaultTypeResolverBuilder { |
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 think that now is a good time for Spring Security to stop using global default typing.
While the AllowlistTypeResolverBuilder
worked well as a security patch in the Jackson 2 support, I think that we should remove it for Jackson 3 support and better align with best practices with Jackson. Instead, I think that we should use https://github.com/FasterXML/jackson-docs/wiki/JacksonPolymorphicDeserialization#12-per-class-annotations This would mean that users would need to take the risk of enabling global default typing or opt into default typing per type.
I'd like to ensure that we have a test that verifies that a custom type does not just work after applying Spring Security's module. Another test would verify that users could add their own type. The docs would be updated to explain that global default typing is off, why it is off, and how they can add their own types.
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.
From your comment, looks like Spring Security Jackson 3 modules would stop configuring AllowlistTypeResolverBuilder
. But would we keep a Jackson 3 AllowlistTypeResolverBuilder
or not (I think not but please confirm)?
Do you expect Jackson 3 mixins provided by Spring Security to use per class annotations instead?
This would mean that users would need to take the risk of enabling global default typing or opt into default typing per type.
I don't understand this sentence, could you please elaborate?
The docs would be updated to explain that global default typing is off, why it is off, and how they can add their own types
A raw draft of what the doc should say would be useful for my own understanding and for this PR, I will likely let you refine post-PR.
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.
From your comment, looks like Spring Security Jackson 3 modules would stop configuring AllowlistTypeResolverBuilder. But would we keep a Jackson 3 AllowlistTypeResolverBuilder or not (I think not but please confirm)?
No. We should remove Jackson 3 AllowlistTypeResolverBuilder
so that we don't need to maintain that logic once we remove Jackson 2 support.
Do you expect Jackson 3 mixins provided by Spring Security to use per class annotations instead?
Yes.
I don't understand this sentence, could you please elaborate?
Sorry that this wasn't clear.
Previously, Spring Security's Jackson support forced enabling global default typing which is dangerous due to deserialization gadgets that might be exposed. Spring Security's Jackson 3 support should only enable per class (with classes that are safe). If users want global default typing, they will need to do that explicitly outside of Spring Security's Jackson 3 support.
A raw draft of what the doc should say would be useful for my own understanding and for this PR, I will likely let you refine post-PR.
Expanding this statement, but keeping to a summary the docs would:
- Explain that Spring Security's Jackson 3 support does not enable global default typing because it is insecure.
- If users want to add additional classes, they can use per-class annotations. Alternatively, if users understand the security risks they can enable global default typing themselves.
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.
So I made a try to remove global default typing, that's very involved. Almost all the tests are broken and this require much more refinements and thinking than adding annotation to individual classes as collection deserialization is broken in multiple ways, mixins require significant updates, etc.
Not saying you should not do it, but that is not directly related to the Jackson 2 to 3 migration, I don't have a real added value here, I lack of Spring Security knowledge to decide what are the right choices to do and this PR is already pretty huge.
I think the side effects should be carefully assessed by the Spring Security team, as it would be a major breaking change that would add its side effects to the Jackson 3 migration itself. It should probably have its own dedicated issue and I recommend handling that after merging this PR.
...ce-provider/src/main/java/org/springframework/security/saml2/jackson/Saml2JacksonModule.java
Outdated
Show resolved
Hide resolved
...-provider/src/main/java/org/springframework/security/saml2/jackson2/Saml2Jackson2Module.java
Show resolved
Hide resolved
@rwinch I have pushed additional commits:
I will do another pass with your feedback to my questions in the comments. |
See spring-projectsgh-17832 Signed-off-by: Sébastien Deleuze <[email protected]>
This commit adds support for Jackson 3 which has the following major differences with the Jackson 2 one: - jackson subpackage instead of jackson2 - Jackson type prefix instead of Jackson2 - JsonMapper instead of ObjectMapper - For configuration, JsonMapper.Builder instead of ObjectMapper since the latter is now immutable - Remove custom Jackson 3 support for unmodifiable collections - AllowlistTypeResolverBuilder in new a public type in order to be used easily with the JsonMapper.Builder API Jackson 3 changes compared to Jackson 2 are documented on FasterXML/jackson-future-ideas#72. This commit does not cover webauthn which is a special case (uses jackson sub-package for Jackson 2 support) which will be handled in a distinct commit. See spring-projectsgh-17832 Signed-off-by: Sébastien Deleuze <[email protected]>
This commit does not cover webauthn which is a special case (uses jackson sub-package for Jackson 2 support) which will be handled in a distinct commit. See spring-projectsgh-17832 Signed-off-by: Sébastien Deleuze <[email protected]>
Since this module was already using the jackson sub-package for Jackson 2 support, both Jackson 2 and Jackson 3 support lives in the same subpackage and the former package-private classes has been renamed with a Jackson2 qualifier. Closes spring-projectsgh-17832 Signed-off-by: Sébastien Deleuze <[email protected]>
@rwinch I have removed the custom support for unmodifiable collections (seems to work fine with Jackson 3), reworked the commit for a cleaner git history, and rebased the commits on |
This PR adds support for Jackson 3 which has the following major differences with the Jackson 2 one:
jackson
subpackage instead ofjackson2
Jackson
type prefix instead ofJackson2
JsonMapper
instead ofObjectMapper
JsonMapper.Builder
instead ofObjectMapper
since the latter is now immutableAllowlistTypeResolverBuilder
in new a public type in order to be used easily with theJsonMapper.Builder
APIJackson 3 changes compared to Jackson 2 are documented on FasterXML/jackson-future-ideas#72.
It also deprecates Jackson 2 support for removal.
There are TODO comments in
SamlJacksonModule
andSaml2JacksonModule
to review.