[keycloak-dev] User Federation provider instances
Marek Posolda
mposolda at redhat.com
Fri Apr 29 05:48:31 EDT 2016
On 29/04/16 11:39, Marek Posolda wrote:
> On 29/04/16 11:06, Stian Thorgersen wrote:
>>
>>
>> On 29 April 2016 at 10:58, Marek Posolda <mposolda at redhat.com
>> <mailto:mposolda at 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.
Well ;-)
But on the other hand with simpler proposal... All
UserFederationProviderFactory implementations provided by people will
always need to load UserFederationProviderModel at the beginning:
UserFederationProviderModel providerModel =
session.getContext().getRealm().getFederationProvider(id);
so there is some shared logic, which can be possibly handled by
keycloak, but with simpler proposal, people will always need to call
this in their UserFederationProviderFactory implementations.
Marek
>
> 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 at redhat.com
>>> <mailto:mposolda at 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 at lists.jboss.org
>>>> <mailto:keycloak-dev at lists.jboss.org>
>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>
>>
>>
>
>
>
> _______________________________________________
> 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/20160429/3e7a5496/attachment-0001.html
More information about the keycloak-dev
mailing list