fix: MFA NewFactor to default to creating unverfied factors (#1692)

## What kind of change does this PR introduce? - Split `NewFactor` into `NewPhoneFactor()` and `NewTOTPFactor()`. - All `New<Type>Factor` methods will now create unverified factors. - Additionally, also guards the `Challenge` endpoint when verification is disabled for a factor. The hope is to reduce cognitive load and the chance of creating a factor in an undesired state Should one wish to obtain a Verified Factor (say for tests) they can call `UpdateStatus`. It is unlikely for this to be a common use case though. Someone might have brought this up prior but only getting to it now
supabase · Jul 31, 2024 · 3d448fa · 3d448fa
1 parent 6aca52b
commit 3d448fa
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 22 deletions.
diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go
@@ -769,7 +769,8 @@ func (ts *AdminTestSuite) TestAdminUserDeleteFactor() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateVerified)
+	f := models.NewTOTPFactor(u, "testSimpleName")
+	require.NoError(ts.T(), f.UpdateStatus(ts.API.db, models.FactorStateVerified))
 	require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey))
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
@@ -793,7 +794,7 @@ func (ts *AdminTestSuite) TestAdminUserGetFactors() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified)
+	f := models.NewTOTPFactor(u, "testSimpleName")
 	require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey))
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
@@ -815,7 +816,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdateFactor() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified)
+	f := models.NewTOTPFactor(u, "testSimpleName")
 	require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey))
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 

diff --git a/internal/api/mfa.go b/internal/api/mfa.go
@@ -108,7 +108,7 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 	if numVerifiedFactors > 0 && !session.IsAAL2() {
 		return forbiddenError(ErrorCodeInsufficientAAL, "AAL2 required to enroll a new factor")
 	}
