UserLoginFailure: Transaction not successfully started thrown in multiple tests

[sguilhen]

I've finished the implementation of the user login failures JPA storage and while testing it I came across this error in tests that remove more than one user via UserManager:

java.lang.IllegalStateException: Transaction not successfully started
	at org.hibernate.engine.transaction.internal.TransactionImpl.commit(TransactionImpl.java:98)
	at org.keycloak.models.map.storage.jpa.JpaMapKeycloakTransaction.commit(JpaMapKeycloakTransaction.java:176)
	at org.keycloak.services.DefaultKeycloakTransactionManager.commit(DefaultKeycloakTransactionManager.java:146)

UserSessionProviderTest is a good example of a test that causes this error. Basically this is what happens:

1. Test calls UserManager.removeUser to remove more than one user as part of the same transaction.
2. For each user that is removed:
   • a UserRemovedEvent is generated and is handled by the MapUserLoginFailureProviderFactory.
   • the factory creates a new MapUserLoginFailureProvider, which on creation calls store.createTransaction(session).
   • the code for the JPA store checks the session for an existing transaction using a predefined key jpa-map-tx-modelType.hashCode, returning the previous tx if one is found in the session or creating a new tx otherwise.
   • the user login failure provider enlists the tx.

So when removing the first user, a new transaction is created for the user login failure and enlisted. From that point on, every time the MapUserLoginFailureProviderFactory handles the removal event, it creates a MapUserLoginFailureProvider that in turn fetches the previously created tx from the session and enlists it again.

When the overall transaction is committed, the user login failure transaction is committed multiple times because it has been enlisted more than once while handling the user removal event, which causes the exception described above.

I could think of a few ways to get around it:

1. removeUser is called multiple times in the same UserManager only by the tests. Our endpoints offer no bulk user removal, so it always processes one removal per transaction. That means we can probably adjust the tests to, for example, remove the created users calling the endpoint directly for each user (adminClient.realm("test").users().get(userId1).remove();), but this requires keeping track o the ids of the users created in each test.
2. make all the enlist methods in DefaultKeycloakTransactionManager check if the same instance has already been enlisted - this would prevent multiple commit() calls to the same transaction object.
3. Adjust the MapUserLoginFailureProvider's constructor so that it somehow checks if the tx already exists in the session before calling enlistAfterComplete. This would ensure that only the provider that created the transaction that is currently active in the session gets to enlist it.

I'm currently leaning towards option 2 and make the enlist methods check if the list of transactions already contain the transaction being enlisted to avoid enlisting it twice. Option 1 works but will require many changes to all the tests that remove users. Option 3 is feasible although I would need to investigate a viable option for looking up the session because the key is constructed by the JpaMapStorageProvider and is not available to the user login failure provider.

[ahus1]

Hi @sguilhen, looking at the code option 2 seems to be the smallest change.

Looking a bit more closer at the implementation I did for LDAP, it is strange that the code in the storage provider already decides if a new transaction should be created or an existing should be used, and some other code would decide if it should be enlisted. Also, that other code would decide if it would be enlist() or enlistAfterCompletion().

            MapKeycloakTransaction<V, M> sessionTx = session.getAttribute(sessionTxPrefix + modelType.hashCode(), MapKeycloakTransaction.class);
            if (sessionTx == null) {
                sessionTx = factory.createTransaction(session, modelType);
                session.setAttribute(sessionTxPrefix + modelType.hashCode(), sessionTx);
            }
            return sessionTx;

Therefore I would propose the following change: the one who creates the transaction should also enlist it with the session, in this case the one who calls factory.createTransaction() would then also enlist it with the session.

WDYT?

[ahus1]

Some afterthoughts after talking to @sguilhen: If the decision of enlist vs. enlistAfterCompletion stays with the caller, a transaction could be enlisted within both or - depending on a future logic - with one or the other depending on who is calling it first.

[sguilhen]

@hmlnarik do you have any thoughts on how to proceed with this? I was able to get the user login failures JPA store working with all tests by adding checks to the transaction manager so it doesn't enlist the same instance, but it feels like we could do better so that this situation doesn't even happen in the first place.

[hmlnarik]

Yeah, I agree with the proposal. The challenge here is that each MapStorageProvider.createTransaction impl is different. HotRodMapStorage doesn't use the session, while JpaMapStorageProvider, LapMapStorageProvider and ConcurrentHashMapStorage all use the session but with different keys. So the particular Map*Provider that is creating and enlisting the transaction has no clue about what format it should use to look up the session.

Note that there might be two separate transactions active at the same time, say for a configuration where there are two LDAP connections targetting separate servers configured in the tree store, both active at the same time.

So it is not desirable for a storage instance to interfere with a transaction created with the other instance. Particular Map*Provider should not be looking up the transaction, it should be the storage that owns and understands how to look the transaction up, e.g.

keycloak/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java

Lines 131 to 132 in fb81242

	MapKeycloakTransaction<V, M> sessionTransaction = session.getAttribute("map-transaction-" + hashCode(), MapKeycloakTransaction.class);
	return sessionTransaction == null ? new ConcurrentHashMapKeycloakTransaction<>(this, keyConverter, cloner, fieldPredicates) : sessionTransaction;

