We discussed some time ago how to ensure that UserFederationProvider lifecycle is properly tight to KeycloakSession http://lists.jboss.org/pipermail/keycloak-dev/2016-April/007123.html . The last we discussed was to add new method on KeycloakSession like: <T extends Provider> T getProvider(Class<T> clazz, String id, String instanceId); where instanceId is the state associated with the provider (in case of UserFederationProvider it will be DB ID of UserFederationProviderModelId). That way, the UserFederationProviderFactory.create can load the UserFederationProviderModel (assumption is that RealmModel is available in KeycloakContext, so UserFederationProviderFactory.create has access to RealmModel + providerDatabaseId to load it from DB). In the thread, you can see that I've initially proposed something similar to your proposal, but it's a bit more complex though. Hopefully going "simple" way and adding just the method with "instanceId" String argument can solve the issue. Marek On 10/06/16 01:36, Ariel Carrera wrote: There is not problem! :) One more thing, I solved the problem of multiple "federation provider" instances, adding this code to the DefaultKeycloakSession (and the method definition in KeycloakSession interface): > public <T extends Provider> void registerProvider(Class<T> clazz, > Provider provider, String id) { > Integer hash = clazz.hashCode() + id.hashCode(); > providers.put(hash, provider); > } And into MyUserFederationProviderFactory.getInstance(session, model) something like this: public UserFederationProvider getInstance(KeycloakSession session, > UserFederationProviderModel model){ > UserFederationProvider provider = (UserFederationProvider) > session.getProvider(UserFederationProvider.class, model.getId()); > if (provider == null){ > lazyInit(session); > provider = new MyUserFederationProvider(session, model, > config, ......); > > ((KeycloakSession)session).registerProvider(UserFederationProvider.class, > provider, model.getId()); > }; > return provider; > } After a few tests and debug it seems to work... creating, catching, and closing provider instances as expected. In future versions as you said, maybe would be better include a way to instantiate a complex object/provider instead of doing > ProviderFactory.create(KeycloakSession session) > some kind of method like > ProviderFactory.create(KeycloakSession session, Object... obj); and the appropriate method into the KeycloakSession > <T extends Provider> T getProvider(Class<T> clazz, Object... obj); > <T extends Provider> T getProvider(Class<T> clazz, String id, Object... > obj); And why not a map into the keycloakSession to store some additional context data to share between providers during same request? It's only a vague idea Regards! 2016-06-09 17:14 GMT-03:00 Bill Burke <bburke(a)redhat.com&gt;: > Its gonna be awhile. Its going to be difficult to make everything both > backward compatible and cover all the current and future use cases we need > to cover. Listen on the dev list. I should post some info soon on what > the new impl will look like. > > On 6/9/16 3:57 PM, Ariel Carrera wrote: > > Yes Bill, exactly! I will waiting to test it Thanks! > > 2016-06-09 16:29 GMT-03:00 Bill Burke < <bburke(a)redhat.com&gt; > bburke(a)redhat.com&gt;: > >> >> >> On 6/9/16 2:52 PM, Ariel Carrera wrote: >> >>> Hi Bill, is a little expensive for me because I am creating a new >>> entity manager to connect with a legacy database, and creating/enlisting a >>> transaction per instance. >>> For example in a simple flow case where a user needs to click "I forgot >>> my password" link to recover the password, there is more than nine or ten >>> instances created to do this. It's really not a big problem but I think >>> that is not necessary and can be implemented like others spi providers >>> catched into the keycloak session. >>> >>> This is good feedback. We need a way to associate a provider, by name, >> to the KeycloakSession. Maybe we just need a way to associate anything >> with the KeycloakSession period. >> >> In my case, another difficulty is synchronization between an old >>> authentication system and keycloak implemented on demand (there is no >>> full/partial syncrhonization because the legacy system is still working and >>> need to work together for a while). Also I implemented synchronization >>> support but at this moment it not used. >>> Every time that keycloak needs to validate a user (isValid) recovered >>> from the user storage or cache, a query to the legacy system is made. Added >>> to this... I need to recover some attributes and roles changes produced on >>> the legacy system.... so I decided to implement a "user federation cache" >>> with a short term expiration to improve the performance with certain >>> synchronization delay tolerance. >>> >>> In a few words I have: a custom User Federation Provider + on deman >>> synchronization + a user Federation Provider Cache (my own cache SPI). >>> >>> Maybe an optional spi to obtain a custom container from infinispan >>> could be a good choice to add to the new implementation and provide another >>> one tool to do things with better performance. >>> >>> I think the new model might solve your caching needs. There will be no >> importing by default. This means no synching, etc. Keycloak will only >> store metadata that your user store can't provide. User Federation >> Providers will work just as the default Keycloak user store and user cache. >> >> > > > -- > Tatú > > > -- Tatú _______________________________________________ keycloak-dev mailing listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

