[keycloak-dev] in-memory only federated users

[Marek Posolda]

On 04/12/15 11:36, Marek Posolda wrote:
> Why it's bad to do simpler things? :-)
>
> 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.
>
> 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.
>
>
> As I mentioned before, there are also 2 additional usecases, which are 
> important to support IMO:
>
> 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.
>
> 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
>
>
> 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.
>
>
> For my proposal, I assume that UserProvider will have just 2 new methods:
>
> UserProvider getNext();
> void setNext(UserProvider next);
>
>
> 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.
>
> 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:
> userFederationManager => inMemory
>
> Then assume the session.users().getUserByUsername is called:
> 1) First delegate is UserFederationManager, so calling 
> UserFederationManager.getUserByUsername
> 2) This line 
> https://github.com/keycloak/keycloak/blob/master/model/api/src/main/java/org/keycloak/models/UserFederationManager.java#L180 
> will call getNext().getUserByUsername() and returns null as the user 
> was not yet looked  for this request.
> 3) Going to federationProviders and call 
> LDAPFederationProvider.getUserByUsername
> 4) LDAPFederationProvider query user in LDAP and calls 
> importUserFromLDAP . This calls session.userStorage().addUser, 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.
>
> 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.
>
> 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:
> cache => federationManager => inMemory
>
> it will work fine and find all users retrieved from LDAP in any 
> previous requests.
>
>
> In summary: UserProvider chaining is:
>
> 1) Very flexible
>
> 2) Supports in-memory, but also other use-cases too. It's all up to 
> admin preference how to configure chain
>
> 3) No dependencies of providers on each other
>
> 4) Minimal changes to UserFederationManager and 
> DefaultCacheUserProvider . Just need to call "getNext()" to retrieve 
> next provider
>
> 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.
6) No need to increase the size of ID column for user table :-)


