fix: create email identity when OAuth-only user sets a password via updateUser#2425
Conversation
nancysangani
force-pushed
the
fix/oauth-account-email-identity-on-password-update
branch
from
7d16440 to
f077d13
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated rate limit for sign-in/sign-up flows, adjusts /token rate-limiting behavior by grant type, and adds a safeguard during password updates to ensure an email identity exists when needed.
Changes:
- Add a new global config setting for sign-in/sign-up rate limiting.
- Introduce a
SignInslimiter and apply it to/tokengrant_type=passwordwhile keeping other grants on existing limiters. - When updating a password, create an
emailidentity if missing and updateapp_metadata.providers.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/conf/configuration.go | Adds RateLimitSignInSignUps config field used for sign-in/sign-up limiter sizing. |
| internal/api/options.go | Adds SignIns limiter and switches Signups to use the new sign-in/sign-up rate limit value. |
| internal/api/token.go | Changes rate limiter selection so password grant uses SignIns, other grants use existing limiters. |
| internal/api/user.go | Creates an email identity on password update when the user has an email but lacks an email identity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if user.GetEmail() != "" { | ||
| if _, terr = models.FindIdentityByIdAndProvider(tx, user.ID.String(), "email"); terr != nil { | ||
| if !models.IsNotFoundError(terr) { | ||
| return apierrors.NewInternalServerError("Error finding email identity").WithInternalError(terr) | ||
| } | ||
| emailIdentity, terr := models.NewIdentity(user, "email", map[string]interface{}{ | ||
| "sub": user.ID.String(), | ||
| "email": user.GetEmail(), | ||
| }) | ||
| if terr != nil { | ||
| return apierrors.NewInternalServerError("Error creating email identity").WithInternalError(terr) | ||
| } | ||
| if terr := tx.Create(emailIdentity); terr != nil { | ||
| return apierrors.NewInternalServerError("Error saving email identity").WithInternalError(terr) | ||
| } | ||
| if terr := user.UpdateAppMetaDataProviders(tx); terr != nil { | ||
| return apierrors.NewInternalServerError("Error updating providers").WithInternalError(terr) | ||
| } | ||
| } |
|
/cc @staaldraad |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| if user.GetEmail() != "" { | ||
| if _, terr = models.FindIdentityByIdAndProvider(tx, user.ID.String(), "email"); terr != nil { | ||
| if !models.IsNotFoundError(terr) { | ||
| return apierrors.NewInternalServerError("Error finding email identity").WithInternalError(terr) | ||
| } | ||
| emailIdentity, terr := models.NewIdentity(user, "email", map[string]interface{}{ |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The code creates an email identity without verifying if the user's email is confirmed. This could allow users with unconfirmed emails from OAuth providers to gain email/password login capabilities without verification. A check for user.EmailConfirmedAt != nil should be added.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a check for user.EmailConfirmedAt != nil alongside the existing user.GetEmail() != "" guard before creating the email identity. This ensures that only users with a verified/confirmed email address can have an email identity created for them. Without this check, an OAuth user whose email was never confirmed could obtain an email+password login capability for an unverified email address.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if user.GetEmail() != "" { | |
| if _, terr = models.FindIdentityByIdAndProvider(tx, user.ID.String(), "email"); terr != nil { | |
| if !models.IsNotFoundError(terr) { | |
| return apierrors.NewInternalServerError("Error finding email identity").WithInternalError(terr) | |
| } | |
| emailIdentity, terr := models.NewIdentity(user, "email", map[string]interface{}{ | |
| if user.GetEmail() != "" && user.EmailConfirmedAt != nil { |
Fixes #2320
Problem
When
updateUseris called with a password on an OAuth-only account, noemailidentity was created inauth.identities. This meant the user could not sign in with email+password even though the password was saved correctly on theusersrow.Fix
Inside the password-update transaction in
UserUpdate, afterUpdatePasswordsucceeds, check whether the user already has anemailidentity. If not (i.e. OAuth-only account), create one using the user's existing confirmed email and callUpdateAppMetaDataProvidersso thatapp_metadata.providersincludes"email".The fix is no-op for users who already have an email identity.
Changes
internal/api/user.go