Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: validate update user params separately #1144

Merged
merged 5 commits into from
Jun 16, 2023
Merged

refactor: validate update user params separately #1144

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
298d1de
fix: duplicate identity error on update user
kangmingtay Jun 15, 2023
e19160b
fix: use raw query instead of updateonly
kangmingtay Jun 16, 2023
b2d895f
fix tests
kangmingtay Jun 16, 2023
2f2671e
refactor: validate update user params separately
kangmingtay Jun 16, 2023
4ac8d02
Merge branch 'master' into km/refactor-update-user
kangmingtay Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions internal/api/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,8 @@ func sendJSON(w http.ResponseWriter, status int, obj interface{}) error {
return err
}

func (a *API) isAdmin(ctx context.Context, u *models.User, aud string) bool {
config := a.config
if aud == "" {
aud = config.JWT.Aud
}
return aud == u.Aud && u.HasRole(config.JWT.AdminGroupName)
func isAdmin(u *models.User, config *conf.GlobalConfiguration) bool {
return config.JWT.Aud == u.Aud && u.HasRole(config.JWT.AdminGroupName)
}

func (a *API) requestAud(ctx context.Context, r *http.Request) string {
Expand Down
176 changes: 97 additions & 79 deletions internal/api/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/fatih/structs"
"github.com/supabase/gotrue/internal/api/provider"
"github.com/supabase/gotrue/internal/api/sms_provider"
"github.com/supabase/gotrue/internal/conf"
"github.com/supabase/gotrue/internal/models"
"github.com/supabase/gotrue/internal/observability"
"github.com/supabase/gotrue/internal/storage"
Expand All @@ -26,6 +27,59 @@ type UserUpdateParams struct {
CodeChallengeMethod string `json:"code_challenge_method"`
}

func (p *UserUpdateParams) Validate(conn *storage.Connection, user *models.User, aud string, config *conf.GlobalConfiguration) error {
var err error
if p.Email != "" && p.Email != user.GetEmail() {
p.Email, err = validateEmail(p.Email)
if err != nil {
return err
}
if duplicateUser, err := models.IsDuplicatedEmail(conn, p.Email, aud, user); err != nil {
return internalServerError("Database error checking email").WithInternalError(err)
} else if duplicateUser != nil {
return unprocessableEntityError(DuplicateEmailMsg)
}
}
if p.Phone != "" {
if p.Channel == "" {
p.Channel = sms_provider.SMSProvider
}
if !sms_provider.IsValidMessageChannel(p.Channel, config.Sms.Provider) {
return badRequestError(InvalidChannelError)
}
if p.Phone != user.GetPhone() {
if p.Phone, err = validatePhone(p.Phone); err != nil {
return err
}
if exists, err := models.IsDuplicatedPhone(conn, p.Phone, aud); err != nil {
return internalServerError("Database error checking phone").WithInternalError(err)
} else if exists {
return unprocessableEntityError(DuplicatePhoneMsg)
}
}
}
if user.IsSSOUser {
if (p.Password != nil && *p.Password != "") || p.Email != "" || p.Phone != "" || p.Nonce != "" {
return unprocessableEntityError("Updating email, phone, password of a SSO account only possible via SSO")
}
}
if p.Password != nil {
if len(*p.Password) < config.PasswordMinLength {
return invalidPasswordLengthError(config.PasswordMinLength)
}
// if password reauthentication is enabled, user can only update password together with a nonce sent
if config.Security.UpdatePasswordRequireReauthentication && p.Nonce == "" {
return unauthorizedError("Password update requires reauthentication.")
}
}
if p.AppData != nil {
if !isAdmin(user, config) {
return unauthorizedError("Updating app_metadata requires admin privileges")
}
}
return nil
}

// UserGet returns a user
func (a *API) UserGet(w http.ResponseWriter, r *http.Request) error {
ctx := r.Context()
Expand Down Expand Up @@ -66,82 +120,32 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
log := observability.GetLogEntry(r)
log.Debugf("Checking params for token %v", params)

if params.Email != "" && params.Email != user.GetEmail() {
params.Email, err = validateEmail(params.Email)
if err != nil {
return err
}
var duplicateUser *models.User
if duplicateUser, err = models.IsDuplicatedEmail(a.db, params.Email, aud, user); err != nil {
return internalServerError("Database error checking email").WithInternalError(err)
} else if duplicateUser != nil {
return unprocessableEntityError(DuplicateEmailMsg)
}
}
if params.Phone != "" && params.Channel == "" {
params.Channel = sms_provider.SMSProvider
}
if params.Phone != "" && !sms_provider.IsValidMessageChannel(params.Channel, config.Sms.Provider) {
return badRequestError(InvalidChannelError)
}

if params.Phone != "" && params.Phone != user.GetPhone() {
params.Phone, err = validatePhone(params.Phone)
if err != nil {
return err
}
var exists bool
if exists, err = models.IsDuplicatedPhone(a.db, params.Phone, aud); err != nil {
return internalServerError("Database error checking phone").WithInternalError(err)
} else if exists {
return unprocessableEntityError(DuplicatePhoneMsg)
}
}

if user.IsSSOUser {
if (params.Password != nil && *params.Password != "") || params.Email != "" || params.Phone != "" || params.Nonce != "" {
return unprocessableEntityError("Updating email, phone, password of a SSO account only possible via SSO")
}
if err := params.Validate(a.db, user, aud, config); err != nil {
return err
}

err = db.Transaction(func(tx *storage.Connection) error {
var terr error
if params.Password != nil {
if len(*params.Password) < config.PasswordMinLength {
return invalidPasswordLengthError(config.PasswordMinLength)
}

isPasswordUpdated := false
if !config.Security.UpdatePasswordRequireReauthentication {
if terr = user.UpdatePassword(tx, *params.Password); terr != nil {
return internalServerError("Error during password storage").WithInternalError(terr)
}
isPasswordUpdated = true
} else if params.Nonce == "" {
return unauthorizedError("Password update requires reauthentication.")
} else {
if config.Security.UpdatePasswordRequireReauthentication {
if terr = a.verifyReauthentication(params.Nonce, tx, config, user); terr != nil {
return terr
}
if terr = user.UpdatePassword(tx, *params.Password); terr != nil {
return internalServerError("Error during password storage").WithInternalError(terr)
}
isPasswordUpdated = true
}

if isPasswordUpdated {
if terr := models.NewAuditLogEntry(r, tx, user, models.UserUpdatePasswordAction, "", nil); terr != nil {
if terr = user.UpdatePassword(tx, *params.Password); terr != nil {
return internalServerError("Error during password storage").WithInternalError(terr)
}
if terr := models.NewAuditLogEntry(r, tx, user, models.UserUpdatePasswordAction, "", nil); terr != nil {
return terr
}
if session != nil {
if terr = models.LogoutAllExceptMe(tx, session.ID, user.ID); terr != nil {
return terr
}
if session != nil {
if terr = models.LogoutAllExceptMe(tx, session.ID, user.ID); terr != nil {
return terr
}
} else {
// logout all sessions if session id is missing
if terr = models.Logout(tx, user.ID); terr != nil {
return terr
}
} else {
// logout all sessions if session id is missing
if terr = models.Logout(tx, user.ID); terr != nil {
return terr
}
}
}
Expand All @@ -153,29 +157,34 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
}

if params.AppData != nil {
if !a.isAdmin(ctx, user, config.JWT.Aud) {
return unauthorizedError("Updating app_metadata requires admin privileges")
}

if terr = user.UpdateAppMetaData(tx, params.AppData); terr != nil {
return internalServerError("Error updating user").WithInternalError(terr)
}
}

var identities []models.Identity
if params.Email != "" && params.Email != user.GetEmail() {
if user.GetEmail() == "" {
// if the user doesn't have an existing email
// then updating the user's email should create a new email identity
identity, terr := a.createNewIdentity(tx, user, "email", structs.Map(provider.Claims{
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
}
identities = append(identities, *identity)
} 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 := a.getReferrer(r)
flowType := getFlowFromChallenge(params.CodeChallenge)
Expand All @@ -198,18 +207,27 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
}

if params.Phone != "" && params.Phone != user.GetPhone() {
if user.GetPhone() == "" {
// if the user doesn't have an existing phone
// then updating the user's phone should create a new phone identity
identity, terr := a.createNewIdentity(tx, user, "phone", structs.Map(provider.Claims{
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
}
identities = append(identities, *identity)
} else {
if terr := identity.UpdateIdentityData(tx, map[string]interface{}{
"phone": params.Phone,
}); terr != nil {
return terr
}
}
identities = append(identities, *identity)
if config.Sms.Autoconfirm {
return user.UpdatePhone(tx, params.Phone)
} else {
Expand Down
8 changes: 7 additions & 1 deletion internal/models/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,11 @@ func (i *Identity) UpdateIdentityData(tx *storage.Connection, updates map[string
}
}
}
return tx.UpdateOnly(i, "identity_data")
// pop doesn't support updates on tables with composite primary keys so we use a raw query here.
return tx.RawQuery(
"update "+(&pop.Model{Value: Identity{}}).TableName()+" set identity_data = ? where provider = ? and id = ?",
i.IdentityData,
i.Provider,
i.ID,
).Exec()
}