4:39 a.m.
On 29/04/16 11:06, Stian Thorgersen wrote:
On 29 April 2016 at 10:58, Marek Posolda <mposolda(a)redhat.com
<mailto:mposolda@redhat.com>> wrote:
On 29/04/16 10:22, Stian Thorgersen wrote:
>
> We have 3 types of providers:
>
> * Server configured - no config or config from keycloak-server
> * Realm configured - config from realm model
> * Instance configured - multiple instances per realm
>
> Removing master realm as we plan to do means that realm
> configured provider factories can get realm from KeycloakContext
> as there's only one realm per-session.
>
In theory yes. In practice there might be still corner cases when
you need to deal with multiple realms inside same KeycloakSession
(like export/import for example). But hope we can handle most of
the cases by assume that KeycloakContext has correct realm set.
Corner cases like that is easy - we'd use create a KeycloakSession
per-realm, making sure KeycloakContext is initialized properly.
> For instance configured I propose we add getProvider(Class c,
> String id, String instanceId) to provider factory. The it's up to
> the provider factory itself to extract the config from the realm
> model or another source. It also means that the session can
> easily keep track of these and only create one id+instanceId per
> session.
>
ah, ok. I somehow missed the proposal.
It should work fine, I think it's quite similar to what I
proposed. Despite I proposed to send whole state to provider
factory (aka. UserFederationProviderModel) instead of just
instanceId and then assume that state must properly implement
"hashCode" to ensure that session can keep track of these and
return provider of already used state.
Yup, very similar, but I think the devil is in the details. In my
proposal the factory itself knows how to extract the state, so it's
then up to the factory to decide how state should be stored. A custom
provider may need to store config in a separate custom entity, which
KeycloakSessionFactory wouldn't know how to retrieve.
Well, for custom SPI
providers, you can simply use "String" as the state
type. Defacto I see the only difference between proposals, that yours is
simpler as it's just always using "String" as state type instead of
having type dynamic.
I am not saying it's big issue though. For example UserFederationManager
now already have all UserFederationProviderModel instances configured
for realm, so with yours, you will need to call:
session.getProvider(UserFederationProvider.class, "ldap",
providerModel.getId());
and session will need to load UserFederationProviderModel again from
realm as it knows just id. But since it's supposed to be cached, there
is no additional performance penalty in loading
UserFederationProviderModel again.
So I agree we can try to go simpler way and possibly enhance just if we
find another SPI limitations.
Marek
Marek
> On 29 Apr 2016 09:43, "Marek Posolda" <mposolda(a)redhat.com
> <mailto:mposolda@redhat.com>> wrote:
>
> Yes, AFAIK we have open JIRA for this for a long time ago.
>
> It's the same issue for IdentityProvider (and maybe some
> others SPI too) that they bypass "official" way for create
> provider via session.getProvider(providerClazz) and hence
> they are not registered in KeycloakSession and "close" method
> is not called for them.
>
> The issue is that our SPI is a bit limited IMO and doesn't
> support "stateful" providers. The providers are created
> through "ProviderFactory.create(KeycloakSession)". So the
> only available state of provider ATM is just ProviderFactory
> + KeycloakSession, which is sometimes not sufficient.
>
>
> I can see 2 possibilities to address:
>
> 1) Always make the provider implementation "stateless" and
> ensure all the state is passed as argument to provider
> methods. This is what we already do for some providers (for
> example all methods of UserProvider has RealmModel as
> parameter). So if we rewrite UserFederation SPI that
> UserFederationProviderModel will be passed as argument to all
> methods of UserFederationProvider, then it can use "official"
> way too.
>
>
> 2) Improve the SPI, so it can properly support "stateful"
> providers. This is more flexible then (1) and I would go this
> way long term.
>
> I am thinking about something like this:
>
> public interface StatefulProvider<S> extends Provider {
> }
>
>
> public class StatefulProviderFactory<T extends
> StatefulProvider, S> {
>
> T create(KeycloakSession session, S state);
>
> .......
> }
>
>
> and on KEycloakSession new method like this:
>
> <S,T extends StatefulProvider<S>>T getProvider(Class<T>
providerClazz, String id,S state);
>
>
> The "state" will need to properly implement equals and
> hashCode, so the SPI can deal with it and not create another
> instance of StatefulProvider if it was called for this
> KeycloakSession with same state before.
>
> Marek
>
>
>
> On 29/04/16 08:00, Stian Thorgersen wrote:
>> Looking at the code for user federation it looks like user
>> federation provider instances with the same configuration
>> can be created multiple times for a single session. Also
>> they are never closed to resources aren't released.
>>
>>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev(a)lists.jboss.org
>> <mailto:keycloak-dev@lists.jboss.org>
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>