Skip to content

Commit

Permalink
fix: only create or update the email / phone identity after it's been…
Browse files Browse the repository at this point in the history 
… verified (supabase#1403)

## 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.
  • Loading branch information
@kangmingtay @LashaJini
kangmingtay authored and LashaJini committed Nov 13, 2024
1 parent c9bfc83 commit fd77961
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 62 deletions.
49 changes: 4 additions & 45 deletions internal/api/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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)
Expand Down
82 changes: 65 additions & 17 deletions internal/api/verify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}

Expand Down

0 comments on commit fd77961

Please sign in to comment.