Plugin migrating users without password validation
matheusmansourelbatti opened this issue · 9 comments
I am running the current commit of this plugin with the standard Dockerfile of keycloak 15.0.0 in it. I have run all previous commits for test as well.
Initially it all worked fine, and my users have been migrated only after my POST REST legacy endpoint returned 200 for a valid password.
Now, after no changes made, to the best of my knowledge, users are being migrated (although not logged in) without their passwords in case they submit a wrong password (GET returns 200, POST returns 401 for wrong password).
Previously user was not migrated in this situation. Now it is, according to this log:
2021-08-23T22:49:49.521910840Z�[0m�[0m22:49:49,521 INFO [com.danielfrak.code.keycloak.providers.rest.remote.UserModelFactory] (default task-4) Creating user model for: matheusmansour
We can see my REST microservice has been called with POST status code different than 200, nonetheless:
2021-08-23T22:49:49.515833Z **GET200** 964 B39 msApache-HttpClient/4.5.13 (Java/11.0.12) https://facily-wp-user-migration-ch4ssh6qga-uc.a.run.app/auth/matheusmansour
Aviso
2021-08-23T22:49:49.579379Z **POST401** 719 B21 msApache-HttpClient/4.5.13 (Java/11.0.12) https://facily-wp-user-migration-ch4ssh6qga-uc.a.run.app/auth/matheusmansour
Any clue why this might be happening? Been stuck with it for 4 days with code breaking in production and had to turn the plugin off. Thanks very much for any help!!
Hi!
It's been a while since I've worked directly with this code but are you sure that the migration worked differently when you used the plugin initially?
Looking at the code in LegacyProvider::getUserModel, the user is migrated immediately after confirming that it exists in the legacy system and it's been that way since at least 05.02.2020 according to Git.
What happens, though, is that the user is created with a federation link (UserModelFactory::create), so Keycloak should know that it needs to use the plugin for password validation.
The federation link is only unset once a correct password has been provided by the user.
Are you experiencing trouble with this setup?
Hi Daniel! Thank you so much for your attentive response!
I might have been mistaken about the previous behaviour. Indeed it works the same way as you described, and there is no problem at all with this migration before password validation.
The real problem I am facing is that Keycloak breaks down on password recovery or registration of a customer present in the legacy DB (either fully migrated, migrated without password (i.e. with fed. link set) or not migrated at all).
This is what I get after trying to recover password for my fully migrated user, for example:
11:55:44,756 INFO [com.danielfrak.code.keycloak.providers.rest.remote.UserModelFactory] (default task-9) Creating user model for: matheus.facily
Is this user is already on Keycloak´s DB, the application breaks down because of duplicate users.
Do you know of a way for UserModelFactory::create to stop being called on these situations of password recovery, for example, when the username is found on the Legacy DB, but all I want is the user to be considered as a simple Keycloak user after migration?
I am getting the same problem with forgotten password behaviour, it seems like UserModelFactory::create ends up being called twice and this throws a duplicate record error. This issue is a showstopper for us, we can't migrate users without the ability for users to recover a lost password, as is always the case many users will not have their correct password. Would really love some further input on this, but right now it seems like maybe it's a bug in Keycloak's user federation and how that interacts with plugins?
After some further digging I found a fix that appears to work, although I am not sure of the further implications. Based on the LDAP provider built into Keycloak, I found there was some code checking if the user already exists in Keycloak before attempting to create it, whereas the legacy plugin just attempts to create the user every time we find by username or find by email; obviously the reset password flow is searching for the user twice and therefore trying to duplicate the record.
I updated the getUserModel in LegacyProvider.java as follows:
private UserModel getUserModel(RealmModel realm, String username, Supplier<Optional<LegacyUser>> user) {
// NOTE: This should be an additional delegate passed in, in my case i am only ever using email
UserModel userModel = session.userLocalStorage().getUserByEmail(username, realm);
if (userModel != null) {
LOG.debugf("Authenticated user [%s] found in Keycloak storage", username);
return userModel;
}
return user.get()
.map(u -> userModelFactory.create(u, realm))
.orElseGet(() -> {
LOG.warnf("User not found in external repository: %s", username);
return null;
});
}
I am not submitting this as a PR right now because this breaks a number of tests and I am not the most familiar with Java testing frameworks and frankly, having been battling with this whole setup for several weeks now, I am a bit burnt out on this. Hopefully someone else can take this and make this into a workable PR.
@chucksellick Thanks a lot for your research into this!
Unfortunately, I find myself short on time to work on this plugin as of late so it would be great if someone could work this into a PR.
I'll get around to doing it eventually but it will likely take some time.
I took a deeper look at the code and, while it definitely feels like a workaround, I think it makes sense.
I reworked it to include the delegate you mentioned and changed the parameter order in session.userLocalStorage().getUserByEmail so it's no longer using a deprecated method:
@Override
public UserModel getUserByUsername(String username, RealmModel realm) {
return getUserModel(realm, username, () -> legacyUserService.findByUsername(username),
() -> session.userLocalStorage().getUserByUsername(realm, username));
}
private UserModel getUserModel(RealmModel realm, String username, Supplier<Optional<LegacyUser>> legacyUserSupplier,
Supplier<UserModel> keycloakUserSupplier) {
UserModel userModel = keycloakUserSupplier.get();
if (userModel != null) {
LOG.debugf("Authenticated user [%s] found in Keycloak storage", username);
return userModel;
}
return legacyUserSupplier.get()
.map(u -> userModelFactory.create(u, realm))
.orElseGet(() -> {
LOG.warnf("User not found in external repository: %s", username);
return null;
});
}
@Override
public UserModel getUserByEmail(String email, RealmModel realm) {
return getUserModel(realm, email, () -> legacyUserService.findByEmail(email),
() -> session.userLocalStorage().getUserByEmail(realm, email));
}Still some work to do on this (like reworking unit tests) but at least we're this much closer to fixing this :)
I included your code in #43.
Unfortunately, I wasn't able to test it for password reset on my setup.
Doesn't seem to work for password reset. Here is the error I get with the PR code (same as before).
2021-12-23T17:15:32.248611373Z 17:15:32,248 INFO [com.danielfrak.code.keycloak.providers.rest.remote.UserModelFactory] (default task-13) Creating user model for: SOMEUSER
2021-12-23T17:15:32.253341478Z 17:15:32,252 WARN [org.hibernate.engine.jdbc.spi.SqlExceptionHelper] (default task-13) SQL Error: 1062, SQLState: 23000
2021-12-23T17:15:32.253891436Z 17:15:32,253 ERROR [org.hibernate.engine.jdbc.spi.SqlExceptionHelper] (default task-13) (conn=3640890) Duplicate entry '7' for key 'PRIMARY'
2021-12-23T17:15:32.261589847Z 17:15:32,259 WARN [org.keycloak.services] (default task-13) KC-SERVICES0013: Failed authentication: org.keycloak.models.ModelDuplicateException: javax.persistence.PersistenceException: org.hibernate.exception.ConstraintViolationException: could not execute statement
Sorry for the delay!
Having had some time to test the plugin more thoroughly, I believe this issue was due to an incorrect legacy endpoint contract and its handling (a mistake on my part).
During password recovery both findByUsername and findByEmail call the same legacy endpoint, with findByUsername seemingly going first. Therefore, if a user already existed and password recovery was triggered for an e-mail, findByUsername(actuallyAnEmail) was called and presumably returned an actual user, confusing keycloak terribly.
I have updated the README.md to highlight that the endpoint must support both username and email and fixed the RestUserService code to properly find by username and e-mail. Additionally, I wrote some end-to-end tests in Cypress to verify that password recovery works both before and after a user is migrated.
I'm closing this issue because I believe this bug should be fixed now. Let me know if it isn't.