[keycloak-dev] federation commited need feedback

[Stian Thorgersen]

----- 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 4:25:59 PM
> Subject: Re: [keycloak-dev] federation commited need feedback
> 
> 
> 
> 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

That's exactly why a ProviderFactory has application scope. Anything that is expensive to create should be stored in the ProviderFactory and reused in Provider instances.

For example JpaConnectionProviderFactory creates one EntityManagerFactory (which is expensive) and reuses it to create a EntityManager for each JpaConnectionProvider.

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

Exactly, but that's what we already do with the transaction. The factory is responsible for enlisting it if it wants to.

See above - Provider instances should be created and closed per session, while factories can if they want reuse resources for multiple providers and then manage closing these itself.

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