Describe the bug

Adding the OAuth2ClientJackson2Module to an ObjectMapper and attempting to roundtrip (serialise to JSON string then parse) a ClientRegistration fails with:

java.lang.IllegalArgumentException: The class with java.util.Collections$UnmodifiableSet and name of java.util.Collections$UnmodifiableSet is not in the allowlist. If you believe this class is safe to deserialize, please provide an explicit mapping using Jackson annotations or by providing a Mixin. If the serialization is only done by a trusted source, you can also enable default typing. See https://github.com/spring-projects/spring-security/issues/4370 for details
    at org.springframework.security.jackson2.SecurityJackson2Modules$AllowlistTypeIdResolver.typeFromId(SecurityJackson2Modules.java:265)
    at com.fasterxml.jackson.databind.jsontype.impl.TypeDeserializerBase._findDeserializer(TypeDeserializerBase.java:159)
    at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer._deserialize(AsArrayTypeDeserializer.java:97)
    at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer.deserializeTypedFromArray(AsArrayTypeDeserializer.java:53)
    at com.fasterxml.jackson.databind.deser.std.StringCollectionDeserializer.deserializeWithType(StringCollectionDeserializer.java:266)
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
    at com.fasterxml.jackson.databind.ObjectMapper._convert(ObjectMapper.java:4388)
    at com.fasterxml.jackson.databind.ObjectMapper.convertValue(ObjectMapper.java:4334)
    at org.springframework.security.oauth2.client.jackson2.JsonNodeUtils.findValue(JsonNodeUtils.java:54)
    at org.springframework.security.oauth2.client.jackson2.ClientRegistrationDeserializer.deserialize(ClientRegistrationDeserializer.java:64)
    at org.springframework.security.oauth2.client.jackson2.ClientRegistrationDeserializer.deserialize(ClientRegistrationDeserializer.java:41)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer._deserializeTypedForId(AsPropertyTypeDeserializer.java:144)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromObject(AsPropertyTypeDeserializer.java:110)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromAny(AsPropertyTypeDeserializer.java:213)
    at com.fasterxml.jackson.databind.JsonDeserializer.deserializeWithType(JsonDeserializer.java:152)
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
    at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4674)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3629)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3597)

Linking to #4370 as the error message does.

To Reproduce

Run sample test code with Spring Security 5.7.3.

Expected behavior

I'm not sure if the class is intended to work on it's own, or if it should always be used in conjunction with other Spring Security modules that are loaded with SecurityJackson2Modules.getModules(getClass().getClassLoader()). If it isn't supposed to work on it's own then having this in the JavaDoc would be helpful.

Sample

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.security.jackson2.SecurityJackson2Modules;
import org.springframework.security.oauth2.client.jackson2.OAuth2ClientJackson2Module;
import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;

public class TestOAuth2Jackson {


    private ClientRegistration clientRegistration;

    @BeforeEach
    void setUp() {
        clientRegistration = ClientRegistration
                .withRegistrationId("test")
                .clientId("clientId")
                .clientSecret("none")
                .clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC)
                .authorizationGrantType(AuthorizationGrantType.IMPLICIT)
                .redirectUri("{baseUrl}/lti/login")
                .scope("openid")
                .authorizationUri("https://server.test/authorization")
                .clientName("Test Client")
                .build();
    }

    @Test
    public void roundTripWithModule() throws JsonProcessingException {
        ObjectMapper mapper = new ObjectMapper();
        mapper.registerModules(new OAuth2ClientJackson2Module());
        final String x = mapper.writeValueAsString(clientRegistration);
        // This fails (and I wasn't expecting it to)
        mapper.readValue(x, ClientRegistration.class);
    }

    @Test
    public void roundTripWithAllModules() throws JsonProcessingException {
        ObjectMapper mapper = new ObjectMapper();
        mapper.registerModules(SecurityJackson2Modules.getModules(getClass().getClassLoader()));
        final String x = mapper.writeValueAsString(clientRegistration);
        // This works.
        mapper.readValue(x, ClientRegistration.class);
    }
}

Comment From: buckett

Not directly related to the bug, but in my Spring Boot project I register the modules with (it seems to work well):

    @Bean
    public Jackson2ObjectMapperBuilderCustomizer jsonCustomizer() {
        return builder -> builder.modulesToInstall(
                SecurityJackson2Modules.getModules(getClass().getClassLoader())
                        .toArray(new Module[0])
        );
    }

Comment From: sjohnr

Thanks for reaching out, @buckett!

I'm not sure if the class is intended to work on it's own, or if it should always be used in conjunction with other Spring Security modules that are loaded with SecurityJackson2Modules.getModules(getClass().getClassLoader()).

It feels like this is a question that would be better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it).

If it isn't supposed to work on it's own then having this in the JavaDoc would be helpful.

Enhancing the documentation is always welcome! If you'd like to submit a PR, I'd be happy to review it.

Since your provided sample does not register a mixin for UnmodifiableSet, the error message points to the fact that you have a mis-configuration. I'm going to close this issue for now, but feel free to add a comment if you still feel like this is a bug, and we can re-open if needed.

Comment From: buckett

@sjohnr I was really asking "Should OAuth2ClientJackson2Module be self contained and useable on its own?"

  • Yes: it should be registering a mixin for UnmodifiableSet and possibly other classes so that it works.
  • No: improve the documentation to point out that it should be registered with the other security modules.

Both of these would ideally result in a code change which is why I'd started the discussion here as I'd normally create before coming in with a PR.

Comment From: sjohnr

Hi @buckett!

I was really asking "Should OAuth2ClientJackson2Module be self contained and useable on its own?"

As mentioned above, I believe the error message points to a mis-configuration and answers this question.

improve the documentation to point out that it should be registered with the other security modules.

Thanks for clarifying. It's been a while since I reviewed the javadoc on these classes, so it's good to get feedback that it would benefit from an enhancement. I'm happy to see an issue for improving the docs, or in this case I would also be happy to review a PR without an accompanying issue (since we can link to this discussion as the driver for the PR).

Given that the issue you reported indicates a bug, and I don't believe this to be a bug, I'll leave this issue closed as-is. If you aren't able to submit a PR, would you mind opening a new issue for the documentation enhancement?