spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.71k stars 5.85k forks source link

OIDC logout not working for JPA/JDBC OAuth2AuthorizationService because DefaultSaml2AuthenticatedPrincipal does not implement equality #15346

Open limburgie opened 2 months ago

limburgie commented 2 months ago

Context

Using Spring Boot 3.3.0 with:

Bug description

We have set up Spring Authorization Server with an implementation of OAuth2AuthorizationService that persists authorizations in the database via JPA (following https://docs.spring.io/spring-authorization-server/reference/guides/how-to-jpa.html). Also, our Authorization Server delegates authentication to a SAML IdP.

The flow works correctly, but signing out fails complaining the "sid" claim in the OIDC token is missing due to SessionInformation not found at token generation time. See https://github.com/spring-projects/spring-authorization-server/issues/1361 for a similar description.

When the JPA based OAuth2AuthorizationService is replaced by InMemoryOAuth2AuthorizationService, the whole flow works as expected.

Debugging shows that the problem is in the SessionRegistryImpl, which keeps a Map<DefaultSaml2AuthenticatedPrincipal, Set<String>> in this scenario. When building the token in OAuth2AuthorizationCodeAuthenticationProvider:249 Spring tries to get session information for the principal, so it does a get() on the map (SessionRegistryImpl:78).

The problem here is that DefaultSaml2AuthenticatedPrincipal was serialized and deserialized because of the JPA implementation of the OAuth2AuthorizationService (as an attribute of OAuth2Authorization). After deserialization, it is a new instance and because DefaultSaml2AuthenticatedPrincipal does not implement an equals/hashCode method, the map retrieval always fails, so the linked Session ID is never found.

Solution

The solution would be to implement an equals/hashCode for DefaultSaml2AuthenticatedPrincipal so the Session ID can be found, the "sid" claim in the OIDC token can be filled with the Session ID and logout will work correctly.

Workaround

As a (dirty) workaround for this issue we have implemented a custom implementation for SessionRegistry that searches the right principal in the map without relying on object identity.

public class CustomSessionRegistry implements SessionRegistry, ApplicationListener<AbstractSessionEvent> {
    ...
    @Override
    public List<SessionInformation> getAllSessions(Object principal, boolean includeExpiredSessions) {
        AuthenticatedPrincipal authPrincipal = (AuthenticatedPrincipal) principal;
        Set<String> sessionsUsedByPrincipal = this.principals.entrySet().stream()
            .filter(p ->((AuthenticatedPrincipal) p.getKey()).getName().equals(authPrincipal.getName()))
            .map(Map.Entry::getValue).findFirst().orElse(null);
        ...
    }
    ....
}

But it goes without saying that the issue should be resolved in DefaultSaml2AuthenticatedPrincipal, similar to e.g. DefaultOAuth2User which is also an AuthenticatedPrincipal implementation.

See also https://stackoverflow.com/questions/77093539/sid-missing-in-id-token-using-spring-authorization-server

jzheaux commented 1 month ago

Hi, @limburgie, can you please confirm whether you mean DefaultOAuth2AuthenticatedPrincipal or DefaultSaml2AuthenticatedPrincipal? It may be that we want to implement equals and hashCode in both, I just want to make sure we attribute the right thing to this ticket.

limburgie commented 1 month ago

Hi @jzheaux, thanks for your response. Our issue would be fixed by implementing it in DefaultSaml2AuthenticatedPrincipal.

limburgie commented 1 week ago

@jzheaux any update on this ticket? Can this issue be considered for a future minor?