[keycloak-dev] User Federation Provider Cache

[Bill Burke]

On 6/16/16 5:23 AM, Marek Posolda wrote:
> On 15/06/16 17:33, Bill Burke wrote:
>> This redesign is quite complex as you have to take a lot of variables
>> into account.  I also am trying to make things backward compatible and
>> have the new SPI co-exist with the old. Just remember though, the
>> biggest reason for this rewrite is to AVOID IMPORTING!
>>
>> Here's what I'm currently doing:
>>
>> Federated Data
>> --------------
>> There will be a built-in federated user data service.  It exists to
>> store additional data about a user, for example:
>>
>> public interface UserAttributeFederatedStorage {
>>     void setSingleAttribute(RealmModel realm, UserModel user, String
>> name, String value);
>>     void setAttribute(RealmModel realm, UserModel user, String name,
>> List<String> values);
>>     void removeAttribute(RealmModel realm, UserModel user, String name);
>>     MultivaluedHashMap<String, String> getAttributes(RealmModel realm,
>> UserModel user);
>> }
>>
>> There will be federated storage options for all data.
>>
>> User Storage Provider SPI
>> -----------------
>> Current UserProvider is going to be split up into smaller interface.
>> SPI will remain the same though.  There is going to be a
>> UserStorageProvider SPI.  Instances can implement one or more of the
>> smaller UserProvider interfaces.
>>
>> User Storage Provider instances will be returning UserModel instances.
>> This means providers will be responsible for implementing some sort of
>> UserAdapter implementation.  These custom UserAdapters will be
>> responsible for interacting with the Federated Data Store and their
>> custom storage:  Merging data between i.e. LDAP and the Federated
>> Storage, managing which store stores what data, managing what data
>> lives where.  Going to provide a helper abstract UserAdapter class
>> that helps managing this data.
> Ok. Am I understand correctly that "smaller" providers are not limited
> to be either fully-LDAP based or fully-localDB based? For example I want
> "attribute-a" of user "john" to be stored in LDAP, however "attribute-b"
> of same user will be stored in local DB. From what you wrote, I assume
> it will work as UserAdapter can delegate to other store if it doesn't
> find "attribute-b" in LDAP?
>
>
> Adding again the scenario with requiredActions, which we currently
> support and which we briefly discussed on F2F. I am adding it just to
> ensure that we won't loose the support for this :)
> - Password in MSAD is expired
> - Keycloak reads some corresponding attribute from MSAD and it sets
> UPDATE_PASSWORD requiredAction on the user based on it
> - Keycloak asks user to update password and this is then propagated to MSAD
>
> In other words, just the UPDATE_PASSWORD requiredAction is handled by
> MSAD, but all the other required actions need to be handled by Keycloak
> local DB.
>

Yes that will work.  Because the provider is providing the UserAdapter, 
it has control over how things are pulled together.  I also intend to 
eventually allow federated-federated-storage.  So, you could lookup from 
LDAP, but gather attributes/role-mappings/etc. from another LDAP store, 
Keycloak, or other places.

>>>
>>> Persistent vs. Transient modes
>>> ----------------------------------------
>>>
>>> I think we should still keep some support for "persistent" mode (the
>>> case when user is imported to Keycloak local DB).
>>>
>>
>> I want to get rid of import and only have it as an option for
>> Brokering or for transient users (users that can't be looked up from a
>> store out of band).  The LDAP adapter will be responsible for knowing
>> what data is stored in LDAP and what data is stored in the Federated
>> Store, so there is no need to "import"
> Ok, so LDAP data are not imported, however the non-LDAP data (or
> overwritten data) are read from local store right? So we still support
> the scenario with overwriting data (unsynced mode) like this?
>
> - User from LDAP authenticates and his pasword is verified against LDAP
> - User changes his password, but LDAP is read-only, so new password
> needs to be written to local DB
> - User authenticates again, but now his password needs to be verified
> against local DB
>

We can provide some default behavior from the UserAdatper helper 
abstract class, but the provider is accountable for merging data.

>>>
>>> Caching
>>> -----------
>>>
>>> I hope new SPI will allow us more flexibility to support various
>>> scenarios. It will be good to support at least those IMO:
>>>
>>
>> See above.  LDAP users will be cached in same way JPA Users currently
>> are.
> So if I understand correctly,we will loose the option (1), which we
> currently support like:
>
> LDAP user data are always read from "online" LDAP in every request, but
> all other user data are cached, so no need to read them from local
> Keycloak DB (even for many hours).
>

Yeah, no more on-demand.  Either you cache or you don't.

> In other words, we will have option to use different expiration times
> (ie. LDAP user is expired from cache every 5 minutes, when fully-local
> user is expired every 2 hours), however you will need to read each 5
> minutes all the data of user "john" from both LDAP and local-db, right?
>
> However loosing this one is probably not so big issue....