-	factor := models.NewPhoneFactor(user, phone, params.FriendlyName, params.FactorType, models.FactorStateUnverified)
+	factor := models.NewPhoneFactor(user, phone, params.FriendlyName)
 	err = db.Transaction(func(tx *storage.Connection) error {
 		if terr := tx.Create(factor); terr != nil {
 			pgErr := utilities.NewPostgresError(terr)
@@ -222,7 +222,7 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 	}
 	svgData.End()
 
-	factor = models.NewFactor(user, params.FriendlyName, params.FactorType, models.FactorStateUnverified)
+	factor = models.NewTOTPFactor(user, params.FriendlyName)
 	if err := factor.SetSecret(key.Secret(), config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey); err != nil {
 		return err
 	}
@@ -352,10 +352,23 @@ func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error {
 
 	user := getUser(ctx)
 	factor := getFactor(ctx)
+
 	ipAddress := utilities.GetIPAddress(r)
-	if factor.IsPhoneFactor() {
+	switch factor.FactorType {
+	case models.Phone:
+		if !config.MFA.Phone.VerifyEnabled {
+			return unprocessableEntityError(ErrorCodeMFAPhoneEnrollDisabled, "MFA verification is disabled for Phone")
+		}
 		return a.challengePhoneFactor(w, r)
+
+	case models.TOTP:
+		if !config.MFA.TOTP.VerifyEnabled {
+			return unprocessableEntityError(ErrorCodeMFATOTPEnrollDisabled, "MFA verification is disabled for TOTP")
+		}
+	default:
+		return badRequestError(ErrorCodeValidationFailed, "factor_type needs to be TOTP or Phone")
 	}
+
 	challenge := factor.CreateChallenge(ipAddress)
 	if err := db.Transaction(func(tx *storage.Connection) error {
 		if terr := tx.Create(challenge); terr != nil {
@@ -626,7 +639,7 @@ func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error {
 				return terr
 			}
 		}
-		if shouldReEncrypt && config.Security.DBEncryption.Encrypt && factor.IsTOTPFactor() {
+		if shouldReEncrypt && config.Security.DBEncryption.Encrypt {
 			es, terr := crypto.NewEncryptedString(factor.ID.String(), []byte(secret), config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey)
 			if terr != nil {
 				return terr

diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go
@@ -62,7 +62,7 @@ func (ts *MFATestSuite) SetupTest() {
 	require.NoError(ts.T(), err, "Error creating test user model")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user")
 	// Create Factor
-	f := models.NewFactor(u, "test_factor", models.TOTP, models.FactorStateUnverified)
+	f := models.NewTOTPFactor(u, "test_factor")
 	require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey))
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 	// Create corresponding session
@@ -277,7 +277,7 @@ func (ts *MFATestSuite) TestChallengeSMSFactor() {
 	phone := "+1234567"
 	friendlyName := "testchallengesmsfactor"
 
-	f := models.NewPhoneFactor(ts.TestUser, phone, friendlyName, models.Phone, models.FactorStateUnverified)
+	f := models.NewPhoneFactor(ts.TestUser, phone, friendlyName)
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error creating new SMS factor")
 	token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID)
 
@@ -369,7 +369,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() {
 
 			if v.factorType == models.TOTP {
 				friendlyName := uuid.Must(uuid.NewV4()).String()
-				f = models.NewFactor(ts.TestUser, friendlyName, models.TOTP, models.FactorStateUnverified)
+				f = models.NewTOTPFactor(ts.TestUser, friendlyName)
 				sharedSecret = ts.TestOTPKey.Secret()
 				f.Secret = sharedSecret
 				require.NoError(ts.T(), ts.API.db.Create(f), "Error updating new test factor")
@@ -379,7 +379,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() {
 				otp, err := crypto.GenerateOtp(numDigits)
 				require.NoError(ts.T(), err)
 				phone := fmt.Sprintf("+%s", otp)
-				f = models.NewPhoneFactor(ts.TestUser, phone, friendlyName, models.Phone, models.FactorStateUnverified)
+				f = models.NewPhoneFactor(ts.TestUser, phone, friendlyName)
 				require.NoError(ts.T(), ts.API.db.Create(f), "Error creating new SMS factor")
 			}
 

diff --git a/internal/models/factor.go b/internal/models/factor.go
@@ -148,8 +148,12 @@ func NewFactor(user *User, friendlyName string, factorType string, state FactorS
 	return factor
 }
 
-func NewPhoneFactor(user *User, phone, friendlyName string, factorType string, state FactorState) *Factor {
-	factor := NewFactor(user, friendlyName, factorType, state)
+func NewTOTPFactor(user *User, friendlyName string) *Factor {
+	return NewFactor(user, friendlyName, TOTP, FactorStateUnverified)
+}
+
+func NewPhoneFactor(user *User, phone, friendlyName string) *Factor {
+	factor := NewFactor(user, friendlyName, Phone, FactorStateUnverified)
 	factor.Phone = storage.NullString(phone)
 	return factor
 }
@@ -238,14 +242,6 @@ func (f *Factor) UpdateFactorType(tx *storage.Connection, factorType string) err
 	return tx.UpdateOnly(f, "factor_type", "updated_at")
 }
 
-func (f *Factor) IsTOTPFactor() bool {
-	return f.FactorType == TOTP
-}
-
-func (f *Factor) IsPhoneFactor() bool {
-	return f.FactorType == Phone
-}
-
 func (f *Factor) DowngradeSessionsToAAL1(tx *storage.Connection) error {
 	sessions, err := FindSessionsByFactorID(tx, f.ID)
 	if err != nil {

diff --git a/internal/models/factor_test.go b/internal/models/factor_test.go
@@ -37,7 +37,7 @@ func (ts *FactorTestSuite) SetupTest() {
 	require.NoError(ts.T(), err)
 	require.NoError(ts.T(), ts.db.Create(user))
 
-	factor := NewFactor(user, "asimplename", TOTP, FactorStateUnverified)
+	factor := NewTOTPFactor(user, "asimplename")
 	require.NoError(ts.T(), factor.SetSecret("topsecret", false, "", ""))
 	require.NoError(ts.T(), ts.db.Create(factor))
 	ts.TestFactor = factor