Invalidate user cache entries when email or username are different from storage #40256
Conversation
|
I pushed some changes that should provide proper eviction of the different user related information that honors the caching defined in the provider. I tested this manually with LDAP switch to non-importing mode. Also, I think the checking when something was cached is obsolete and can be removed, as either the ISPN behavior was fixed, or just misunderstood when this was written. IMHO there is no need check the |
I also stumbled upon the assumption that ISPN could not guarantee eviction of entries, and it would be great if it were redundant. I'll look at your changes and the test failures. |
|
@pedroigor - maybe removing the redundant code could be deferred to 26.3, while fixing the current problems at hand could be backported to 26.2. Let's discuss more of this tomorrow in the meeting I scheduled. |
Works for me. Looks like we can not rely on the entry lifespan on ISPN, and the assumption was correct, at least regarding testing it. There are test failures that are related to skipping the code in I think the failure is due to the fact that calling |
36c6cdb to
28c2ad9
Compare
|
After talking with @pruivo, I updated the tests to enable/disable the ISPN time service to make sure entries are evicted in ISPN as expected when changing the time offset during tests. There are failures in tests with a weird assumption that realm resources instances (e.g: |
I've seen some cases where caching was disabled completely, then there wouldn't be an adapter returned. Let's have a look later today to see if this is the case in your setup. |
|
@pedroigor - I fixed the tests, and then restored the original logic as much as possible to avoid breaking any existing functionality. When testing this, I found problems with Once the tests are green, I would suggest to backport the PR ti 26.2.x. For 26.3, I would like to deprecate |
Unreported flaky test detectedIf 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.cluster.ComponentInvalidationClusterTest#crudWithFailover |
Unreported flaky test detectedIf 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.cluster.ComponentInvalidationClusterTest#crudWithFailover |
|
@pedroigor - I think I fixed the one test that remained failing, please review my latest commit. |
| return importValidation(realm, user); | ||
| user = importValidation(realm, user); | ||
| // Case when username was changed directly in the userStorage and doesn't correspond anymore to the username from local DB | ||
| if (user != null && username.equalsIgnoreCase(user.getUsername())) { |
There was a problem hiding this comment.
Looks like we assume this check is done by https://www.keycloak.org/docs/26.2.5/server_development/#imp 8000 orteduservalidation-interface.
The importValidation will return the same model instance if your provider is not implementing it. In this case, you will eventually return a stale model, potentially different than the username from the provider.
But I guess our recommendation is to implement the ImportedUserValidation interface for user storage providers if the underlying store is writable and you want consistency?
There was a problem hiding this comment.
The change your are annotating is a change you did :D ... I'm good with this change as it reads to me as follows: Validate the given user. If the user doesn't exist any more (null returned), or if the username that is part of the search has changed (second part of the if), then do not return directly, but try all the other stores to find a (new) possible match.
UPDATE: I think this is better than before as returning null from an invalidation and not trying to re-do the lookup is worse.
There was a problem hiding this comment.
I'm not sure about this change now :)
The federated user is linked with a specific provider, and in this case, the importValidation returning null indicates the provider no longer recognizes the user, and the account will be deleted from the local database. There is no need to run the lookup. The account will be eventually imported from a different storage, if the case.
If the provider is a ImportedUserValidation and the username does not match the value of the search, it is perhaps safer to also remove the user from the local database? Or we just remove this check and rely on the provider to run this check as suggested by the documentation?
I'll review this code.
testsuite/model/src/test/java/org/keycloak/testsuite/model/user/UserSyncTest.java
Outdated
Show resolved
Hide resolved
6011964 to
22b01b7
Compare
…om storage Closes keycloak#40085 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
…n provider Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
…r/UserSyncTest.java Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Closes #40085
MAX_LIFESPANby leveraging the existing mechanisms on Infinispan, therefore removing redundant code on the Keycloak side.