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

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

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