diff --git a/CHANGELOG.md b/CHANGELOG.md index 599e6fc9..381ad865 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - Added `mailer.Message.InlineAttachments` field for attaching inline files to an email (_aka. `cid` links_). +- Invalidate all record tokens when the auth record email is changed programmatically or by a superuser ([#5964](https://github.com/pocketbase/pocketbase/issues/5964)). + - ⚠️ Removed the "dry submit" when executing the collections Create API rule (you can find more details why this change was introduced and how it could affect your app in https://github.com/pocketbase/pocketbase/discussions/6073). For most users it should be non-breaking change, BUT if you have Create API rules that uses self-references or view counters you may have to adjust them manually. diff --git a/apis/record_auth_email_change_confirm.go b/apis/record_auth_email_change_confirm.go index 70579bf6..ad09ec1d 100644 --- a/apis/record_auth_email_change_confirm.go +++ b/apis/record_auth_email_change_confirm.go @@ -38,9 +38,8 @@ func recordConfirmEmailChange(e *core.RequestEvent) error { event.NewEmail = newEmail return e.App.OnRecordConfirmEmailChangeRequest().Trigger(event, func(e *core.RecordConfirmEmailChangeRequestEvent) error { - authRecord.Set(core.FieldNameEmail, e.NewEmail) - authRecord.Set(core.FieldNameVerified, true) - authRecord.RefreshTokenKey() // invalidate old tokens + e.Record.SetEmail(e.NewEmail) + e.Record.SetVerified(true) if err := e.App.Save(e.Record); err != nil { return firstApiError(err, e.BadRequestError("Failed to confirm email change.", err)) diff --git a/apis/record_auth_password_reset_confirm.go b/apis/record_auth_password_reset_confirm.go index 9a45820f..c73b0d76 100644 --- a/apis/record_auth_password_reset_confirm.go +++ b/apis/record_auth_password_reset_confirm.go @@ -47,12 +47,12 @@ func recordConfirmPasswordReset(e *core.RequestEvent) error { } } - err = form.app.Save(authRecord) + err = e.App.Save(authRecord) if err != nil { return firstApiError(err, e.BadRequestError("Failed to set new password.", err)) } - form.app.Store().Remove(getPasswordResetResendKey(authRecord)) + e.App.Store().Remove(getPasswordResetResendKey(authRecord)) return e.NoContent(http.StatusNoContent) }) diff --git a/apis/record_auth_password_reset_confirm_test.go b/apis/record_auth_password_reset_confirm_test.go index c6b34614..d22ef80c 100644 --- a/apis/record_auth_password_reset_confirm_test.go +++ b/apis/record_auth_password_reset_confirm_test.go @@ -186,11 +186,20 @@ func TestRecordConfirmPasswordReset(t *testing.T) { t.Fatal("Expected the user to be unverified") } + oldTokenKey := user.TokenKey() + // manually change the email to check whether the verified state will be updated user.SetEmail("test_update@example.com") - if err := app.Save(user); err != nil { + if err = app.Save(user); err != nil { t.Fatalf("Failed to update user test email: %v", err) } + + // resave with the old token key since the email change above + // would change it and will make the password token invalid + user.SetTokenKey(oldTokenKey) + if err = app.Save(user); err != nil { + t.Fatalf("Failed to restore original user tokenKey: %v", err) + } }, AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { _, err := app.FindAuthRecordByToken( diff --git a/apis/record_auth_with_oauth2_test.go b/apis/record_auth_with_oauth2_test.go index 81804d61..da950485 100644 --- a/apis/record_auth_with_oauth2_test.go +++ b/apis/record_auth_with_oauth2_test.go @@ -558,12 +558,21 @@ func TestRecordAuthWithOAuth2(t *testing.T) { t.Fatalf("Expected password %q to be valid", "1234567890") } + oldTokenKey := user.TokenKey() + // manually unset the user email user.SetEmail("") - if err := app.Save(user); err != nil { + if err = app.Save(user); err != nil { t.Fatal(err) } + // resave with the old token key since the email change above + // would change it and will make the password token invalid + user.SetTokenKey(oldTokenKey) + if err = app.Save(user); err != nil { + t.Fatalf("Failed to restore original user tokenKey: %v", err) + } + // register the test provider auth.Providers["test"] = func() auth.Provider { return &oauth2MockProvider{ diff --git a/core/record_model.go b/core/record_model.go index d2f4d404..136926ad 100644 --- a/core/record_model.go +++ b/core/record_model.go @@ -1413,12 +1413,18 @@ func onRecordValidate(e *RecordEvent) error { func onRecordSaveExecute(e *RecordEvent) error { if e.Record.Collection().IsAuth() { - // ensure that the token key is different on password change - old := e.Record.Original() - if !e.Record.IsNew() && - old.TokenKey() == e.Record.TokenKey() && - old.Get(FieldNamePassword) != e.Record.Get(FieldNamePassword) { - e.Record.RefreshTokenKey() + // ensure that the token key is regenerated on password change or email change + if !e.Record.IsNew() { + lastSavedRecord, err := e.App.FindRecordById(e.Record.Collection(), e.Record.Id) + if err != nil { + return err + } + + if lastSavedRecord.TokenKey() == e.Record.TokenKey() && + (lastSavedRecord.Get(FieldNamePassword) != e.Record.Get(FieldNamePassword) || + lastSavedRecord.Email() != e.Record.Email()) { + e.Record.RefreshTokenKey() + } } // cross-check that the auth record id is unique across all auth collections. diff --git a/core/record_model_test.go b/core/record_model_test.go index b01698ff..9ed208a4 100644 --- a/core/record_model_test.go +++ b/core/record_model_test.go @@ -1822,44 +1822,59 @@ func TestRecordSaveIdUpdateNoValidation(t *testing.T) { } } -func TestRecordSaveWithChangedPassword(t *testing.T) { +func TestRecordSaveWithAutoTokenKeyRefresh(t *testing.T) { t.Parallel() app, _ := tests.NewTestApp() defer app.Cleanup() - record, err := app.FindAuthRecordByEmail("nologin", "test@example.com") - if err != nil { - t.Fatal(err) + scenarios := []struct { + name string + payload map[string]any + expectedChange bool + }{ + { + "no email or password change", + map[string]any{"name": "example"}, + false, + }, + { + "password change", + map[string]any{"password": "1234567890"}, + true, + }, + { + "email change", + map[string]any{"email": "test_update@example.com"}, + true, + }, } - originalTokenKey := record.TokenKey() + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + record, err := app.FindFirstRecordByFilter("nologin", "1=1") + if err != nil { + t.Fatal(err) + } - t.Run("no password change shouldn't change the tokenKey", func(t *testing.T) { - record.Set("name", "example") + originalTokenKey := record.TokenKey() - if err := app.Save(record); err != nil { - t.Fatal(err) - } + record.Load(s.payload) - tokenKey := record.TokenKey() - if tokenKey == "" || originalTokenKey != tokenKey { - t.Fatalf("Expected tokenKey to not change, got %q VS %q", originalTokenKey, tokenKey) - } - }) + err = app.Save(record) + if err != nil { + t.Fatal(err) + } - t.Run("password change should change the tokenKey", func(t *testing.T) { - record.Set("password", "1234567890") + newTokenKey := record.TokenKey() - if err := app.Save(record); err != nil { - t.Fatal(err) - } + hasChange := originalTokenKey != newTokenKey - tokenKey := record.TokenKey() - if tokenKey == "" || originalTokenKey == tokenKey { - t.Fatalf("Expected tokenKey to change, got %q VS %q", originalTokenKey, tokenKey) - } - }) + if hasChange != s.expectedChange { + t.Fatalf("Expected hasChange %v, got %v", s.expectedChange, hasChange) + } + }) + } } func TestRecordDelete(t *testing.T) {