Add version column to credential table to avoid simultaneous recovery codes updates #38313
Conversation
There was a problem hiding this comment.
@rmartinc - I had a look at this change as it relates to JPA, and IMHO it isn't in all situations doing what you might expect to to JPA intrinsics.
I was debugging this with a password change as just an example. While I didn't test with recovery code, IMHO the locking should work in all cases.
I started Keycloak with --log-level="INFO,org.hibernate.SQL:debug" to get the SQL statements logged.
When changing the password, JPA already loads the data into its persistence context before it reaches the new pessimistic write code. The following line is logged before I reach the newly changed code.
select c1_0.USER_ID,c1_0.ID,c1_0.CREATED_DATE,c1_0.CREDENTIAL_DATA,c1_0.PRIORITY,c1_0.SALT,c1_0.SECRET_DATA,c1_0.TYPE,c1_0.USER_LABEL from CREDENTIAL c1_0 where c1_0.USER_ID=?
2025-03-21 12:31:02,398 DEBUG [org.hibernate.SQL] (executor-thread-4) select ID from CREDENTIAL where ID=? for update
Once I step over the new em.find() with the locking, it only issues a lock for the row.
select ID from CREDENTIAL where ID=? for update
This shows that the row is locked, but the data had already been loaded. So there is actually a small window between loading the data and locking the data, which is IMHO a problem as it would still allow for lost writes.
If you want to prevent concurrent updates / lost writes with a database that is running in read committed mode, I suggest to add a version column. This would then prevent any concurrent update, and it would fail at the end of the transaction when Hibernate flushes and commits.
See
for an example.|
Ok @ahus1, I'll try to update the DB to include the version column. |
… codes updates Closes keycloak#26106 Signed-off-by: rmartinc <rmartinc@redhat.com>
|
@ahus1 I have updated to use the |
|
@rmartinc - thank you for the change. I slightly updated the description of the issue which was still referring to pessimistic locking. Please review it as well and update it as necessary to avoid confusion of future developers that might come across this PR. |
Closes #26106
The update credential now locks using Hibernate's optimistic locking and the model can decide if the update is valid based on the current value. This way the recovery credential model can check if the current value is valid for the update. In the case of the recovery codes, new value should be one code less than the current one, because recovery codes always use the next code and remove it. I have also managed the idea of adding a new method in the credential interfaces. I have been going back and forth between the two solutions. I see pros and cons in both solutions. IMHO changing interfaces is cleaner but it's a backwards incompatible change, and I think there will be implementations of those interfaces out there for other credential types. At least I could not find a change for the interfaces that could have a default implementation or similar.
Test added although it is a mess. The idea is adding a delayed authenticator that just sleeps 1s to allow to test the problem. Besides the button should be clicked using JavaScript because if not selenium waits the new page to come and the issue is not reproduced.