I've never been a fan of how creating user feds outside of the session was done. It's a completely broken concept and has several flaws: a) KeycloakSession doesn't manage instances - we have issues with both multiple instances being created as well as instances not being closed. b) The code that requires an instance needs to know how to create one c) No way to create a custom way to configure/setup - the model approach may work for some, but what if a custom provider wants to store config differently With that in mind this needs to be fix and not monkey patched. When requesting an instance of a user federation it should be: session.getProvider(UserFederationProvider.class, String instanceId)

That's it. It would then be up to the factory of figuring out how to instantiate it, not the calling code.

On 13 June 2016 at 15:06, Bill Burke <bburke(a)redhat.com <mailto:bburke@redhat.com>> wrote: On 6/13/16 4:19 AM, Stian Thorgersen wrote: > I've never been a fan of how creating user feds outside of the > session was done. It's a completely broken concept and has > several flaws: > > a) KeycloakSession doesn't manage instances - we have issues with > both multiple instances being created as well as instances not > being closed. > b) The code that requires an instance needs to know how to create one > c) No way to create a custom way to configure/setup - the model > approach may work for some, but what if a custom provider wants > to store config differently > > With that in mind this needs to be fix and not monkey patched. > > When requesting an instance of a user federation it should be: > > session.getProvider(UserFederationProvider.class, String instanceId) > > That's it. It would then be up to the factory of figuring out how > to instantiate it, not the calling code. > A user fed provider is often a generic thing that can be configured multiple times for multiple different stores (i.e. LDAP). So, the model is a must. We don't want people configuring fed providers within keycloak-server.json Model will be used by most (all) providers so it needs to be a parameter for creation. This generic getProvider() method on KeycloakSession just doesn't fit for most situations. Most mappers fall into this category too. I have thought about defining a generic ConfigurationModel and datastore that would be used by everything (mappers, fed providers, etc.) Yes, I know. Please read the thread me and Marek and when we discussed this. This really has to be sorted out otherwise we'll continue to have issues with it.

