Re: [keycloak-dev] federation commited need feedback

----- Original Message ----- > From: "Bill Burke" <bburke(a)redhat.com&gt; > To: "Stian Thorgersen" <stian(a)redhat.com&gt; > Cc: keycloak-dev(a)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(a)redhat.com&gt; >>> To: "Stian Thorgersen" <stian(a)redhat.com&gt; >>> Cc: keycloak-dev(a)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 >