<div dir="ltr">UserFederationManager hides the complexity of loading providers directly. However, that's not always the case. For example if we implement some feature that allows displaying only users from a specific provider in the admin console.<div><br></div><div>It needs to be a generic mechanism though and not all providers are loaded/used through a manager either. For example identity providers. For those you just need to get the correct provider and don't need model when dealing with callbacks, etc..</div><div><br></div><div>As I said not all providers will want to use the model at all. It may very well be that someone wants to implement a custom provider that stores the config differently. In which case if you inject the config that's not possible anymore.</div><div><br></div><div>It also complicates if someone wants to use a provider from their own custom provider. They would have to know how to load the config first to get the provider.</div><div><br></div><div>If you in the future refactor the config you now need to change everywhere that uses it as well, not just the one place that creates the provider.</div><div><br></div><div>For the record this is not something new and as I said it's basic principles of dependency injection (although we don't support injection, it's pretty similar). You shouldn't need to know how to construct something to be able to use it. </div></div><div class="gmail_extra"><br><div class="gmail_quote">On 24 June 2016 at 06:21, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div class="h5">
<div>On 23/06/16 15:40, Stian Thorgersen
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On 23 June 2016 at 15:33, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span>
<div>On 23/06/16 14:28, Stian Thorgersen wrote:<br>
</div>
<blockquote 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 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">
<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>
</span> 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.<span><br>
<blockquote 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>
</span> 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.</div>
</blockquote>
<div><br>
</div>
<div>Not sure why a provider should be scoped per-client. A
ResourceServer in either case it's an internal thing and
there should be a getResourceServer(ClientModel client)
rather than getProvider(ResourceServer..). Not sure what
the code does now though.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span><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">
<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>
</span> 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.</div>
</blockquote>
<div><br>
</div>
<div>It's basic principals of dependency injection and
loosely coupling to not require knowing how to create
something to be able to use it. I strongly disagree that
the calling code needs to know how to load the config.
There are multiple places that may need to use a provider,
which would then require all those places to be able to
load the config. Further, not all providers may want to
use the model directly. If it's up to the factory itself
to load the config the config can be located elsewhere.</div>
</div>
</div>
</div>
</blockquote></div></div>
Sure, the caller shouldn't be required to know how to create
provider, it's the responsibility of factory. But the caller should
know what it wants. And calling code (Keycloak) always knows model
in all cases I can think of.<br>
<br>
For example, in case of UserFederation the workflow is like:<br>
- Admin creates some user federation provider (eg. ldap) in admin
console. The model is saved to Keycloak db<br>
- Then some user "john" wants to login.<br>
- Keycloak (UserFederationManager) needs to load all the
UserFederationProviderModel from DB as the providerId is saved on
the model. Then when it knows the providerId is "ldap", it can
finally call session.getProvider and instantiate provider based on
that.<br>
<br>
Similarly when some user has "federationLink" on him, it points to
the ID of UserFederationProviderModel. So you first need to load
this model to know the providerId. <br>
<br>
It's very similar to IdentityProvider. When you click on login
screen to "Login with 3rdPartyOidc", Keycloak first needs to load
the IdentityProviderModel by alias "3rd-party-oidc" and then see
that providerId is "oidc", so it can call session.getProvider to
retrieve the IdentityProvider implementation ( OIDCIdentityPRovider
).<br>
<br>
We have the generic mechanism to create provider models in admin
console and the key/value pairs, which are saved as part of model.
People are not required to use this if their implementation doesn't
need any metadata configured through keycloak admin console (eg.
someone can store all metadata about their UserFederation provider
implementation into their private DB or into known property file in
filesystem etc), and then they don't need model. But then they also
don't need instanceId.<br>
<br>
Honestly ATM, I can't see any benefit in having "String instanceId"
instead of "ProviderModel providerModel" . Just one disadvantage
that factory would always need to download model, which the caller
already knows. However, maybe if you do the POC based on instanceId,
I will see that I am wrong...;)<span class="HOEnZb"><font color="#888888"><br>
<br>
Marek
</font></span><span class=""><blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span><font color="#888888"><br>
<br>
Marek</font></span><span><br>
<blockquote 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><br>
<br>
On 23/06/16 12:01, Stian Thorgersen
wrote:<br>
</div>
</div>
</div>
<blockquote type="cite">
<div>
<div>
<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 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>
<br>
</div>
</div>
</blockquote>
<br>
</span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</span></div>
</blockquote></div><br></div>