JdbcOAuth2AuthorizedClientService should support update when saving

[stavshamir]

Description
Configuring JdbcOAuth2AuthorizedClientService as follows:

@Bean
public OAuth2AuthorizedClientService authorizedClientService(
		JdbcOperations jdbcOperations,
		ClientRegistrationRepository clientRegistrationRepository) {
	return new JdbcOAuth2AuthorizedClientService(jdbcOperations, clientRegistrationRepository);
}

Allows only one authorization of the same user - trying to log in from one browser, and then from another, throws a DuplicateKeyException, making it unusable for SSO.

More importantly, it throws the same exception when a refresh token is used for re-authorization when the access token is expired, meaning this class can't be used when re-authorization flow is required.

This behavior is inconsistent with the in memory implementation which is backed by a Map and put to save the authorized client.

To Reproduce

1. Configure JdbcOAuth2AuthorizedClientService.
2. Log in to an OAuth2 provider.
3. Log in with the same user from another client - OR - wait for the access token to expire, and try to access a protected endpoint to trigger re-authorization.

Expected behavior
Re-authorization and login from a different client should not throw an exception. Instead, it should behave the same as InMemoryOAuth2AuthorizedClientService behaves when saveAuthorizedClient is called.

The solution to this bug must provide an atomic operation at the database level to be able to support concurrent access from multiple application instances. The only idea I had to achieve that is to use a ON DUPLICATE KEY UPDATE statement, but this will make this class non-generic.

If you have another idea, I would be glad to implement it and create a pull request.

[jgrandja]

@stavshamir Thank you for the detailed report.

Yes, I agree the solution to this bug will need to ensure an atomic update.
The ON DUPLICATE KEY UPDATE would be nice if it was standard but since it's not we cannot go with this option.

How about, saveAuthorizedClient() does an update if exists else insert. The transaction demarcation could be declared in a wrapper class of OAuth2AuthorizedClientService that simply delegates to JdbcOAuth2AuthorizedClientService?

Would you be interested in submitting a PR for this fix?

[stavshamir]

Even if we wrap the use a transaction, the following scenario may occur:

• App1 starts authorization for user X -> User X is not persisted
• App2 start authorization for user X -> User X is not persisted -> User X is inserted
• App1 continues to insert user X -> duplicate key is thrown

Then what? Retry?

After some more thought, I came up with this:

@Override
public void saveAuthorizedClient(OAuth2AuthorizedClient authorizedClient, Authentication principal) {
    Assert.notNull(authorizedClient, "authorizedClient cannot be null");
    Assert.notNull(principal, "principal cannot be null");

    if (existsAuthorizedClient(authorizedClient, principal)) {
        updateAuthorizedClient(authorizedClient, principal);
    } else {
        try {
            insertAuthorizedClient(authorizedClient, principal);
        } catch (DuplicateKeyException e) {
            updateAuthorizedClient(authorizedClient, principal);
        }
    }
}

I believe this would make the result consistent with the in memory implementation.
Do you find this solution acceptable? I am interested to submit a PR.

[jgrandja]

@stavshamir Good point. The scenario you describe is still an issue. The one thing I want to point out is that this issue is NOT a Spring Security issue but rather a Solution Architecture / Environment issue. Spring Security is not responsible for handling or co-ordinating distributed transactions. Instead this needs to be solved within the Application Architecture and Environment configuration. Makes sense?

Your proposed solution will definitely improve on the scenario you describe. Yes, please go with this and much appreciated for delivering this fix.