fix: add last_challenged_at field to mfa factors

internal/api/errorcodes.go

@@ -81,4 +81,5 @@ const (
 	ErrorCodeMFAPhoneVerifyDisabled            ErrorCode = "mfa_phone_verify_not_enabled"
 	ErrorCodeMFATOTPEnrollDisabled             ErrorCode = "mfa_totp_enroll_not_enabled"
 	ErrorCodeMFATOTPVerifyDisabled             ErrorCode = "mfa_totp_verify_not_enabled"
+	ErrorCodeVerifiedFactorExists              ErrorCode = "mfa_verified_factor_exists"
 )

internal/api/mfa.go

@@ -89,13 +89,35 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 	if err := models.DeleteExpiredFactors(db, config.MFA.FactorExpiryDuration); err != nil {
 		return err
 	}
+	var factorsToDelete []models.Factor
+	for _, factor := range user.Factors {
+		switch {
+		case factor.FriendlyName == params.FriendlyName:
+			return unprocessableEntityError(
+				ErrorCodeMFAFactorNameConflict,
+				fmt.Sprintf("A factor with the friendly name %q for this user already exists", factor.FriendlyName),
+			)
+
+		case factor.IsPhoneFactor():
+			if factor.IsVerified() {
+				return unprocessableEntityError(
+					ErrorCodeVerifiedFactorExists,
+					"A verified phone factor already exists, unenroll the existing factor to continue",
+				)
+			}
+			if factor.IsUnverified() && factor.Phone.String() == phone {
+				factorsToDelete = append(factorsToDelete, factor)
+			}
 
-	for _, factor := range factors {
-		if factor.IsVerified() {
-			numVerifiedFactors += 1
+		case factor.IsVerified():
+			numVerifiedFactors++
 		}
 	}
 
+	if err := db.Destroy(&factorsToDelete); err != nil {
+		return internalServerError("Database error deleting unverified phone factors").WithInternalError(err)
+	}
+
 	if factorCount >= int(config.MFA.MaxEnrolledFactors) {
 		return unprocessableEntityError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue")
 	}
@@ -110,12 +132,7 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 	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)
-			if pgErr.IsUniqueConstraintViolated() {
-				return unprocessableEntityError(ErrorCodeMFAFactorNameConflict, fmt.Sprintf("A factor with the friendly name %q for this user likely already exists", factor.FriendlyName))
-			}
 			return terr
-
 		}
 		if terr := models.NewAuditLogEntry(r, tx, user, models.EnrollFactorAction, r.RemoteAddr, map[string]interface{}{
 			"factor_id":   factor.ID,
@@ -132,7 +149,7 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 		ID:           factor.ID,
 		Type:         models.Phone,
 		FriendlyName: factor.FriendlyName,
-		Phone:        string(factor.Phone),
+		Phone:        params.Phone,
 	})
 }
 
@@ -284,26 +301,26 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error
 	if !sms_provider.IsValidMessageChannel(channel, config) {
 		return badRequestError(ErrorCodeValidationFailed, InvalidChannelError)
 	}
-	latestValidChallenge, err := factor.FindLatestUnexpiredChallenge(a.db, config.MFA.ChallengeExpiryDuration)
-	if err != nil {
-		if !models.IsNotFoundError(err) {
-			return internalServerError("error finding latest unexpired challenge")
+
+	if factor.IsPhoneFactor() && factor.LastChallengedAt != nil {
+		if !factor.LastChallengedAt.Add(config.MFA.Phone.MaxFrequency).Before(time.Now()) {
+			return tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(factor.LastChallengedAt, config.MFA.Phone.MaxFrequency))
 		}
-	} else if latestValidChallenge != nil && !latestValidChallenge.SentAt.Add(config.MFA.Phone.MaxFrequency).Before(time.Now()) {
-		return tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(latestValidChallenge.SentAt, config.MFA.Phone.MaxFrequency))
 	}
 	otp, err := crypto.GenerateOtp(config.MFA.Phone.OtpLength)
 	if err != nil {
 		panic(err)
 	}
-	message, err := generateSMSFromTemplate(config.MFA.Phone.SMSTemplate, otp)
-	if err != nil {
-		return internalServerError("error generating sms template").WithInternalError(err)
-	}
 	challenge, err := factor.CreatePhoneChallenge(ipAddress, otp, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey)
 	if err != nil {
 		return internalServerError("error creating SMS Challenge")
 	}
