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

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

Closed
limburgie opened this issue Jul 2, 2024 · 4 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@limburgie
Copy link

limburgie commented Jul 2, 2024

Context

Using Spring Boot 3.3.0 with:

  • spring-boot-starter-oauth2-authorization-server
  • spring-boot-starter-oauth2-client
  • spring-boot-starter-security
  • spring-boot-starter-web
  • spring-security-saml2-service-provider 6.3.0

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 spring-projects/spring-authorization-server#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

@limburgie limburgie added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jul 2, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Jul 18, 2024

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.

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 18, 2024
@limburgie
Copy link
Author

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

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 18, 2024
@limburgie
Copy link
Author

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

@bjpewe
Copy link

bjpewe commented Oct 16, 2024

Any update on this ticket?

@jzheaux jzheaux self-assigned this Oct 25, 2024
@jzheaux jzheaux added type: enhancement A general enhancement in: saml2 An issue in SAML2 modules and removed type: bug A general bug status: feedback-provided Feedback has been provided labels Oct 25, 2024
@jzheaux jzheaux added this to the 6.4.0 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants