[keycloak-dev] Transactions WAS Re: User Federation Provider Cache

[Marek Posolda]

On 17/06/16 15:45, Bill Burke wrote:
>
>
> On 6/17/16 4:59 AM, Marek Posolda wrote:
>> On 16/06/16 16:38, Bill Burke wrote:
>>>>>> 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)?
>> I am thinking about adding some helper classes, so the people don't need
>> to directly deal with our transaction API and manually enlist
>> transactions in their code etc.
>>
>> Also it will be good to have some support for validations (aka. pseudo
>> 2-phase commit), so that all "small" userStores have possibility to
>> first validate if created/updated user is valid for them and then the
>> real update is send. This will help to avoid situation like:
>> - transaction.commit is invoked
>> - store1 successfully register the user and commits
>> - store2 rejects to register user because of some validation error (ie.
>> duplicated email)
>> - now some data of invalid user already saved in store1 -> FAIL .
>> Because at this point, whole transaction should be rollback and user
>> shouldn't be saved anywhere.
>>
>>
>> I am thinking about pseudo-code similar to this:
>>
>> public abstract class FederationTransaction implements 
>> KeycloakTransaction {
>>
>>     protected TransactionState state = TransactionState.NOT_STARTED;
>>     protected final TxUserStore userStore;
>>     protected final TxUserModel user;
>>
>>     public FederationTransaction(TxUserStore userStore, TxUserModel 
>> user) {
>>         this.userStore = userStore;
>>         this.user = user;
>>     }
>>
>>     // Implement common stuff like begin, rollback, setRollbackOnly...
>>
>> }
>>
>>
>> // This transaction just validates in 3rd party store if user is ok
>> public class ValidationTransaction extends FederationTransaction {
>>
>>     public ValidationTransaction(TxUserStore userStore, TxUserModel 
>> user) {
>>         super(userStore, user);
>>     }
>>
>>     @Override
>>     protected void commit() {
>>         userStore.validateUser(user);
>>     }
>>
>> }
>>
>>
>> // This transaction finally sends commit to 3rd party store
>> public class CommitTransaction extends FederationTransaction {
>>
>>     public CommitTransaction(TxUserStore userStore, TxUserModel user) {
>>         super(userStore, user);
>>     }
>>
>>     @Override
>>     protected void commit() {
>>         userStore.createOrUpdateUser(user);
>>     }
>>
>> }
>>
>>
>> public abstract class TxUserStore implements UserStore {
>>
>>     private final KeycloakSession session;
>>
>>     public TxUserStore(KeycloakSession session) {
>>         this.session = session;
>>     }
>>
>>     public KeycloakSession getSession() {
>>         return session;
>>     }
>>
>>     // Send request to storage to validate user
>>     protected abstract void validateUser(TxUserModel user) throws
>> FederationValidationException;
>>
>>     // Send request to create or update user during transaction.commit
>> when all stores successfully validated user
>>     protected abstract void createOrUpdateUser(TxUserModel user);
>> }
>>
>>
>>
>> public class TxUserModel extends UserModelDelegate {
>>
>>     private ValidationTransaction validationTransaction;
>>     private CommitTransaction commitTransaction;
>>
>>     private final TxUserStore store;
>>
>>     public TxUserModel(UserModel delegate, TxUserStore store) {
>>         super(delegate);
>>         this.store = store;
>>     }
>>
>>
>>     protected void ensureTransactionEnlisted() {
>>         if (commitTransaction == null) {
>>             validationTransaction = new ValidationTransaction(store, 
>> this);
>>             commitTransaction = new CommitTransaction(store, this);
>>
>>             // Ensure all validation transactions are called before
>> sending real "commit" to any storage
>>
>> store.getSession().getTransaction().enlistPrepare(validationTransaction); 
>>
>>
>>             // Enlisting real transaction here
>>
>> store.getSession().getTransaction().enlistAfterCompletion(commitTransaction); 
>>
>>         }
>>     }
>>
>> }
>>
>>
>>
>> Now in your real store implementation, you need to do just this to
>> ensure that registration of user is postponed to transaction commit:
>>
>> public UserModel register(UserModel register(RealmModel realm, UserModel
>> user) {
>>   MyTxUserModel wrappedUser = new MyTxUserModel(user, this);
>>   wrappedUser.ensureTransactionEnlisted();
>> }
>>
>>
>> And also ensure that transaction is enlisted during any update calls. So
>> in MyTxUserModel override the needed setter methods like this:
>>
>>    @Override
>>    public void setFirstName(String firstName) {
>>         ensureTransactionEnlisted();
>>         super.setFirstName(firstName);
>>     }
>>
>>     @Override
>>     public void setLastName(String lastName) {
>>         ensureTransactionEnlisted();
>>         super.setLastName(lastName);
>>     }
>>
>>
>> With this approach, when you have:
>> UserModel john = session.users().addUser("john", realm);
>> john.setFirstName("John");
>> john.setLastName("Doe");
>>
>> you have the register call postponed until the transaction.commit .
>>
>> The thing is, that with postponed registration you won't know the ID of
>> newly created user at the moment when "addUser" is called. However not
>> sure if that's big issue... Another approach might be that UserStore
>> will have possibility to specify if it supports transactions or not.
>> Then we can have method "rollback" on UserStore, which will be called if
>> keycloakTransaction.rollback is called.
>>
>
> Isn't adding:
>
> addUser(org.keycloak.models.entities.UserEntity)
>
> much simpler than all of the things you propose?  We change the 
> registration flow to build up a UserEntity object that is eventually 
> used to create the user.
It is slightly easier, however it doesn't handle transactions and 
single-step updates.

In more details, scenarios like this:

a) With yours, user will be added right away and not at 
transaction.commit. This might be bad especially for non-transactional 
stores like LDAP. For example scenario like:
- userStorage.addUser(userEntity) is called. User will be created right 
away at 3rd party store
- transaction.setRollbackOnly is called by keycloak
- transaction.rollback at the end of request. However your user was 
already registered at the store.

b) Case when you are updating user and doing something like:

user.setFirstName("john");
user.setLastName("Doe");
user.setEmail("john at email.cz");

and you want to ensure that just one update call is done to the storage 
instead of 3 update calls.


How to handle b) if you don't want people to either update after each 
setter or manually deal with our transaction API and implement all the 
boilerplate code for enlist transaction by themselves? Maybe add the 
method for explicit update call:

userStorage.updateUser(userEntity)

can handle it? However that's still not transaction aware though...


In my proposal, I am trying to simplify the dealing with our transaction 
API and add some helper classes, so people don't need to manually enlist 
transaction and use the same boilerplate code to enlist transaction at 
each setter and implement it by themselves. We will mostly implement for 
them. Only thing they need is to call "ensureTransactionStarted" in 
their setters and 2 lines of code at "register" method to ensure 
transaction is started and enlisted.

Marek
>
> Bill