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

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(a)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(a)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(a)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