I'm working on a new SPI right now. Here is my notebad to capture how things work, issues weed to consider, and problems we have to solve: https://github.com/keycloak/keycloak/wiki/2.0-User-Federation-Storage-SPI On 6/13/16 3:39 AM, Marek Posolda wrote: > We discussed some time ago how to ensure that UserFederationProvider > lifecycle is properly tight to KeycloakSession > http://lists.jboss.org/pipermail/keycloak-dev/2016-April/007123.html > . The last we discussed was to add new method on KeycloakSession like: > > <T extends Provider> T getProvider(Class<T> clazz, String id, String > instanceId); > > where instanceId is the state associated with the provider (in case > of UserFederationProvider it will be DB ID of > UserFederationProviderModelId). That way, the > UserFederationProviderFactory.create can load the > UserFederationProviderModel (assumption is that RealmModel is > available in KeycloakContext, so UserFederationProviderFactory.create > has access to RealmModel + providerDatabaseId to load it from DB). > > In the thread, you can see that I've initially proposed something > similar to your proposal, but it's a bit more complex though. > Hopefully going "simple" way and adding just the method with > "instanceId" String argument can solve the issue. > > Marek > > On 10/06/16 01:36, Ariel Carrera wrote: >> There is not problem! :) >> One more thing, I solved the problem of multiple "federation >> provider" instances, adding this code to the DefaultKeycloakSession >> (and the method definition in KeycloakSession interface): >> >> public <T extends Provider> void registerProvider(Class<T> >> clazz, Provider provider, String id) { >> Integer hash = clazz.hashCode() + id.hashCode(); >> providers.put(hash, provider); >> } >> >> >> And into MyUserFederationProviderFactory.getInstance(session, model) >> something like this: >> >> public UserFederationProvider getInstance(KeycloakSession >> session, UserFederationProviderModel model){ >> UserFederationProvider provider = (UserFederationProvider) >> session.getProvider(UserFederationProvider.class, model.getId()); >> if (provider == null){ >> lazyInit(session); >> provider = new MyUserFederationProvider(session, model, >> config, ......); >> ((KeycloakSession)session).registerProvider(UserFederationProvider.class, >> provider, model.getId()); >> }; >> return provider; >> } >> >> >> After a few tests and debug it seems to work... creating, catching, >> and closing provider instances as expected. >> >> >> In future versions as you said, maybe would be better include a way >> to instantiate a complex object/provider instead of doing >> >> ProviderFactory.create(KeycloakSession session) >> some kind of method like >> ProviderFactory.create(KeycloakSession session, Object... obj); >> >> and the appropriate method into the KeycloakSession >> >> <T extends Provider> T getProvider(Class<T> clazz, Object... obj); >> <T extends Provider> T getProvider(Class<T> clazz, String id, >> Object... obj); >> >> >> And why not a map into the keycloakSession to store some additional >> context data to share between providers during same request? It's >> only a vague idea >> >> Regards! >> >> 2016-06-09 17:14 GMT-03:00 Bill Burke <bburke(a)redhat.com >> <mailto:bburke@redhat.com>>: >> >> Its gonna be awhile. Its going to be difficult to make >> everything both backward compatible and cover all the current >> and future use cases we need to cover. Listen on the dev list. >> I should post some info soon on what the new impl will look like. >> >> >> On 6/9/16 3:57 PM, Ariel Carrera wrote: >>> Yes Bill, exactly! I will waiting to test it Thanks! >>> >>> 2016-06-09 16:29 GMT-03:00 Bill Burke <bburke(a)redhat.com&gt;: >>> >>> >>> >>> On 6/9/16 2:52 PM, Ariel Carrera wrote: >>> >>> Hi Bill, is a little expensive for me because I am >>> creating a new entity manager to connect with a legacy >>> database, and creating/enlisting a transaction per >>> instance. >>> For example in a simple flow case where a user needs to >>> click "I forgot my password" link to recover the >>> password, there is more than nine or ten instances >>> created to do this. It's really not a big problem but I >>> think that is not necessary and can be implemented like >>> others spi providers catched into the keycloak session. >>> >>> This is good feedback. We need a way to associate a >>> provider, by name, to the KeycloakSession. Maybe we just >>> need a way to associate anything with the KeycloakSession >>> period. >>> >>> In my case, another difficulty is synchronization >>> between an old authentication system and keycloak >>> implemented on demand (there is no full/partial >>> syncrhonization because the legacy system is still >>> working and need to work together for a while). Also I >>> implemented synchronization support but at this moment >>> it not used. >>> Every time that keycloak needs to validate a user >>> (isValid) recovered from the user storage or cache, a >>> query to the legacy system is made. Added to this... I >>> need to recover some attributes and roles changes >>> produced on the legacy system.... so I decided to >>> implement a "user federation cache" with a short term >>> expiration to improve the performance with certain >>> synchronization delay tolerance. >>> >>> In a few words I have: a custom User Federation >>> Provider + on deman synchronization + a user Federation >>> Provider Cache (my own cache SPI). >>> >>> Maybe an optional spi to obtain a custom container from >>> infinispan could be a good choice to add to the new >>> implementation and provide another one tool to do >>> things with better performance. >>> >>> I think the new model might solve your caching needs. >>> There will be no importing by default. This means no >>> synching, etc. Keycloak will only store metadata that your >>> user store can't provide. User Federation Providers will >>> work just as the default Keycloak user store and user cache. >>> >>> >>> >>> >>> -- >>> Tatú >> >> >> >> >> -- >> Tatú >> >> >> _______________________________________________ >> keycloak-dev mailing list >> keycloak-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/keycloak-dev > > > > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

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.

There will be a UserStorageManager class that implements all UserProvider instances. Our caching layer will delegate to that instead of the actual storage provider. The UserStorageManager will work similarly to UserFederationManager. So, it would work like this: 1. session.users().getUserByUsername(realm, username) 2. User cache looks in cache, doesn't find it, 3. UserStorageManager.getUserByUsername() 4. UserStorageManager iterates all UserStorageProviders looking to see which one offers session.users().getUserByUsername() and invoke it. 5. LDAP.getUserByUsername() 6. LDAP finds the user, creates a UserAdapter, returns it. 7. User cache gets the UserModel returned by LDAP, it caches exactly like it is implemented now. The LDAP UserAdapter, is responsible for pulling data and merging it from LDAP store and Federation Store. User ids are an URN "f:" + providerId + ":" + provider-user-metadata UserStorageManager.getUserById() will parse the ID and call directly on the UserStorageProvider that initially loaded the user. Updates are invoked directly on UserModel class. LDAP UserAdapter will be responsible for figuring out what data is stored/updated in LDAP and what should be stored in Federated Storage. More comments follow: On 6/15/16 4:47 AM, Marek Posolda wrote: > Hi Bill, > > few notes from me. I am mostly adding the scenarios, which will be good > to support/improve or which we should still keep supporting after > refactoring IMO. Sorry for bit longer email :) > > Note: When I talk "LDAP", I usually mean "LDAP or any other 3rd party > federation storage" > > > 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"

