fix: update mfa admin methods (#1774)

## What kind of change does this PR introduce? Update admin MFA methods to allow an admin to update a phone factor's phone number. Also disallows and removes factor type as an updatable field. Having the factor type field is redundant as it previously allowed for update of only one factor type (TOTP).
supabase · Sep 12, 2024 · 567ea7e · 567ea7e
1 parent 7e472ad
commit 567ea7e
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 22 deletions.
diff --git a/internal/api/admin.go b/internal/api/admin.go
@@ -39,7 +39,7 @@ type adminUserDeleteParams struct {
 
 type adminUserUpdateFactorParams struct {
 	FriendlyName string `json:"friendly_name"`
-	FactorType   string `json:"factor_type"`
+	Phone        string `json:"phone"`
 }
 
 type AdminListUsersResponse struct {
@@ -618,11 +618,13 @@ func (a *API) adminUserUpdateFactor(w http.ResponseWriter, r *http.Request) erro
 				return terr
 			}
 		}
-		if params.FactorType != "" {
-			if params.FactorType != models.TOTP {
-				return badRequestError(ErrorCodeValidationFailed, "Factor Type not valid")
+
+		if params.Phone != "" && factor.IsPhoneFactor() {
+			phone, err := validatePhone(params.Phone)
+			if err != nil {
+				return badRequestError(ErrorCodeValidationFailed, "Invalid phone number format (E.164 required)")
 			}
-			if terr := factor.UpdateFactorType(tx, params.FactorType); terr != nil {
+			if terr := factor.UpdatePhone(tx, phone); terr != nil {
 				return terr
 			}
 		}

diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go
@@ -816,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.NewTOTPFactor(u, "testSimpleName")
+	f := models.NewPhoneFactor(u, "123456789", "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")
 
@@ -833,20 +833,12 @@ func (ts *AdminTestSuite) TestAdminUserUpdateFactor() {
 			ExpectedCode: http.StatusOK,
 		},
 		{
-			Desc: "Update factor: valid factor type",
+			Desc: "Update Factor phone number",
 			FactorData: map[string]interface{}{
-				"friendly_name": "john",
-				"factor_type":   models.TOTP,
+				"phone": "+1976154321",
 			},
 			ExpectedCode: http.StatusOK,
 		},
-		{
-			Desc: "Update factor: invalid factor",
-			FactorData: map[string]interface{}{
-				"factor_type": "invalid_factor",
-			},
-			ExpectedCode: http.StatusBadRequest,
-		},
 	}
 
 	// Initialize factor data

diff --git a/internal/models/factor.go b/internal/models/factor.go
@@ -245,18 +245,17 @@ func (f *Factor) UpdateFriendlyName(tx *storage.Connection, friendlyName string)
 	return tx.UpdateOnly(f, "friendly_name", "updated_at")
 }
 
+func (f *Factor) UpdatePhone(tx *storage.Connection, phone string) error {
+	f.Phone = storage.NullString(phone)
+	return tx.UpdateOnly(f, "phone", "updated_at")
+}
+
 // UpdateStatus modifies the factor status
 func (f *Factor) UpdateStatus(tx *storage.Connection, state FactorState) error {
 	f.Status = state.String()
 	return tx.UpdateOnly(f, "status", "updated_at")
 }
 
-// UpdateFactorType modifies the factor type
-func (f *Factor) UpdateFactorType(tx *storage.Connection, factorType string) error {
-	f.FactorType = factorType
-	return tx.UpdateOnly(f, "factor_type", "updated_at")
-}
-
 func (f *Factor) DowngradeSessionsToAAL1(tx *storage.Connection) error {
 	sessions, err := FindSessionsByFactorID(tx, f.ID)
 	if err != nil {