keycloak-user-migration
keycloak-user-migration copied to clipboard
daniel-frak
Duplicate migrated users in systems that allow username change
In systems that allow usernames to change (in my use-case the email address is the username and users may change their email) keycloak-user-migration allows migration user accounts to be created multiple times once a user changes their username.
Reproduction steps
- In a Keycloak instance that allows username changes
- Authenticate with a user found in the REST API
- Change username of user
- Sign out
- Authenticate with original user credentials found in step 2 (this migrates the user again)
- Sign in to Keycloak as admin user for realm and check users, observe both users exist - original username and changed username
Tested resolution
I was able to solve this scenario by adding a DELETE method to the RestUserClient. I then went to my REST service and added a DELETE method handler that marks the record as being migrated, preventing the user from being able to be migrated again as occurs in step 5 of the reproduction steps.
If you are open to this solution, please let me know and I'll be happy to open a Pull Request.
As a note, it is imperative that #9 is also solved to truly resolve this scenario. Otherwise if the user sets their credential through the forgot password (or other flows as mentioned in the commit message 3354631) the issue described is able to reproduced with similar reproduction steps.
Is step 3 about changing the username in Keycloak itself? Am I correct in understanding that the DELETE method would need to be implemented legacy-side to do something so that the GET methods no longer return that user?
If so, the solution makes sense to me. I'm a little wary of using DELETE for this, but I suppose it is the easiest way to do this. An alternative would probably be treating a migration as a REST "resource" and using POST on some "user migration" endpoint to mark it but that might be overkill. I'm open to discussion if you want to ponder this further. Otherwise, DELETE isn't too bad and I will definitely appreciate the bug fix :)
Step 3: Yes, changing the username in Keycloak.
Yes, the intention is to prevent GET/POST from returning 200 status codes for the username in future requests.
In my team we discussed whether DELETE vs POST vs PUT for quite a while and finally settled on DELETE.
Since the logic for handling the delete resides in the custom legacy API it would be up to the implementer's discretion whether they want to actually delete the record(s) for the username or update their records to respond with non-200 responses to future GET/POST requests for the username.
Essentially we argued ourselves into DELETE, since the behavior from keycloak-user-migration's perspective is that GET/POST requests in the future for the username will never respond with a status code of 200 again (ie: deleted).
Once #9 is merged, I'll open a PR for this feature. I'm definitely open to use something other than DELETE if you prefer, I'd just need to know the spec (ie: POST with { markMigrated: true } or something).
Since you've discussed this with your team, DELETE is fine and will probably be easier to explain in the docs.
One thing I ask for the PR is that you document not only the "how", but also the "why" in the readme, so that everyone using the plugin is fully aware of the fact that they must handle a DELETE and the reasons behind it (maybe even a nice RuntimeException could remind them if DELETE doesn't return 200...?).
If I'm correct, the DELETE is not needed if the username can never change, so maybe we could make it optional? I'm thinking of a toggle in the settings for whether the username can change (or could we check that from realm properties...?) and if it's on then the client has to implement DELETE, otherwise you can just go about your day with the standard set of endpoints. Again, just thinking out loud and not something I'm adamant about - what's your opinion on this?
Again, though, big thanks for the contribution! This is going to save people some headaches (me included).
One of the reasons we argued ourselves to using DELETE was that it would allow most legacy APIs to operate unchanged -- if well designed to serve a 400 (bad request) on an unexpected method.
However, since this tool is in production use and legacy APIs may not be well designed (I've seen servers fail to close connections on unexpected requests), I think adding a toggle is a fantastic idea. I'd lean towards defaulting the toggle to off and documenting in the DELETE README that the toggle needs to be enabled and the REST API needs to handle the request if usernames may be changed within Keycloak.
With this, I think we can avoid needing to throw a RuntimeException since the feature would be opt in.
Great! It seems we're on the same page, then. I think the DELETE documentation would fit neatly in the Optional - additional configuration section of README.md.
The toggle being off by default is definitely a good idea, IMO.
I see the RuntimeException as a last resort kind of thing - I imagine a user of the plugin skipping the README.md (admittedly a bad idea), noticing the toggle and thinking that's all he needs to support username changes. Then he gets an error message like A DELETE request didn't return HTTP 200. Make sure to implement the endpoint to enable support for changing usernames (in the logs). I'll leave that one up to you, though.
Did this stall out? What is remaining for this to close? I see #9 got merged. But a follow-up PR does not seem to be linked here.
Sorry @heddn, my team was under a time crunch, so we just slammed DELETE in without the toggle. I've since not been the one maintaining the Keycloak install so I lost track of it.
@codycraven I'm happy without the toggle as a PR. We don't need it disabled. If that code is still sitting around, maybe someone could post it up? Adding a small tweak to toggle off/on might be easier then building the whole thing.
@heddn I've created and closed a PR (#48) for you to reference the code.
I closed the PR since I know it's not the desired solution and I don't have test coverage for it. It would be great if you'd work off this and add documentation for DELETE, the toggle, and a test or two so that future users will have the solution available since you have a project at hand to verify it with.