On 23 June 2016 at 14:19, Marek Posolda <mposolda(a)redhat.com> wrote:
+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?
Yes, I just included the two existing methods to point out that they will
still be there.
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.
Going forward we should rely on the realm being available in
KeycloakContext IMO. The whole purpose of it is so we don't have to pass
details around all the time, including the realm.
I see two options to it:
* Don't require the realm to load provider config. If instances ids are
UUIDs this would work, but I don't think they always are right?
* Add RealmModel to the lookup, so it becomes:
getProvider(Class<T> clazz, String providerId, RealModel realm, String
instanceId)
That would also require a invalidateProviders(RealmModel realm) that can
clear all provider instances for a specific realm
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
I don't like that at all as it requires loading and retrieving the model to
be able to get the instance. It should be the responsibility of the factory
and provider framework to be able to do that, not the code that wants to
use the provider.
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
listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev