fix: add last_challenged_at field to mfa factors (#1705)

## What kind of change does this PR introduce?

Deprecates `sent_at` on Challenge in favour of the `last_challenged_at`
field on factors. We use this to calculate whether it's appropriate to
allow for more SMS-es to be sent.

Base is pointed to #1693 as it depends on the PR and diffs are smaller
when pointed against #1693

Author: J0

internal/api/mfa.go

@@ -303,26 +303,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,
@@ -347,9 +347,10 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error
 		}
 	}
 	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,
@@ -376,8 +377,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{}{

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

@@ -119,16 +119,17 @@ func ParseAuthenticationMethod(authMethod string) (AuthenticationMethod, error)
 type Factor struct {
 	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"`
+	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 {
@@ -212,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
 }
 

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;