Do the re-hash of password in a spearate transaction to continue login in case of model exception #39140
Conversation
There was a problem hiding this comment.
Thank you for this PR, the approach looks valid to me as the re-hashing of passwords is always a best-effort task. The approach with another DB transaction might increase the overhead a little bit, but it shouldn't be significant as the rehashing of passwords is a rare occurrence.
To make those attempts a bit more visible, I would like to log them as "info", still that is just my opinion on this.
| } catch (ModelException e) { | ||
| logger.debug("Error re-hasing the password in a different transaction", e); |
There was a problem hiding this comment.
| } catch (ModelException e) { | |
| logger.debug("Error re-hasing the password in a different transaction", e); | |
| } catch (ModelException e) { | |
| logger.info("Error re-hashing the password in a different transaction", e); |
There was a problem hiding this comment.
Done! And I have rebased too to incorporate fips changes. If tests go OK I will move this out of draft.
…n in case of model exception Closes keycloak#38970 Signed-off-by: rmartinc <rmartinc@redhat.com>
Since this is a best-effort task would it make sense to perform this in background and not block the request processing while rehashing happens? |
I'd say it would make things more difficult to test, and again it is a rare occasion. We don't have any queue functionality today, and then we might limit the queue to avoid it to take many resources, we need to figure out how many threads should work on it ... so I'd rather keep it as is. |
|
I have added backport to 26.2 for the issue. So I will provide PR for that branch when merged. |
Closes #38970
The previous change in #26106 does not allow concurrent changes for the credential. This is normally OK but it can happen that login perform a re-hash when the realm policy has changed (manually or via migration). This is not a common situation but doing parallel logins in that circumstance can lead to ModelException in the login. This PR performs the re-hash in a separate transaction.
@mposolda @ahus1 WDYT?