8:33 a.m.
On 23/06/16 14:28, Stian Thorgersen wrote:
On 23 June 2016 at 14:19, Marek Posolda <mposolda(a)redhat.com
<mailto:mposolda@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?
Even if
they are just UUID, we will require to refactor model and have
all the models lookup methods (e.g. "getUserFederationPRoviderModel",
"getIdentityProviderModel" ) available globally on RealmProvider rather
than on RealmModel. Not sure if it's very good, especially since in
admin console, you create providers per particular realm.
* 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
Not sure adding RealmModel is
sufficient... Some providers might not be
scoped per-realm but rather per-client though. For example recently
added authz based ResourceServer is scoped per client, so I can imagine
it can be valid use-case to have providers scoped per-client as well.
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.
Well, I don't see that as an issue, but
rather an advantage. It's better
if model is loaded by caller rather than an implementation. So the
custom UserFederationProviderFactory (or IdentityProviderFactory)
implemented by customers don't need to contain same code for lookup the
model based on instanceId String.
Marek
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(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/keycloak-dev