> Assuming the scenario: > - john is imported from LDAP with "unsynced" mode > - john changes his password in account management. LDAP is unsynced > (read-only), so the password needs to be changed in Keycloak storage > - After server restart, john should have set the new password, not the > old one from LDAP. Hence the only possibility seems to keep support for > "import" this user into Keycloak DB and have user persistent. > > The similar situation applies for any update of user, which can't be > saved back to LDAP (either because LDAP is read-only or because it > doesn't support store the keycloak metadata like social links, consents, > required actions etc) > > Regarding this, I am thinking about possible import modes like: > 1) Persistent : User attributes from LDAP are imported into Keycloak DB > (same behaviour like now) > No more importing. > 2) Transient : User attributes from LDAP are not imported into Keycloak > DB, but just cached locally. Any updates of the user are either dropped > after server restart or disallowed. For example: Maybe consents can be > just dropped, but some other things like requiredRoles should be rather > disallowed to change? For example if admin adds some requiredAction to > user "john", the requiredAction shouldn't disappear after server > restart. This would be a security hole IMO. So in that case, if admin > tries to manually add requiredAction to such user, the exception will be > thrown so admin is aware that this is not supported. Anyway, for support > any writing to cache, the cache will need to be replicated though (for > example: user consent saved into cache on node1 should be visible on > node2 as well). > > 3) Hybrid : LDAP user is not imported into Keycloak DB immediatelly. > They are imported "on-demand" after user is updated and the update can't > be saved to LDAP > No more on-demand loading of data. IIRC, we discussed this in Brno. LDAP users will be cached in the same way JPA Users currently are. > > 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.

> 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(&quot;john(a)email.cz&quot;); > > 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.

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

> > Proxy yes/no? > ------------------ > Proxy yes, but it won't work the same. See above. Bill

>>> 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(&quot;john(a)email.cz&quot;); >>> >>> 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(&quot;john(a)email.org&quot;); > > 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 &quot;john(a)email.org&quot; . 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)?

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(&quot;john(a)email.cz&quot;); >>>> >>>> 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(&quot;john(a)email.org&quot;); >> >> 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 &quot;john(a)email.org&quot; . 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.

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(&quot;john(a)email.cz&quot;); >>>>> >>>>> 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(&quot;john(a)email.org&quot;); >>> >>> 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 &quot;john(a)email.org&quot; . 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

On 16/06/16 16:38, Bill Burke wrote: >>>> 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. yeah, I also can't see how to do pagination without imports, assuming the 3rd party store (ie. LDAP) doesn't have pagination support. So then again, the question is if SPI should still have the option to support imports? Or maybe don't have it OOTB, but if customers start asking for pagination, we have the option to say them "ok, we will try to add it" instead of "no, you can't do that and there is no way to support it with current SPI" . Marek _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

Some comments on https://github.com/keycloak/keycloak/wiki/2.0-User-Federation-Storage-SPI: * Different cache policies per provider may be difficult. Wouldn't that require having a Infinispan cache per provider? That wouldn't be very manageable.

* My vote goes to remove UserFederationProvider unless it has no impact on design of the new providers. We shouldn't sacrifice the usability/design of the new SPI for the sake of this.

* We should be careful about changing behavior of LDAP provider as it's supported in product

* JTA - should we support proper JTA transactions for consistency?

First on pagination. I was thinking about this a bit. Do we really think that admins are going to paginate through 1000s of users? Or will it be more like a couple of hundred at most? Right now I've implemented something that is pretty inefficient to keep it backward compatible right now. Basically I iterate all providers from the beginning until the page desired is identified and filled up. Minimally it is a stop gap until I get everything working. The API will need to change if we want pagination to work efficiently. Invokers on the API will need to keep an index as we need to know the last provider queried and what was the index for the last entry read. As a result, the REST API would have to change too. On 6/23/16 8:53 AM, Stian Thorgersen wrote: > Some comments on > https://github.com/keycloak/keycloak/wiki/2.0-User-Federation-Storage-SPI: > > > * Different cache policies per provider may be difficult. Wouldn't that > require having a Infinispan cache per provider? That wouldn't be very > manageable. I think there is some flexibility here in the Infinispan API. I know the old api at least allowed you to define zones in the cache.

