BackChannelLogoutController injects ServerOAuth2AuthorizedClientRepository instead of SpringAddonsServerOAuth2AuthorizedClientRepository

[kwonglau]

Is your feature request related to a problem? Please describe.

I would like to store the oauth2_authorized_client using R2dbcReactiveOAuth2AuthorizedClientService offered by Spring.

when customizing ServerOAuth2AuthorizedClientRepository bean, ReactiveSpringAddonsBackChannelLogoutBeans#BackChannelLogoutController component creation is throwing error because it depends on SpringAddonsServerOAuth2AuthorizedClientRepository instead of ServerOAuth2AuthorizedClientRepository

  @Bean
  ReactiveOAuth2AuthorizedClientService authorizedClientService(DatabaseClient databaseClient,
                                                                ReactiveClientRegistrationRepository clientRegistrationRepository) {
    return new R2dbcReactiveOAuth2AuthorizedClientService(databaseClient, clientRegistrationRepository);
  }

  @Bean
  ServerOAuth2AuthorizedClientRepository authorizedClientRepository(ReactiveOAuth2AuthorizedClientService authorizedClientService) {
    return new AuthenticatedPrincipalServerOAuth2AuthorizedClientRepository(authorizedClientService);
  }

Describe the solution you'd like

I would like the spring-addons keeps the current backchannel logout functionality and be able to store the oauth2_authorized_client to the db as well.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

[ch4mpy]

The reason why BackChannelLogoutController is depending explicitly on SpringAddonsServerOAuth2AuthorizedClientRepository is because it calls removeAuthorizedClients(logoutIss, logoutSub) which is not part of ServerOAuth2AuthorizedClientRepository interface (Spring implementations for authorized clients do not support "multi-tenancy": it work only with a single subject for a given user).

