KEYCLOAK-2045 Add support for flexible Validation

[thomasdarimont]

This proposal shall serve as a more flexible foundation for user profile validations and much more.

This is a squashed and cleaned-up version of my experimental branch poc/validation-spi.

See discussion on mailing list: https://groups.google.com/g/keycloak-dev/c/-XjQu7rn56s

Some API examples can be found in the DefaultValidatorProviderTest

Additionally some integration examples can be seen here:

• UpdateProfile.java
• Validation.java

[pedroigor]

@thomasdarimont Nice stuff!

I don't have much to add but a few implementation points like those for loops to lookup keys that could maybe be improved with a map.

The other thing would be the correct place for this new SPI, if not in server-spi-private. At least for now and while we get more feedback from people using it.

[thomasdarimont]

@pedroigor thanks for the quick review. Okay, so I'm going to move everything under server-spi-private for now and address the loop idea.

[thomasdarimont]

@thomasdarimont I adapted the PR according to your changes.

I moved the interfaces from server-spi to server-spi-private and refactored the key lookups for ValidationKey and ValidationContextKey. I just noticed that the org.keycloak.validation.ClientValidationSPI could get replaced with the generic validation SPI.

I also simplified the ValidatorProvider interface to the following methods:

    ValidationResult validate(ValidationContext context, Object value, Set<ValidationKey> keys);

    default ValidationResult validate(ValidationContext context, Object value, ValidationKey... keys) {
        return validate(context, value, new LinkedHashSet<>(Arrays.asList(keys)));
    }

    default ValidationResult validate(ValidationContext context, Object value, ValidationKey key) {
        return validate(context, value, Collections.singleton(key));
    }

    default ValidationResult validate(ValidationContext context, Object value) {
        return validate(context, value, Collections.emptySet());
    }

I'm not sure whether it would make sense to create the ValidationContext also based on the given value since it usually provides access to the Realm, Client, Attributes etc. If we leave that out we won't be able to access this information.

I think the API is now quite pragmatic to serve most use cases in a lean way.

        UserModel user = new InMemoryUserAdapter(session, realm, "1");
        user.setFirstName("Theo");
        user.setLastName("Tester");
        user.setEmail("tester@allowed");

        ValidationContext context = new ValidationContext(USER_REGISTRATION_CONTEXT_KEY);
        // Note that we don't specify the ValidationKey here, since it is inferred by the given value (UserModel)
        ValidationResult result = validator.validate(context, user);
        assertTrue("A valid user should be valid", result.isValid());

The ValidationKey to use is now determined in org.keycloak.services.validation.DefaultValidatorProvider#resolveValidationKeysFromValue

[thomasdarimont]

To ease access to the validation API, we could also add a method to KeycloakSession like:

/**
 * Provides access to a built-in ValidatorProvider which can validate values according to various validation rules.
 */
ValidatorProvider validation();

... once the ValidatorProvider was promoted to server-spi

[thomasdarimont]

We could also offer a variant like:

 ValidationResult validate(Object value)

which could create a ValidationContext internally which extracts information like the current, realm, client, etc. from the KeycloakSession.getContext()

[thomasdarimont]

could be extracted from the KeycloakSession

[thomasdarimont]

could be extracted from the KeycloakSession

[thomasdarimont]

We could add a ctor variant that accepts a KeycloakSession, with this we could access Realm and Client information via session.getContext()

[thomasdarimont]

Just rebased this on current master

[pedroigor]

@thomasdarimont I liked it very much. Great work. It LGTM and I'm OK to merge it.

One thing though, what do you think about this construct?

ValidationContext context = validator.forUserProfile();

context.withFailFast(true);

ValidationResult result = context.validate("user@kc.org", ValidationKey.USER_EMAIL);

Which could possibly be chained as:

validator.forUserProfile().withFailFast().validate("user@kc.org", ValidationKey.USER_EMAIL);

Or this one to validate everything on a user:

validator.forUserProfile().withFailFast().validate(user);

Considering that the built-in context keys are usually what you'll be validating maybe we could make the usage simpler by not forcing client code to explicitly use constants, what I think makes code more readable.

[thomasdarimont]

Hi @pedroigor,

Thank you for your feedback! Of course we could add some convenience methods but I'd add those to a dedicated class instead of the default Validator interface. E.g. having a ClientValidator or ProfileValidator facade which delegates to the generic valdiation SPI. Another option that the current PR allows is to define a configurable ClientValidation or ProfileValidation that can be parameterized with context specific configuration.

Btw. I just noticed that a stripped down version of a validation framework was just added to keycloak in 11.0.3
f660adc

With this PR I think there are now at least 4 different ways to do validation in Keycloak.

• Java EE Bean Validation
• The old ValidatorClass
• the new Validator provider added with 11.0.3
• this PR.

I need to check again whether those can be combined, or whether the new functionality partially supersedes the Validation implementation of this PR.

Cheers,
Thomas

[pedroigor]

As per the different validation approaches, that is one of my main concerns right now :)

So far the validation we had was specific for clients but now it is changing the scope and becoming a really overlap with your changes and make things even more interesting (or not). See #7586.

Regarding the convenience methods, my main point is related to not making this API so generic but tied with our requirements and use cases. Thus the proposed convenience methods from a single place instead of spreading them across different classes.

[pedroigor]

@thomasdarimont How we are going to proceed with this one?

[thomasdarimont]

@pedroigor sorry for the late reply I was busy quite this week... as discussed. I'll catch up with the latest design document and create a smaller (but extensible) version of this PR without all the bells and whistles.

We can then close this PR.

[pedroigor]

Thanks.