Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.account.WebAuthnSigningInTest#setUpLinksTest

Keycloak CI - WebAuthn IT (firefox)

java.lang.AssertionError: 
Set up link for "webauthn-passwordless" is not visible
Expected: is <true>
     but: was <false>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
...

Report flaky test

@ahus1 I don't quite follow what you are proposing here. There is a connection pool behind the scenes and we just need to make sure they are released when processing streams during the response phase. This fix is specifically to this phase of the request lifetime.

Can you please elaborate more?

@pedroigor - please have a look at this approach which uses a closing stream to close the query at the end: pedroigor#17

I debugged it locally, and found that there is exactly one context opened and one closed. I think this aligns better with the concepts that the code could ensure that the query should always be closed.

Not to muddy things, but wanted to share also this approach: https://github.com/keycloak/keycloak/compare/main...shawkins:iss38660?expand=1

Where we scope the contexts to the session per LDAPConfig.

The observation is that each resue of an initialcontext, even from the pool, if it's an SSL connection that seems to have a cost in the milliseconds. I was chatting with @pedroigor about this, so I know that it may not be desirable for a fix, but something to consider for longer term.

Thanks @ahus1 and @shawkins.

@ahus1 I think your approach is more error-prone than just making sure we have the context closed whenever creating it. Much cleaner IMO, and easy to track resource clean up for LDAP. But I'm not hard on it, please let me know if you really want your solution and I'll update the PR.

In the future, I also see we moving the initialization code currently in LdapOperationManager to the new type I'm proposing here so that creating/managing of LDAP context is more encapsulated and easy to track.

@shawkins +1 for what you are proposing as a follow-up. We know there are other things in LDAP that we need to improve we should also consider what you are proposing here. I also changed the PR to use KeycloakSession.enlistForClose because at the moment when streams are consumed we might have concurrency issues when enlisting transactions due to how (messy) searches and streams are being used in LDAP.

@pedroigor - I'd say that closing the context when we are closing the stream is the smaller change to fix the bug at hand to backport it to 26.2. It also ensures that close is only called once on each context.

Going forward in 26.3, I'd like to see @shawkins' proposed change, as it returns the same instance when you call it with the same properties. With that change in place, we could gradually remove all the try-with-resources and manually closing contexts from 26.3 or 26.4, and rely on the cleanup of the contexts at the end of the transaction.

I also changed the PR to use KeycloakSession.enlistForClose because at the moment when streams are consumed we might have concurrency issues when enlisting transactions due to how (messy) searches and streams are being used in LDAP.

It would be a little simpler to just have this handling in the LDAPContextManager.create method - before returning the LDAPContextManager, just enlist its close.

A related change is that the LDAPContextManager.close method should clear the fields, not just close them. This will keep the footprint of the LDAPContextManager associated with the Session as small as possible - alternatively you could make sure the LDAPContextManager instances are removed from the Session on close. I don't fully know the usage pattern with LDAP, but my understanding is that you could have a request that is repeatedly using / closing LDAPContextManagers.

It would be a little simpler to just have this handling in the LDAPContextManager.create method - before returning the LDAPContextManager, just enlist its close.

Yes, but isn't better to encapsulate this within a specific class? Like I said, that would also help to simplify code later by moving initialization code to this new class.

AFAIK, the LdapOperationManager is created once per ldap provider instance.

See, I'm happy to have more (smart) people involved in reviewing the LDAP provider but I think we should do that as a follow-up and try to fix this specific problem now. But if you think we should change the approach completely, please let me know, and I'll update the PR.

Going forward in 26.3, I'd like to see @shawkins' proposed change, as it returns the same instance when you call it with the same properties. With that change in place, we could gradually remove all the try-with-resources and manually closing contexts from 26.3 or 26.4, and rely on the cleanup of the contexts at the end of the transaction.

Agree, but we need to be careful and make sure reusing LDAP contexts is safe, considering how the LDAP provider is implemented.

Also, note that most of the time, mainly for searches like this PR is trying to fix, you have a single ldap context per workerthread/session/request. Another reason why I think we can do this as a follow-up.

Yes, but isn't better to encapsulate this within a specific class? Like I said, that would also help to simplify code later by moving initialization code to this new class.

