diff --git a/CHANGELOG.md b/CHANGELOG.md index 55868f38..0022cf7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ - Added `superuser otp EMAIL` command as fallback for generating superuser OTPs from the command line in case OTP has been enabled for the `_superusers` but the SMTP server has deliverability issues. +- ⚠️ Changed `OnRecordRequestOTPRequest` hook to be triggered even if there is no record matching the email (aka. `e.Record` could be `nil`), allowing you to manually create a new user with the OTP request and assigning it to `e.Record`. + +- Added new `sentTo` system field to the `_otps` collection (it is managed programmatically or by superusers; it could be anything - email, phone, messanger app id, etc.). + By default when the OTP is submitted via email it is automatically populated with the user email address (via the `OnMailerRecordOTPSend` hook's finalizer). + This allow us on valid `auth-with-otp` request to automatically mark the user email as verified. + - Added `RateLimitRule.Audience` optional field for restricting a rate limit rule for `"@guest"`-only, `"@auth"`-only, `""`-any (default). - Added default max limits for the expressions count and length of the search filter and sort params. diff --git a/CHANGELOG_16_22.md b/CHANGELOG_16_22.md index e2f1d9cc..3073b5e9 100644 --- a/CHANGELOG_16_22.md +++ b/CHANGELOG_16_22.md @@ -2,6 +2,11 @@ > For the most recent versions, please refer to [CHANGELOG.md](./CHANGELOG.md) --- +## v0.22.24 + +- Delete new uploaded record files in case of DB persist error ([#5845](https://github.com/pocketbase/pocketbase/issues/5845)). + + ## v0.22.23 - Updated the hooks watcher to account for the case when hooksDir is a symlink ([#5789](https://github.com/pocketbase/pocketbase/issues/5789)). diff --git a/apis/record_auth_otp_request.go b/apis/record_auth_otp_request.go index 39ebf77e..edb1e714 100644 --- a/apis/record_auth_otp_request.go +++ b/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))) } }) } diff --git a/apis/record_auth_otp_request_test.go b/apis/record_auth_otp_request_test.go index 84ba4c96..a0a607b7 100644 --- a/apis/record_auth_otp_request_test.go +++ b/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)) } }, }, diff --git a/apis/record_auth_with_otp.go b/apis/record_auth_with_otp.go index 31db084f..ff19085f 100644 --- a/apis/record_auth_with_otp.go +++ b/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 }) } diff --git a/apis/record_auth_with_otp_test.go b/apis/record_auth_with_otp_test.go index 4defb014..c312a7a7 100644 --- a/apis/record_auth_with_otp_test.go +++ b/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 diff --git a/core/app.go b/core/app.go index 6275e3f5..72e6b03d 100644 --- a/core/app.go +++ b/core/app.go @@ -1361,6 +1361,9 @@ type App interface { // OnRecordRequestOTPRequest hook is triggered on each Record // request OTP API request. // + // [RecordCreateOTPRequestEvent.Record] could be nil if no matching identity is found, allowing + // you to manually create or locate a different Record model (by reassigning [RecordCreateOTPRequestEvent.Record]). + // // If the optional "tags" list (Collection ids or names) is specified, // then all event handlers registered via the created hook will be // triggered and called only if their event data origin matches the tags. diff --git a/core/otp_model.go b/core/otp_model.go index dd4fc52e..c4998343 100644 --- a/core/otp_model.go +++ b/core/otp_model.go @@ -85,6 +85,20 @@ func (m *OTP) SetRecordRef(recordId string) { m.Set("recordRef", recordId) } +// SentTo returns the "sentTo" record field value. +// +// It could be any string value (email, phone, message app id, etc.) +// and usually is used as part of the auth flow to update the verified +// user state in case for example the sentTo value matches with the user record email. +func (m *OTP) SentTo() string { + return m.GetString("sentTo") +} + +// SetSentTo updates the "sentTo" record field value. +func (m *OTP) SetSentTo(val string) { + m.Set("sentTo", val) +} + // Created returns the "created" record field value. func (m *OTP) Created() types.DateTime { return m.GetDateTime("created") diff --git a/core/otp_model_test.go b/core/otp_model_test.go index fd670c5e..f0d5155a 100644 --- a/core/otp_model_test.go +++ b/core/otp_model_test.go @@ -85,6 +85,30 @@ func TestOTPCollectionRef(t *testing.T) { } } +func TestOTPSentTo(t *testing.T) { + t.Parallel() + + app, _ := tests.NewTestApp() + defer app.Cleanup() + + otp := core.NewOTP(app) + + testValues := []string{"test_1", "test2", ""} + for i, testValue := range testValues { + t.Run(fmt.Sprintf("%d_%q", i, testValue), func(t *testing.T) { + otp.SetSentTo(testValue) + + if v := otp.SentTo(); v != testValue { + t.Fatalf("Expected getter %q, got %q", testValue, v) + } + + if v := otp.GetString("sentTo"); v != testValue { + t.Fatalf("Expected field value %q, got %q", testValue, v) + } + }) + } +} + func TestOTPCreated(t *testing.T) { t.Parallel() diff --git a/mails/record.go b/mails/record.go index 8a59cb67..dacc9511 100644 --- a/mails/record.go +++ b/mails/record.go @@ -40,6 +40,8 @@ func SendRecordAuthAlert(app core.App, authRecord *core.Record) error { } // SendRecordOTP sends OTP email to the specified auth record. +// +// This method will also update the "sentTo" field of the related OTP record to the mail sent To address (if the OTP exists and not already assigned). func SendRecordOTP(app core.App, authRecord *core.Record, otpId string, pass string) error { mailClient := app.NewMailClient() @@ -72,7 +74,44 @@ func SendRecordOTP(app core.App, authRecord *core.Record, otpId string, pass str } return app.OnMailerRecordOTPSend().Trigger(event, func(e *core.MailerRecordEvent) error { - return e.Mailer.Send(e.Message) + err := e.Mailer.Send(e.Message) + if err != nil { + return err + } + + var toAddress string + if len(e.Message.To) > 0 { + toAddress = e.Message.To[0].Address + } + if toAddress == "" { + return nil + } + + otp, err := e.App.FindOTPById(otpId) + if err != nil { + e.App.Logger().Error( + "Failed to find OTP to update its sentTo field", + "error", err, + "otpId", otpId, + ) + return nil + } + + if otp.SentTo() != "" { + return nil // was already sent to another target + } + + otp.SetSentTo(toAddress) + if err = e.App.Save(otp); err != nil { + e.App.Logger().Error( + "Failed to update OTP sentTo field", + "error", err, + "otpId", otpId, + "to", toAddress, + ) + } + + return nil }) } diff --git a/migrations/1640988000_init.go b/migrations/1640988000_init.go index 57fe9e02..541f045d 100644 --- a/migrations/1640988000_init.go +++ b/migrations/1640988000_init.go @@ -179,7 +179,12 @@ func createOTPsCollection(txApp core.App) error { System: true, Hidden: true, Required: true, - Cost: 8, // low cost for better performce and because it is not critical + Cost: 8, // low cost for better performance and because it is not critical + }) + col.Fields.Add(&core.TextField{ + Name: "sentTo", + System: true, + Hidden: true, }) col.Fields.Add(&core.AutodateField{ Name: "created", diff --git a/migrations/1717233559_v0.23_migrate4.go b/migrations/1717233559_v0.23_migrate4.go new file mode 100644 index 00000000..b471ddcd --- /dev/null +++ b/migrations/1717233559_v0.23_migrate4.go @@ -0,0 +1,30 @@ +package migrations + +import ( + "github.com/pocketbase/pocketbase/core" +) + +// note: this migration will be deleted in future version + +// add new OTP sentTo text field (if not already) +func init() { + core.SystemMigrations.Register(func(txApp core.App) error { + otpCollection, err := txApp.FindCollectionByNameOrId(core.CollectionNameOTPs) + if err != nil { + return err + } + + field := otpCollection.Fields.GetByName("sentTo") + if field != nil { + return nil // already exists + } + + otpCollection.Fields.Add(&core.TextField{ + Name: "sentTo", + System: true, + Hidden: true, + }) + + return txApp.Save(otpCollection) + }, nil) +}