<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 29 April 2016 at 13:16, 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"><div><div class="h5">
<div>On 29/04/16 12:42, 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 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>
<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"></a><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.<br>
</div>
</div>
</div>
</div>
</blockquote></div></div>
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....<br>
<br>
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... <br></div></blockquote><div><br></div><div>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.</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">
<br>
I don't have strong opinion that simpler proposal with "String" is
not enough.<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</blockquote></span>
+1<span class=""><font color="#888888"><br>
<br>
Marek</font></span><div><div class="h5"><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>
<div>
<div><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"></a><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>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>