> * My vote goes to remove UserFederationProvider unless it has no impact > on design of the new providers. We shouldn't sacrifice the > usability/design of the new SPI for the sake of this. With what I got so far, I've written it to be completely backward compatible and allow them to co-exist. > * We should be careful about changing behavior of LDAP provider as it's > supported in product Honestly, we made a very bad decision to not refactor the federation SPI and to force users to import. It is going to be a really big headache to migrate existing LDAP users to the new LDAP provider. IMO, it is ultra important to stop importing everything. > * JTA - should we support proper JTA transactions for consistency? > Yes, but not all federation providers will be able to take advantage of it, i.e. LDAP. Bill

On 23/06/16 16:31, Bill Burke wrote: > First on pagination. I was thinking about this a bit. Do we really > think that admins are going to paginate through 1000s of users? Or will it > be more like a couple of hundred at most? Right now I've implemented > something that is pretty inefficient to keep it backward compatible right > now. Basically I iterate all providers from the beginning until the page > desired is identified and filled up. Minimally it is a stop gap until I > get everything working. > > The API will need to change if we want pagination to work efficiently. > Invokers on the API will need to keep an index as we need to know the last > provider queried and what was the index for the last entry read. As a > result, the REST API would have to change too. > +1 to change the REST API. The admin console already works in a way that there are arrows for the "next" page and "previous" page right? So if we don't have support for "Go directly for users from 1500 to 1600" it won't be a big headache though...

It will be also good for LDAP as some LDAP servers have support for pagination extension. This is based on the functionality, that you can send pagination query to return first 10 users and LDAP will return you a "cookie" for going to next page. So just browsing between "next" page and "previous" page is easily possible. The most important LDAP vendors like MSAD and RHDS have the support for this.

> On 6/23/16 8:53 AM, Stian Thorgersen wrote: > >> Some comments on >> https://github.com/keycloak/keycloak/wiki/2.0-User-Federation-Storage-SPI: >> >> >> * Different cache policies per provider may be difficult. Wouldn't that >> require having a Infinispan cache per provider? That wouldn't be very >> manageable. >> > > I think there is some flexibility here in the Infinispan API. I know the > old api at least allowed you to define zones in the cache. > > * My vote goes to remove UserFederationProvider unless it has no impact >> on design of the new providers. We shouldn't sacrifice the >> usability/design of the new SPI for the sake of this. >> > > With what I got so far, I've written it to be completely backward > compatible and allow them to co-exist. > > * We should be careful about changing behavior of LDAP provider as it's >> supported in product >> > > Honestly, we made a very bad decision to not refactor the federation SPI > and to force users to import. It is going to be a really big headache to > migrate existing LDAP users to the new LDAP provider. IMO, it is ultra > important to stop importing everything. > Maybe I am missing something, but not sure why is it a headache? Is it because of the UUID of user is going to change to new format like f:<PROVIDER-UUID> , so users may not be referenced from the applications with correct UUID? This looks to be a migration issue for all existing federation providers, not just LDAP specific? Marek > * JTA - should we support proper JTA transactions for consistency? >> >> > Yes, but not all federation providers will be able to take advantage of > it, i.e. LDAP. > > Bill >

On 6/24/16 12:34 AM, Marek Posolda wrote: > * We should be careful about changing behavior of LDAP provider as it's >>> supported in product >>> >> >> Honestly, we made a very bad decision to not refactor the federation >> SPI and to force users to import. It is going to be a really big >> headache to migrate existing LDAP users to the new LDAP provider. >> IMO, it is ultra important to stop importing everything. >> > Maybe I am missing something, but not sure why is it a headache? Is it > because of the UUID of user is going to change to new format like > f:<PROVIDER-UUID> , so users may not be referenced from the applications > with correct UUID? This looks to be a migration issue for all existing > federation providers, not just LDAP specific? > > We don't support federation providers in product except for LDAP and Kerberos. There are a few migration headaches: * New model won't support on-demand loading or proxying. It will work just as the current JPA and Mongo UserProviders work. * With importing, you don't know where the import begins and ends and it will be very hard to unwind it to use a non-importing LDAP impl. Honestly, I just don't want to support import and sync. I think it is really really bad. As for existing deployments, it looks like I can keep the UserFederationProvider SPI and have it co-exist with the new one. We can do that and @Deprecate the UserFederation SPI. We might want to consider keeping support for the old LDAP provider in product too. For example, if one exists, show the admin console screens for it, if one doesn't don't show the screens for it. Bill