<div dir="ltr">What about splitting the federation logic from the store logic?<div><br></div><div>We'd have multiple user store implementations:</div><div><br></div><div>* JPA</div><div>* Mongo</div><div>* LDAP</div><div>* Maybe something similar to PL IDM to allow people to easily use their own schema for users</div><div><br></div><div>We could have store mappers as well in a mapping layer.</div><div><br></div><div>This would allow users to configure the user store for a specific realm. So they can store everything about users in LDAP and not have to worry about federation.</div><div><br></div><div>Then we'd have separate federation providers which could be re-used for different store types. So it would look something like:</div><div><br></div><div>cache -> federation -> mappers -> store</div><div><br></div><div>In theory you could even use that to share users between multiple realms. Have multiple separate sets of users in different dbs, ldap, etc..</div><div><br></div><div>I think it's an idea worth considering?</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 4 December 2015 at 11:41, 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 text="#000000" bgcolor="#FFFFFF"><div><div class="h5">
<div>On 04/12/15 11:36, Marek Posolda wrote:<br>
</div>
<blockquote type="cite">
<div>Why it's bad to do simpler things? <span><span> :-) </span></span><br>
<br>
AFAIK filter pattern (or interceptor/chain whatever you call it)
is proven to work in many places. The provider at level X can
always decide if it delegates call to method "getUserByXXX" to
next provider (and then proxy/cache or do whatever according to
his logic) or return something by itself.<br>
<br>
If I understand correctly your proposal, it requires
UserFederationProvider to decide, if it wants to import or just
return InMemoryUserModel . So if we want to support in-memory
for LDAP, we will need to have 2 versions of
LDAPFederationProvider (current which imports into userStorage()
and another, which will return InMemoryUserModel instances ).
That's not ideal IMO.<br>
<br>
<br>
As I mentioned before, there are also 2 additional usecases,
which are important to support IMO: <br>
<br>
1) Case when admin changes some user attributes directly in LDAP
and he wants the LDAP data to be immediately visible in
Keycloak. This is what we currently support (see
FederationProvidersIntegrationTest.testDirectLDAPUpdate() ).
Maybe I am missing something with your proposal, but if we
hardcode CacheProvider to be always first, we lost this.<br>
<br>
2) Case when admin doesn't change user attributes in LDAP
directly, but rather prefer to save performance and read data
from cache. In this case, admin configures the chain like you
proposed: CacheProvider => UserFederationManager =>
UserProvider<br>
<br>
<br>
IMO it will be cool to have single implementation of
LDAPFederationProvider (and others), which works for both those
cases and also for in-memory at the same time. Just let admin to
decide how he want to configure chain of UserProviders, but not
require UserFederationProvider itself to care about it.<br>
<br>
<br>
For my proposal, I assume that UserProvider will have just 2 new
methods:<br>
<br>
UserProvider getNext();<br>
void setNext(UserProvider next);<br>
<br>
<br>
Only change in current UserFederationManager and
DefaultCacheUserProvider is, that they will call "getNext()"
when they need delegate. They don't care about what the delegate
actually is, that's not their responsibility.<br>
<br>
For the "in-memory" provider, it might be the per-request
in-memory provider (users stored just per-request). So if you
have chain like:<br>
userFederationManager => inMemory <br>
<br>
Then assume the session.users().getUserByUsername is called:<br>
1) First delegate is UserFederationManager, so calling
UserFederationManager.getUserByUsername<br>
2) This line <a href="https://github.com/keycloak/keycloak/blob/master/model/api/src/main/java/org/keycloak/models/UserFederationManager.java#L180" target="_blank">https://github.com/keycloak/keycloak/blob/master/model/api/src/main/java/org/keycloak/models/UserFederationManager.java#L180</a>
will call getNext().getUserByUsername() and returns null as the
user was not yet looked for this request.<br>
3) Going to federationProviders and call
LDAPFederationProvider.getUserByUsername<br>
4) LDAPFederationProvider query user in LDAP and calls <span style="background-color:#e4e4ff">importUserFromLDAP . This
calls </span>session.userStorage().<span style="background-color:#e4e4ff">addUser</span><span style="background-color:#e4e4ff"></span>, which will put user
into in-memory provider (I assume that session.userStorage()
will be kept and will always point to the next delegate after
UserFederationManager ). The LDAPFederationProvider will then
return LDAP proxy of UserModel.<br>
<br>
The in-memory provider will also work with searching (
searchUser ) as federationLoad will first pre-load users into
in-memory and then calls "query" and proxy users. <br>
<br>
The only limitation I can see now is calling of
session.users().getUsers() as this doesn't preload users from
federation. But if people add cache and use chain like:<br>
cache => federationManager => inMemory<br>
<br>
it will work fine and find all users retrieved from LDAP in any
previous requests.<br>
<br>
<br>
In summary: UserProvider chaining is:<br>
<br>
1) Very flexible<br>
<br>
2) Supports in-memory, but also other use-cases too. It's all up
to admin preference how to configure chain<br>
<br>
3) No dependencies of providers on each other<br>
<br>
4) Minimal changes to UserFederationManager and
DefaultCacheUserProvider . Just need to call "getNext()" to
retrieve next provider<br>
<br>
5) Current UserFederationProvider will work fine for all cases
and automatically gains "in-memory" support without need to
change anything in their code. Assuming that for backwards
compatibility, we will keep "session.userStorage()" to always
point to next delegate of UserFederationManager . If it's JPA,
then imports user like now. If it will be "in-memory" it will
just return cache user for this request and return per-request
inMemory user.<br>
</div>
</blockquote></div></div>
6) No need to increase the size of ID column for user table <span><span> :-) </span></span><br>
<br>
<br>
<blockquote type="cite"><div><div class="h5">
<div> <br>
Marek<br>
<br>
<br>
On 03/12/15 18:58, Bill Burke wrote:<br>
</div>
<blockquote type="cite">Still
don't think it is as simple as you state. We don't need an "in
memory" provider. We want UserFederationProvider to create a
temporary request/only in-memory UserModel for federation
providers that don't want to import. This UserModel may be
proxied for any write operations. <br>
<br>
My current thinking is that we change the flow from: <br>
<br>
UserFederationManager=>CacheProvider=>UserProvider <br>
<br>
to <br>
<br>
CacheProvider->UserFederationManager->UserProvider/UserFederationProvider
<br>
<br>
KeycloakSession changes: <br>
<br>
* users() returns the CacheUserProvider instead of
UserFederationManager <br>
* userStorage() is not changed <br>
* federationManager() returns UserFederationManager <br>
<br>
UserCacheProvider changes: <br>
<br>
* Gets a new method cache(UserModel user); <br>
* References UserFederationManager instead of the DB provider
directly <br>
<br>
UserFederationManager changes: <br>
* Instead of calling userStorage(), it gets the DB provider
directly <br>
<br>
UserFederationProvider: <br>
* Imports using userStorage() or, allocates a new class
InMemoryUserModel (or extension of that class). This class is
just an in memory implementation of UserModel <br>
* Returns the imported UserModel or the InMemoryUserModel <br>
<br>
So <br>
<br>
session.users().getUserByXXXX() does the folloing: <br>
<br>
1. UserCacheProvider.getUserByXXX is invoked <br>
2. It looks looks in cache, if found return <br>
3. invoke UserFederationManager.getUserByXXX <br>
4. If UserFederationManager finds it return UserModel <br>
5. Look in UserFederationProvider <br>
6. UserFederationProvider imports or returns InMemoryUserModel <br>
7. UserCacheProvider.getUserByXXX caches the user. <br>
<br>
cache.UserAdapter.getDelegateForUpdate() does the following: <br>
1. invokes UserFederationManager.getUserById() <br>
2. ID is parsed to see if it is a federated ID or not (see
original post) <br>
<br>
Alternatively, we could just invoke
userFederationManager.getUserByusername() since username can't
be null. <br>
<br>
<br>
<br>
On 12/3/2015 11:59 AM, Marek Posolda wrote: <br>
<blockquote type="cite">On 03/12/15 16:57, Bill Burke wrote: <br>
<blockquote type="cite">Either we redo the federation SPI or
work with the current one. <br>
<br>
It is just not as simple as you state. You can't just chain
in a <br>
generic InMemoryProvider. Federation needs to be able to
proxy the <br>
UserModel so that it can handle write methods if it wants
to. Or <br>
delegate lookup of certain things to LDAP. <br>
</blockquote>
I am not seeing why it's an issue? The InMemory will be kind
of same <br>
thing like currently JPA. It just won't store the things into
database, <br>
but into memory. That's the only difference. It will just be
the <br>
provider at the end of the chain. UserFederationManager can
proxy users <br>
exactly like now and doesn't require any code changes. <br>
<br>
So when admin configure: <br>
<br>
userFederationMAnager => inMemory <br>
<br>
The call of "session.userStorage()" from UserFederationManager
will <br>
return underlying InMemory instead of current JPA. <br>
<br>
<blockquote type="cite">Also, UserFederationManager has to be
first in the chain so that if <br>
something is found in cache, it can let the federation
provider proxy <br>
the cache if it wants to. <br>
</blockquote>
That's not a problem as well. <br>
<br>
What I mean is the flexibility to configure things exactly how
you want <br>
for various cases. <br>
<br>
3 basic setups: <br>
<br>
1. userFederation => cache => JPA <br>
<br>
This is what we have now and it will be the default setup. It
's useful <br>
for deployments when admins are often doing changes directly
in their <br>
LDAP and they want the change imediatelly visible in Keycloak.
So the <br>
UserFederationProvider proxy is always the top and when you
call: <br>
<br>
user.getFirstName() <br>
<br>
you will retrieve the firstName from LDAP. The disadvantage of
this <br>
setup is performance (each HTTP request needs to query LDAP
like it's now) <br>
<br>
<br>
2. cache => userFederation => JPA <br>
<br>
This will be useful for deployments when amins are not doing
changes <br>
directly in their LDAP. So once you retrieve the user from <br>
LDAP+KeycloakDB, you will save him to cache and call to: <br>
<br>
user.getFirstName() <br>
<br>
will always return the value from cache. Hence when admin
changes <br>
directly in LDAP, it won't be immediately visible in Keycloak.
<br>
<br>
But on the other hand update from Keycloak to LDAP is not an
issue. When <br>
you call: <br>
<br>
user.setFirstName("foo") <br>
<br>
the cache will call getDelegateForUpdate (exactly like now)
and it will <br>
return proxy object, so the saving of firstName is propagated
to LDAP <br>
(if it's writable) and to Keycloak JPA DB as well. <br>
<br>
<br>
3. userFederation => inMemory <br>
<br>
The federation backed by pure in-memory storage. The
federation proxy is <br>
on top, writing and reading to/from LDAP is not a problem and
has <br>
preference over the content from memory. The only difference
from (1) is <br>
that underlying backend is pure memory (infinispan) instead of
JPA DB <br>
<br>
There is also alternative to use combination of 2 and 3: <br>
cache => userFederation => inMemory <br>
<br>
etc etc. <br>
<br>
<br>
I can see this as most flexible approach without dependencies
of various <br>
providers on each other. <br>
<br>
Marek <br>
<blockquote type="cite"> <br>
What we need is a special interface for the cache: <br>
<br>
cache.cacheUser(UserModel user); <br>
<br>
The cache would also work with UserFederationManager rather
than a <br>
generic UserProvider. UserFederationManager would gain
methods like: <br>
UserFederationManager.getUncachedUserById() which the cache
would <br>
invoke. UserFederationManager would break down the user id
and either <br>
know it was local storage or something that would have to be
delegated <br>
to a UserProvider. <br>
<br>
<br>
<br>
<br>
<br>
On 12/3/2015 10:32 AM, Marek Posolda wrote: <br>
<blockquote type="cite">IMO the more important use-case that
in-memory federated users is the <br>
caching of federated users. <br>
<br>
Currently if you call: session.users().getUserById() and
the user with <br>
ID "123" is LDAP (or other federationProvider) user, there
is always <br>
call to UserFederationProvider.validateAndProxy , which
results in LDAP <br>
query. <br>
<br>
If we introduce the chaining of UserProvider (something I
already <br>
proposed earlier), you can switch UserFederationProvider
with cache, so <br>
you will have: <br>
cache => userFederationManager => JPA <br>
<br>
instead of current: <br>
<br>
userFederationManager => cache => JPA <br>
<br>
<br>
With that in mind, we can easily implement in-memory as
another <br>
implementation of UserProvider, which will hold users
purely in-memory. <br>
Our current DefaultCacheUserProvider always require
delegate to call <br>
write operations. But this in-memory provider would be
something <br>
different. It won't use any delegate as it will be in the
end of the <br>
chain. So for in-memory federation you will just
configure: <br>
<br>
userFederationManager => inMemoryProvider <br>
<br>
and you're done. No needs for special ID handling or
something like <br>
that. <br>
<br>
With chaining of UserProvider we have biggest flexibility
for various <br>
needs IMO. That's why I would rather go this way TBH. <br>
Marek <br>
<br>
On 02/12/15 17:48, Bill Burke wrote: <br>
<blockquote type="cite">I'm looking into in-memory only
no-import federated users. What we <br>
would want to do is allow the UserFederationProvider to
create an <br>
in-memory UserModel and allow for that UserModel to be
cached via our <br>
current architecture. <br>
<br>
The current design assumes that all federated users are
imported. This <br>
includes our caching layer too! To add to that, the
user isn't cached <br>
until the 2nd request i.e.: <br>
<br>
1. username/password page would hit the
UserFederationProvider and the <br>
user would be imported into Keycloak. This imported
user is not <br>
cached, <br>
only imported into the database for this request's
KeycloakSession <br>
2. OTP Page or code 2 token would then want to lookup
the user by id as <br>
that is what is stored in the ClientSession. It would
hit the keycloak <br>
database as it is not cached yet. This lookup loads the
cache for <br>
the user. <br>
<br>
Getting in-memory zero-import to work is even more
tricky. The issue is <br>
that ClientSession and UserSession need to lookup
clients by id. If <br>
the <br>
user is not in cache, then the cache needs to lookup the
user by id <br>
within storage. This lookup also needs to happen if a
write operation <br>
is performed on a cache user (getDelegateForUpdate()).
So, Keycloak <br>
needs to know that that ID is not in local storage and
must be <br>
looked up <br>
from a fed provider. The ID must be formed so that the
provider fed <br>
provider can resolve the lookup. I could use a URI for
the ID i.e. <br>
<br>
fed:{providerId}:{login-name} <br>
<br>
The problem with this is that this id would need to be
larger than 36 <br>
characters which is the current column size for
UserEntity.id and any <br>
other table that references users. I could possibly
do: <br>
<br>
fed:{providerAlias}:{login-name} <br>
<br>
But its quite possible that combination would be larger
than 36 <br>
characters. We could also just shrink it to: <br>
<br>
fed:{login-name} <br>
<br>
But then we would have to iterate over every federation
provider to <br>
find <br>
and load the user. <br>
<br>
So in summary: <br>
* IDs need to expand from 36 characters to something
larger. (255 <br>
maybe). Don't some DBs have constraints on string
primary key <br>
size? DB <br>
scripts could possibly be <br>
* CachedUserProvider and UserFederationManager
interfaces would need to <br>
be refactored <br>
* I don't think UserFederationProvider interface would
need to change. <br>
But users would have to code for in-memory rather than
throwing a <br>
switch <br>
to just turn it on. <br>
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
<fieldset></fieldset>
<br>
</div></div><span class=""><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>
</span></blockquote>
<br>
</div>
<br>_______________________________________________<br>
keycloak-dev mailing list<br>
<a href="mailto:keycloak-dev@lists.jboss.org">keycloak-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/keycloak-dev" rel="noreferrer" target="_blank">https://lists.jboss.org/mailman/listinfo/keycloak-dev</a><br></blockquote></div><br></div>