diff --git a/docs/backend/admin-controllers.md b/docs/backend/admin-controllers.md index 091f1e0..763a42a 100644 --- a/docs/backend/admin-controllers.md +++ b/docs/backend/admin-controllers.md @@ -221,4 +221,4 @@ Scope reads and writes with `pact.ListExtendQuery` and `pact.FormExtendQuery` ra ## Toolbar actions -`toolbar.buttons` in `config_list.yaml` lists the built-in `create` and `delete` and any action the controller registers through `pact.HasAdminActions`. See [Partials and widgets](partials-and-widgets.md) for actions and the rest of the extension points. +`toolbar.buttons` in `config_list.yaml` lists the built-in `create` and `delete` and any action the controller registers through `pact.HasAdminActions`. The declarations are enforced by the server, not only shown by the SPA. `POST /{controller}` needs a `config_form.yaml` and `create` in `toolbar.buttons`; `PUT` and `DELETE /{controller}/{id}` need a form (the form screen carries the delete button, as in WinterCMS); `POST /{controller}/bulk-delete` needs `delete` in `toolbar.buttons`, which in turn needs `showCheckboxes: true`. A write the controller does not declare answers 403 `forbidden`. See [Partials and widgets](partials-and-widgets.md) for actions and the rest of the extension points. diff --git a/modules/cabana/README.md b/modules/cabana/README.md index e11b1e5..4bf0589 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -37,9 +37,9 @@ All paths are relative to `/api/v1`. A controller ID `vendor.plugin.cont | GET `/navigation`, GET `/settings` | Navigation and settings entries the administrator may open. | | GET `/settings/{code}/schema`, GET and PUT `/settings/{code}` | Settings form schema, values and update. | | GET `/{vendor}/{plugin}/{controller}/schema/list`, `.../schema/form`, `.../schema/relation/{name}` | Localized list, form and relation schemas. | -| GET and POST `/{vendor}/{plugin}/{controller}` | List records; create a record. | -| GET, PUT and DELETE `/{vendor}/{plugin}/{controller}/{id}` | Show, update and delete a record. | -| POST `/{vendor}/{plugin}/{controller}/bulk-delete` | Delete a set of records in one transaction. | +| GET and POST `/{vendor}/{plugin}/{controller}` | List records; create a record (needs a form and `create` in `toolbar.buttons`). | +| GET, PUT and DELETE `/{vendor}/{plugin}/{controller}/{id}` | Show, update and delete a record (update and delete need a form). | +| POST `/{vendor}/{plugin}/{controller}/bulk-delete` | Delete a set of records in one transaction; needs `delete` in `toolbar.buttons`. | | POST `/{vendor}/{plugin}/{controller}/widgets/{field}` | Run the action of a `type: widget` field with an optional `record_id` and the fill snapshot; answers `{message, fill}`. | | POST `/{vendor}/{plugin}/{controller}/toolbar/{action}` | Run a registered toolbar action with an empty `{}` body; answers `{message, fill: {}}`. | | GET `/{vendor}/{plugin}/{controller}/partials/{name}` | Render a declared header or form partial as a node tree; `?id=` (form partials only) passes the scoped record to the view model. | diff --git a/modules/cabana/crud_lifecycle_test.go b/modules/cabana/crud_lifecycle_test.go index 98e1417..e500267 100644 --- a/modules/cabana/crud_lifecycle_test.go +++ b/modules/cabana/crud_lifecycle_test.go @@ -498,3 +498,63 @@ func containsString(items []string, want string) bool { } return false } + +// TestCRUDOperationsFollowDeclarations pins WR-03: the compiled list and form +// decide which writes the server accepts, not only what the SPA shows. +func TestCRUDOperationsFollowDeclarations(t *testing.T) { + _, httpSvc, cc, db, hooks := hookFixture(t) + ctx := principalCtx(hooks, superUser()) + created := crudCall(httpSvc, http.MethodPost, "", []byte(`{"name":"Ada"}`), ctx) + if created.Code != http.StatusCreated { + t.Fatalf("declared create=%d %s", created.Code, created.Body.String()) + } + id := uintString(decodeData(t, created.Body.Bytes())["id"]) + bulk := func() *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"ids":[`+id+`]}`)).WithContext(ctx) + req.SetPathValue("vendor", "acme") + req.SetPathValue("plugin", "demo") + req.SetPathValue("controller", "records") + rec := httptest.NewRecorder() + httpSvc.bulkDelete(rec, req) + return rec + } + forbidden := func(name string, rec *httptest.ResponseRecorder) { + t.Helper() + if rec.Code != http.StatusForbidden || !strings.Contains(rec.Body.String(), `"forbidden"`) { + t.Fatalf("%s=%d %s, want 403 forbidden", name, rec.Code, rec.Body.String()) + } + } + + // No toolbar create button: the create route is closed, update is not. + list := *cc.List + cc.List = &list + cc.List.ToolbarButtons = []string{"delete"} + forbidden("create without a toolbar create button", crudCall(httpSvc, http.MethodPost, "", []byte(`{"name":"Bea"}`), ctx)) + if got := crudCall(httpSvc, http.MethodPut, id, []byte(`{"name":"Cid"}`), ctx); got.Code != http.StatusOK { + t.Fatalf("update=%d %s", got.Code, got.Body.String()) + } + + // No toolbar delete button: bulk delete is closed. + cc.List.ToolbarButtons = []string{"create"} + forbidden("bulk delete without a toolbar delete button", bulk()) + if n := countCrud(t, db); n != 1 { + t.Fatalf("rows=%d, a refused bulk delete removed data", n) + } + + // No form: nothing can be created, updated or deleted one by one. + form := cc.Form + cc.Form = nil + forbidden("create without a form", crudCall(httpSvc, http.MethodPost, "", []byte(`{"name":"Dan"}`), ctx)) + forbidden("update without a form", crudCall(httpSvc, http.MethodPut, id, []byte(`{"name":"Eve"}`), ctx)) + forbidden("delete without a form", crudCall(httpSvc, http.MethodDelete, id, nil, ctx)) + if n := countCrud(t, db); n != 1 { + t.Fatalf("rows=%d, a refused write changed data", n) + } + cc.Form = form + + // Declared again: bulk delete works. + cc.List.ToolbarButtons = []string{"create", "delete"} + if got := bulk(); got.Code != http.StatusOK || deletedCount(t, got.Body.Bytes()) != 1 { + t.Fatalf("declared bulk delete=%d %s", got.Code, got.Body.String()) + } +} diff --git a/modules/cabana/crud_test.go b/modules/cabana/crud_test.go index 6cf3985..fa7d2a0 100644 --- a/modules/cabana/crud_test.go +++ b/modules/cabana/crud_test.go @@ -47,6 +47,9 @@ const crudListConfig = `modelClass: Record list: ~/plugins/acme/demo/models/record/columns.yaml recordsPerPage: 20 showSearch: true +showCheckboxes: true +toolbar: + buttons: [create, delete] ` const crudColumns = `columns: diff --git a/modules/cabana/http.go b/modules/cabana/http.go index 4ec8b5b..d5cc0ca 100644 --- a/modules/cabana/http.go +++ b/modules/cabana/http.go @@ -606,6 +606,9 @@ func (s *service) show(w http.ResponseWriter, r *http.Request) { func (s *service) create(w http.ResponseWriter, r *http.Request) { s.protect(w, r, func(cc *CompiledController) { + if !s.operationDeclared(w, r, cc, "create") { + return + } body, err := decodeObject(r) if err != nil { writeCRUDError(w, err) @@ -627,6 +630,9 @@ func (s *service) create(w http.ResponseWriter, r *http.Request) { func (s *service) update(w http.ResponseWriter, r *http.Request) { s.protect(w, r, func(cc *CompiledController) { + if !s.operationDeclared(w, r, cc, "update") { + return + } id, err := pathID(r) if err != nil { writeCRUDError(w, err) @@ -653,6 +659,9 @@ func (s *service) update(w http.ResponseWriter, r *http.Request) { func (s *service) bulkDelete(w http.ResponseWriter, r *http.Request) { s.protect(w, r, func(cc *CompiledController) { + if !s.operationDeclared(w, r, cc, "bulk-delete") { + return + } in, err := decodeBulk(r) if err != nil { writeCRUDError(w, err) @@ -684,6 +693,9 @@ func decodeBulk(r *http.Request) (BulkDeleteInput, error) { func (s *service) deleteRecord(w http.ResponseWriter, r *http.Request) { s.protect(w, r, func(cc *CompiledController) { + if !s.operationDeclared(w, r, cc, "delete") { + return + } id, err := pathID(r) if err != nil { writeCRUDError(w, err) @@ -759,6 +771,23 @@ func (s *service) protect(w http.ResponseWriter, r *http.Request, fn func(*Compi fn(cc) } +// operationDeclared refuses a write the controller's YAML does not declare, so +// the compiled list and form are the capability, not a hint for the SPA. Create +// needs a form and a toolbar `create` button; update and single-record delete +// (the form screen's delete button, as in Winter) need a form; bulk delete needs +// the toolbar `delete` button, which in turn needs showCheckboxes. The answer is +// 403 with the same envelope as a permission failure. +func (s *service) operationDeclared(w http.ResponseWriter, r *http.Request, cc *CompiledController, op string) bool { + if cc.operationDeclared(op) { + return true + } + if principal, _ := bouncer.User(r.Context()); principal != nil { + s.logAuth(r, "denied", principal.ID) + } + WriteError(w, http.StatusForbidden, "forbidden", msgForbidden) + return false +} + func projectRow(row any, controller pact.AdminController, cols []ListColumn) map[string]any { v := reflect.ValueOf(row) for v.Kind() == reflect.Pointer { diff --git a/modules/cabana/registry.go b/modules/cabana/registry.go index cd1603e..5196b3b 100644 --- a/modules/cabana/registry.go +++ b/modules/cabana/registry.go @@ -107,6 +107,29 @@ func compileRegistry(items []controllerRef) (*Registry, error) { return &Registry{byID: byID, assets: assets}, nil } +// operationDeclared reports whether the controller's compiled list and form +// declare the write operation: create, update, delete or bulk-delete. +func (cc *CompiledController) operationDeclared(op string) bool { + switch op { + case "create": + return cc.Form != nil && (cc.List == nil || hasToolbarButton(cc.List, "create")) + case "update", "delete": + return cc.Form != nil + case "bulk-delete": + return cc.List != nil && hasToolbarButton(cc.List, "delete") + } + return false +} + +func hasToolbarButton(list *ListSchema, name string) bool { + for _, button := range list.ToolbarButtons { + if button == name { + return true + } + } + return false +} + func withoutAction(actions []string, drop string) []string { out := make([]string, 0, len(actions)) for _, action := range actions { diff --git a/modules/cabana/relation_field_test.go b/modules/cabana/relation_field_test.go index 6981b5d..eee713d 100644 --- a/modules/cabana/relation_field_test.go +++ b/modules/cabana/relation_field_test.go @@ -25,6 +25,9 @@ modelClass: Record const p10ListConfig = `modelClass: Record list: ~/plugins/acme/demo/models/record/columns.yaml recordsPerPage: 20 +showCheckboxes: true +toolbar: + buttons: [create, delete] ` const p10Columns = `columns: diff --git a/modules/cabana/security_coverage_test.go b/modules/cabana/security_coverage_test.go index 8443161..b19657c 100644 --- a/modules/cabana/security_coverage_test.go +++ b/modules/cabana/security_coverage_test.go @@ -294,7 +294,11 @@ func phase09DeniedService() *service { controller := orderController{perms: []string{"acme.demo.access"}} return &service{reg: &Registry{ byID: map[string]*CompiledController{ - "acme.demo.widgets": {Controller: controller, Relations: map[string]*CompiledRelation{}}, + "acme.demo.widgets": { + Controller: controller, Relations: map[string]*CompiledRelation{}, + // Declare every write so the denial under test is the permission. + Form: &FormSchema{}, List: &ListSchema{ToolbarButtons: []string{"create", "delete"}}, + }, }, settings: map[string]*CompiledSetting{ "demo": {Item: pact.SettingsItem{Code: "demo", Permissions: []string{"acme.demo.manage_settings"}}},