3:22 a.m.
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.
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.
On 29 Apr 2016 09:43, "Marek Posolda" <mposolda(a)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@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
3:58 a.m.
New subject: User Federation provider instances
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.
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.
Marek
On 29 Apr 2016 09:43, "Marek Posolda"
<mposolda(a)redhat.com
<mailto:mposolda@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 list
> keycloak-dev(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
4:06 a.m.
New subject: User Federation provider instances
On 29 April 2016 at 10:58, Marek Posolda <mposolda(a)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.
Marek
On 29 Apr 2016 09:43, "Marek Posolda" <mposolda(a)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@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
4:39 a.m.
New subject: User Federation provider instances
On 29/04/16 11:06, Stian Thorgersen wrote:
On 29 April 2016 at 10:58, Marek Posolda <mposolda(a)redhat.com
<mailto:mposolda@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.
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(a)redhat.com
> <mailto:mposolda@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 list
>> keycloak-dev(a)lists.jboss.org
>> <mailto:keycloak-dev@lists.jboss.org>
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
4:48 a.m.
New subject: User Federation provider instances
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(a)redhat.com
> <mailto:mposolda@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.
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(a)redhat.com
>> <mailto:mposolda@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 list
>>> keycloak-dev(a)lists.jboss.org
>>> <mailto:keycloak-dev@lists.jboss.org>
>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>
>
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev
5:42 a.m.
New subject: User Federation provider instances
On 29 April 2016 at 11:48, Marek Posolda <mposolda(a)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(a)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.
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.
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(a)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@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>
>
>
>
_______________________________________________
keycloak-dev mailing
listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
6:16 a.m.
New subject: User Federation provider instances
On 29/04/16 12:42, Stian Thorgersen wrote:
On 29 April 2016 at 11:48, Marek Posolda <mposolda(a)redhat.com
<mailto:mposolda@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(a)redhat.com
>> <mailto:mposolda@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...
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(a)redhat.com
>>> <mailto:mposolda@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 list
>>>> keycloak-dev(a)lists.jboss.org
>>>> <mailto:keycloak-dev@lists.jboss.org>
>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>
>>
>>
>
>
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
6:49 a.m.
New subject: User Federation provider instances
On 29 April 2016 at 13:16, Marek Posolda <mposolda(a)redhat.com> wrote:
On 29/04/16 12:42, Stian Thorgersen wrote:
On 29 April 2016 at 11:48, Marek Posolda <mposolda(a)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(a)redhat.com>
> mposolda(a)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(a)redhat.com>
>> mposolda(a)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@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>>
>>
>>
>
>
>
> _______________________________________________
> keycloak-dev mailing
listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>
>
>
3046
days inactive
3046
days old
7 comments
2 participants
participants (2)
-
Marek Posolda -
Stian Thorgersen