From c4120f40015985eba069d269894a81faf03853b5 Mon Sep 17 00:00:00 2001 From: Joel Lee Date: Mon, 5 Aug 2024 12:51:44 +0200 Subject: [PATCH] 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 --- internal/api/mfa.go | 26 +++++++------ internal/models/challenge.go | 1 - internal/models/factor.go | 38 +++++++++++++------ ...20240729123726_add_mfa_phone_config.up.sql | 2 - ...a_factors_column_last_challenged_at.up.sql | 1 + 5 files changed, 41 insertions(+), 27 deletions(-) create mode 100644 migrations/20240802193726_add_mfa_factors_column_last_challenged_at.up.sql diff --git a/internal/api/mfa.go b/internal/api/mfa.go index f62a418d4b..8ad0207ece 100644 --- a/internal/api/mfa.go +++ b/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{}{ diff --git a/internal/models/challenge.go b/internal/models/challenge.go index 57a9700616..24d3345863 100644 --- a/internal/models/challenge.go +++ b/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 { diff --git a/internal/models/factor.go b/internal/models/factor.go index 14e483b53a..7c6f6dd30c 100644 --- a/internal/models/factor.go +++ b/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 } diff --git a/migrations/20240729123726_add_mfa_phone_config.up.sql b/migrations/20240729123726_add_mfa_phone_config.up.sql index cd52973ed2..ec94d7bcbe 100644 --- a/migrations/20240729123726_add_mfa_phone_config.up.sql +++ b/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); diff --git a/migrations/20240802193726_add_mfa_factors_column_last_challenged_at.up.sql b/migrations/20240802193726_add_mfa_factors_column_last_challenged_at.up.sql new file mode 100644 index 0000000000..bc3eea9894 --- /dev/null +++ b/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;