[keycloak-dev] User Federation provider instances

Fri Apr 29 06:42:48 EDT 2016

On 29 April 2016 at 11:48, Marek Posolda <mposolda at redhat.com> wrote:

> 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> 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.
>

Depends. Should the "caller" actually load the UserFederationProviderModel
at all? It seems like all the caller needs to know is the instanceId and
shouldn't need to deal with loading the model/config.

Another thing, but that would require db changes for sure, could we have a
generic configuration mechanism? So rather than having to create a table
for each SPI we could have a single providers table. That would make it
much easier to introduce new SPIs.

>
>
> 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> 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 listkeycloak-dev at lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>>
>>
>>
>
>
>
> _______________________________________________
> keycloak-dev mailing listkeycloak-dev at lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160429/4ea54e40/attachment.html 