[keycloak-dev] User Federation provider instances

Fri Apr 29 07:49:47 EDT 2016

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

> On 29/04/16 12:42, Stian Thorgersen wrote:
>
>
>
> 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>
>> 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.
>
> Yeah, we can change the "caller" ( UserFederationManager ) to load just
> id. The UserModel has just "federationLink" with the ID, so we don't need
> to load the full UserFederationProviderModel in UserFederationManager.
> During registration or lookup new user, you need a list of full provider
> models though as they need to be sorted by priority. But that's very
> minor....
>
> There is still also the second minor issue I mentioned above, that
> UserFederationProvider implementation always needs
> UserFederationProviderModel (besides some simple impl, which don't have any
> custom config). So in many cases the UserFederationProviderFactory.create
> implementations will always start with load of UserFederationProviderModel
> as they have only ID. So that's common logic, which can be theoretically
> handled by our SPI framework instead. But also quite minor though...
>

Yes, it common in UserFederation, but other SPIs have different mechanisms.
So in that case the SPI would have to load the UserFederationProviderModel,
that could work as well though. It's not a big deal though and I think
having the provider itself responsible for loading is more flexible. One
provider may not want to use UserFederationProviderModel, but instead use
another model or even another store completely.

>
> I don't have strong opinion that simpler proposal with "String" is not
> enough.
>
> 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.
>
> +1
>
> Marek
>
>
>
>>
>>
>> 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>
>>> 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/4994b35c/attachment-0001.html 