All I could do here is adding an interface for this removeAuthorizedClients(logoutIss, logoutSub) so that you could provide with your own implementation (for instance proxy Spring's R2DBC impl).

Meanwhile, if you disable back-channel logout, the problem should be gone.

[ch4mpy]

The main issue here is that in the case of a back-channel logout, the request is initiated by the authorization server and as a consequence is not part of the user session. This means that you will have a null Authentication and have to store somewhere the relation between (issuer, username) and authorizedClientId. SpringAddonsServerOAuth2AuthorizedClientRepository does this working with sessions and static ConcurrentHashMaps.

Here is what I thought of to use database instead:

Declare an interface as follow:

public interface MultiTenantServerOAuth2AuthorizedClientRepository extends ServerOAuth2AuthorizedClientRepository {
    /**
     * Removes an authorized client and returns a list of sessions to invalidate (those for which the current user has no more authorized client after this one
     * was removed)
     *
     * @param  issuer        OP issuer URI
     * @param  principalName current user name for this OP
     * @return               the list of user sessions for which this authorized client was the last one (and should be terminated)
     */
    Flux<WebSession> removeAuthorizedClients(String issuer, String principalName);
}

And change

• SpringAddonsServerOAuth2AuthorizedClientRepository so that it implements MultiTenantServerOAuth2AuthorizedClientRepository instead of ServerOAuth2AuthorizedClientRepository
• BackChannelLogoutController to expect a MultiTenantServerOAuth2AuthorizedClientRepository instead of a SpringAddonsServerOAuth2AuthorizedClientRepository.

Instead of the two beans you expose above, you would provide something like that (does about the same as SpringAddonsServerOAuth2AuthorizedClientRepository using a database instead of the sessions):

@Component
static class MyMultiTenantServerOAuth2AuthorizedClientRepository implements MultiTenantServerOAuth2AuthorizedClientRepository {

    private final ReactiveClientRegistrationRepository clientRegistrationRepository;
    private final DatabaseClient databaseClient;
    private final AuthenticatedPrincipalServerOAuth2AuthorizedClientRepository delegate;

    public MyMultiTenantServerOAuth2AuthorizedClientRepository(
            DatabaseClient databaseClient,
            ReactiveClientRegistrationRepository clientRegistrationRepository) {
        this.clientRegistrationRepository = clientRegistrationRepository;
        this.databaseClient = databaseClient;
        this.delegate = new AuthenticatedPrincipalServerOAuth2AuthorizedClientRepository(new R2dbcReactiveOAuth2AuthorizedClientService(databaseClient, clientRegistrationRepository));
    }

    @Override
    public <
            T extends OAuth2AuthorizedClient>
            Mono<T>
            loadAuthorizedClient(String clientRegistrationId, Authentication principal, ServerWebExchange exchange) {
        return delegate.loadAuthorizedClient(clientRegistrationId, principal, exchange);
    }

    @Override
    public Mono<Void> saveAuthorizedClient(OAuth2AuthorizedClient authorizedClient, Authentication principal, ServerWebExchange exchange) {
        final var issuer = authorizedClient.getClientRegistration().getProviderDetails().getIssuerUri();
        final var principalName = principal.getName();
        databaseClient.sql(...); // Save in DB that this principalName is authorized by this issuer with that authorized client ID
        return delegate.saveAuthorizedClient(authorizedClient, principal, exchange);
    }

    @Override
    public Mono<Void> removeAuthorizedClient(String clientRegistrationId, Authentication principal, ServerWebExchange exchange) {
        return clientRegistrationRepository.findByRegistrationId(clientRegistrationId).flatMap(clientRegistration -> {
            final var issuer = clientRegistration.getProviderDetails().getIssuerUri();
            final var principalName = principal.getName();
            databaseClient.sql(...); // Remove from DB that this principalName is authorized by this issuer
            return delegate.removeAuthorizedClient(clientRegistrationId, principal, exchange);
        });
    }

    @Override
    public Flux<WebSession> removeAuthorizedClients(String issuer, String principalName) {
        // fetch from DB the authorized client ID for this principalName and that issuer
        // and then flatMap delegate.removeAuthorizedClient(clientRegistrationId, principal, null)
        databaseClient.sql(...).flatMap(clientRegistrationId -> {
            delegate.removeAuthorizedClient(clientRegistrationId, new StubAuthentication(principalName), null);
        });
    }
}

static class StubAuthentication implements Authentication {
    private static final long serialVersionUID = -8037754046522288880L;
    private final String name;

    StubAuthentication(String name) {
        this.name = name;
    }

    @Override
    public String getName() {
        return name;
    }

    @Override
    public Collection<? extends GrantedAuthority> getAuthorities() {
        return List.of();
    }

    @Override
    public Object getCredentials() {
        return name;
    }

    @Override
    public Object getDetails() {
        return name;
    }

    @Override
    public Object getPrincipal() {
        return name;
    }

    @Override
    public boolean isAuthenticated() {
        return true;
    }

    @Override
    public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException {
    }

}

What do you think?

[kwonglau]

I like what you posted above.

If AuthenticatedPrincipalServerOAuth2AuthorizedClientRepository delegate could be ServerOAuth2AuthorizedClientRepository delegate, and allow the developer to wire the implementation they want, that would be better.

Thank you for your quick response. much appreciate.

[ch4mpy]

The issue is not stale. I'm still experimenting with cleaner solutions that what I exposed just before. I think I can provide with better integration with custom ReactiveOAuth2AuthorizedClientService or ServerOAuth2AuthorizedClientRepository (as well as hooking into Spring Session, if used, as sessions need to be hacked a bit for Back-Channel Logout to work properly).

[kwonglau]

Thank you so much

[ch4mpy]

I think I'm going to remove the Back-Channel Logout support from this lib because of https://github.com/spring-projects/spring-security/pull/12570 which should bring it in Spring Security (it might need some hacking because of what I report in that thread about multi-tenancy and session termination, but this is another story).

However, what is reported is still worth some changes: instrumentation to bring OAuth2 clients multi-tenancy would be much more transparent for spring-addons users if this was achieved with AOP rather than with specific implementations of authorized client repositories.

What I call "client multi-tenancy" is allowing a user to be authenticated with more than just on OpenID Provider at a time and being able to retrieve the right authorized client along with the right principal name: username is likely to change from an authorization server to another, specifically when the default claim (subject) is used. This is a requirement to send requests to different groups of API (expecting different issuers), each with the right access token.

[ch4mpy]

@kwonglau you can see there the details of of what changes.

I removed SpringAddonsServerOAuth2AuthorizedClientRepository. Instead, spring-addons now uses AOP to instrument the authorized client repository you configure. Also, this instrumentation happens only if client multi-tenancy support is explicitly enabled. This is the case in the resource-server_with_ui tutorial.

Back-Channel Logout support is removed from spring-addons. Getting it to work properly on reactive applications with sessions instrumented via AOP is actually quite complicated.

There is a PR with a lot of activity on Spring Security to bring this feature in the framework and I don't want to provide with features already provided by the core framework. So let's wait until the feature is released, and maybe I'll add here some behavior around client multi-tenancy.

[ch4mpy]

@kwonglau the changes I referred to in my last comment are released with 7.1.0. Please close if it it feels ok to you (or comment why otherwize).

[kwonglau]

@ch4mpy thank you so much the the fix. I created a pr https://github.com/ch4mpy/spring-addons/pull/143 to fix some minor issues. mainly forgot to call subscribe on multiple places.