>
> Marek
>
>
> On 03/12/15 18:58, Bill Burke wrote:
>> 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.
>>
>> My current thinking is that we change the flow from:
>>
>> UserFederationManager=>CacheProvider=>UserProvider
>>
>> to
>>
>> CacheProvider->UserFederationManager->UserProvider/UserFederationProvider 
>>
>>
>> KeycloakSession changes:
>>
>> * users() returns the CacheUserProvider instead of UserFederationManager
>> * userStorage() is not changed
>> * federationManager() returns UserFederationManager
>>
>> UserCacheProvider changes:
>>
>> * Gets a new method cache(UserModel user);
>> * References UserFederationManager instead of the DB provider directly
>>
>> UserFederationManager changes:
>> * Instead of calling userStorage(), it gets the DB provider directly
>>
>> UserFederationProvider:
>> * Imports using userStorage() or, allocates a new class 
>> InMemoryUserModel (or extension of that class).  This class is just 
>> an in memory implementation of UserModel
>> * Returns the imported UserModel or the InMemoryUserModel
>>
>> So
>>
>> session.users().getUserByXXXX() does the folloing:
>>
>> 1. UserCacheProvider.getUserByXXX is invoked
>> 2. It looks looks in cache, if found return
>> 3. invoke UserFederationManager.getUserByXXX
>> 4. If UserFederationManager finds it return UserModel
>> 5. Look in UserFederationProvider
>> 6. UserFederationProvider imports or returns InMemoryUserModel
>> 7. UserCacheProvider.getUserByXXX caches the user.
>>
>> cache.UserAdapter.getDelegateForUpdate() does the following:
>> 1. invokes UserFederationManager.getUserById()
>> 2. ID is parsed to see if it is a federated ID or not (see original 
>> post)
>>
>> Alternatively, we could just invoke 
>> userFederationManager.getUserByusername() since username can't be null.
>>
>>
>>
>> On 12/3/2015 11:59 AM, Marek Posolda wrote:
>>> On 03/12/15 16:57, Bill Burke wrote:
>>>> Either we redo the federation SPI or work with the current one.
>>>>
>>>> It is just not as simple as you state.  You can't just chain in a
>>>> generic InMemoryProvider.  Federation needs to be able to proxy the
>>>> UserModel so that it can handle write methods if it wants to. Or
>>>> delegate lookup of certain things to LDAP.
>>> I am not seeing why it's an issue? The InMemory will be kind of same
>>> thing like currently JPA. It just won't store the things into database,
>>> but into memory. That's the only difference. It will just be the
>>> provider at the end of the chain. UserFederationManager can proxy users
>>> exactly like now and doesn't require any code changes.
>>>
>>> So when admin configure:
>>>
>>> userFederationMAnager => inMemory
>>>
>>> The call of "session.userStorage()" from UserFederationManager will
>>> return underlying InMemory instead of current JPA.
>>>
>>>> Also, UserFederationManager has to be first in the chain so that if
>>>> something is found in cache, it can let the federation provider proxy
>>>> the cache if it wants to.
>>> That's not a problem as well.
>>>
>>> What I mean is the flexibility to configure things exactly how you want
>>> for various cases.
>>>
>>> 3 basic setups:
>>>
>>> 1. userFederation => cache => JPA
>>>
>>> This is what we have now and it will be the default setup.  It 's 
>>> useful
>>> for deployments when admins are often doing changes directly in their
>>> LDAP and they want the change imediatelly visible in Keycloak. So the
>>> UserFederationProvider proxy is always the top and when you call:
>>>
>>> user.getFirstName()
>>>
>>> you will retrieve the firstName from LDAP. The disadvantage of this
>>> setup is performance (each HTTP request needs to query LDAP like 
>>> it's now)
>>>
>>>
>>> 2. cache => userFederation => JPA
>>>
>>> This will be useful for deployments when amins are not doing changes
>>> directly in their LDAP. So once you retrieve the user from
>>> LDAP+KeycloakDB, you will save him to cache and call to:
>>>
>>> user.getFirstName()
>>>
>>> will always return the value from cache. Hence when admin changes
>>> directly in LDAP, it won't be immediately visible in Keycloak.
>>>
>>> But on the other hand update from Keycloak to LDAP is not an issue. 
>>> When
>>> you call:
>>>
>>> user.setFirstName("foo")
>>>
>>> the cache will call getDelegateForUpdate (exactly like now) and it will
>>> return proxy object, so the saving of firstName is propagated to LDAP
>>> (if it's writable) and to Keycloak JPA DB as well.
>>>
>>>
>>> 3. userFederation => inMemory
>>>
>>> The federation backed by pure in-memory storage. The federation 
>>> proxy is
>>> on top, writing and reading to/from LDAP is not a problem and has
>>> preference over the content from memory. The only difference from 
>>> (1) is
>>> that underlying backend is pure memory (infinispan) instead of JPA DB
>>>
>>> There is also alternative to use combination of 2 and 3:
>>> cache => userFederation => inMemory
>>>
>>> etc etc.
>>>
>>>
>>> I can see this as most flexible approach without dependencies of 
>>> various
>>> providers on each other.
>>>
>>> Marek
>>>>
>>>> What we need is a special interface for the cache:
>>>>
>>>> cache.cacheUser(UserModel user);
>>>>
>>>> The cache would also work with UserFederationManager rather than a
>>>> generic UserProvider. UserFederationManager would gain methods like:
>>>> UserFederationManager.getUncachedUserById() which the cache would
>>>> invoke.  UserFederationManager would break down the user id and either
>>>> know it was local storage or something that would have to be delegated
>>>> to a UserProvider.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 12/3/2015 10:32 AM, Marek Posolda wrote:
>>>>> IMO the more important use-case that in-memory federated users is the
>>>>> caching of federated users.
>>>>>
>>>>> Currently if you call: session.users().getUserById() and the user 
>>>>> with
>>>>> ID "123" is LDAP (or other federationProvider) user, there is always
>>>>> call to UserFederationProvider.validateAndProxy , which results in 
>>>>> LDAP
>>>>> query.
>>>>>
>>>>> If we introduce the chaining of UserProvider (something I already
>>>>> proposed earlier), you can switch UserFederationProvider with 
>>>>> cache, so
>>>>> you will have:
>>>>> cache => userFederationManager => JPA
>>>>>
>>>>> instead of current:
>>>>>
>>>>> userFederationManager => cache => JPA
>>>>>
>>>>>
>>>>> With that in mind, we can easily implement in-memory as another
>>>>> implementation of UserProvider, which will hold users purely 
>>>>> in-memory.
>>>>> Our current DefaultCacheUserProvider always require delegate to call
>>>>> write operations. But this in-memory provider would be something
>>>>> different. It won't use any delegate as it will be in the end of the
>>>>> chain. So for in-memory federation you will just configure:
>>>>>
>>>>> userFederationManager => inMemoryProvider
>>>>>
>>>>> and you're done. No needs for special ID handling or something like
>>>>> that.
>>>>>
>>>>> With chaining of UserProvider we have biggest flexibility for various
>>>>> needs IMO. That's why I would rather go this way TBH.
>>>>> Marek
>>>>>
>>>>> On 02/12/15 17:48, Bill Burke wrote:
>>>>>> I'm looking into in-memory only no-import federated users.  What we
>>>>>> would want to do is allow the UserFederationProvider to create an
>>>>>> in-memory UserModel and allow for that UserModel to be cached via 
>>>>>> our
>>>>>> current architecture.
>>>>>>
>>>>>> The current design assumes that all federated users are 
>>>>>> imported.  This
>>>>>> includes our caching layer too!  To add to that, the user isn't 
>>>>>> cached
>>>>>> until the 2nd request i.e.:
>>>>>>
>>>>>> 1. username/password page would hit the UserFederationProvider 
>>>>>> and the
>>>>>> user would be imported into Keycloak.  This imported user is not
>>>>>> cached,
>>>>>> only imported into the database for this request's KeycloakSession
>>>>>> 2. OTP Page or code 2 token would then want to lookup the user by 
>>>>>> id as
>>>>>> that is what is stored in the ClientSession.  It would hit the 
>>>>>> keycloak
>>>>>> database as it is not cached yet.  This lookup loads the cache for
>>>>>> the user.
>>>>>>
>>>>>> Getting in-memory zero-import to work is even more tricky. The 
>>>>>> issue is
>>>>>> that ClientSession and UserSession need to lookup clients by id.  If
>>>>>> the
>>>>>> user is not in cache, then the cache needs to lookup the user by id
>>>>>> within storage.  This lookup also needs to happen if a write 
>>>>>> operation
>>>>>> is performed on a cache user (getDelegateForUpdate()). So, Keycloak
>>>>>> needs to know that that ID is not in local storage and must be
>>>>>> looked up
>>>>>> from a fed provider.  The ID must be formed so that the provider fed
>>>>>> provider can resolve the lookup.  I could use a URI for the ID i.e.
>>>>>>
>>>>>> fed:{providerId}:{login-name}
>>>>>>
>>>>>> The problem with this is that this id would need to be larger 
>>>>>> than 36
>>>>>> characters which is the current column size for UserEntity.id and 
>>>>>> any
>>>>>> other table that references users.   I could possibly do:
>>>>>>
>>>>>> fed:{providerAlias}:{login-name}
>>>>>>
>>>>>> But its quite possible that combination would be larger than 36
>>>>>> characters.  We could also just shrink it to:
>>>>>>
>>>>>> fed:{login-name}
>>>>>>
>>>>>> But then we would have to iterate over every federation provider to
>>>>>> find
>>>>>> and load the user.
>>>>>>
>>>>>> So in summary:
>>>>>> * IDs need to expand from 36 characters to something larger. (255
>>>>>> maybe).  Don't some DBs have constraints on string primary key
>>>>>> size?  DB
>>>>>> scripts could possibly be
>>>>>> * CachedUserProvider and UserFederationManager interfaces would 
>>>>>> need to
>>>>>> be refactored
>>>>>> * I don't think UserFederationProvider interface would need to 
>>>>>> change.
>>>>>> But users would have to code for in-memory rather than throwing a
>>>>>> switch
>>>>>> to just turn it on.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20151204/783d73ae/attachment-0001.html