[keycloak-dev] Thinking about a change to providers

[Marek Posolda]

+1 on having "invalidateProvider" method.

For the other stuff,  we already have the first 2 "getProvider" methods, 
so the new stuff will be the methods with "String instanceId" parameter, 
right?

We already discuss adding the "String instanceId" . Now when thinking 
more, it looks that it is not so convenient.

When adding again UserFederation SPI as an example:

- UserFederationProviderFactory needs UserFederationProviderModel to 
create instance of UserFederationProvider
- So factory needs to lookup model from cache/db. Hence the instanceId 
would need to be compound of something like: 
<REALM-UUID>::<USER-FEDERATION-PROVIDER-MODEL-ID>
That's because to lookup UserFederationProviderModel, you first need 
RealmModel and then find the UserFederationProviderModel by it's ID 
within the realm.

You may admit that RealmModel is available on KeycloakContext. However I 
don't think that we can rely on it. KeycloakContext is available in REST 
requests, but in some other cases (ie. ExportImport, periodic tasks 
etc), it's not available. Caller usually have the RealmModel and he can 
manually set it to KeycloakContext before calling session.getProvider, 
however that doesn't look like good approach to me and should be rather 
avoided. So in shortcut, we shouldn't rely on realm being available in 
KeycloakContext IMO.

The logic for parse the "instanceId" and retrieve 
UserFederationProviderModel from DB would be boilerplate code same to 
all UserFederationProviderFactory impls.


With that in mind, it really seems to me that instead of "String 
instanceId", it may work better to have some common configuration class 
like "ProviderModel" . Then signature will look like:

* getProvider(Class<T> clazz, String providerId, ProviderModel  model)

All the model subclasses (UserFederationProviderModel, 
IdentityProviderModel, PasswordPolicyModel ...) will be subclasses of 
ProviderModel

Marek

On 23/06/16 12:01, Stian Thorgersen wrote:
> Currently it's expected that the factory is application scoped, while 
> provider instances are request scoped. Factories can if they want 
> return the same instance for provider to make it application scoped.
>
> This works as long as config is server-wide, but not if there are 
> config per-realm or even multiple different instances per-realm. This 
> applies to for example User Federation SPI (multiple per-realm), 
> Password Hashing SPI (one per-realm), etc.
>
> Currently the User Federation SPI creates and manages instances 
> outside of the session factory and session, which results in multiple 
> instances created per-request, not all being closed properly, etc..
>
> With that in mind I'd like to change the provider factories so that 
> there can be multiple provider factory instances. It's not completely 
> figured out, but I wanted to discuss it before I start a POC around it.
>
> We'd have the following methods on KeycloakSession:
>
> * getProvider(Class<T> clazz, Provider.class) - returns default provider
> * getProvider(Class<T> clazz, Provider.class, String providerId) - 
> returns a specific provider, with the default config
> * getProvider(Class<T> clazz, Provider.class, String providerId, 
> String instanceId) - returns a specific provider, with the specific config
>
> We'd also add a method:
>
> * invalidateProvider(Class<T> clazz, Provider.class, String 
> providerId, String instanceId) - this would be called when the config 
> for a specific provider instance is updated
>
> Behind the covers the instances would be maintained. Each provider 
> factory would internally be responsible to retrieve config and cache 
> config for instances.
>
> Does this sound like an idea worth pursuing? I'd like to try it out on 
> the PasswordPolicy SPI first.
>
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160623/be748368/attachment.html