Repository: supabase/gotrue
Pull request #1321 (2023-11-27 15:04:50)

feat: refactor for central password strength check
Refactors all places where the password strength check (right now just length check) is enforced to a single method on the API `checkPasswordStrength`.

To do this, both `SignupParams` and `UserUpdateParams` had to be reworked. Furthermore user update now splits basic validation logic from user update validation logic and the main updating transaction which should drastically speed up the method itself. 


Patch:
diff --git a/internal/api/admin.go b/internal/api/admin.go
index 024d6ecb..fa8132e3 100644
--- a/internal/api/admin.go
+++ b/internal/api/admin.go
@@ -3,7 +3,6 @@ package api
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"net/http"
 	"strings"
 	"time"
@@ -144,7 +143,6 @@ func (a *API) adminUserUpdate(w http.ResponseWriter, r *http.Request) error {
 	user := getUser(ctx)
 	adminUser := getAdminUser(ctx)
 	params, err := a.getAdminParams(r)
-	config := a.config
 	if err != nil {
 		return err
 	}
@@ -176,6 +174,18 @@ func (a *API) adminUserUpdate(w http.ResponseWriter, r *http.Request) error {
 		}
 	}
 
+	if params.Password != nil {
+		password := *params.Password
+
+		if err := a.checkPasswordStrength(ctx, password); err != nil {
+			return err
+		}
+
+		if err := user.SetPassword(ctx, password); err != nil {
+			return err
+		}
+	}
+
 	err = db.Transaction(func(tx *storage.Connection) error {
 		if params.Role != "" {
 			if terr := user.SetRole(tx, params.Role); terr != nil {
@@ -196,11 +206,7 @@ func (a *API) adminUserUpdate(w http.ResponseWriter, r *http.Request) error {
 		}
 
 		if params.Password != nil {
-			if len(*params.Password) < config.PasswordMinLength {
-				return invalidPasswordLengthError(config.PasswordMinLength)
-			}
-
-			if terr := user.UpdatePassword(tx, *params.Password, nil); terr != nil {
+			if terr := user.UpdatePassword(tx, nil); terr != nil {
 				return terr
 			}
 		}
@@ -284,9 +290,6 @@ func (a *API) adminUserUpdate(w http.ResponseWriter, r *http.Request) error {
 	})
 
 	if err != nil {
-		if errors.Is(err, invalidPasswordLengthError(config.PasswordMinLength)) {
-			return err
-		}
 		return internalServerError("Error updating user").WithInternalError(err)
 	}
 
diff --git a/internal/api/mail.go b/internal/api/mail.go
index d2de1068..786496d5 100644
--- a/internal/api/mail.go
+++ b/internal/api/mail.go
@@ -97,12 +97,6 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
 
 	var signupUser *models.User
 	if params.Type == signupVerification && user == nil {
-		if params.Password == "" {
-			return unprocessableEntityError("Signup requires a valid password")
-		}
-		if len(params.Password) < config.PasswordMinLength {
-			return invalidPasswordLengthError(config.PasswordMinLength)
-		}
 		signupParams := &SignupParams{
 			Email:    params.Email,
 			Password: params.Password,
@@ -111,6 +105,10 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
 			Aud:      aud,
 		}
 
+		if err := a.validateSignupParams(ctx, signupParams); err != nil {
+			return err
+		}
+
 		signupUser, err = signupParams.ToUserModel(false /* <- isSSOUser */)
 		if err != nil {
 			return err
diff --git a/internal/api/password.go b/internal/api/password.go
new file mode 100644
index 00000000..7b98d4ee
--- /dev/null
+++ b/internal/api/password.go
@@ -0,0 +1,13 @@
+package api
+
+import "context"
+
+func (a *API) checkPasswordStrength(ctx context.Context, password string) error {
+	config := a.config
+
+	if len(password) < config.PasswordMinLength {
+		return invalidPasswordLengthError(config.PasswordMinLength)
+	}
+
+	return nil
+}
diff --git a/internal/api/signup.go b/internal/api/signup.go
index f392f600..ff403010 100644
--- a/internal/api/signup.go
+++ b/internal/api/signup.go
@@ -31,17 +31,20 @@ type SignupParams struct {
 	CodeChallenge       string                 `json:"code_challenge"`
 }
 
-func (p *SignupParams) Validate(passwordMinLength int, smsProvider string) error {
+func (a *API) validateSignupParams(ctx context.Context, p *SignupParams) error {
+	config := a.config
+
 	if p.Password == "" {
 		return unprocessableEntityError("Signup requires a valid password")
 	}
-	if len(p.Password) < passwordMinLength {
-		return invalidPasswordLengthError(passwordMinLength)
+
+	if err := a.checkPasswordStrength(ctx, p.Password); err != nil {
+		return err
 	}
 	if p.Email != "" && p.Phone != "" {
 		return unprocessableEntityError("Only an email address or phone number should be provided on signup.")
 	}
-	if p.Provider == "phone" && !sms_provider.IsValidMessageChannel(p.Channel, smsProvider) {
+	if p.Provider == "phone" && !sms_provider.IsValidMessageChannel(p.Channel, config.Sms.Provider) {
 		return badRequestError(InvalidChannelError)
 	}
 	// PKCE not needed as phone signups already return access token in body
@@ -123,8 +126,10 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 	if err := json.Unmarshal(body, params); err != nil {
 		return badRequestError("Could not read Signup params: %v", err)
 	}
+
 	params.ConfigureDefaults()
-	if err := params.Validate(config.PasswordMinLength, config.Sms.Provider); err != nil {
+
+	if err := a.validateSignupParams(ctx, params); err != nil {
 		return err
 	}
 
diff --git a/internal/api/token.go b/internal/api/token.go
index 30f82c37..f4d5584d 100644
--- a/internal/api/token.go
+++ b/internal/api/token.go
@@ -140,7 +140,7 @@ func (a *API) ResourceOwnerPasswordGrant(ctx context.Context, w http.ResponseWri
 		return internalServerError("Database error querying schema").WithInternalError(err)
 	}
 
-	if user.IsBanned() || !user.Authenticate(params.Password) {
+	if user.IsBanned() || !user.Authenticate(ctx, params.Password) {
 		return oauthError("invalid_grant", InvalidLoginMessage)
 	}
 
diff --git a/internal/api/user.go b/internal/api/user.go
index 78c7684c..70577396 100644
--- a/internal/api/user.go
+++ b/internal/api/user.go
@@ -1,6 +1,7 @@
 package api
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"net/http"
@@ -10,9 +11,7 @@ import (
 	"github.com/gofrs/uuid"
 	"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"
 	"github.com/supabase/gotrue/internal/utilities"
 )
@@ -30,58 +29,35 @@ type UserUpdateParams struct {
 	CodeChallengeMethod string                 `json:"code_challenge_method"`
 }
 
-func (p *UserUpdateParams) Validate(tx *storage.Connection, user *models.User, aud string, config *conf.GlobalConfiguration) error {
+func (a *API) validateUserUpdateParams(ctx context.Context, p *UserUpdateParams) error {
+	config := a.config
+
 	var err error
-	if p.Email != "" && p.Email != user.GetEmail() {
+	if p.Email != "" {
 		p.Email, err = validateEmail(p.Email)
 		if err != nil {
 			return err
 		}
-		if duplicateUser, err := models.IsDuplicatedEmail(tx, 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.Phone, err = validatePhone(p.Phone); err != nil {
+			return err
+		}
 		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(tx, 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 {
-		password := *p.Password
-
-		if len(password) < config.PasswordMinLength {
-			return invalidPasswordLengthError(config.PasswordMinLength)
-		}
 
-		if user.EncryptedPassword != "" && user.Authenticate(password) {
-			return unprocessableEntityError("New password should be different from the old password.")
-		}
-	}
-	if p.AppData != nil {
-		if !isAdmin(user, config) {
-			return unauthorizedError("Updating app_metadata requires admin privileges")
+	if p.Password != nil {
+		if err := a.checkPasswordStrength(ctx, *p.Password); err != nil {
+			return err
 		}
 	}
+
 	return nil
 }
 
@@ -122,37 +98,84 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
 
 	user := getUser(ctx)
 	session := getSession(ctx)
-	log := observability.GetLogEntry(r)
-	log.Debugf("Checking params for token %v", params)
 
-	if err := params.Validate(a.db, user, aud, config); err != nil {
+	if err := a.validateUserUpdateParams(ctx, params); err != nil {
 		return err
 	}
 
-	err = db.Transaction(func(tx *storage.Connection) error {
-		var terr error
-		if params.Password != nil {
-			if config.Security.UpdatePasswordRequireReauthentication {
-				now := time.Now()
-				// we require reauthentication if the user hasn't signed in recently in the current session
-				if session == nil || now.After(session.CreatedAt.Add(24*time.Hour)) {
-					if len(params.Nonce) == 0 {
-						return badRequestError("Password update requires reauthentication")
-					}
-					if terr = a.verifyReauthentication(params.Nonce, tx, config, user); terr != nil {
-						return terr
-					}
+	if params.AppData != nil && !isAdmin(user, config) {
+		if !isAdmin(user, config) {
+			return unauthorizedError("Updating app_metadata requires admin privileges")
+		}
+	}
+
+	if user.IsSSOUser {
+		updatingForbiddenFields := false
+
+		updatingForbiddenFields = updatingForbiddenFields || (params.Password != nil && *params.Password != "")
+		updatingForbiddenFields = updatingForbiddenFields || (params.Email != "" && params.Email != user.GetEmail())
+		updatingForbiddenFields = updatingForbiddenFields || (params.Phone != "" && params.Phone != user.GetPhone())
+		updatingForbiddenFields = updatingForbiddenFields || (params.Nonce != "")
+
+		if updatingForbiddenFields {
+			return unprocessableEntityError("Updating email, phone, password of a SSO account only possible via SSO")
+		}
+	}
+
+	if params.Email != "" && user.GetEmail() != params.Email {
+		if duplicateUser, err := models.IsDuplicatedEmail(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 != "" && user.GetPhone() != params.Phone {
+		if exists, err := models.IsDuplicatedPhone(db, params.Phone, aud); err != nil {
+			return internalServerError("Database error checking phone").WithInternalError(err)
+		} else if exists {
+			return unprocessableEntityError(DuplicatePhoneMsg)
+		}
+	}
+
+	if params.Password != nil {
+		if config.Security.UpdatePasswordRequireReauthentication {
+			now := time.Now()
+			// we require reauthentication if the user hasn't signed in recently in the current session
+			if session == nil || now.After(session.CreatedAt.Add(24*time.Hour)) {
+				if len(params.Nonce) == 0 {
+					return badRequestError("Password update requires reauthentication")
 				}
+				if err := a.verifyReauthentication(params.Nonce, db, config, user); err != nil {
+					return err
+				}
+			}
+		}
+
+		password := *params.Password
+		if password != "" {
+			if user.EncryptedPassword != "" && user.Authenticate(ctx, password) {
+				return unprocessableEntityError("New password should be different from the old password.")
 			}
+		}
 
+		if err := user.SetPassword(ctx, password); err != nil {
+			return err
+		}
+	}
+
+	err = db.Transaction(func(tx *storage.Connection) error {
+		var terr error
+		if params.Password != nil {
 			var sessionID *uuid.UUID
 			if session != nil {
 				sessionID = &session.ID
 			}
 
-			if terr = user.UpdatePassword(tx, *params.Password, sessionID); terr != nil {
+			if terr = user.UpdatePassword(tx, sessionID); terr != nil {
 				return internalServerError("Error during password storage").WithInternalError(terr)
 			}
+
 			if terr := models.NewAuditLogEntry(r, tx, user, models.UserUpdatePasswordAction, "", nil); terr != nil {
 				return terr
 			}
diff --git a/internal/api/verify.go b/internal/api/verify.go
index 8afe6293..ae0b26ae 100644
--- a/internal/api/verify.go
+++ b/internal/api/verify.go
@@ -291,19 +291,24 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyP
 func (a *API) signupVerify(r *http.Request, ctx context.Context, conn *storage.Connection, user *models.User) (*models.User, error) {
 	config := a.config
 
+	if user.EncryptedPassword == "" && user.InvitedAt != nil {
+		// sign them up with temporary password, and require application
+		// to present the user with a password set form
+		password, err := password.Generate(64, 10, 0, false, true)
+		if err != nil {
+			return nil, err
+		}
+
+		if err := user.SetPassword(ctx, password); err != nil {
+			return nil, err
+		}
+	}
+
 	err := conn.Transaction(func(tx *storage.Connection) error {
 		var terr error
-		if user.EncryptedPassword == "" {
-			if user.InvitedAt != nil {
-				// sign them up with temporary password, and require application
-				// to present the user with a password set form
-				password, err := password.Generate(64, 10, 0, false, true)
-				if err != nil {
-					internalServerError("error creating user").WithInternalError(err)
-				}
-				if terr = user.UpdatePassword(tx, password, nil); terr != nil {
-					return internalServerError("Error storing password").WithInternalError(terr)
-				}
+		if user.EncryptedPassword == "" && user.InvitedAt != nil {
+			if terr = user.UpdatePassword(tx, nil); terr != nil {
+				return internalServerError("Error storing password").WithInternalError(terr)
 			}
 		}
 
diff --git a/internal/models/user.go b/internal/models/user.go
index 1b59ed95..d52a7047 100644
--- a/internal/models/user.go
+++ b/internal/models/user.go
@@ -239,13 +239,24 @@ func (u *User) SetPhone(tx *storage.Connection, phone string) error {
 	return tx.UpdateOnly(u, "phone")
 }
 
-// UpdatePassword updates the user's password
-func (u *User) UpdatePassword(tx *storage.Connection, password string, sessionID *uuid.UUID) error {
-	pw, err := crypto.GenerateFromPassword(context.Background(), password)
+func (u *User) SetPassword(ctx context.Context, password string) error {
+	if password == "" {
+		u.EncryptedPassword = ""
+		return nil
+	}
+
+	pw, err := crypto.GenerateFromPassword(ctx, password)
 	if err != nil {
 		return err
 	}
+
 	u.EncryptedPassword = pw
+
+	return nil
+}
+
+// UpdatePassword updates the user's password. Use SetPassword outside of a transaction first!
+func (u *User) UpdatePassword(tx *storage.Connection, sessionID *uuid.UUID) error {
 	if err := tx.UpdateOnly(u, "encrypted_password"); err != nil {
 		return err
 	}
@@ -266,8 +277,8 @@ func (u *User) UpdatePhone(tx *storage.Connection, phone string) error {
 }
 
 // Authenticate a user from a password
-func (u *User) Authenticate(password string) bool {
-	err := crypto.CompareHashAndPassword(context.Background(), u.EncryptedPassword, password)
+func (u *User) Authenticate(ctx context.Context, password string) bool {
+	err := crypto.CompareHashAndPassword(ctx, u.EncryptedPassword, password)
 	return err == nil
 }
 


Test patch:
diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go
index fb7f67a3..ea2fab89 100644
--- a/internal/api/admin_test.go
+++ b/internal/api/admin_test.go
@@ -350,7 +350,7 @@ func (ts *AdminTestSuite) TestAdminUserCreate() {
 				expectedPassword = fmt.Sprintf("%v", c.params["password"])
 			}
 
-			assert.Equal(ts.T(), c.expected["isAuthenticated"], u.Authenticate(expectedPassword))
+			assert.Equal(ts.T(), c.expected["isAuthenticated"], u.Authenticate(context.Background(), expectedPassword))
 
 			// remove created user after each case
 			require.NoError(ts.T(), ts.API.db.Destroy(u))
diff --git a/internal/api/user_test.go b/internal/api/user_test.go
index 2297a128..43046523 100644
--- a/internal/api/user_test.go
+++ b/internal/api/user_test.go
@@ -2,6 +2,7 @@ package api
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
@@ -306,7 +307,7 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
 			u, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
 			require.NoError(ts.T(), err)
 
-			require.Equal(ts.T(), c.expected.isAuthenticated, u.Authenticate(c.newPassword))
+			require.Equal(ts.T(), c.expected.isAuthenticated, u.Authenticate(context.Background(), c.newPassword))
 		})
 	}
 }
@@ -363,7 +364,7 @@ func (ts *UserTestSuite) TestUserUpdatePasswordReauthentication() {
 	u, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
 	require.NoError(ts.T(), err)
 
-	require.True(ts.T(), u.Authenticate("newpass"))
+	require.True(ts.T(), u.Authenticate(context.Background(), "newpass"))
 	require.Empty(ts.T(), u.ReauthenticationToken)
 	require.NotEmpty(ts.T(), u.ReauthenticationSentAt)
 }
diff --git a/internal/models/linking_test.go b/internal/models/linking_test.go
index d2e46944..5c68ac43 100644
--- a/internal/models/linking_test.go
+++ b/internal/models/linking_test.go
@@ -81,7 +81,7 @@ func (ts *AccountLinkingTestSuite) TestCreateAccountDecisionWithAccounts() {
 
 	// when the email doesn't exist in the system -- conventional provider
 	decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{
-		provider.Email{
+		{
 			Email:    "other@example.com",
 			Verified: true,
 			Primary:  true,
@@ -93,7 +93,7 @@ func (ts *AccountLinkingTestSuite) TestCreateAccountDecisionWithAccounts() {
 
 	// when looking for an email that doesn't exist in the SSO linking domain
 	decision, err = DetermineAccountLinking(ts.db, ts.config, []provider.Email{
-		provider.Email{
+		{
 			Email:    "other@samltest.id",
 			Verified: true,
 			Primary:  true,
@@ -116,7 +116,7 @@ func (ts *AccountLinkingTestSuite) TestAccountExists() {
 	require.NoError(ts.T(), ts.db.Create(identityA))
 
 	decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{
-		provider.Email{
+		{
 			Email:    "test@example.com",
 			Verified: true,
 			Primary:  true,
@@ -299,7 +299,7 @@ func (ts *AccountLinkingTestSuite) TestMultipleAccounts() {
 	// identities in the same "default" linking domain with the same email
 	// address pointing to two different user accounts
 	decision, err := DetermineAccountLinking(ts.db, ts.config, []provider.Email{
-		provider.Email{
+		{
 			Email:    "test@example.com",
 			Verified: true,
 			Primary:  true,