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@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@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@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.
Marek