[keycloak-dev] Thinking about a change to providers

Fri Jun 24 04:26:35 EDT 2016

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.

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..

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.

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.

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.

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.

On 24 June 2016 at 06:21, Marek Posolda <mposolda at redhat.com> wrote:

> On 23/06/16 15:40, Stian Thorgersen wrote:
>
>
>
> On 23 June 2016 at 15:33, Marek Posolda <mposolda at redhat.com> wrote:
>
>> On 23/06/16 14:28, Stian Thorgersen wrote:
>>
>>
>>
>> On 23 June 2016 at 14:19, Marek Posolda < <mposolda at redhat.com>
>> mposolda at redhat.com> wrote:
>>
>>> +1 on having "invalidateProvider" method.
>>>
>>> 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?
>>>
>>
>> Yes, I just included the two existing methods to point out that they will
>> still be there.
>>
>>
>>>
>>> We already discuss adding the "String instanceId" . Now when thinking
>>> more, it looks that it is not so convenient.
>>>
>>> When adding again UserFederation SPI as an example:
>>>
>>> - UserFederationProviderFactory needs UserFederationProviderModel to
>>> create instance of UserFederationProvider
>>> - 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>
>>> That's because to lookup UserFederationProviderModel, you first need
>>> RealmModel and then find the UserFederationProviderModel by it's ID within
>>> the realm.
>>>
>>> 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.
>>>
>>
>> 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.
>>
>> I see two options to it:
>>
>> * 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?
>>
>> 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.
>>
>> * Add RealmModel to the lookup, so it becomes:
>>   getProvider(Class<T> clazz, String providerId, RealModel realm, String
>> instanceId)
>>   That would also require a invalidateProviders(RealmModel realm) that
>> can clear all provider instances for a specific realm
>>
>> 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.
>>
>
> 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.
>
>
>>
>>
>>
>>>
>>> The logic for parse the "instanceId" and retrieve
>>> UserFederationProviderModel from DB would be boilerplate code same to all
>>> UserFederationProviderFactory impls.
>>>
>>>
>>> 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:
>>>
>>> * getProvider(Class<T> clazz, String providerId, ProviderModel  model)
>>>
>>> All the model subclasses (UserFederationProviderModel,
>>> IdentityProviderModel, PasswordPolicyModel ...) will be subclasses of
>>> ProviderModel
>>>
>>
>> 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.
>>
>> 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.
>>
>
> 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.
>
> 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.
>
> For example, in case of UserFederation the workflow is like:
> - Admin creates some user federation provider (eg. ldap) in admin console.
> The model is saved to Keycloak db
> - Then some user "john" wants to login.
> - 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.
>
> 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.
>
> 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 ).
>
> 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.
>
> 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...;)
>
> Marek
>
>
>
>>
>>
>> Marek
>>
>>
>>> Marek
>>>
>>>
>>> On 23/06/16 12:01, Stian Thorgersen wrote:
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> 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..
>>>
>>> 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.
>>>
>>> We'd have the following methods on KeycloakSession:
>>>
>>> * getProvider(Class<T> clazz, Provider.class) - returns default provider
>>> * getProvider(Class<T> clazz, Provider.class, String providerId) -
>>> returns a specific provider, with the default config
>>> * getProvider(Class<T> clazz, Provider.class, String providerId, String
>>> instanceId) - returns a specific provider, with the specific config
>>>
>>> We'd also add a method:
>>>
>>> * invalidateProvider(Class<T> clazz, Provider.class, String providerId,
>>> String instanceId) - this would be called when the config for a specific
>>> provider instance is updated
>>>
>>> Behind the covers the instances would be maintained. Each provider
>>> factory would internally be responsible to retrieve config and cache config
>>> for instances.
>>>
>>> Does this sound like an idea worth pursuing? I'd like to try it out on
>>> the PasswordPolicy SPI first.
>>>
>>>
>>> _______________________________________________
>>> keycloak-dev mailing listkeycloak-dev at lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160624/88886812/attachment-0001.html 