+
+	message, err := generateSMSFromTemplate(config.MFA.Phone.SMSTemplate, otp)
+	if err != nil {
+		return internalServerError("error generating sms template").WithInternalError(err)
+	}
+
 	if config.Hook.SendSMS.Enabled {
 		input := hooks.SendSMSInput{
 			User: user,
@@ -323,14 +340,15 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error
 			return internalServerError("Failed to get SMS provider").WithInternalError(err)
 		}
 		// We omit messageID for now, can consider reinstating if there are requests.
-		if _, err = smsProvider.SendMessage(string(factor.Phone), message, channel, otp); err != nil {
+		if _, err = smsProvider.SendMessage(factor.Phone.String(), message, channel, otp); err != nil {
 			return internalServerError("error sending message").WithInternalError(err)
 		}
 	}
 	if err := db.Transaction(func(tx *storage.Connection) error {
-		if terr := tx.Create(challenge); terr != nil {
+		if terr := factor.WriteChallengeToDatabase(tx, challenge); terr != nil {
 			return terr
 		}
+
 		if terr := models.NewAuditLogEntry(r, tx, user, models.CreateChallengeAction, r.RemoteAddr, map[string]interface{}{
 			"factor_id":     factor.ID,
 			"factor_status": factor.Status,
@@ -357,8 +375,9 @@ func (a *API) challengeTOTPFactor(w http.ResponseWriter, r *http.Request) error
 	ipAddress := utilities.GetIPAddress(r)
 
 	challenge := factor.CreateChallenge(ipAddress)
+
 	if err := db.Transaction(func(tx *storage.Connection) error {
-		if terr := tx.Create(challenge); terr != nil {
+		if terr := factor.WriteChallengeToDatabase(tx, challenge); terr != nil {
 			return terr
 		}
 		if terr := models.NewAuditLogEntry(r, tx, user, models.CreateChallengeAction, r.RemoteAddr, map[string]interface{}{
@@ -417,6 +436,7 @@ func (a *API) verifyTOTPFactor(w http.ResponseWriter, r *http.Request, params *V
 		return internalServerError("Database error finding Challenge").WithInternalError(err)
 	}
 
+	// Ambiguous so as not to leak whether there is a verified challenge
 	if challenge.VerifiedAt != nil || challenge.IPAddress != currentIP {
 		return unprocessableEntityError(ErrorCodeMFAIPAddressMismatch, "Challenge and verify IP addresses mismatch")
 	}
@@ -485,6 +505,7 @@ func (a *API) verifyTOTPFactor(w http.ResponseWriter, r *http.Request, params *V
 		if terr = models.NewAuditLogEntry(r, tx, user, models.VerifyFactorAction, r.RemoteAddr, map[string]interface{}{
 			"factor_id":    factor.ID,
 			"challenge_id": challenge.ID,
+			"factor_type":  factor.FactorType,
 		}); terr != nil {
 			return terr
 		}
@@ -524,7 +545,7 @@ func (a *API) verifyTOTPFactor(w http.ResponseWriter, r *http.Request, params *V
 		if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
 			return internalServerError("Failed to update sessions. %s", terr)
 		}
-		if terr = models.DeleteUnverifiedFactors(tx, user); terr != nil {
+		if terr = models.DeleteUnverifiedFactors(tx, user, factor.FactorType); terr != nil {
 			return internalServerError("Error removing unverified factors. %s", terr)
 		}
 		return nil
@@ -643,7 +664,7 @@ func (a *API) verifyPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 		if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
 			return internalServerError("Failed to update sessions. %s", terr)
 		}
-		if terr = models.DeleteUnverifiedFactors(tx, user); terr != nil {
+		if terr = models.DeleteUnverifiedFactors(tx, user, factor.FactorType); terr != nil {
 			return internalServerError("Error removing unverified factors. %s", terr)
 		}
 		return nil

internal/api/mfa_test.go

@@ -198,6 +198,87 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 	}
 }
 
+func (ts *MFATestSuite) TestDuplicateEnrollPhoneFactor() {
+	testPhoneNumber := "+12345677889"
+	altPhoneNumber := "+987412444444"
+	friendlyName := "phone_factor"
+	altFriendlyName := "alt_phone_factor"
+	token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID)
+
+	var cases = []struct {
+		desc                    string
+		earlierFactorName       string
+		laterFactorName         string
+		phone                   string
+		secondPhone             string
+		expectedCode            int
+		expectedNumberOfFactors int
+	}{
+		{
+			desc:                    "Phone: Only the latest factor should persist when enrolling two unverified phone factors with the same number",
+			earlierFactorName:       friendlyName,
+			laterFactorName:         altFriendlyName,
+			phone:                   testPhoneNumber,
+			secondPhone:             testPhoneNumber,
+			expectedNumberOfFactors: 1,
+		},
+
+		{
+			desc:                    "Phone: Both factors should persist when enrolling two different unverified numbers",
+			earlierFactorName:       friendlyName,
+			laterFactorName:         altFriendlyName,
+			phone:                   testPhoneNumber,
+			secondPhone:             altPhoneNumber,
+			expectedNumberOfFactors: 2,
+		},
+	}
+
+	for _, c := range cases {
+		ts.Run(c.desc, func() {
+			// Delete all test factors to start from clean slate
+			require.NoError(ts.T(), ts.API.db.Destroy(ts.TestUser.Factors))
+			_ = performEnrollFlow(ts, token, c.earlierFactorName, models.Phone, ts.TestDomain, c.phone, http.StatusOK)
+
+			w := performEnrollFlow(ts, token, c.laterFactorName, models.Phone, ts.TestDomain, c.secondPhone, http.StatusOK)
+			enrollResp := EnrollFactorResponse{}
+			require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&enrollResp))
+
+			laterFactor, err := models.FindFactorByFactorID(ts.API.db, enrollResp.ID)
+			require.NoError(ts.T(), err)
+			require.False(ts.T(), laterFactor.IsVerified())
+
+			require.NoError(ts.T(), ts.API.db.Eager("Factors").Find(ts.TestUser, ts.TestUser.ID))
+			require.Equal(ts.T(), len(ts.TestUser.Factors), c.expectedNumberOfFactors)
+
+		})
+	}
+}
+
+func (ts *MFATestSuite) TestDuplicateEnrollPhoneFactorWithVerified() {
+	testPhoneNumber := "+12345677889"
+	friendlyName := "phone_factor"
+	altFriendlyName := "alt_phone_factor"
+	token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID)
+
+	ts.Run("Phone: Enrolling a factor with a verified phone factor present should fail", func() {
+		require.NoError(ts.T(), ts.API.db.Destroy(ts.TestUser.Factors))
+
+		// Setup verified factor
+		w := performEnrollFlow(ts, token, friendlyName, models.Phone, ts.TestDomain, testPhoneNumber, http.StatusOK)
+		enrollResp := EnrollFactorResponse{}
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&enrollResp))
+		firstFactor, err := models.FindFactorByFactorID(ts.API.db, enrollResp.ID)
+		require.NoError(ts.T(), err)
+		require.NoError(ts.T(), firstFactor.UpdateStatus(ts.API.db, models.FactorStateVerified))
+
+		expectedStatusCode := http.StatusUnprocessableEntity
+		_ = performEnrollFlow(ts, token, altFriendlyName, models.Phone, ts.TestDomain, testPhoneNumber, expectedStatusCode)
+
+		require.NoError(ts.T(), ts.API.db.Eager("Factors").Find(ts.TestUser, ts.TestUser.ID))
+		require.Equal(ts.T(), len(ts.TestUser.Factors), 1)
+	})
+}
+
 func (ts *MFATestSuite) TestDuplicateTOTPEnrollsReturnExpectedMessage() {
 	friendlyName := "mary"
 	issuer := "https://issuer.com"

internal/models/challenge.go

@@ -15,7 +15,6 @@ type Challenge struct {
 	IPAddress  string     `json:"ip_address" db:"ip_address"`
 	Factor     *Factor    `json:"factor,omitempty" belongs_to:"factor"`
 	OtpCode    string     `json:"otp_code,omitempty" db:"otp_code"`
-	SentAt     *time.Time `json:"sent_at,omitempty" db:"sent_at"`
 }
 
 func (Challenge) TableName() string {

internal/models/factor.go

@@ -117,17 +117,19 @@ func ParseAuthenticationMethod(authMethod string) (AuthenticationMethod, error)
 }
 
 type Factor struct {
-	ID           uuid.UUID          `json:"id" db:"id"`
-	User         User               `json:"-" belongs_to:"user"`
-	UserID       uuid.UUID          `json:"-" db:"user_id"`
-	CreatedAt    time.Time          `json:"created_at" db:"created_at"`
-	UpdatedAt    time.Time          `json:"updated_at" db:"updated_at"`
-	Status       string             `json:"status" db:"status"`
-	FriendlyName string             `json:"friendly_name,omitempty" db:"friendly_name"`
-	Secret       string             `json:"-" db:"secret"`
-	FactorType   string             `json:"factor_type" db:"factor_type"`
-	Challenge    []Challenge        `json:"-" has_many:"challenges"`
-	Phone        storage.NullString `json:"phone" db:"phone"`
+	ID uuid.UUID `json:"id" db:"id"`
+	// TODO: Consider removing this nested user field. We don't use it.
+	User             User               `json:"-" belongs_to:"user"`
+	UserID           uuid.UUID          `json:"-" db:"user_id"`
+	CreatedAt        time.Time          `json:"created_at" db:"created_at"`
+	UpdatedAt        time.Time          `json:"updated_at" db:"updated_at"`
+	Status           string             `json:"status" db:"status"`
+	FriendlyName     string             `json:"friendly_name,omitempty" db:"friendly_name"`
+	Secret           string             `json:"-" db:"secret"`
+	FactorType       string             `json:"factor_type" db:"factor_type"`
+	Challenge        []Challenge        `json:"-" has_many:"challenges"`
+	Phone            storage.NullString `json:"phone" db:"phone"`
+	LastChallengedAt *time.Time         `json:"last_challenged_at" db:"last_challenged_at"`
 }
 
 func (Factor) TableName() string {
@@ -196,8 +198,8 @@ func FindFactorByFactorID(conn *storage.Connection, factorID uuid.UUID) (*Factor
 	return &factor, nil
 }
 
-func DeleteUnverifiedFactors(tx *storage.Connection, user *User) error {
-	if err := tx.RawQuery("DELETE FROM "+(&pop.Model{Value: Factor{}}).TableName()+" WHERE user_id = ? and status = ?", user.ID, FactorStateUnverified.String()).Exec(); err != nil {
+func DeleteUnverifiedFactors(tx *storage.Connection, user *User, factorType string) error {
+	if err := tx.RawQuery("DELETE FROM "+(&pop.Model{Value: Factor{}}).TableName()+" WHERE user_id = ? and status = ? and factor_type = ?", user.ID, FactorStateUnverified.String(), factorType).Exec(); err != nil {
 		return err
 	}
 
@@ -211,16 +213,29 @@ func (f *Factor) CreateChallenge(ipAddress string) *Challenge {
 		FactorID:  f.ID,
 		IPAddress: ipAddress,
 	}
+
 	return challenge
 }
+func (f *Factor) WriteChallengeToDatabase(tx *storage.Connection, challenge *Challenge) error {
+	if challenge.FactorID != f.ID {
+		return errors.New("Can only write challenges that you own")
+	}
+	now := time.Now()
+	f.LastChallengedAt = &now
+	if terr := tx.Create(challenge); terr != nil {
+		return terr
+	}
+	if err := tx.UpdateOnly(f, "last_challenged_at"); err != nil {
+		return err
+	}
+	return nil
+}
 
 func (f *Factor) CreatePhoneChallenge(ipAddress string, otpCode string, encrypt bool, encryptionKeyID, encryptionKey string) (*Challenge, error) {
 	phoneChallenge := f.CreateChallenge(ipAddress)
 	if err := phoneChallenge.SetOtpCode(otpCode, encrypt, encryptionKeyID, encryptionKey); err != nil {
 		return nil, err
 	}
-	now := time.Now()
-	phoneChallenge.SentAt = &now
 	return phoneChallenge, nil
 }
 
@@ -263,6 +278,14 @@ func (f *Factor) IsVerified() bool {
 	return f.Status == FactorStateVerified.String()
 }
 
+func (f *Factor) IsUnverified() bool {
+	return f.Status == FactorStateUnverified.String()
+}
+
+func (f *Factor) IsPhoneFactor() bool {
+	return f.FactorType == Phone
+}
+
 func (f *Factor) FindChallengeByID(conn *storage.Connection, challengeID uuid.UUID) (*Challenge, error) {
 	var challenge Challenge
 	err := conn.Q().Where("id = ? and factor_id = ?", challengeID, f.ID).First(&challenge)

migrations/20240729123726_add_mfa_phone_config.up.sql

@@ -6,9 +6,7 @@ end $$;
 
 
 alter table {{ index .Options "Namespace" }}.mfa_factors add column if not exists phone text unique default null;
-alter table {{ index .Options "Namespace" }}.mfa_challenges add column if not exists sent_at timestamptz null;
 alter table {{ index .Options "Namespace" }}.mfa_challenges add column if not exists otp_code text null;
 
-create index if not exists idx_sent_at on {{ index .Options "Namespace" }}.mfa_challenges(sent_at);
 
 create unique index if not exists unique_verified_phone_factor on {{ index .Options "Namespace" }}.mfa_factors (user_id, phone);

migrations/20240802193726_add_mfa_factors_column_last_challenged_at.up.sql

@@ -0,0 +1 @@
+alter table {{ index .Options "Namespace" }}.mfa_factors add column if not exists last_challenged_at timestamptz unique default null;