<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 29 April 2016 at 11:48, Marek Posolda <span dir="ltr"><<a href="mailto:mposolda@redhat.com" target="_blank">mposolda@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<div>On 29/04/16 11:39, Marek Posolda wrote:<br>
</div>
<blockquote type="cite">
<div>On 29/04/16 11:06, Stian Thorgersen
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On 29 April 2016 at 10:58, Marek
Posolda <span dir="ltr"><<a href="mailto:mposolda@redhat.com" target="_blank">mposolda@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span>
<div>On 29/04/16 10:22, Stian Thorgersen wrote:<br>
</div>
<blockquote type="cite">
<p dir="ltr">We have 3 types of providers:</p>
<p dir="ltr">* Server configured - no config or
config from keycloak-server<br>
* Realm configured - config from realm model<br>
* Instance configured - multiple instances per
realm</p>
<p dir="ltr">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.</p>
</blockquote>
</span> 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.</div>
</blockquote>
<div><br>
</div>
<div>Corner cases like that is easy - we'd use create a
KeycloakSession per-realm, making sure KeycloakContext
is initialized properly.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span><br>
<blockquote type="cite">
<p dir="ltr">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.</p>
</blockquote>
</span> ah, ok. I somehow missed the proposal. <br>
<br>
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.</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
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. <br>
<br>
<br>
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:<br>
<br>
session.getProvider(UserFederationProvider.class, "ldap",
providerModel.getId());<br>
<br>
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.<br>
</blockquote></span>
Well <span><span> ;-) </span></span><br>
<br>
But on the other hand with simpler proposal... All
UserFederationProviderFactory implementations provided by people
will always need to load UserFederationProviderModel at the
beginning:<br>
<br>
UserFederationProviderModel providerModel =
session.getContext().getRealm().getFederationProvider(id);<br>
<br>
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.</div></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class=""><font color="#888888"><br>
<br>
Marek</font></span><div><div class="h5"><br>
<blockquote type="cite"> <br>
So I agree we can try to go simpler way and possibly enhance just
if we find another SPI limitations.<br>
<br>
Marek<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span><font color="#888888"><br>
<br>
Marek</font></span><span><br>
<blockquote type="cite">
<div class="gmail_quote">On 29 Apr 2016 09:43,
"Marek Posolda" <<a href="mailto:mposolda@redhat.com" target="_blank">mposolda@redhat.com</a>>
wrote:<br type="attribution">
<blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div> Yes, AFAIK we have open JIRA for this
for a long time ago. <br>
<br>
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.<br>
<br>
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. <br>
<br>
<br>
I can see 2 possibilities to address:<br>
<br>
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. <br>
<br>
<br>
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.<br>
<br>
I am thinking about something like this:<br>
<br>
public interface StatefulProvider<S>
extends Provider {<br>
}<br>
<br>
<br>
public class StatefulProviderFactory<T
extends StatefulProvider, S> {<br>
<br>
<span style="color:rgb(32,153,157)">T </span>create(KeycloakSession
session, S state);<br>
<br>
....... <br>
}<br>
<br>
<br>
and on KEycloakSession new method like
this:<br>
<br>
<pre style="color:rgb(0,0,0);font-family:'DejaVu Sans Mono';font-size:9pt;background-color:rgb(255,255,255)"><<span style="color:rgb(32,153,157)">S</span>, <span style="color:rgb(32,153,157)">T </span><span style="color:rgb(0,0,128);font-weight:bold">extends </span>StatefulProvider<<span style="color:rgb(32,153,157)">S</span>>> <span style="color:rgb(32,153,157)">T </span>getProvider(Class<<span style="color:rgb(32,153,157)">T</span>> providerClazz, String id, <span style="color:rgb(32,153,157)">S </span>state);</pre>
<br>
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.<br>
<br>
Marek
<div><br>
<br>
<br>
On 29/04/16 08:00, Stian Thorgersen
wrote:<br>
</div>
</div>
<blockquote type="cite">
<div>
<div dir="ltr">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.</div>
<br>
<fieldset></fieldset>
<br>
</div>
<pre>_______________________________________________
keycloak-dev mailing list
<a href="mailto:keycloak-dev@lists.jboss.org" target="_blank">keycloak-dev@lists.jboss.org</a>
<a href="https://lists.jboss.org/mailman/listinfo/keycloak-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a></pre>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
keycloak-dev mailing list
<a href="mailto:keycloak-dev@lists.jboss.org" target="_blank">keycloak-dev@lists.jboss.org</a>
<a href="https://lists.jboss.org/mailman/listinfo/keycloak-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a></pre>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>