Fixes a NullPointerException after import validation #20151

havarnov · 2023-05-04T08:59:37Z

If the import validation (when getting a user by email) returns null, indicating that the user entity should be removed from local storage, an email equality check results in a NullPointerException.

This commit fixes this issue by explicitly checking for null.

Closes #20150

havarnov · 2023-05-04T13:06:00Z

model/legacy-private/src/main/java/org/keycloak/storage/UserStorageManager.java

@@ -340,7 +340,7 @@ public UserModel getUserByEmail(RealmModel realm, String email) {
        if (user != null) {
            user = importValidation(realm, user);
            // Case when email was changed directly in the userStorage and doesn't correspond anymore to the email from local DB
-            if (email.equalsIgnoreCase(user.getEmail())) {
+            if (user != null && email.equalsIgnoreCase(user.getEmail())) {


Maybe it would be correct to return null here, if importValidation() returns null?

Sorry, I missed this question before. I don't think we want to return null in that case. If importValidation returns null it means the user from Keycloak database is invalid and was removed. Returning null in that situation would mean we say Keycloak does not know the user by this email, however, this is not true and it can be still present in an external provider. We need to query the user again.

But that's what I mean, if importValidation returns null we return user which is set to null.

As I wrote, we DO NOT want to return null when validation fails, because some provider may still contain the user.

For example:

Some provider contains user1 and it is synced to KC DB

The user1 is changed in that provider (Keycloak DB is not updated).

If we query this user by email, the validation will fail because the user was changed

Returning null in that case would be wrong because the provider still contains the user. We need to reload the user from the provider (or any other provider knowing user by that email).

mhajas

Thank you for the PR @havarnov! Can you please also create some tests for this? I believe the best class to place this is: https://github.com/keycloak/keycloak/blob/main/testsuite/model/src/test/java/org/keycloak/testsuite/model/user/UserSyncTest.java

What we need is:

Create a user in LDAP
Load the user by email (imports user to KC database)
Remove the user from LDAP only
Assert no exception is thrown and no user was returned

martin-kanis · 2023-06-20T11:49:05Z

@havarnov Thank you for the PR. Will you be able to add the missing test anytime soon? I can offer more guidance if needed.

havarnov · 2023-06-20T13:20:16Z

@martin-kanis sry, I've been busy with other stuff. I'll see if I can make it this week!

I do have one comment I'd like some feedback on, though: https://github.com/keycloak/keycloak/pull/20151/files#r1184988078

havarnov · 2023-06-21T12:45:31Z

I'm having issues running the tests locally. Is there some kind of setup I'm missing maybe?

org.junit.AssumptionViolatedException: Some required providers not found: got: <[interface org.keycloak.models.UserProvider, interface org.keycloak.cluster.ClusterProvider, interface org.keycloak.models.RealmProvider, interface org.keycloak.storage.UserStorageProvider]>, expected: an empty collection

mhajas · 2023-08-10T15:53:08Z

I'm having issues running the tests locally. Is there some kind of setup I'm missing maybe?

org.junit.AssumptionViolatedException: Some required providers not found: got: <[interface org.keycloak.models.UserProvider, interface org.keycloak.cluster.ClusterProvider, interface org.keycloak.models.RealmProvider, interface org.keycloak.storage.UserStorageProvider]>, expected: an empty collection

You need to enable some profile in order to have these providers on class path. Try to run the test with -Plegacy-jpa-federation+ldap+infinispan.

havarnov · 2023-08-21T07:55:23Z

@mhajas I'm sry, but I can't get the UserSyncTests to run. I'm doing the following

cd keycloak

# build project
mvn clean install -DskipTests

# run tests
cd testsuite/model
mvn test -Plegacy-jpa-federation+ldap+infinispan -Dtest=UserSyncTest

The actual test fails with:

[ERROR]   UserSyncTest.testManyUsersImport:113 » Model org.keycloak.models.ModelException: Error creating subcontext [uid=user1,ou=People,dc=keycloak,dc=org]

Is there any dependencies I'm missing, e.g. start a specific container or something?

mhajas · 2023-08-21T14:53:59Z

Is there any dependencies I'm missing, e.g. start a specific container or something?

No, it should work out of the box. It seems you are doing the correct thing. Have you done any changes to the UserSyncTest class? If yes, can you push the changes so I can give it a try?

havarnov · 2023-08-22T06:37:10Z

No changes to UserSyncTest. I just did a rebase against main, so I get this error on bugfix/import-validation-null-pointer-bug without any changes.

mhajas · 2023-08-22T08:39:15Z

I was able to reproduce it @havarnov. It seems there is a bug in the test class. We are not cleaning the LDAP users properly between test methods and it is failing.

It was not caught by the CI because for some reason the test passes for second attempt when we use -Dsurefire.rerunFailingTestsCount=2. I am not sure why it is not reported as flaky test yet though, I am investigating it.

To workaround this, you can run only one test within test class. For example:

mvn test -Plegacy-jpa-federation+ldap+infinispan -Dtest=UserSyncTest#testManyUsersImport

this way it passes.

I will let you know once I resolve this.

mhajas · 2023-08-22T10:30:49Z

@havarnov I created a PR for fixing the failing test. You can try whether the code I added works for you:
#22603

havarnov · 2023-08-22T12:34:16Z

@mhajas thanks both the work-around and your PR worked for me :D

If the import validation (when getting a user by email) returns null, indicating that the user entity should be removed from local storage, an email equality check results in a NullPointerException. This commit fixes this issue by explicitly checking for null. Closes keycloak#20150

havarnov · 2023-08-22T13:45:07Z

@mhajas I've now added a test that fails if the fix is not applied. But, I'm not totally sure why I need the two calls to UserStorageSyncManager.syncAllUsers() to get this to work. Shouldn't LDAPTestUtils.addLDAPUser() also add the user to the localStorage?

…n it is removed from LDAP Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas · 2023-08-23T15:17:01Z

@havarnov We probably don't need to do synchronization at all in this test. I updated the test a little bit. Could you please have a look at whether it makes sense?

mhajas

Thanks for the contribution @havarnov

havarnov requested a review from a team May 4, 2023 08:59

havarnov requested a review from a team as a code owner May 4, 2023 08:59

ghost added the team/store label May 4, 2023

havarnov force-pushed the bugfix/import-validation-null-pointer-bug branch from da39ded to 0b6248c Compare May 4, 2023 09:00

havarnov commented May 4, 2023

View reviewed changes

mhajas requested changes May 9, 2023

View reviewed changes

mhajas self-assigned this May 9, 2023

mhajas added the missing/tests Tests are missing label May 9, 2023

havarnov force-pushed the bugfix/import-validation-null-pointer-bug branch from 0b6248c to 64794ba Compare August 22, 2023 06:33

havarnov force-pushed the bugfix/import-validation-null-pointer-bug branch from 64794ba to 64a5295 Compare August 22, 2023 13:43

Update test with NO_CACHE option to avoid loading user from cache whe…

e91592b

…n it is removed from LDAP Signed-off-by: Michal Hajas <mhajas@redhat.com>

ghost added the flaky-test label Aug 23, 2023

mhajas approved these changes Oct 23, 2023

View reviewed changes

mhajas merged commit bc55846 into keycloak:main Oct 23, 2023
72 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes a NullPointerException after import validation #20151

Fixes a NullPointerException after import validation #20151

havarnov commented May 4, 2023 •

edited

Loading

havarnov May 4, 2023

mhajas Aug 10, 2023

havarnov Aug 22, 2023

mhajas Aug 22, 2023

mhajas left a comment

martin-kanis commented Jun 20, 2023

havarnov commented Jun 20, 2023

havarnov commented Jun 21, 2023

mhajas commented Aug 10, 2023

havarnov commented Aug 21, 2023

mhajas commented Aug 21, 2023

havarnov commented Aug 22, 2023

mhajas commented Aug 22, 2023

mhajas commented Aug 22, 2023

havarnov commented Aug 22, 2023

havarnov commented Aug 22, 2023

mhajas commented Aug 23, 2023 •

edited

Loading

mhajas left a comment

Fixes a NullPointerException after import validation #20151

Fixes a NullPointerException after import validation #20151

Conversation

havarnov commented May 4, 2023 • edited Loading

havarnov May 4, 2023

Choose a reason for hiding this comment

mhajas Aug 10, 2023

Choose a reason for hiding this comment

havarnov Aug 22, 2023

Choose a reason for hiding this comment

mhajas Aug 22, 2023

Choose a reason for hiding this comment

mhajas left a comment

Choose a reason for hiding this comment

martin-kanis commented Jun 20, 2023

havarnov commented Jun 20, 2023

havarnov commented Jun 21, 2023

mhajas commented Aug 10, 2023

havarnov commented Aug 21, 2023

mhajas commented Aug 21, 2023

havarnov commented Aug 22, 2023

mhajas commented Aug 22, 2023

mhajas commented Aug 22, 2023

havarnov commented Aug 22, 2023

havarnov commented Aug 22, 2023

mhajas commented Aug 23, 2023 • edited Loading

mhajas left a comment

Choose a reason for hiding this comment

havarnov commented May 4, 2023 •

edited

Loading

mhajas commented Aug 23, 2023 •

edited

Loading