From e49025c8e52ea9589d4bb1fa80535d128a4b95d0 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Fri, 4 Apr 2025 22:53:14 +0300 Subject: [PATCH] moved the Create and Manage API rule checks out of the OnRecordCreateRequest hook finalizer --- CHANGELOG.md | 30 ++++++-- apis/record_crud.go | 151 +++++++++++++++++++-------------------- apis/record_crud_test.go | 26 ++----- 3 files changed, 106 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e66915f5..24a046c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,14 +1,34 @@ ## v0.27.0 (WIP) +- ⚠️ Moved the Create and Manage API rule checks out of the `OnRecordCreateRequest` hook finalizer, **aka. now all API rules are checked BEFORE triggering their corresponding `*Request` hook.**. + This was done to minimize the confusion regarding the firing order of the request operations, making it more predictable and consistent with the other record List/View/Update/Delete request actions. + It could be a minor break change if you are relying on the old behavior or have a Go `tests.ApiScenario` that is testing a Create API rule failure and expect `OnRecordCreateRequest` to be fired. In that case for example you may have to update your test scenario like: + ```go + tests.ApiScenario{ + Name: "Example test that checks a Create API rule failure" + Method: http.MethodPost, + URL: "/api/collections/example/records", + ... + // old: + ExpectedEvents: map[string]int{ + "*": 0, + "OnRecordCreateRequest": 1, + }, + // new: + ExpectedEvents: map[string]int{"*": 0}, + } + ``` + If you are having difficulties adjusting your code, feel free to open a [Q&A discussion](https://github.com/pocketbase/pocketbase/discussions) with the failing/problematic code sample and I'll try to assist you migrating it. + +- Added new `geoPoint` field for storing `{lon:x,lat:y}` GPS coordinates (@todo docs and more info for the updated fexpr functions support, geoDistance, record.GetGeoPoint, etc.). + +- Updated the `select` field UI to accommodate better larger lists and RTL languages ([#4674](https://github.com/pocketbase/pocketbase/issues/4674)). + - Updated the mail attachments auto MIME type detection to use `gabriel-vasile/mimetype` for consistency and broader sniffing signatures support. -- Updated the `select` field UI to accomodate better larger lists and RTL languages ([#4674](https://github.com/pocketbase/pocketbase/issues/4674)). - -- Added new `geoPoint` field for storing `{lon:x,lat:y}` record value (@todo docs). - - Forced `text/javascript` Content-Type when serving `.js`/`.mjs` collection uploaded files ([#6597](https://github.com/pocketbase/pocketbase/issues/6597)). -- Soft-deprecated the `$http.send`'s `result.raw` field in favour of `result.body` that contains the response body as plain bytes slice to avoid the discrepencies between Go and the JSVM when casting binary data to string. +- Soft-deprecated the `$http.send`'s `result.raw` field in favor of `result.body` that contains the response body as plain bytes slice to avoid the discrepancies between Go and the JSVM when casting binary data to string. (@todo update docs to use the new field) - Minor UI fixes (_removed the superuser fields from the auth record create/update body examples, etc._). diff --git a/apis/record_crud.go b/apis/record_crud.go index e0c0358f..dc1f83a6 100644 --- a/apis/record_crud.go +++ b/apis/record_crud.go @@ -251,6 +251,73 @@ func recordCreate(optFinalizer func(data any) error) func(e *core.RequestEvent) } } + // check the request and record data against the create and manage rules + if !hasSuperuserAuth && collection.CreateRule != nil { + dummyRecord := record.Clone() + + dummyRandomPart := "__pb_create__" + security.PseudorandomString(6) + + // set an id if it doesn't have already + // (the value doesn't matter; it is used only to minimize the breaking changes with earlier versions) + if dummyRecord.Id == "" { + dummyRecord.Id = "__temp_id__" + dummyRandomPart + } + + // unset the verified field to prevent manage API rule misuse in case the rule relies on it + dummyRecord.SetVerified(false) + + // export the dummy record data into db params + dummyExport, err := dummyRecord.DBExport(e.App) + if err != nil { + return e.BadRequestError("Failed to create record", fmt.Errorf("dummy DBExport error: %w", err)) + } + + dummyParams := make(dbx.Params, len(dummyExport)) + selects := make([]string, 0, len(dummyExport)) + var param string + for k, v := range dummyExport { + k = inflector.Columnify(k) // columnify is just as extra measure in case of custom fields + param = "__pb_create__" + k + dummyParams[param] = v + selects = append(selects, "{:"+param+"} AS [["+k+"]]") + } + + // shallow clone the current collection + dummyCollection := *collection + dummyCollection.Id += dummyRandomPart + dummyCollection.Name += inflector.Columnify(dummyRandomPart) + + withFrom := fmt.Sprintf("WITH {{%s}} as (SELECT %s)", dummyCollection.Name, strings.Join(selects, ",")) + + // check non-empty create rule + if *dummyCollection.CreateRule != "" { + ruleQuery := e.App.DB().Select("(1)").PreFragment(withFrom).From(dummyCollection.Name).AndBind(dummyParams) + + resolver := core.NewRecordFieldResolver(e.App, &dummyCollection, requestInfo, true) + + expr, err := search.FilterData(*dummyCollection.CreateRule).BuildExpr(resolver) + if err != nil { + return e.BadRequestError("Failed to create record", fmt.Errorf("create rule build expression failure: %w", err)) + } + ruleQuery.AndWhere(expr) + + resolver.UpdateQuery(ruleQuery) + + var exists int + err = ruleQuery.Limit(1).Row(&exists) + if err != nil || exists == 0 { + return e.BadRequestError("Failed to create record", fmt.Errorf("create rule failure: %w", err)) + } + } + + // check for manage rule access + manageRuleQuery := e.App.DB().Select("(1)").PreFragment(withFrom).From(dummyCollection.Name).AndBind(dummyParams) + if !form.HasManageAccess() && + hasAuthManageAccess(e.App, requestInfo, &dummyCollection, manageRuleQuery) { + form.GrantManagerAccess() + } + } + var isOptFinalizerCalled bool event := new(core.RecordRequestEvent) @@ -262,73 +329,6 @@ func recordCreate(optFinalizer func(data any) error) func(e *core.RequestEvent) form.SetApp(e.App) form.SetRecord(e.Record) - // temporary save the record and check it against the create and manage rules - if !hasSuperuserAuth && e.Collection.CreateRule != nil { - dummyRecord := e.Record.Clone() - - dummyRandomPart := "__pb_create__" + security.PseudorandomString(6) - - // set an id if it doesn't have already - // (the value doesn't matter; it is used only to minimize the breaking changes with earlier versions) - if dummyRecord.Id == "" { - dummyRecord.Id = "__temp_id__" + dummyRandomPart - } - - // unset the verified field to prevent manage API rule misuse in case the rule relies on it - dummyRecord.SetVerified(false) - - // export the dummy record data into db params - dummyExport, err := dummyRecord.DBExport(e.App) - if err != nil { - return e.BadRequestError("Failed to create record", fmt.Errorf("dummy DBExport error: %w", err)) - } - - dummyParams := make(dbx.Params, len(dummyExport)) - selects := make([]string, 0, len(dummyExport)) - var param string - for k, v := range dummyExport { - k = inflector.Columnify(k) // columnify is just as extra measure in case of custom fields - param = "__pb_create__" + k - dummyParams[param] = v - selects = append(selects, "{:"+param+"} AS [["+k+"]]") - } - - // shallow clone the current collection - dummyCollection := *e.Collection - dummyCollection.Id += dummyRandomPart - dummyCollection.Name += inflector.Columnify(dummyRandomPart) - - withFrom := fmt.Sprintf("WITH {{%s}} as (SELECT %s)", dummyCollection.Name, strings.Join(selects, ",")) - - // check non-empty create rule - if *dummyCollection.CreateRule != "" { - ruleQuery := e.App.DB().Select("(1)").PreFragment(withFrom).From(dummyCollection.Name).AndBind(dummyParams) - - resolver := core.NewRecordFieldResolver(e.App, &dummyCollection, requestInfo, true) - - expr, err := search.FilterData(*dummyCollection.CreateRule).BuildExpr(resolver) - if err != nil { - return e.BadRequestError("Failed to create record", fmt.Errorf("create rule build expression failure: %w", err)) - } - ruleQuery.AndWhere(expr) - - resolver.UpdateQuery(ruleQuery) - - var exists int - err = ruleQuery.Limit(1).Row(&exists) - if err != nil || exists == 0 { - return e.BadRequestError("Failed to create record", fmt.Errorf("create rule failure: %w", err)) - } - } - - // check for manage rule access - manageRuleQuery := e.App.DB().Select("(1)").PreFragment(withFrom).From(dummyCollection.Name).AndBind(dummyParams) - if !form.HasManageAccess() && - hasAuthManageAccess(e.App, requestInfo, &dummyCollection, manageRuleQuery) { - form.GrantManagerAccess() - } - } - err := form.Submit() if err != nil { return firstApiError(err, e.BadRequestError("Failed to create record", err)) @@ -441,6 +441,14 @@ func recordUpdate(optFinalizer func(data any) error) func(e *core.RequestEvent) } form.Load(data) + manageRuleQuery := e.App.DB().Select("(1)").From(collection.Name).AndWhere(dbx.HashExp{ + collection.Name + ".id": record.Id, + }) + if !form.HasManageAccess() && + hasAuthManageAccess(e.App, requestInfo, collection, manageRuleQuery) { + form.GrantManagerAccess() + } + var isOptFinalizerCalled bool event := new(core.RecordRequestEvent) @@ -452,15 +460,6 @@ func recordUpdate(optFinalizer func(data any) error) func(e *core.RequestEvent) form.SetApp(e.App) form.SetRecord(e.Record) - manageRuleQuery := e.App.DB().Select("(1)").From(e.Collection.Name).AndWhere(dbx.HashExp{ - // note: use the original record id and not e.Record.Id because it may get overwritten - e.Collection.Name + ".id": e.Record.LastSavedPK(), - }) - if !form.HasManageAccess() && - hasAuthManageAccess(e.App, requestInfo, e.Collection, manageRuleQuery) { - form.GrantManagerAccess() - } - err := form.Submit() if err != nil { return firstApiError(err, e.BadRequestError("Failed to update record.", err)) diff --git a/apis/record_crud_test.go b/apis/record_crud_test.go index 9e8e5b3b..0d7b3cc1 100644 --- a/apis/record_crud_test.go +++ b/apis/record_crud_test.go @@ -1563,10 +1563,7 @@ func TestRecordCrudCreate(t *testing.T) { Body: strings.NewReader(`{"title":"test123"}`), ExpectedStatus: 400, ExpectedContent: []string{`"data":{}`}, - ExpectedEvents: map[string]int{ - "*": 0, - "OnRecordCreateRequest": 1, - }, + ExpectedEvents: map[string]int{"*": 0}, }, { Name: "auth record submit in restricted collection (rule failure check)", @@ -1579,10 +1576,7 @@ func TestRecordCrudCreate(t *testing.T) { }, ExpectedStatus: 400, ExpectedContent: []string{`"data":{}`}, - ExpectedEvents: map[string]int{ - "*": 0, - "OnRecordCreateRequest": 1, - }, + ExpectedEvents: map[string]int{"*": 0}, }, { Name: "auth record submit in restricted collection (rule pass check) + expand relations", @@ -1731,10 +1725,7 @@ func TestRecordCrudCreate(t *testing.T) { }, ExpectedStatus: 400, ExpectedContent: []string{`"data":{}`}, - ExpectedEvents: map[string]int{ - "*": 0, - "OnRecordCreateRequest": 1, - }, + ExpectedEvents: map[string]int{"*": 0}, }, { Name: "submit via multipart form data with @jsonPayload key and satisfied @request.body rule", @@ -1974,14 +1965,9 @@ func TestRecordCrudCreate(t *testing.T) { "total+":4, "total-":2 }`), - ExpectedStatus: 400, - ExpectedContent: []string{ - `"data":{}`, - }, - ExpectedEvents: map[string]int{ - "*": 0, - "OnRecordCreateRequest": 1, - }, + ExpectedStatus: 400, + ExpectedContent: []string{`"data":{}`}, + ExpectedEvents: map[string]int{"*": 0}, }, { Name: "@request.body.field with compute modifers (rule pass check)",