From 81b349d7926309f068b84fd14ad8ff5edcea26e3 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Fri, 9 Feb 2024 01:34:21 +0800 Subject: [PATCH] fix: only create or update the email / phone identity after it's been verified (again) (#1409) ## What kind of change does this PR introduce? * We only want to create / update the email / phone identity after it's been verified during the email change / phone change flow * Previously, the email / phone identity was created / updated immediately after the user update endpoint is called. This results in the identity being created before it's been confirmed. Like #1403, but adding again. --- internal/api/user.go | 49 +++---------------------- internal/api/verify.go | 82 +++++++++++++++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 62 deletions(-) diff --git a/internal/api/user.go b/internal/api/user.go index 02fd099b2..83d067a26 100644 --- a/internal/api/user.go +++ b/internal/api/user.go @@ -7,9 +7,7 @@ import ( "net/http" "time" - "github.com/fatih/structs" "github.com/gofrs/uuid" - "github.com/supabase/auth/internal/api/provider" "github.com/supabase/auth/internal/api/sms_provider" "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/storage" @@ -193,29 +191,7 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error { } } - var identities []models.Identity if params.Email != "" && params.Email != user.GetEmail() { - identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), "email") - if terr != nil { - if !models.IsNotFoundError(terr) { - return terr - } - // updating the user's email should create a new email identity since the user doesn't have one - identity, terr = a.createNewIdentity(tx, user, "email", structs.Map(provider.Claims{ - Subject: user.ID.String(), - Email: params.Email, - })) - if terr != nil { - return terr - } - } else { - if terr := identity.UpdateIdentityData(tx, map[string]interface{}{ - "email": params.Email, - }); terr != nil { - return terr - } - } - identities = append(identities, *identity) mailer := a.Mailer(ctx) referrer := utilities.GetReferrer(r, config) flowType := getFlowFromChallenge(params.CodeChallenge) @@ -238,29 +214,13 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error { } if params.Phone != "" && params.Phone != user.GetPhone() { - identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), "phone") - if terr != nil { - if !models.IsNotFoundError(terr) { - return terr - } - // updating the user's phone should create a new phone identity since the user doesn't have one - identity, terr = a.createNewIdentity(tx, user, "phone", structs.Map(provider.Claims{ - Subject: user.ID.String(), - Phone: params.Phone, - })) - if terr != nil { - return terr - } - } else { - if terr := identity.UpdateIdentityData(tx, map[string]interface{}{ - "phone": params.Phone, + if config.Sms.Autoconfirm { + if _, terr := a.smsVerify(r, ctx, tx, user, &VerifyParams{ + Type: phoneChangeVerification, + Phone: params.Phone, }); terr != nil { return terr } - } - identities = append(identities, *identity) - if config.Sms.Autoconfirm { - return user.UpdatePhone(tx, params.Phone) } else { smsProvider, terr := sms_provider.GetSmsProvider(*config) if terr != nil { @@ -271,7 +231,6 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error { } } } - user.Identities = append(user.Identities, identities...) if terr = models.NewAuditLogEntry(r, tx, user, models.UserModifiedAction, "", nil); terr != nil { return internalServerError("Error recording audit log entry").WithInternalError(terr) diff --git a/internal/api/verify.go b/internal/api/verify.go index c33806409..ecacc1e9a 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -10,7 +10,9 @@ import ( "strings" "time" + "github.com/fatih/structs" "github.com/sethvargo/go-password/password" + "github.com/supabase/auth/internal/api/provider" "github.com/supabase/auth/internal/api/sms_provider" "github.com/supabase/auth/internal/crypto" "github.com/supabase/auth/internal/models" @@ -258,7 +260,7 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyP return nil } case smsVerification, phoneChangeVerification: - user, terr = a.smsVerify(r, ctx, tx, user, params.Type) + user, terr = a.smsVerify(r, ctx, tx, user, params) default: return unprocessableEntityError("Unsupported verification type") } @@ -367,28 +369,53 @@ func (a *API) recoverVerify(r *http.Request, ctx context.Context, conn *storage. return user, nil } -func (a *API) smsVerify(r *http.Request, ctx context.Context, conn *storage.Connection, user *models.User, otpType string) (*models.User, error) { +func (a *API) smsVerify(r *http.Request, ctx context.Context, conn *storage.Connection, user *models.User, params *VerifyParams) (*models.User, error) { config := a.config err := conn.Transaction(func(tx *storage.Connection) error { - var terr error - if terr = models.NewAuditLogEntry(r, tx, user, models.UserSignedUpAction, "", nil); terr != nil { + if terr := triggerEventHooks(ctx, tx, SignupEvent, user, config); terr != nil { return terr } - if terr = triggerEventHooks(ctx, tx, SignupEvent, user, config); terr != nil { - return terr - } - - if otpType == smsVerification { - if terr = user.ConfirmPhone(tx); terr != nil { + if params.Type == smsVerification { + if terr := models.NewAuditLogEntry(r, tx, user, models.UserSignedUpAction, "", nil); terr != nil { + return terr + } + if terr := user.ConfirmPhone(tx); terr != nil { return internalServerError("Error confirming user").WithInternalError(terr) } - } else if otpType == phoneChangeVerification { - if terr = user.ConfirmPhoneChange(tx); terr != nil { + } else if params.Type == phoneChangeVerification { + if terr := models.NewAuditLogEntry(r, tx, user, models.UserModifiedAction, "", nil); terr != nil { + return terr + } + if identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), "phone"); terr != nil { + if !models.IsNotFoundError(terr) { + return terr + } + // confirming the phone change should create a new phone identity if the user doesn't have one + if _, terr = a.createNewIdentity(tx, user, "phone", structs.Map(provider.Claims{ + Subject: user.ID.String(), + Phone: params.Phone, + PhoneVerified: true, + })); terr != nil { + return terr + } + } else { + if terr := identity.UpdateIdentityData(tx, map[string]interface{}{ + "phone": params.Phone, + "phone_verified": true, + }); terr != nil { + return terr + } + } + if terr := user.ConfirmPhoneChange(tx); terr != nil { return internalServerError("Error confirming user").WithInternalError(terr) } } + + if terr := tx.Load(user, "Identities"); terr != nil { + return internalServerError("Error refetching identities").WithInternalError(terr) + } return nil }) if err != nil { @@ -478,17 +505,38 @@ func (a *API) emailChangeVerify(r *http.Request, ctx context.Context, conn *stor // one email is confirmed at this point if GOTRUE_MAILER_SECURE_EMAIL_CHANGE_ENABLED is enabled err := conn.Transaction(func(tx *storage.Connection) error { - var terr error - - if terr = models.NewAuditLogEntry(r, tx, user, models.UserModifiedAction, "", nil); terr != nil { + if terr := models.NewAuditLogEntry(r, tx, user, models.UserModifiedAction, "", nil); terr != nil { return terr } - if terr = triggerEventHooks(ctx, tx, EmailChangeEvent, user, config); terr != nil { + if terr := triggerEventHooks(ctx, tx, EmailChangeEvent, user, config); terr != nil { return terr } - if terr = user.ConfirmEmailChange(tx, zeroConfirmation); terr != nil { + if identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), "email"); terr != nil { + if !models.IsNotFoundError(terr) { + return terr + } + // confirming the email change should create a new email identity if the user doesn't have one + if _, terr = a.createNewIdentity(tx, user, "email", structs.Map(provider.Claims{ + Subject: user.ID.String(), + Email: params.Email, + EmailVerified: true, + })); terr != nil { + return terr + } + } else { + if terr := identity.UpdateIdentityData(tx, map[string]interface{}{ + "email": params.Email, + "email_verified": true, + }); terr != nil { + return terr + } + } + if terr := tx.Load(user, "Identities"); terr != nil { + return internalServerError("Error refetching identities").WithInternalError(terr) + } + if terr := user.ConfirmEmailChange(tx, zeroConfirmation); terr != nil { return internalServerError("Error confirm email").WithInternalError(terr) }