JDBC implementation of RegisteredClientRepository

[rlewczuk]

Code seems to be more-or-less ready for preliminary review, yet I still have some questions in comments below.

Closes gh-265

[rlewczuk]

Question: provided set of tests mostly copies those from InMemoryRegisteredClientRepositoryTests. This leads to considerable amount of code duplication, grouping those tests into common superclass would not only reduce duplication but also create baseline for testing future implementations of RegisteredClientRepository (eg. Redis variant). For now I've decided against it, so I don't touch any unrelated code. Should I refactor those into common base ? If so, should it be done as part of this change or as separate one ?

[rlewczuk]

Question: as RegisteredClient is immutable, there us no update semantics implemented in JdbcRegisteredClientRepository. Is it valid approach ?

[rlewczuk]

Question: JdbcClientRepository checks for duplicated client secret. Does it make sens when password encoders are used ?

[lanmingle]

你好！

我曾经在使用 mongo 进行适配，我建议使用转换器，这样更灵活。

---

Hello!

I used to use mongo for adaptation, and I recommend using a converter, which is more flexible.

---

Reference Code

model

@Getter
@Setter
@ToString
@EqualsAndHashCode(of = {"key"})
public class OidcPersistentKeyValue {

    private String key;

    private Object value;

    public OidcPersistentKeyValue() {
    }

    public OidcPersistentKeyValue(String key, Object value) {
        this.key = key;
        this.value = value;
    }

}

/**
 * @see org.springframework.security.oauth2.server.authorization.config.ClientSettings
 */
@Getter
@Setter
@ToString(callSuper = true)
public class OidcClientSettings extends OidcPersistentKeyValue {

    public static final String CLIENT_SETTING_BASE = "setting.client.";

    public static final String REQUIRE_PROOF_KEY = CLIENT_SETTING_BASE.concat("require-proof-key");

    public static final String REQUIRE_USER_CONSENT = CLIENT_SETTING_BASE.concat("require-user-consent");

    public OidcClientSettings() {
    }

    public OidcClientSettings(String key, Object value) {
        super(key, value);
    }

}

/**
 * @see org.springframework.security.oauth2.server.authorization.config.TokenSettings
 */
@Getter
@Setter
@ToString(callSuper = true)
public class OidcTokenSettings extends OidcPersistentKeyValue {

    public static final String TOKEN_SETTING_BASE = "setting.token.";

    public static final String ACCESS_TOKEN_TIME_TO_LIVE = TOKEN_SETTING_BASE.concat("access-token-time-to-live");

    public static final String REUSE_REFRESH_TOKENS = TOKEN_SETTING_BASE.concat("reuse-refresh-tokens");

    public static final String REFRESH_TOKEN_TIME_TO_LIVE = TOKEN_SETTING_BASE.concat("refresh-token-time-to-live");

    public OidcTokenSettings() {
    }

    public OidcTokenSettings(String key, Object value) {
        super(key, value);
    }

}

/**
 * @see org.springframework.security.oauth2.core.ClientAuthenticationMethod
 */
public enum OidcClientAuthenticationMethod {

    BASIC,

    POST,

    NONE,

}

/**
 * @see org.springframework.security.oauth2.core.AuthorizationGrantType
 */
public enum OidcAuthorizationGrantType {

    AUTHORIZATION_CODE,

//    @Deprecated
//    IMPLICIT,

    REFRESH_TOKEN,

    CLIENT_CREDENTIALS,

    PASSWORD,

}

domain

@Getter
@Setter
@ToString
@EqualsAndHashCode(of = {"id"})
@Document(collection = "OidcRegisteredClient")
@CompoundIndexes({
        @CompoundIndex(def = "{'clientId': 1}", unique = true)
})
public class OidcRegisteredClient {

    @Id
    private String id;

    private String clientId;

    private String clientSecret;

    private Set<OidcClientAuthenticationMethod> clientAuthenticationMethods;

    private Set<OidcAuthorizationGrantType> authorizationGrantTypes;

    private Set<String> redirectUris;

    private Set<String> scopes;

    private Set<OidcClientSettings> clientSettings;

    private Set<OidcTokenSettings> tokenSettings;

}

repository

public interface OidcRegisteredClientRepository extends MongoRepository<OidcRegisteredClient, String> {

    @Query("{ 'clientId': :#{#clientId} }")
    Optional<OidcRegisteredClient> getByClientId(@Param("clientId") String clientId);

}

converter

@Slf4j
@Component
public class RegisteredClientConverter implements Converter<OidcRegisteredClient, RegisteredClient> {

    @Override
    public RegisteredClient convert(OidcRegisteredClient source) {
        return this.toConvert(OidcRegisteredClientBuilder.create(source).build());
    }

    private RegisteredClient toConvert(OidcRegisteredClient merge) {
        return RegisteredClient.withId(merge.getId())
                .clientId(merge.getClientId())
                .clientSecret(merge.getClientSecret())
                .clientAuthenticationMethods((consumer) -> this.toClientAuthenticationMethods(consumer, merge.getClientAuthenticationMethods()))
                .authorizationGrantTypes((consumer) -> this.toAuthorizationGrantTypes(consumer, merge.getAuthorizationGrantTypes()))
                .redirectUris((consumer) -> this.toRedirectUris(consumer, merge.getRedirectUris()))
                .scopes((consumer) -> this.toScopes(consumer, merge.getScopes()))
                .clientSettings((consumer) -> this.toClientSettings(consumer, merge.getClientSettings()))
                .tokenSettings((consumer) -> this.toTokenSettings(consumer, merge.getTokenSettings()))
                .build();
    }

