From d34c8ec048200c778c416ebc20236cf10fc44e33 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Thu, 26 Dec 2024 13:24:03 +0200 Subject: [PATCH] added record.SetRandomPassword() helper and updated oauth2 autogenerated password handling --- CHANGELOG.md | 3 ++ apis/record_auth_with_oauth2.go | 9 +----- apis/record_auth_with_oauth2_test.go | 12 ++++++++ apis/record_crud.go | 21 ++++++++++++-- core/record_model_auth.go | 21 ++++++++++++++ core/record_model_auth_test.go | 41 ++++++++++++++++++++++++++++ forms/record_upsert.go | 26 +++++++++--------- forms/record_upsert_test.go | 30 ++++++++++---------- 8 files changed, 126 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09865f9b..066ca3e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ - Added auth collection select for the settings "Send test email" popup ([#6166](https://github.com/pocketbase/pocketbase/issues/6166)). +- Added `record.SetRandomPassword()` to simplify random password generation usually used in the OAuth2 or OTP record creation flows. + _The generated ~30 chars random password is assigned directly as bcrypt hash and ignores the `password` field plain value validators like min/max length or regex pattern._ + - ⚠️ 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_with_oauth2.go b/apis/record_auth_with_oauth2.go index 1d37e331..4f8a1273 100644 --- a/apis/record_auth_with_oauth2.go +++ b/apis/record_auth_with_oauth2.go @@ -16,7 +16,6 @@ import ( "github.com/pocketbase/pocketbase/tools/auth" "github.com/pocketbase/pocketbase/tools/dbutils" "github.com/pocketbase/pocketbase/tools/filesystem" - "github.com/pocketbase/pocketbase/tools/security" "golang.org/x/oauth2" ) @@ -224,12 +223,6 @@ func oauth2Submit(e *core.RecordAuthWithOAuth2RequestEvent, optExternalAuth *cor payload[core.FieldNameEmail] = e.OAuth2User.Email - // set a random password if none is set - if v, _ := payload[core.FieldNamePassword].(string); v == "" { - payload[core.FieldNamePassword] = security.RandomString(30) - payload[core.FieldNamePassword+"Confirm"] = payload[core.FieldNamePassword] - } - // map known fields (unless the field was explicitly submitted as part of CreateData) if _, ok := payload[e.Collection.OAuth2.MappedFields.Id]; !ok && e.Collection.OAuth2.MappedFields.Id != "" { payload[e.Collection.OAuth2.MappedFields.Id] = e.OAuth2User.Id @@ -292,7 +285,7 @@ func oauth2Submit(e *core.RecordAuthWithOAuth2RequestEvent, optExternalAuth *cor // set random password for users with unverified email // (this is in case a malicious actor has registered previously with the user email) if !isLoggedAuthRecord && e.Record.Email() != "" && !e.Record.Verified() { - e.Record.SetPassword(security.RandomString(30)) + e.Record.SetRandomPassword() needUpdate = true } diff --git a/apis/record_auth_with_oauth2_test.go b/apis/record_auth_with_oauth2_test.go index da950485..2ca11f81 100644 --- a/apis/record_auth_with_oauth2_test.go +++ b/apis/record_auth_with_oauth2_test.go @@ -958,6 +958,8 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "createData": { "email": "invalid", "emailVisibility": true, + "password": "1234567890", + "passwordConfirm": "1234567890", "name": "test_name", "username": "test_username", "rel": "0yxhwia2amd8gec" @@ -1027,6 +1029,16 @@ func TestRecordAuthWithOAuth2(t *testing.T) { "OnModelValidate": 4, "OnRecordValidate": 4, }, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + user, err := app.FindFirstRecordByData("users", "username", "test_username") + if err != nil { + t.Fatal(err) + } + + if !user.ValidatePassword("1234567890") { + t.Fatalf("Expected password %q to be valid", "1234567890") + } + }, }, { Name: "creating user (with mapped OAuth2 fields and avatarURL->file field)", diff --git a/apis/record_crud.go b/apis/record_crud.go index dac5a0cd..e6c2e68f 100644 --- a/apis/record_crud.go +++ b/apis/record_crud.go @@ -94,9 +94,9 @@ func recordsList(e *core.RequestEvent) error { // Add a randomized throttle in case of too many empty search filter attempts. // - // This is just for extra precaution since security researches raised concern regarding the possibity of eventual + // This is just for extra precaution since security researches raised concern regarding the possibility of eventual // timing attacks because the List API rule acts also as filter and executes in a single run with the client-side filters. - // This is by design and it is an accepted tradeoff between performance, usability and correctness. + // This is by design and it is an accepted trade off between performance, usability and correctness. // // While technically the below doesn't fully guarantee protection against filter timing attacks, in practice combined with the network latency it makes them even less feasible. // A properly configured rate limiter or individual fields Hidden checks are better suited if you are really concerned about eventual information disclosure by side-channel attacks. @@ -220,6 +220,16 @@ func recordCreate(optFinalizer func(data any) error) func(e *core.RequestEvent) return firstApiError(err, e.BadRequestError("Failed to read the submitted data.", err)) } + // set a random password for the OAuth2 ignoring its plain password validators + var skipPlainPasswordRecordValidators bool + if requestInfo.Context == core.RequestInfoContextOAuth2 { + if _, ok := data[core.FieldNamePassword]; !ok { + data[core.FieldNamePassword] = security.RandomString(30) + data[core.FieldNamePassword+"Confirm"] = data[core.FieldNamePassword] + skipPlainPasswordRecordValidators = true + } + } + // replace modifiers fields so that the resolved value is always // available when accessing requestInfo.Body requestInfo.Body = data @@ -230,6 +240,13 @@ func recordCreate(optFinalizer func(data any) error) func(e *core.RequestEvent) } form.Load(data) + if skipPlainPasswordRecordValidators { + // unset the plain value to skip the plain password field validators + if raw, ok := record.GetRaw(core.FieldNamePassword).(*core.PasswordFieldValue); ok { + raw.Plain = "" + } + } + var isOptFinalizerCalled bool event := new(core.RecordRequestEvent) diff --git a/core/record_model_auth.go b/core/record_model_auth.go index 00fae0f8..07858f43 100644 --- a/core/record_model_auth.go +++ b/core/record_model_auth.go @@ -1,5 +1,7 @@ package core +import "github.com/pocketbase/pocketbase/tools/security" + // Email returns the "email" record field value (usually available with Auth collections). func (m *Record) Email() string { return m.GetString(FieldNameEmail) @@ -51,6 +53,25 @@ func (m *Record) SetPassword(password string) { m.Set(FieldNamePassword, password) } +// SetRandomPassword sets the "password" auth record field to a random autogenerated value. +// +// The autogenerated password is ~30 characters and it is set directly as hash, +// aka. the field plain password value validators (length, pattern, etc.) are ignored +// (this is usually used as part of the auto created OTP or OAuth2 user flows). +func (m *Record) SetRandomPassword() string { + pass := security.RandomString(30) + + m.Set(FieldNamePassword, pass) + m.RefreshTokenKey() // manually refresh the token key because the plain password is resetted + + // unset the plain value to skip the field validators + if raw, ok := m.GetRaw(FieldNamePassword).(*PasswordFieldValue); ok { + raw.Plain = "" + } + + return pass +} + // ValidatePassword validates a plain password against the "password" record field. // // Returns false if the password is incorrect. diff --git a/core/record_model_auth_test.go b/core/record_model_auth_test.go index da748f00..f7168500 100644 --- a/core/record_model_auth_test.go +++ b/core/record_model_auth_test.go @@ -1,9 +1,11 @@ package core_test import ( + "context" "testing" "github.com/pocketbase/pocketbase/core" + "github.com/pocketbase/pocketbase/tests" "github.com/pocketbase/pocketbase/tools/security" ) @@ -117,3 +119,42 @@ func TestRecordPassword(t *testing.T) { }) } } + +func TestRecordSetRandomPassword(t *testing.T) { + t.Parallel() + + app, _ := tests.NewTestApp() + defer app.Cleanup() + + oldTokenKey := "old_tokenKey" + record := core.NewRecord(core.NewAuthCollection("test")) + record.SetTokenKey(oldTokenKey) + + pass := record.SetRandomPassword() + + if pass == "" { + t.Fatal("Expected non-empty generated random password") + } + + if !record.ValidatePassword(pass) { + t.Fatal("Expected the generated random password to be valid") + } + + if record.TokenKey() == oldTokenKey { + t.Fatal("Expected token key to change") + } + + f, ok := record.Collection().Fields.GetByName(core.FieldNamePassword).(*core.PasswordField) + if !ok { + t.Fatal("Expected *core.PasswordField") + } + + // ensure that the field validators will be ignored + f.Min = 1 + f.Max = 2 + f.Pattern = `\d+` + + if err := f.ValidateValue(context.Background(), app, record); err != nil { + t.Fatalf("Expected password field plain value validators to be ignored, got %v", err) + } +} diff --git a/forms/record_upsert.go b/forms/record_upsert.go index 04b36c0c..653823bc 100644 --- a/forms/record_upsert.go +++ b/forms/record_upsert.go @@ -27,9 +27,9 @@ type RecordUpsert struct { accessLevel int // extra password fields - Password string `form:"password" json:"password"` - PasswordConfirm string `form:"passwordConfirm" json:"passwordConfirm"` - OldPassword string `form:"oldPassword" json:"oldPassword"` + password string + passwordConfirm string + oldPassword string } // NewRecordUpsert creates a new [RecordUpsert] form from the provided [core.App] and [core.Record] instances @@ -93,13 +93,13 @@ func (form *RecordUpsert) Load(data map[string]any) { // load the special auth form fields if isAuth { if v, ok := data["password"]; ok { - form.Password = cast.ToString(v) + form.password = cast.ToString(v) } if v, ok := data["passwordConfirm"]; ok { - form.PasswordConfirm = cast.ToString(v) + form.passwordConfirm = cast.ToString(v) } if v, ok := data["oldPassword"]; ok { - form.OldPassword = cast.ToString(v) + form.oldPassword = cast.ToString(v) } excludeFields = append(excludeFields, "passwordConfirm", "oldPassword") // skip non-schema password fields @@ -137,9 +137,9 @@ func (form *RecordUpsert) validateFormFields() error { validateData := map[string]any{ "email": form.record.Email(), "verified": form.record.Verified(), - "password": form.Password, - "passwordConfirm": form.PasswordConfirm, - "oldPassword": form.OldPassword, + "password": form.password, + "passwordConfirm": form.passwordConfirm, + "oldPassword": form.oldPassword, } return validation.Validate(validateData, @@ -165,17 +165,17 @@ func (form *RecordUpsert) validateFormFields() error { validation.Key( "password", validation.When( - (isNew || form.PasswordConfirm != "" || form.OldPassword != ""), + (isNew || form.passwordConfirm != "" || form.oldPassword != ""), validation.Required, ), ), validation.Key( "passwordConfirm", validation.When( - (isNew || form.Password != "" || form.OldPassword != ""), + (isNew || form.password != "" || form.oldPassword != ""), validation.Required, ), - validation.By(validators.Equal(form.Password)), + validation.By(validators.Equal(form.password)), ), validation.Key( "oldPassword", @@ -183,7 +183,7 @@ func (form *RecordUpsert) validateFormFields() error { // - form.HasManageAccess() is not satisfied // - changing the existing password validation.When( - !isNew && !form.HasManageAccess() && (form.Password != "" || form.PasswordConfirm != ""), + !isNew && !form.HasManageAccess() && (form.password != "" || form.passwordConfirm != ""), validation.Required, validation.By(form.checkOldPassword), ), diff --git a/forms/record_upsert_test.go b/forms/record_upsert_test.go index bd88629e..82633593 100644 --- a/forms/record_upsert_test.go +++ b/forms/record_upsert_test.go @@ -64,15 +64,15 @@ func TestRecordUpsertLoad(t *testing.T) { `"text":"test_text"`, `"number":456`, `"select_many":["optionB","optionC"]`, - `"password":""`, - `"oldPassword":""`, - `"passwordConfirm":""`, `"created":""`, `"updated":""`, `"json":null`, }, notExpected: []string{ `"custom"`, + `"password"`, + `"oldPassword"`, + `"passwordConfirm"`, `"select_many-"`, `"select_many+"`, }, @@ -89,9 +89,11 @@ func TestRecordUpsertLoad(t *testing.T) { record: core.NewRecord(usersCol), expected: []string{ `"email":"test@example.com"`, - `"oldPassword":"123"`, `"password":"456"`, - `"passwordConfirm":"789"`, + }, + notExpected: []string{ + `"oldPassword"`, + `"passwordConfirm"`, }, }, { @@ -110,8 +112,10 @@ func TestRecordUpsertLoad(t *testing.T) { `"email":"test@example.com"`, `"tokenKey":""`, `"password":"456"`, - `"oldPassword":"123"`, - `"passwordConfirm":"789"`, + }, + notExpected: []string{ + `"oldPassword"`, + `"passwordConfirm"`, }, }, { @@ -130,8 +134,10 @@ func TestRecordUpsertLoad(t *testing.T) { `"email":"test@example.com"`, `"tokenKey":"abc"`, `"password":"456"`, - `"oldPassword":"123"`, - `"passwordConfirm":"789"`, + }, + notExpected: []string{ + `"oldPassword"`, + `"passwordConfirm"`, }, }, { @@ -168,11 +174,7 @@ func TestRecordUpsertLoad(t *testing.T) { form.Load(s.data) - loaded := map[string]any{ - "oldPassword": form.OldPassword, - "password": form.Password, - "passwordConfirm": form.PasswordConfirm, - } + loaded := map[string]any{} maps.Copy(loaded, s.record.FieldsData()) maps.Copy(loaded, s.record.CustomData())