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

[Bill Burke]

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.

Bill