The user cache will see one UserModel and load from there.  So, it 
caches LDAP and any other federated storage for the user that the 
UserModel provides.



>>
>>> 3) I agree that will be cool to support different expiration time based
>>> if it's LDAP user or just Keycloak-local user. Infinispan allows it
>>> though with something like : cache.put("ldapuser", ldapUser, 60,
>>> TimeUnit.SECONDS);
>>>
>>
>> I think this will be an option that we will want to support.
>>
>>>
>>>
>>> Transactions and 3rd party updates
>>> -----------------------------------------------
>>>
>>> - Will be good to improve registration of user to LDAP. Ideally during
>>> registration new user to LDAP, we should allow to send all data at once.
>>> (currently UserFederationProvider.register supports sending just
>>> username). Also we should allow to specify if register to 3rd party
>>> provider should be done *before* or *after* the registration to Keycloak
>>> local DB. For details, see https://issues.jboss.org/browse/KEYCLOAK-1075
>>> and all the comments from users...
>>>
>>> - Also updating user should be ideally at once. For example if you call:
>>> user.setFirstName("john");
>>> user.setLastName("Doe");
>>> user.setEmail("john at email.cz");
>>>
>>> we shouldn't have 3 update calls to LDAP, but just one. Maybe we can
>>> address this with transaction abstraction? I've already did something
>>> for LDAP provider (see TxAwareLDAPUserModelDelegate ), however will be
>>> good to provide something more generic for user storage SPI. Then when
>>> KeycloakTransactionManager.commit is called, the data are send to
>>> federation storage
>>>
>>
>> This would be an implementation detail.  Similar to JPA Users, the
>> LDAP UserAdapter will be remembered per session.  The LDAP adapter
>> could queue up updates then at commit time flush them all.
>>
>> Maybe you should consider writing an LDAP version of JPA?  ;-)
>> Probably a lot of code could be borrowed from PL IDM API.
> I've wrote already something similar for Mongo ;) Also I already
> addressed the multiple-updates issue for LDAP (see above). However this
> one is mainly issue for 3rd party providers rather than for LDAP
> provider. Especially since 3rd party provider may have some validations
> for some attributes.
>
>
> For example consider this scenario, which can happen in current
> implementation:
>
> UserModel john = session.users().addUser(realm, "john");
> john.setEmail("john at email.org");
>
> All of this happens during "addUser" call:
>
> - User "john" is saved to local-db
> - SomeUserFederationProvider.register calls the registration request to
> 3rd party federation storage. User "john" is saved in 3rd party storage
> now.
>
> Then john.setEmail is invoked
> - There is request to 3rd party federation storage to update email.
> - 3rd party storage refuse to save the email because there is already an
> existing user with email "john at email.org" . So it sends back the
> validation error. Now keycloak can rollback the transaction, however
> user "john" was already registered in 3rd party federation storage from
> previous "register" call -> FAIL
>
> So now you need to manually remove the user from 3rd party storage,
> which is dirty hack. There are other possible hacks to address this. For
> example someone "postpone" the registration in "register" method by
> adding some helper attributes and sends the registration request later
> when all the attributes are set. I suggest to read the comments in
> https://issues.jboss.org/browse/KEYCLOAK-1075 for details.
>
> IMO the proper solution is to ensure that "register" request to 3rd
> party is postponed until the transaction.commit when user is filled with
> all the attributes. Similarly update should be done just once. IMO we
> should provide something at SPI level to simplify this scenario.
>

We already have a KeycloakTransaction you can register with.

> I understand that main motivation for you is to avoid "imports", however
> since we are refactoring anyway, we should try to address other SPI
> limitations too. And IMO this one is pretty bad and worth improve ;)

So, have a method addUser(RealmModel realm, UserEntity user)?

>>
>>
>>
>>>
>>> Sync users
>>> --------------
>>> We should still keep the option to sync users into Keycloak DB as we
>>> have now. Note some persistent storages like LDAP are limited with
>>> pagination. So the easiest possibility for some admins is just to sync
>>> users, so they can easily search them in admin console.
>>>
>> Doing a full import just to support pagination is overkill.  I'm
>> guessing that a lot of deployments will not manage users through the
>> Keycload admin console.  We can offer a manual a Sync SPI that
>> providers can implement.
> Maybe it's overkill for 90% of deployments, but remaining 10% want to
> see all LDAP users in admin console immediatelly and hence want to sync
> them? IMO it's always good to have SPI flexible so it can easily support
> all the possible requirements. However I understand that it's not always
> possible...
>

This is only an issue for large query result sets where you want to do 
pagination.  IIRC, we couldn't figure out a way to do this in a scalable 
manner without imports.