Some afterthoughts after talking to @sguilhen: If the decision of enlist vs. enlistAfterCompletion stays with the caller, a transaction could be enlisted within both or - depending on a future logic - with one or the other depending on who is calling it first.

Where the transaction is enlisted depends on the semantics of the caller and thus cannot be enlisted at both, nor arbitrarily altered in FCFS manner.

[...] it feels like we could do better so that this situation doesn't even happen in the first place.

I agree. The legacy JPA transaction is never enlisted twice since it is created in a separate provider. Sadly, we don't have this comfort here, as there might be several JPA servers being targetted (cf. several LDAP servers example above).

It seems that once we solve the question how to get unique key to a session attribute, we'd be done. In case of JPA, the transaction should be the same for the same connection which in turn is represented by a respective map storage ProviderFactory. IOW, contrary to ConcurrentHashMapStorage above, rather than hash code on that storage, we need an identifier that is passed to each map storage from the factory and used as the key (could be a hash code, but unique a identifier obtained from an AtomicInteger could be even better as it is collision-free). WDYT?

[sguilhen]

That seems reasonable to me. So each storage instance is created with a different identifier that it will use in it's createTransaction method, meaning that you will only get to reuse the same tx if the same storage instance is being called to create transactions multiple times?

[hmlnarik]

Almost - this works in case of CHM or HotRod where the transaction is processed per each storage (i.e. per each model class). In JPA the transaction is obtained from the datasource which is likely shared across storages created by the same MapStorageProvider for different areas; here the identifier (and thus the transaction) would be shared within the session across areas.

[hmlnarik]

Attached is the diagram of the setup that we agreed on in the call:

• JPA transaction (which holds reference to DB transaction) is created lazily and enlisted in keycloak session upon first call to getStorage
• per-area transactions (e.g. JpaGroupMapKeycloakTransaction) can for its operations rely on existence on JPA transaction, and thus the transaction-processing methods will be empty.
• A per-area map storage instance may contain a direct or indirect reference to the main JPA transaction, (e.g. by reference to an entity manager or a specific field referring directly the JPA transaction - as needed)

[hmlnarik]

You're correct. This is handled in

keycloak/services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java

Lines 353 to 370 in a6dd9dc

	public <T extends Provider> T getComponentProvider(Class<T> clazz, String componentId, Function<KeycloakSessionFactory, ComponentModel> modelGetter) {
	Integer hash = clazz.hashCode() + componentId.hashCode();
	T provider = (T) providers.get(hash);
	final RealmModel realm = getContext().getRealm();
	// KEYCLOAK-11890 - Avoid using HashMap.computeIfAbsent() to implement logic in outer if() block below,
	// since per JDK-8071667 the remapping function should not modify the map during computation. While
	// allowed on JDK 1.8, attempt of such a modification throws ConcurrentModificationException with JDK 9+
	if (provider == null) {
	final String realmId = realm == null ? null : realm.getId();
	ProviderFactory<T> providerFactory = factory.getProviderFactory(clazz, realmId, componentId, modelGetter);
	if (providerFactory != null) {
	provider = providerFactory.create(this);
	providers.put(hash, provider);
	}
	}
	return provider;
	}

but if there is a different way to obtain the JpaMapStorageProvider instance, then we should better ATM put the logic into the create method

[sguilhen]

The thing is currently we don't invoke this method when obtaining the map storage. When a Map*ProviderFactory creates a particular provider, it calls getStorage:

keycloak/model/map/src/main/java/org/keycloak/models/map/common/AbstractMapProviderFactory.java

Lines 59 to 65 in ed79c2a

	protected MapStorage<V, M> getStorage(KeycloakSession session) {
	ProviderFactory<MapStorageProvider> storageProviderFactory = getComponentFactory(session.getKeycloakSessionFactory(),
	MapStorageProvider.class, storageConfigScope, MapStorageSpi.NAME);
	final MapStorageProvider factory = storageProviderFactory.create(session);
	return factory.getStorage(modelType);
	}

Which essentially obtains the storage provider factory and then calls create(session) on it instead of calling the method you've referenced. This has the downside of the session not being able to track and cache the particular storage provider, so I wonder if we shouldn't change the getStorage implementation to call this method instead and reap the benefits of the session cache. Do you envision any problems with this change? For JPA we surely want the same provider instance to be used for the duration of the session and I can't see any of the other implementations requiring a new instance of their provider every time getStorage is called.

[hmlnarik]

Ah, yes, that's right. The reason is that in area Map*Providers, there is a storage option in config which can either point to system-wide storage, adjusted storage config, or is just not there. It's related to #10324. In that case, let's go with the workaround you suggested (i.e. implementing the in-session caching of the created JpaMapStorageProviders), just add a comment in appropriate place in the code that should be revisited once #10324 is resolved

[sguilhen]

I don't even think we will have to revisit this later because the getComponentProvider method uses a hash for key that includes the componentId, and this componentId will be different for each profile. So as long as the component id is computed in a way that takes the profile into consideration (which is something included in #10324 discussion) we should have proper providers being handed out by session.getComponentProvider.

[sguilhen]

This work is tracked by #11230