Repository: supabase/gotrue
Pull request #1087 (2023-04-17 04:59:49)

refactor: simplify MFA checks
## What kind of change does this PR introduce?

The current MFA checks are quite unreadable. Have refactored parts of it and I think there's more to refactor.





Patch:
diff --git a/cmd/admin_cmd.go b/cmd/admin_cmd.go
index 5537ced7..8f4e80ee 100644
--- a/cmd/admin_cmd.go
+++ b/cmd/admin_cmd.go
@@ -66,7 +66,7 @@ func adminCreateUser(config *conf.GlobalConfiguration, args []string) {
 	defer db.Close()
 
 	aud := getAudience(config)
-	if user, err := models.IsDuplicatedEmail(db, args[0], aud); user != nil {
+	if user, err := models.IsDuplicatedEmail(db, args[0], aud, nil); user != nil {
 		logrus.Fatalf("Error creating new user: user already exists")
 	} else if err != nil {
 		logrus.Fatalf("Error checking user email: %+v", err)
diff --git a/internal/api/admin.go b/internal/api/admin.go
index 38f5ddbd..81ac549e 100644
--- a/internal/api/admin.go
+++ b/internal/api/admin.go
@@ -320,7 +320,7 @@ func (a *API) adminUserCreate(w http.ResponseWriter, r *http.Request) error {
 		if err != nil {
 			return err
 		}
-		if user, err := models.IsDuplicatedEmail(db, params.Email, aud); err != nil {
+		if user, err := models.IsDuplicatedEmail(db, params.Email, aud, nil); err != nil {
 			return internalServerError("Database error checking email").WithInternalError(err)
 		} else if user != nil {
 			return unprocessableEntityError(DuplicateEmailMsg)
diff --git a/internal/api/external.go b/internal/api/external.go
index 05ac139e..ceb15353 100644
--- a/internal/api/external.go
+++ b/internal/api/external.go
@@ -225,9 +225,7 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 	if flowState != nil {
 		// This means that the callback is using PKCE
 		// Set the flowState.AuthCode to the query param here
-		q := url.Values{}
-		q.Set("code", flowState.AuthCode)
-		rurl += "?" + q.Encode()
+		rurl = a.prepPKCERedirectURL(rurl, flowState.AuthCode)
 	} else if token != nil {
 		q := url.Values{}
 		q.Set("provider_token", providerAccessToken)
diff --git a/internal/api/mail.go b/internal/api/mail.go
index f4fa1b24..c1f15803 100644
--- a/internal/api/mail.go
+++ b/internal/api/mail.go
@@ -149,7 +149,7 @@ func (a *API) GenerateLink(w http.ResponseWriter, r *http.Request) error {
 					return unprocessableEntityError("Signup requires a valid password")
 				}
 				if len(params.Password) < config.PasswordMinLength {
-					return unprocessableEntityError(fmt.Sprintf("Password should be at least %d characters", config.PasswordMinLength))
+					return invalidPasswordLengthError(config.PasswordMinLength)
 				}
 				signupParams := &SignupParams{
 					Email:    params.Email,
@@ -182,7 +182,7 @@ func (a *API) GenerateLink(w http.ResponseWriter, r *http.Request) error {
 			if terr != nil {
 				return unprocessableEntityError("The new email address provided is invalid")
 			}
-			if duplicateUser, terr := models.IsDuplicatedEmail(tx, params.NewEmail, user.Aud); terr != nil {
+			if duplicateUser, terr := models.IsDuplicatedEmail(tx, params.NewEmail, user.Aud, user); terr != nil {
 				return internalServerError("Database error checking email").WithInternalError(terr)
 			} else if duplicateUser != nil {
 				return unprocessableEntityError(DuplicateEmailMsg)
@@ -340,6 +340,9 @@ func (a *API) sendMagicLink(tx *storage.Connection, u *models.User, mailer maile
 // sendEmailChange sends out an email change token to the new email.
 func (a *API) sendEmailChange(tx *storage.Connection, config *conf.GlobalConfiguration, u *models.User, mailer mailer.Mailer, email string, referrerURL string, otpLength int) error {
 	var err error
+	if u.EmailChangeSentAt != nil && !u.EmailChangeSentAt.Add(config.SMTP.MaxFrequency).Before(time.Now()) {
+		return MaxFrequencyLimitError
+	}
 	otpNew, err := crypto.GenerateOtp(otpLength)
 	if err != nil {
 		return err
diff --git a/internal/api/mfa.go b/internal/api/mfa.go
index 1605bcf8..dc555aee 100644
--- a/internal/api/mfa.go
+++ b/internal/api/mfa.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"net/http"
-	"time"
 
 	"net/url"
 
@@ -78,8 +77,7 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 		return unprocessableEntityError("MFA enrollment only supported for non-SSO users at this time")
 	}
 
-	factorType := params.FactorType
-	if factorType != models.TOTP {
+	if params.FactorType != models.TOTP {
 		return badRequestError("factor_type needs to be totp")
 	}
 
@@ -105,7 +103,7 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 
 	numVerifiedFactors := 0
 	for _, factor := range factors {
-		if factor.Status == models.FactorStateVerified.String() {
+		if factor.IsVerified() {
 			numVerifiedFactors += 1
 		}
 	}
@@ -190,11 +188,9 @@ func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error {
 		return err
 	}
 
-	creationTime := challenge.CreatedAt
-	expiryTime := creationTime.Add(time.Second * time.Duration(config.MFA.ChallengeExpiryDuration))
 	return sendJSON(w, http.StatusOK, &ChallengeFactorResponse{
 		ID:        challenge.ID,
-		ExpiresAt: expiryTime.Unix(),
+		ExpiresAt: challenge.GetExpiryTime(config.MFA.ChallengeExpiryDuration).Unix(),
 	})
 }
 
@@ -217,7 +213,7 @@ func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error {
 		return badRequestError("invalid body: unable to parse JSON").WithInternalError(err)
 	}
 
-	if factor.UserID != user.ID {
+	if !factor.IsOwnedBy(user) {
 		return internalServerError(InvalidFactorOwnerErrorMessage)
 	}
 
@@ -233,8 +229,7 @@ func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error {
 		return badRequestError("Challenge and verify IP addresses mismatch")
 	}
 
-	hasExpired := time.Now().After(challenge.CreatedAt.Add(time.Second * time.Duration(config.MFA.ChallengeExpiryDuration)))
-	if hasExpired {
+	if challenge.HasExpired(config.MFA.ChallengeExpiryDuration) {
 		err := a.db.Transaction(func(tx *storage.Connection) error {
 			if terr := tx.Destroy(challenge); terr != nil {
 				return internalServerError("Database error deleting challenge").WithInternalError(terr)
@@ -264,7 +259,7 @@ func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error {
 		if terr = challenge.Verify(tx); terr != nil {
 			return terr
 		}
-		if factor.Status != models.FactorStateVerified.String() {
+		if !factor.IsVerified() {
 			if terr = factor.UpdateStatus(tx, models.FactorStateVerified); terr != nil {
 				return terr
 			}
@@ -306,10 +301,10 @@ func (a *API) UnenrollFactor(w http.ResponseWriter, r *http.Request) error {
 	factor := getFactor(ctx)
 	session := getSession(ctx)
 
-	if factor.Status == models.FactorStateVerified.String() && session.GetAAL() != models.AAL2.String() {
+	if factor.IsVerified() && !session.IsAAL2() {
 		return badRequestError("AAL2 required to unenroll verified factor")
 	}
-	if factor.UserID != user.ID {
+	if !factor.IsOwnedBy(user) {
 		return internalServerError(InvalidFactorOwnerErrorMessage)
 	}
 
diff --git a/internal/api/signup.go b/internal/api/signup.go
index c14443ff..c643efde 100644
--- a/internal/api/signup.go
+++ b/internal/api/signup.go
@@ -35,7 +35,7 @@ func (p *SignupParams) Validate(passwordMinLength int, smsProvider string) error
 		return unprocessableEntityError("Signup requires a valid password")
 	}
 	if len(p.Password) < passwordMinLength {
-		return unprocessableEntityError(fmt.Sprintf("Password should be at least %d characters", passwordMinLength))
+		return invalidPasswordLengthError(passwordMinLength)
 	}
 	if p.Email != "" && p.Phone != "" {
 		return unprocessableEntityError("Only an email address or phone number should be provided on signup.")
@@ -122,7 +122,7 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 		if err != nil {
 			return err
 		}
-		user, err = models.IsDuplicatedEmail(db, params.Email, params.Aud)
+		user, err = models.IsDuplicatedEmail(db, params.Email, params.Aud, nil)
 	case "phone":
 		if !config.External.Phone.Enabled {
 			return badRequestError("Phone signups are disabled")
diff --git a/internal/api/token.go b/internal/api/token.go
index b3520586..491830b2 100644
--- a/internal/api/token.go
+++ b/internal/api/token.go
@@ -600,7 +600,7 @@ func (a *API) PKCE(ctx context.Context, w http.ResponseWriter, r *http.Request)
 		return err
 	}
 	if flowState.IsExpired(a.config.External.FlowStateExpiryDuration) {
-		return forbiddenError("invalid oauth state, oauth state has expired")
+		return forbiddenError("invalid flow state, flow state has expired")
 	}
 
 	user, err := models.FindUserByID(db, *flowState.UserID)
diff --git a/internal/api/user.go b/internal/api/user.go
index 84643d32..753b2f01 100644
--- a/internal/api/user.go
+++ b/internal/api/user.go
@@ -2,6 +2,7 @@ package api
 
 import (
 	"encoding/json"
+	"errors"
 	"net/http"
 
 	"github.com/fatih/structs"
@@ -69,7 +70,7 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
 			return err
 		}
 		var duplicateUser *models.User
-		if duplicateUser, err = models.IsDuplicatedEmail(a.db, params.Email, aud); err != nil {
+		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)
@@ -176,6 +177,9 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
 			mailer := a.Mailer(ctx)
 			referrer := a.getReferrer(r)
 			if terr = a.sendEmailChange(tx, config, user, mailer, params.Email, referrer, config.Mailer.OtpLength); terr != nil {
+				if errors.Is(terr, MaxFrequencyLimitError) {
+					return tooManyRequestsError("For security purposes, you can only request this once every 60 seconds")
+				}
 				return internalServerError("Error sending change email").WithInternalError(terr)
 			}
 		}
diff --git a/internal/api/verify.go b/internal/api/verify.go
index 55cf58a3..0df7b24b 100644
--- a/internal/api/verify.go
+++ b/internal/api/verify.go
@@ -167,9 +167,7 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request) error {
 		q.Set("type", params.Type)
 		rurl = token.AsRedirectURL(rurl, q)
 	} else if isPKCEFlow(flowType) {
-		q := url.Values{}
-		q.Set("code", authCode)
-		rurl += "?" + q.Encode()
+		rurl = a.prepPKCERedirectURL(rurl, authCode)
 	}
 	http.Redirect(w, r, rurl, http.StatusSeeOther)
 	return nil
@@ -372,6 +370,12 @@ func (a *API) prepRedirectURL(message string, rurl string) string {
 	return rurl + "#" + q.Encode()
 }
 
+func (a *API) prepPKCERedirectURL(rurl, code string) string {
+	q := url.Values{}
+	q.Set("code", code)
+	return rurl + "?" + q.Encode()
+}
+
 func (a *API) emailChangeVerify(r *http.Request, ctx context.Context, conn *storage.Connection, params *VerifyParams, user *models.User) (*models.User, error) {
 	config := a.config
 
diff --git a/internal/models/challenge.go b/internal/models/challenge.go
index f86d5656..c4ec259a 100644
--- a/internal/models/challenge.go
+++ b/internal/models/challenge.go
@@ -22,8 +22,6 @@ func (Challenge) TableName() string {
 	return tableName
 }
 
-const ChallengePrefix = "challenge"
-
 func NewChallenge(factor *Factor, ipAddress string) (*Challenge, error) {
 	id := uuid.Must(uuid.NewV4())
 
@@ -44,10 +42,18 @@ func FindChallengeByChallengeID(tx *storage.Connection, challengeID uuid.UUID) (
 }
 
 // Update the verification timestamp
-func (f *Challenge) Verify(tx *storage.Connection) error {
+func (c *Challenge) Verify(tx *storage.Connection) error {
 	now := time.Now()
-	f.VerifiedAt = &now
-	return tx.UpdateOnly(f, "verified_at")
+	c.VerifiedAt = &now
+	return tx.UpdateOnly(c, "verified_at")
+}
+
+func (c *Challenge) HasExpired(expiryDuration float64) bool {
+	return time.Now().After(c.GetExpiryTime(expiryDuration))
+}
+
+func (c *Challenge) GetExpiryTime(expiryDuration float64) time.Time {
+	return c.CreatedAt.Add(time.Second * time.Duration(expiryDuration))
 }
 
 func findChallenge(tx *storage.Connection, query string, args ...interface{}) (*Challenge, error) {
diff --git a/internal/models/factor.go b/internal/models/factor.go
index c54c888b..1f5af01b 100644
--- a/internal/models/factor.go
+++ b/internal/models/factor.go
@@ -199,6 +199,14 @@ func (f *Factor) DowngradeSessionsToAAL1(tx *storage.Connection) error {
 	return updateFactorAssociatedSessions(tx, f.UserID, f.ID, AAL1.String())
 }
 
+func (f *Factor) IsOwnedBy(user *User) bool {
+	return f.UserID == user.ID
+}
+
+func (f *Factor) IsVerified() bool {
+	return f.Status == FactorStateVerified.String()
+}
+
 func DeleteFactorsByUserId(tx *storage.Connection, userId uuid.UUID) error {
 	if err := tx.RawQuery("DELETE FROM "+(&pop.Model{Value: Factor{}}).TableName()+" WHERE user_id = ?", userId).Exec(); err != nil {
 		return err
diff --git a/internal/models/flow_state.go b/internal/models/flow_state.go
index 3cf25e87..006ea64e 100644
--- a/internal/models/flow_state.go
+++ b/internal/models/flow_state.go
@@ -166,5 +166,5 @@ func (f *FlowState) VerifyPKCE(codeVerifier string) error {
 }
 
 func (f *FlowState) IsExpired(expiryDuration time.Duration) bool {
-	return f.CreatedAt.After(time.Now().Add(expiryDuration))
+	return time.Now().After(f.CreatedAt.Add(expiryDuration))
 }
diff --git a/internal/models/sessions.go b/internal/models/sessions.go
index c502a7a7..ad6fa1e2 100644
--- a/internal/models/sessions.go
+++ b/internal/models/sessions.go
@@ -201,3 +201,7 @@ func (s *Session) GetAAL() string {
 	}
 	return *(s.AAL)
 }
+
+func (s *Session) IsAAL2() bool {
+	return s.GetAAL() == AAL2.String()
+}
diff --git a/internal/models/user.go b/internal/models/user.go
index 9b058c08..d45d4657 100644
--- a/internal/models/user.go
+++ b/internal/models/user.go
@@ -514,7 +514,8 @@ func FindUserByPhoneChangeAndAudience(tx *storage.Connection, phone, aud string)
 }
 
 // IsDuplicatedEmail returns whether a user exists with a matching email and audience.
-func IsDuplicatedEmail(tx *storage.Connection, email, aud string) (*User, error) {
+// If a currentUser is provided, we will need to filter out any identities that belong to the current user.
+func IsDuplicatedEmail(tx *storage.Connection, email, aud string, currentUser *User) (*User, error) {
 	var identities []Identity
 
 	if err := tx.Eager().Q().Where("email = ?", strings.ToLower(email)).All(&identities); err != nil {
@@ -528,7 +529,9 @@ func IsDuplicatedEmail(tx *storage.Connection, email, aud string) (*User, error)
 	userIDs := make(map[string]uuid.UUID)
 	for _, identity := range identities {
 		if !identity.IsForSSOProvider() {
-			userIDs[identity.UserID.String()] = identity.UserID
+			if (currentUser != nil && currentUser.ID != identity.UserID) || (currentUser == nil) {
+				userIDs[identity.UserID.String()] = identity.UserID
+			}
 		}
 	}
 
diff --git a/migrations/20230322519590_add_flow_state_table.up.sql b/migrations/20230322519590_add_flow_state_table.up.sql
index a9b473e0..a8842e5b 100644
--- a/migrations/20230322519590_add_flow_state_table.up.sql
+++ b/migrations/20230322519590_add_flow_state_table.up.sql
@@ -16,5 +16,5 @@ create table if not exists {{ index .Options "Namespace" }}.flow_state(
        created_at timestamptz null,
        updated_at timestamptz null
 );
-create index idx_auth_code on {{ index .Options "Namespace" }}.flow_state(auth_code);
-comment on table {{ index .Options "Namespace" }}.flow_state is 'stores metadata for oauth provider logins';
+create index if not exists idx_auth_code on {{ index .Options "Namespace" }}.flow_state(auth_code);
+comment on table {{ index .Options "Namespace" }}.flow_state is 'stores metadata for pkce logins';
diff --git a/migrations/20230402418590_add_authentication_method_to_flow_state_table.up.sql b/migrations/20230402418590_add_authentication_method_to_flow_state_table.up.sql
index 41ebf6e6..e83af856 100644
--- a/migrations/20230402418590_add_authentication_method_to_flow_state_table.up.sql
+++ b/migrations/20230402418590_add_authentication_method_to_flow_state_table.up.sql
@@ -1,2 +1,6 @@
 alter table {{index .Options "Namespace" }}.flow_state
-add column authentication_method text not null;
+add column if not exists authentication_method text not null;
+create index if not exists idx_user_id_auth_method on {{index .Options "Namespace" }}.flow_state (user_id, authentication_method);
+
+-- Update comment as we have generalized the table
+comment on table {{ index .Options "Namespace" }}.flow_state is 'stores metadata for pkce logins';


Test patch:
diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go
index 8383c884..f54a1551 100644
--- a/internal/api/mfa_test.go
+++ b/internal/api/mfa_test.go
@@ -137,7 +137,7 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			factors, err := models.FindFactorsByUser(ts.API.db, user)
 			ts.Require().NoError(err)
 			latestFactor := factors[len(factors)-1]
-			require.Equal(ts.T(), models.FactorStateUnverified.String(), latestFactor.Status)
+			require.False(ts.T(), latestFactor.IsVerified())
 			if c.friendlyName != "" && c.expectedCode == http.StatusOK {
 				require.Equal(ts.T(), c.friendlyName, latestFactor.FriendlyName)
 			}
@@ -402,7 +402,7 @@ func (ts *MFATestSuite) TestSessionsMaintainAALOnRefresh() {
 	require.NoError(ts.T(), err)
 	ctx, err = ts.API.maybeLoadUserOrSession(ctx)
 	require.NoError(ts.T(), err)
-	require.Equal(ts.T(), models.AAL2.String(), getSession(ctx).GetAAL())
+	require.True(ts.T(), getSession(ctx).IsAAL2())
 }
 
 // Performing MFA Verification followed by a sign in should return an AAL1 session and an AAL2 session
@@ -431,7 +431,7 @@ func (ts *MFATestSuite) TestMFAFollowedByPasswordSignIn() {
 	require.Equal(ts.T(), models.AAL1.String(), getSession(ctx).GetAAL())
 	session, err := models.FindSessionByUserID(ts.API.db, token.User.ID)
 	require.NoError(ts.T(), err)
-	require.Equal(ts.T(), models.AAL2.String(), session.GetAAL())
+	require.True(ts.T(), session.IsAAL2())
 }
 
 func signUp(ts *MFATestSuite, email, password string) (signUpResp AccessTokenResponse) {
diff --git a/internal/models/user_test.go b/internal/models/user_test.go
index f54b5f60..0949a65e 100644
--- a/internal/models/user_test.go
+++ b/internal/models/user_test.go
@@ -162,19 +162,19 @@ func (ts *UserTestSuite) TestFindUserWithRefreshToken() {
 func (ts *UserTestSuite) TestIsDuplicatedEmail() {
 	_ = ts.createUserWithEmail("david.calavera@netlify.com")
 
-	e, err := IsDuplicatedEmail(ts.db, "david.calavera@netlify.com", "test")
+	e, err := IsDuplicatedEmail(ts.db, "david.calavera@netlify.com", "test", nil)
 	require.NoError(ts.T(), err)
 	require.NotNil(ts.T(), e, "expected email to be duplicated")
 
-	e, err = IsDuplicatedEmail(ts.db, "davidcalavera@netlify.com", "test")
+	e, err = IsDuplicatedEmail(ts.db, "davidcalavera@netlify.com", "test", nil)
 	require.NoError(ts.T(), err)
-	require.Nil(ts.T(), e, "expected email to not be duplicated")
+	require.Nil(ts.T(), e, "expected email to not be duplicated", nil)
 
-	e, err = IsDuplicatedEmail(ts.db, "david@netlify.com", "test")
+	e, err = IsDuplicatedEmail(ts.db, "david@netlify.com", "test", nil)
 	require.NoError(ts.T(), err)
-	require.Nil(ts.T(), e, "expected same email to not be duplicated")
+	require.Nil(ts.T(), e, "expected same email to not be duplicated", nil)
 
-	e, err = IsDuplicatedEmail(ts.db, "david.calavera@netlify.com", "other-aud")
+	e, err = IsDuplicatedEmail(ts.db, "david.calavera@netlify.com", "other-aud", nil)
 	require.NoError(ts.T(), err)
 	require.Nil(ts.T(), e, "expected same email to not be duplicated")
 }