[keycloak-dev] federation commited need feedback

[Bill Burke]

On 7/24/2014 11:02 AM, Stian Thorgersen wrote:
>
>
> ----- Original Message -----
>> From: "Bill Burke" <bburke at redhat.com>
>> To: "Stian Thorgersen" <stian at redhat.com>
>> Cc: keycloak-dev at lists.jboss.org
>> Sent: Thursday, 24 July, 2014 3:56:35 PM
>> Subject: Re: [keycloak-dev] federation commited need feedback
>>
>>
>>
>> On 7/24/2014 10:32 AM, Stian Thorgersen wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Bill Burke" <bburke at redhat.com>
>>>> To: "Stian Thorgersen" <stian at redhat.com>
>>>> Cc: keycloak-dev at lists.jboss.org
>>>> Sent: Thursday, 24 July, 2014 2:47:57 PM
>>>> Subject: Re: [keycloak-dev] federation commited need feedback
>>>>
>>>>
>>>>
>>>> On 7/24/2014 9:25 AM, Stian Thorgersen wrote:
>>>>> Looks good. Only two comments from me:
>>>>>
>>>>> 1. FederationManager.getFederationProvider uses factories directly
>>>>>
>>>>> Why is this? This will cause the provider instance not to be registered
>>>>> with the session, so won't be closed automatically when the session is
>>>>> closed, nor will it be able to attach to the transaction.
>>>>>
>>>>
>>>> FederationProviders are not singletons within KeycloakSession like
>>>> RealmProvider, UserProvider, and UserSessionProvider are.  You can have
>>>> multiple FederationProvider instances per realm.
>>>>
>>>>
>>>> public interface FederationProviderFactory extends
>>>> ProviderFactory<FederationProvider> {
>>>>        FederationProvider getInstance(KeycloakSession session,
>>>> FederationProviderModel model);
>>>> }
>>>>
>>>>
>>>> FederationProviders implementations have the option to enlist themselves
>>>> with the KeycloakSession transaction manager.  LDAP (really Picketlink)
>>>> doesn't have the concept of a session or transaction, so it doesn't
>>>> enlist itself.
>>>>
>>>> I guess we do need a enlistForClose() method on KeycloakSession though.
>>>
>>> As long as the lifespan of a FederationProvider is the same as the
>>> KeycloakSession they should be created through the KeycloakSession IMO. We
>>> just need to add an additional createProvider that can create a instance
>>> tied to a specific realm.
>>>
>>
>> I disagree.  Its really up to the implementation on whether the provider
>> is managed by KeycloakSession or not or whether its lifespan is tied to
>> KeycloakSession or not.
>>
>
> Why would a provider not be tied to a session? IMO there should always be a session.
>

It might be expensive to create the provider and/or the provider may 
have no concept of a session.  For example, Picketlink PartitionManagers 
are not tied to a KeycloakSession

>> We already manage transactions in this manner.  For example
>> JpaUserProvider does not enlist its own KeycloakTransaction and its
>> close() method is a no-op.  CacheUserProvider and JpaConnectionProvider
>> enlist their own transaction objects manually.  JpaConnectionProvider's
>> close() method actually does something.
>
> Commit transactions and closing providers are tied to the session, that's the whole point. With your code you're creating providers that are never closed.
>

Whether or not a transaction is enlisted is up to the implementation 
though.  Also different provider factories may have differing create() 
methods with different parameters, so it doesn't make sense for the 
KeycloakSession to manage Provider creation all the time.

FYI I added an "enlistForClose()" method to KeycloakSession.

>>
>> --
>> Bill Burke
>> JBoss, a division of Red Hat
>> http://bill.burkecentral.com
>>

-- 
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com