    private void toClientAuthenticationMethods(Set<ClientAuthenticationMethod> sources, Set<OidcClientAuthenticationMethod> others) {
        if (CollUtil.isNotEmpty(others)) {
            others.forEach(source -> sources.add(new ClientAuthenticationMethod(source.name().toLowerCase())));
        }
    }

    private void toAuthorizationGrantTypes(Set<AuthorizationGrantType> sources, Set<OidcAuthorizationGrantType> others) {
        if (CollUtil.isNotEmpty(others)) {
            others.forEach(source -> sources.add(new AuthorizationGrantType(source.name().toLowerCase())));
        }
    }

    private void toRedirectUris(Set<String> sources, Set<String> others) {
        if (CollUtil.isNotEmpty(others)) {
            sources.addAll(others.stream().filter(StrUtil::isNotEmpty).collect(Collectors.toSet()));
        }
    }

    private void toScopes(Set<String> sources, Set<String> others) {
        if (CollUtil.isNotEmpty(others)) {
            sources.addAll(others.stream().filter(StrUtil::isNotEmpty).collect(Collectors.toSet()));
        }
    }

    private void toClientSettings(ClientSettings source, Set<OidcClientSettings> others) {
        if (CollUtil.isNotEmpty(others)) {
            others.forEach(other -> {
                String key = other.getKey();
                Object val = other.getValue();
                if (ClientSettings.REQUIRE_PROOF_KEY.equals(key)) {
                    source.setting(key, Convert.convert(Boolean.class, val, OidcRegisteredClientBuilder.REQUIRE_PROOF_KEY));
                } else if (ClientSettings.REQUIRE_USER_CONSENT.equals(key)) {
                    source.setting(key, Convert.convert(Boolean.class, val, OidcRegisteredClientBuilder.REQUIRE_USER_CONSENT));
                }
            });
        }
    }

    private void toTokenSettings(TokenSettings source, Set<OidcTokenSettings> others) {
        if (CollUtil.isNotEmpty(others)) {
            others.forEach(other -> {
                String key = other.getKey();
                Object val = other.getValue();
                if (TokenSettings.ACCESS_TOKEN_TIME_TO_LIVE.equals(key)) {
                    source.setting(key, Convert.convert(Duration.class, val, OidcRegisteredClientBuilder.ACCESS_TOKEN_TIME_TO_LIVE));
                } else if (TokenSettings.REFRESH_TOKEN_TIME_TO_LIVE.equals(key)) {
                    source.setting(key, Convert.convert(Duration.class, val, OidcRegisteredClientBuilder.REFRESH_TOKEN_TIME_TO_LIVE));
                } else if (TokenSettings.REUSE_REFRESH_TOKENS.equals(key)) {
                    source.setting(key, Convert.convert(Boolean.class, val, OidcRegisteredClientBuilder.REUSE_REFRESH_TOKENS));
                }
            });
        }
    }

}

RegisteredClientRepository

@Slf4j
@Repository
public class DelegateRegisteredClientRepository implements RegisteredClientRepository {

    @Autowired
    private OidcRegisteredClientService registeredClientService;

    @Autowired
    private RegisteredClientConverter registeredClientConverter;

    @Override
    public RegisteredClient findById(String id) {
        return this.registeredClientService.getById(id).map(registeredClient -> this.registeredClientConverter.convert(registeredClient)).orElse(null);
    }

    @Override
    public RegisteredClient findByClientId(String clientId) {
        return this.registeredClientService.getByClientId(clientId).map(registeredClient -> this.registeredClientConverter.convert(registeredClient)).orElse(null);
    }

}

[jgrandja]

@rlewczuk

FYI, @sjohnr is a new member on our team and he will be handling the review of this PR. We are still targeting this for the 0.1.2 release (Jul 8) so @sjohnr will be providing initial feedback soon.

[sjohnr]

Hello @rlewczuk, thanks for your work on this! Just so you know, I'm about to head out of town for the week, but wanted to give a bit of feedback sooner than later. This feedback focuses on the schema and two additional items. I'll do a more thorough review of the code early next week hopefully.

See feedback inline.

[sjohnr]

For the settings, please consider using a JSON field, client_settings varchar(1000) DEFAULT NULL and token_settings varchar(1000) DEFAULT NULL. See #304 for an example of this (e.g. attributes).

[rlewczuk]

Implemented but I have the same remarks as with client_secret LOB. It is more complex both in code and in day-to-day maintenance if application administrator wants to make manual changes in database.

[sjohnr]

@rlewczuk I've added some additional comments and requested a few more changes. Let me know if you have questions or comments. Please see my feedback inline.

[rlewczuk]

I'll take care of it next week or so as I'm currently bogged with other project I'm finishing right now. Thanks for review.

[rlewczuk]

Ok, all fixes are here and we're ready for second round.

[sjohnr]

Thanks @rlewczuk! This is now in main as 769cf8f.