I'm not sure what longer term changes you have in mind, but it seems simpler / better to do:
https://github.com/keycloak/keycloak/compare/main...shawkins:iss38660-b?expand=1

The only concers here are

• an accumulation of LDAPContextManager on the session - but if they have already been closed, the footprint is minimal. With the change that you have proposed every LdapContext used within the Session will still be associated with it, and holding on to far more state.
• subtle change in the contract. After a LDAPContextManager has been closed, you can call getLdapContext and get a fresh LdapContext.

And of course there's nothing wrong with making a change like this, or the one you are proposing, for safety along with the change proposed by @ahus1

Yes, but isn't better to encapsulate this within a specific class? Like I said, that would also help to simplify code later by moving initialization code to this new class.

I'm not sure what longer term changes you have in mind, but it seems simpler / better to do: https://github.com/keycloak/keycloak/compare/main...shawkins:iss38660-b?expand=1

Regardless the middle/long-term plans we have for LDAP, I prefer encapsulating this logic and make sure whenever creating an LDAP context we don't forget binding it to the session.

The only concers here are

• an accumulation of LDAPContextManager on the session - but if they have already been closed, the footprint is minimal. With the change that you have proposed every LdapContext used within the Session will still be associated with it, and holding on to far more state.

How many instances of LDAPContextManager do you see accumulated per request?

• subtle change in the contract. After a LDAPContextManager has been closed, you can call getLdapContext and get a fresh LdapContext.

We don't trust/respect the contract already. See https://github.com/keycloak/keycloak/blob/main/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java#L1132.

I'm telling you, LDAP is an area that really deserves a review and we have plans to go over it very soon once we start the SCIM work, which will also involve rethinking the User Storage SPI.

And of course there's nothing wrong with making a change like this, or the one you are proposing, for safety along with the change proposed by @ahus1

Can you please agree then to move forward with this proposal? If you want to create issues for core-iam team to discuss in more detail other parts of LDAP, we are open to starting to discuss them.

Regardless the middle/long-term plans we have for LDAP, I prefer encapsulating this logic and make sure whenever creating an LDAP context we don't forget binding it to the session.

I'm failing to see how it is less encapsulated - the only place we're creating an LdapContext is LDAPContextManager.

How many instances of LDAPContextManager do you accumulate per request?

That's what I was trying to confirm with you - in the case of search you seem to be indicating that only a single instance of an LDAPContextManager is created / used, but for other operations you may, even for the same LDAPConfig, create and use multiple LDAPContextManager if I understand correctly. Since I don't know how many of these that could be (or correspondingly how many references to LdapContexts), it makes sense to me to accumulate as little state as possible on the Session.

Can you please agree then to move forward with this proposal?

You don't need my approval to move forward. I'm coming at this with a set of fresh eyes - but my impressions and concerns may not be valid.

If you want to create issues for core-iam team to discuss in more detail other parts of LDAP, we are open to starting to discuss them.

Sounds good.

Regardless the middle/long-term plans we have for LDAP, I prefer encapsulating this logic and make sure whenever creating an LDAP context we don't forget binding it to the session.

I'm failing to see how it is less encapsulated - the only place we're creating an LdapContext is LDAPContextManager.

For me, having a SessionBoundInitialLdapContext and using it whenever we need to create a context is much better and clearer. It is not hidden within the code in the context manager, but very explicit that creating an ldap context will bind it to the session.

In fact, the LDAPContextManager should be the SessionBoundInitialLdapContext itself, if you think about it.

How many instances of LDAPContextManager do you accumulate per request?

That's what I was trying to confirm with you - in the case of search you seem to be indicating that only a single instance of an LDAPContextManager is created / used, but for other operations you may, even for the same LDAPConfig, create and use multiple LDAPContextManager if I understand correctly. Since I don't know how many of these that could be (or correspondingly how many references to LdapContexts), it makes sense to me to accumulate as little state as possible on the Session.

Can you please agree then to move forward with this proposal?

You don't need my approval to move forward. I'm coming at this with a set of fresh eyes - but my impressions and concerns may not be valid.

If you want to create issues for core-iam team to discuss in more detail other parts of LDAP, we are open to starting to discuss them.

Sounds good.