<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 23/06/16 14:28, Stian Thorgersen
wrote:<br>
</div>
<blockquote
cite="mid:CAJgngAfZZEZKWCcfgR91vKd+FPiNO9hFmru6vDc2vNcmGvyUAg@mail.gmail.com"
type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On 23 June 2016 at 14:19, Marek
Posolda <span dir="ltr"><<a moz-do-not-send="true"
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>+1 on having "invalidateProvider" method.<br>
<br>
For the other stuff, we already have the first 2
"getProvider" methods, so the new stuff will be the
methods with "String instanceId" parameter, right?<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Yes, I just included the two existing methods to point
out that they will still be there.</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">
<div> <br>
We already discuss adding the "String instanceId" .
Now when thinking more, it looks that it is not so
convenient. <br>
<br>
When adding again UserFederation SPI as an example:<br>
<br>
- UserFederationProviderFactory needs
UserFederationProviderModel to create instance of
UserFederationProvider<br>
- So factory needs to lookup model from cache/db.
Hence the instanceId would need to be compound of
something like:
<REALM-UUID>::<USER-FEDERATION-PROVIDER-MODEL-ID><br>
That's because to lookup UserFederationProviderModel,
you first need RealmModel and then find the
UserFederationProviderModel by it's ID within the
realm.<br>
<br>
You may admit that RealmModel is available on
KeycloakContext. However I don't think that we can
rely on it. KeycloakContext is available in REST
requests, but in some other cases (ie. ExportImport,
periodic tasks etc), it's not available. Caller
usually have the RealmModel and he can manually set it
to KeycloakContext before calling session.getProvider,
however that doesn't look like good approach to me and
should be rather avoided. So in shortcut, we shouldn't
rely on realm being available in KeycloakContext IMO.
<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Going forward we should rely on the realm being
available in KeycloakContext IMO. The whole purpose of it
is so we don't have to pass details around all the time,
including the realm.</div>
<div><br>
</div>
<div>I see two options to it:</div>
<div><br>
</div>
<div>* Don't require the realm to load provider config. If
instances ids are UUIDs this would work, but I don't think
they always are right?</div>
</div>
</div>
</div>
</blockquote>
Even if they are just UUID, we will require to refactor model and
have all the models lookup methods (e.g.
"getUserFederationPRoviderModel", "getIdentityProviderModel" )
available globally on RealmProvider rather than on RealmModel. Not
sure if it's very good, especially since in admin console, you
create providers per particular realm.<br>
<blockquote
cite="mid:CAJgngAfZZEZKWCcfgR91vKd+FPiNO9hFmru6vDc2vNcmGvyUAg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>* Add RealmModel to the lookup, so it becomes:</div>
<div> getProvider(Class<T> clazz, String providerId,
RealModel realm, String instanceId)<br>
</div>
<div> That would also require a <span
style="color:rgb(80,0,80)">invalidateProviders(RealmModel
realm) that can clear all provider instances for a
specific realm</span><br>
</div>
</div>
</div>
</div>
</blockquote>
Not sure adding RealmModel is sufficient... Some providers might not
be scoped per-realm but rather per-client though. For example
recently added authz based ResourceServer is scoped per client, so I
can imagine it can be valid use-case to have providers scoped
per-client as well.<br>
<blockquote
cite="mid:CAJgngAfZZEZKWCcfgR91vKd+FPiNO9hFmru6vDc2vNcmGvyUAg@mail.gmail.com"
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">
<div> <br>
The logic for parse the "instanceId" and retrieve
UserFederationProviderModel from DB would be
boilerplate code same to all
UserFederationProviderFactory impls.<br>
<br>
<br>
With that in mind, it really seems to me that instead
of "String instanceId", it may work better to have
some common configuration class like "ProviderModel" .
Then signature will look like: <br>
<br>
* getProvider(Class<T> clazz, String providerId,
ProviderModel model)<br>
<br>
All the model subclasses (UserFederationProviderModel,
IdentityProviderModel, PasswordPolicyModel ...) will
be subclasses of ProviderModel<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>I don't like that at all as it requires loading and
retrieving the model to be able to get the instance. It
should be the responsibility of the factory and provider
framework to be able to do that, not the code that wants
to use the provider.</div>
</div>
</div>
</div>
</blockquote>
Well, I don't see that as an issue, but rather an advantage. It's
better if model is loaded by caller rather than an implementation.
So the custom UserFederationProviderFactory (or
IdentityProviderFactory) implemented by customers don't need to
contain same code for lookup the model based on instanceId String.<br>
<br>
Marek<br>
<blockquote
cite="mid:CAJgngAfZZEZKWCcfgR91vKd+FPiNO9hFmru6vDc2vNcmGvyUAg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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> <br>
Marek
<div>
<div class="h5"><br>
<br>
On 23/06/16 12:01, Stian Thorgersen wrote:<br>
</div>
</div>
</div>
<blockquote type="cite">
<div>
<div class="h5">
<div dir="ltr">Currently it's expected that the
factory is application scoped, while provider
instances are request scoped. Factories can if
they want return the same instance for provider
to make it application scoped.
<div><br>
</div>
<div>This works as long as config is
server-wide, but not if there are config
per-realm or even multiple different instances
per-realm. This applies to for example User
Federation SPI (multiple per-realm), Password
Hashing SPI (one per-realm), etc.</div>
<div><br>
</div>
<div>Currently the User Federation SPI creates
and manages instances outside of the session
factory and session, which results in multiple
instances created per-request, not all being
closed properly, etc..</div>
<div><br>
</div>
<div>With that in mind I'd like to change the
provider factories so that there can be
multiple provider factory instances. It's not
completely figured out, but I wanted to
discuss it before I start a POC around it.</div>
<div><br>
</div>
<div>We'd have the following methods on
KeycloakSession:</div>
<div><br>
</div>
<div>* getProvider(Class<T> clazz,
Provider.class) - returns default provider<br>
</div>
<div>* getProvider(Class<T> clazz,
Provider.class, String providerId) - returns a
specific provider, with the default config<br>
</div>
<div>* getProvider(Class<T> clazz,
Provider.class, String providerId, String
instanceId) - returns a specific provider,
with the specific config<br>
</div>
<div><br>
</div>
<div>We'd also add a method:</div>
<div><br>
</div>
<div>* invalidateProvider(Class<T> clazz,
Provider.class, String providerId, String
instanceId) - this would be called when the
config for a specific provider instance is
updated</div>
<div><br>
</div>
<div>Behind the covers the instances would be
maintained. Each provider factory would
internally be responsible to retrieve config
and cache config for instances.</div>
<div><br>
</div>
<div>Does this sound like an idea worth
pursuing? I'd like to try it out on the
PasswordPolicy SPI first.</div>
</div>
<br>
<fieldset></fieldset>
<br>
</div>
</div>
<pre>_______________________________________________
keycloak-dev mailing list
<a moz-do-not-send="true" href="mailto:keycloak-dev@lists.jboss.org" target="_blank">keycloak-dev@lists.jboss.org</a>
<a moz-do-not-send="true" 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>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>