otp changes - added sentTo field, allow e.Record to be nil when requesting OTP, etc.

apis/record_auth_otp_request.go

@@ -1,6 +1,7 @@
 package apis
 
 import (
+	"database/sql"
 	"errors"
 	"fmt"
 	"net/http"
@@ -32,12 +33,10 @@ func recordRequestOTP(e *core.RequestEvent) error {
 	}
 
 	record, err := e.App.FindAuthRecordByEmail(collection, form.Email)
-	if err != nil {
-		// eagerly write a dummy 200 response as a very rudimentary user emails enumeration protection
-		e.JSON(http.StatusOK, map[string]string{
-			"otpId": core.GenerateDefaultRandomId(),
-		})
-		return fmt.Errorf("failed to fetch %s record with email %s: %w", collection.Name, form.Email, err)
+
+	// ignore not found errors to allow custom record find implementations
+	if err != nil && !errors.Is(err, sql.ErrNoRows) {
+		return e.InternalServerError("", err)
 	}
 
 	event := new(core.RecordCreateOTPRequestEvent)
@@ -46,7 +45,18 @@ func recordRequestOTP(e *core.RequestEvent) error {
 	event.Collection = collection
 	event.Record = record
 
+	originalApp := e.App
+
 	return e.App.OnRecordRequestOTPRequest().Trigger(event, func(e *core.RecordCreateOTPRequestEvent) error {
+		if e.Record == nil {
+			// write a dummy 200 response as a very rudimentary user emails enumeration protection
+			e.JSON(http.StatusOK, map[string]string{
+				"otpId": core.GenerateDefaultRandomId(),
+			})
+
+			return fmt.Errorf("failed to fetch %s record with email %s: %w", collection.Name, form.Email, err)
+		}
+
 		var otp *core.OTP
 
 		// limit the new OTP creations for a single user
@@ -90,11 +100,10 @@ func recordRequestOTP(e *core.RequestEvent) error {
 			// send OTP email
 			// (in the background as a very basic timing attacks and emails enumeration protection)
 			// ---
-			app := e.App
 			routine.FireAndForget(func() {
-				err = mails.SendRecordOTP(app, e.Record, otp.Id, e.Password)
+				err = mails.SendRecordOTP(originalApp, e.Record, otp.Id, e.Password)
 				if err != nil {
-					app.Logger().Error("Failed to send OTP email", "error", errors.Join(err, e.App.Delete(otp)))
+					originalApp.Logger().Error("Failed to send OTP email", "error", errors.Join(err, originalApp.Delete(otp)))
 				}
 			})
 		}

apis/record_auth_otp_request_test.go

@@ -86,7 +86,10 @@ func TestRecordRequestOTP(t *testing.T) {
 			ExpectedContent: []string{
 				`"otpId":"`, // some fake random generated string
 			},
-			ExpectedEvents: map[string]int{"*": 0},
+			ExpectedEvents: map[string]int{
+				"*":                         0,
+				"OnRecordRequestOTPRequest": 1,
+			},
 			AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) {
 				if app.TestMailer.TotalSend() != 0 {
 					t.Fatalf("Expected zero emails, got %d", app.TestMailer.TotalSend())
@@ -137,6 +140,57 @@ func TestRecordRequestOTP(t *testing.T) {
 				"OnModelCreate":              1,
 				"OnModelCreateExecute":       1,
 				"OnModelAfterCreateSuccess":  1,
+				"OnModelValidate":            2, // + 1 for the OTP update after the email send
+				"OnRecordCreate":             1,
+				"OnRecordCreateExecute":      1,
+				"OnRecordAfterCreateSuccess": 1,
+				"OnRecordValidate":           2,
+				// OTP update
+				"OnModelUpdate":              1,
+				"OnModelUpdateExecute":       1,
+				"OnModelAfterUpdateSuccess":  1,
+				"OnRecordUpdate":             1,
+				"OnRecordUpdateExecute":      1,
+				"OnRecordAfterUpdateSuccess": 1,
+			},
+			AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) {
+				if app.TestMailer.TotalSend() != 1 {
+					t.Fatalf("Expected 1 email, got %d", app.TestMailer.TotalSend())
+				}
+
+				// ensure that sentTo is set
+				otps, err := app.FindRecordsByFilter(core.CollectionNameOTPs, "sentTo='test@example.com'", "", 0, 0)
+				if err != nil || len(otps) != 1 {
+					t.Fatalf("Expected to find 1 OTP with sentTo %q, found %d", "test@example.com", len(otps))
+				}
+			},
+		},
+		{
+			Name:   "existing auth record with intercepted email (with < 9 non-expired)",
+			Method: http.MethodPost,
+			URL:    "/api/collections/users/request-otp",
+			Body:   strings.NewReader(`{"email":"test@example.com"}`),
+			Delay:  100 * time.Millisecond,
+			BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) {
+				// prevent email sent
+				app.OnMailerRecordOTPSend("users").BindFunc(func(e *core.MailerRecordEvent) error {
+					return nil
+				})
+			},
+			ExpectedStatus: 200,
+			ExpectedContent: []string{
+				`"otpId":"`,
+			},
+			NotExpectedContent: []string{
+				`"otpId":"otp_`,
+			},
+			ExpectedEvents: map[string]int{
+				"*":                          0,
+				"OnRecordRequestOTPRequest":  1,
+				"OnMailerRecordOTPSend":      1,
+				"OnModelCreate":              1,
+				"OnModelCreateExecute":       1,
+				"OnModelAfterCreateSuccess":  1,
 				"OnModelValidate":            1,
 				"OnRecordCreate":             1,
 				"OnRecordCreateExecute":      1,
@@ -144,8 +198,14 @@ func TestRecordRequestOTP(t *testing.T) {
 				"OnRecordValidate":           1,
 			},
 			AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) {
-				if app.TestMailer.TotalSend() != 1 {
-					t.Fatalf("Expected 1 email, got %d", app.TestMailer.TotalSend())
+				if app.TestMailer.TotalSend() != 0 {
+					t.Fatalf("Expected 0 emails, got %d", app.TestMailer.TotalSend())
+				}
+
+				// ensure that there is no OTP with user email as sentTo
+				otps, err := app.FindRecordsByFilter(core.CollectionNameOTPs, "sentTo='test@example.com'", "", 0, 0)
+				if err != nil || len(otps) != 0 {
+					t.Fatalf("Expected to find 0 OTPs with sentTo %q, found %d", "test@example.com", len(otps))
 				}
 			},
 		},

apis/record_auth_with_otp.go

@@ -51,7 +51,7 @@ func recordAuthWithOTP(e *core.RequestEvent) error {
 		return e.BadRequestError("Invalid or expired OTP", fmt.Errorf("missing auth record: %w", err))
 	}
 
-	// since otps are usually simple digit numbers we enforce an extra rate limit rule to prevent enumerations
+	// since otps are usually simple digit numbers, enforce an extra rate limit rule as basic enumaration protection
 	err = checkRateLimit(e, "@pb_otp_"+event.Record.Id, core.RateLimitRule{MaxRequests: 5, Duration: 180})
 	if err != nil {
 		return e.TooManyRequestsError("Too many attempts, please try again later with a new OTP.", nil)
@@ -63,23 +63,33 @@ func recordAuthWithOTP(e *core.RequestEvent) error {
 	// ---
 
 	return e.App.OnRecordAuthWithOTPRequest().Trigger(event, func(e *core.RecordAuthWithOTPRequestEvent) error {
+		// update the user email verified state in case the OTP originate from an email address matching the current record one
+		//
+		// note: don't wait for success auth response (it could fail because of MFA) and because we already validated the OTP above
+		otpSentTo := e.OTP.SentTo()
+		if !e.Record.Verified() && otpSentTo != "" && e.Record.Email() == otpSentTo {
+			e.Record.SetVerified(true)
+			err = e.App.Save(e.Record)
+			if err != nil {
+				e.App.Logger().Error("Failed to update record verified state after successful OTP validation",
+					"error", err,
+					"otpId", e.OTP.Id,
+					"recordId", e.Record.Id,
+				)
+			}
+		}
+
 		err = RecordAuthResponse(e.RequestEvent, e.Record, core.MFAMethodOTP, nil)
 		if err != nil {
 			return err
 		}
 
 		// try to delete the used otp
-		if e.OTP != nil {
-			err = e.App.Delete(e.OTP)
-			if err != nil {
-				e.App.Logger().Error("Failed to delete used OTP", "error", err, "otpId", e.OTP.Id)
-			}
+		err = e.App.Delete(e.OTP)
+		if err != nil {
+			e.App.Logger().Error("Failed to delete used OTP", "error", err, "otpId", e.OTP.Id)
 		}
 
-		// note: we don't update the user verified state the same way as in the password reset confirmation
-		// at the moment because it is not clear whether the otp confirmation came from the user email
-		// (e.g. it could be from an sms or some other channel)
-
 		return nil
 	})
 }

apis/record_auth_with_otp_test.go

@@ -190,7 +190,7 @@ func TestRecordAuthWithOTP(t *testing.T) {
 			ExpectedEvents:  map[string]int{"*": 0},
 		},
 		{
-			Name:   "valid otp with valid password",
+			Name:   "valid otp with valid password (enabled MFA)",
 			Method: http.MethodPost,
 			URL:    "/api/collections/users/auth-with-otp",
 			Body: strings.NewReader(`{
@@ -236,7 +236,7 @@ func TestRecordAuthWithOTP(t *testing.T) {
 			},
 		},
 		{
-			Name:   "valid otp with valid password (disabled MFA)",
+			Name:   "valid otp with valid password and empty sentTo (disabled MFA)",
 			Method: http.MethodPost,
 			URL:    "/api/collections/users/auth-with-otp",
 			Body: strings.NewReader(`{
@@ -249,8 +249,15 @@ func TestRecordAuthWithOTP(t *testing.T) {
 					t.Fatal(err)
 				}
 
+				// ensure that the user is unverified
+				user.SetVerified(false)
+				if err = app.Save(user); err != nil {
+					t.Fatal(err)
+				}
+
+				// disable MFA
 				user.Collection().MFA.Enabled = false
-				if err := app.Save(user.Collection()); err != nil {
+				if err = app.Save(user.Collection()); err != nil {
 					t.Fatal(err)
 				}
 
@@ -297,6 +304,106 @@ func TestRecordAuthWithOTP(t *testing.T) {
 				"OnRecordDeleteExecute":      1,
 				"OnRecordAfterDeleteSuccess": 1,
 			},
+			AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) {
+				user, err := app.FindAuthRecordByEmail("users", "test@example.com")
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if user.Verified() {
+					t.Fatal("Expected the user to remain unverified because sentTo != email")
+				}
+			},
+		},
+		{
+			Name:   "valid otp with valid password and nonempty sentTo=email (disabled MFA)",
+			Method: http.MethodPost,
+			URL:    "/api/collections/users/auth-with-otp",
+			Body: strings.NewReader(`{
+				"otpId":"` + strings.Repeat("a", 15) + `",
+				"password":"123456"
+			}`),
+			BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) {
+				user, err := app.FindAuthRecordByEmail("users", "test@example.com")
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				// ensure that the user is unverified
+				user.SetVerified(false)
+				if err = app.Save(user); err != nil {
+					t.Fatal(err)
+				}
+
+				// disable MFA
+				user.Collection().MFA.Enabled = false
+				if err = app.Save(user.Collection()); err != nil {
+					t.Fatal(err)
+				}
+
+				otp := core.NewOTP(app)
+				otp.Id = strings.Repeat("a", 15)
+				otp.SetCollectionRef(user.Collection().Id)
+				otp.SetRecordRef(user.Id)
+				otp.SetPassword("123456")
+				otp.SetSentTo(user.Email())
+				if err := app.Save(otp); err != nil {
+					t.Fatal(err)
+				}
+			},
+			ExpectedStatus: 200,
+			ExpectedContent: []string{
+				`"token":"`,
+				`"record":{`,
+				`"email":"test@example.com"`,
+			},
+			NotExpectedContent: []string{
+				`"meta":`,
+				// hidden fields
+				`"tokenKey"`,
+				`"password"`,
+			},
+			ExpectedEvents: map[string]int{
+				"*":                          0,
+				"OnRecordAuthWithOTPRequest": 1,
+				"OnRecordAuthRequest":        1,
+				"OnRecordEnrich":             1,
+				// ---
+				"OnModelValidate": 2, // +1 because of the verified user update
+				// authOrigin create
+				"OnModelCreate":             1,
+				"OnModelCreateExecute":      1,
+				"OnModelAfterCreateSuccess": 1,
+				// OTP delete
+				"OnModelDelete":             1,
+				"OnModelDeleteExecute":      1,
+				"OnModelAfterDeleteSuccess": 1,
+				// user verified update
+				"OnModelUpdate":             1,
+				"OnModelUpdateExecute":      1,
+				"OnModelAfterUpdateSuccess": 1,
+				// ---
+				"OnRecordValidate":           2,
+				"OnRecordCreate":             1,
+				"OnRecordCreateExecute":      1,
+				"OnRecordAfterCreateSuccess": 1,
+				"OnRecordDelete":             1,
+				"OnRecordDeleteExecute":      1,
+				"OnRecordAfterDeleteSuccess": 1,
+				"OnRecordUpdate":             1,
+				"OnRecordUpdateExecute":      1,
+				"OnRecordAfterUpdateSuccess": 1,
+			},
+			AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) {
+				user, err := app.FindAuthRecordByEmail("users", "test@example.com")
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if !user.Verified() {
+					t.Fatal("Expected the user to be marked as verified")
+				}
+			},
 		},
 
 		// rate limit checks