Repository navigation
Conversation
Persons created without a Keycloak account (e.g. guest donors), or whose link to Keycloak was lost, have keycloakId = null. Forgot-password still signed a reset token with sub: null, so submitting the new password called Keycloak's reset-password with id null and always failed with a generic error on the change-password page. - When keycloakId is missing, look up the Keycloak user by email and store the link on the person. - If no Keycloak user exists, return 404 instead of sending a link that can never work. - Reject reset tokens without a sub before calling Keycloak. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 of 3 tasks
|
❌ Not all tests have run for this PR. Please add the |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs podkrepi-bg/frontend#2232
Hi, and thanks for maintaining this.
A quick note first: I'm not familiar enough with your stack to be confident about this change. It's a suggestion based on an analysis I did with Claude (an AI assistant), so please review it carefully, and feel free to adjust or close it.
What I think is happening
When I tried to reset my password, the token in the email link had
"sub": null. Reading the code, it looks like a person record can exist without akeycloakId.sendMailForPasswordChangethen signs the reset token withsub: null, and when the new password is submitted,updateForgottenPasswordcalls Keycloak's reset-password withid: null. That seems bound to fail, and the change-password page shows its generic error.Suggested change
sendMailForPasswordChange: ifkeycloakIdis missing, look up the Keycloak user by email (admin.users.find({ email, exact: true })) and save the link on the person.404instead of sending a link that can't work. The forgot-password form already shows "User not found" for a 404, so the user at least gets a clearer message.updateForgottenPassword: reject tokens without asubbefore calling Keycloak.Security, as far as I can tell
The reset link still goes only to the email address on the person record, so only the owner of that address can use it. Registration already links a person to a Keycloak user by email in a similar way (the
createPersonupsert). I may be missing something here, though, so a second look would be very welcome.An edge case I'm unsure about
If another person record already holds the same
keycloakId(the column is@unique), the update would fail with a 500. I'd expect this to be rare, but I'm happy to handle it differently if you prefer.Tests
subis rejected.jest apps/api/src/auth: 32 passed, andtsc --noEmitis clean.Thank you for your time!
🤖 Generated with Claude Code