diff --git a/README.md b/README.md index 6475c4e..051aef7 100644 --- a/README.md +++ b/README.md @@ -64,6 +64,10 @@ Every route except the calendar feed itself needs `Authorization: Bearer = MaterialFilesMax { - return nil, "A material holds at most " + strconv.Itoa(MaterialFilesMax) + " files.", nil - } - f, err := attach.Store(ctx, tx, bucket, attach.Upload{FileName: name, Body: body, Public: true}, lim) +// fileRefusal is a file the material routes turn down, with no error: +// Reason is "count" (the material holds MaterialFilesMax files), "size" +// (attach.ErrTooLarge) or "type" (a type attach.Store refuses), and Text the +// message of the answer. The zero value is no refusal. +type fileRefusal struct { + Reason string + Text string +} + +// fileRefusalFor maps an attach.Store error to the refusal of file name +// under lim, and reports false for an error that is not a refusal (nil +// included). attach.ErrNotImage, the refusal of image mode, is a type +// refusal, not a 500. +func fileRefusalFor(err error, name string, lim attach.Limits) (fileRefusal, bool) { switch { case errors.Is(err, attach.ErrTooLarge): - return nil, name + ": the file is larger than " + sizeMB(lim.MaxBytes) + ".", nil - case errors.Is(err, attach.ErrFileType), errors.Is(err, attach.ErrMIMEType): - return nil, name + ": this file type is not allowed.", nil - case err != nil: - return nil, "", err + return fileRefusal{Reason: "size", Text: name + ": the file is larger than " + sizeMB(lim.MaxBytes) + "."}, true + case errors.Is(err, attach.ErrFileType), errors.Is(err, attach.ErrMIMEType), errors.Is(err, attach.ErrNotImage): + return fileRefusal{Reason: "type", Text: name + ": this file type is not allowed."}, true + } + return fileRefusal{}, false +} + +// storeMaterialUpload stores one file through attach.Store as a public file +// of m and binds it to the material, inside tx. It is the only way the API +// writes a material file: the multipart route, the inline (base64) route, +// the signed upload and the editor image route all come through it. The +// material's MaterialFilesMax cap and attach's extension, MIME, image and +// size checks (lim) apply. A refusal is returned with no error; the +// returned file, when not nil, has blobs the caller deletes when tx does +// not commit. +func storeMaterialUpload(ctx context.Context, tx *gorm.DB, bucket *blob.Bucket, m models.Material, name string, body io.Reader, lim attach.Limits) (*attach.File, fileRefusal, error) { + var have int64 + if err := materialFiles(tx.Model(&attach.File{}), m).Count(&have).Error; err != nil { + return nil, fileRefusal{}, err + } + if have >= MaterialFilesMax { + return nil, fileRefusal{Reason: "count", Text: "A material holds at most " + strconv.Itoa(MaterialFilesMax) + " files."}, nil + } + f, err := attach.Store(ctx, tx, bucket, attach.Upload{FileName: name, Body: body, Public: true}, lim) + if why, ok := fileRefusalFor(err, name, lim); ok { + return nil, why, nil + } + if err != nil { + return nil, fileRefusal{}, err } err = tx.Model(&attach.File{}).Where("id = ?", f.ID).Updates(map[string]any{ "attachment_type": m.MorphName(), "attachment_id": strconv.FormatUint(uint64(m.ID), 10), "field": models.MaterialFilesField, }).Error - return f, "", err + return f, fileRefusal{}, err +} + +// storeMaterialFile is storeMaterialUpload with the refusal as its text, +// for the management routes that answer only the text. +func storeMaterialFile(ctx context.Context, tx *gorm.DB, bucket *blob.Bucket, m models.Material, name string, body io.Reader, lim attach.Limits) (*attach.File, string, error) { + f, why, err := storeMaterialUpload(ctx, tx, bucket, m, name, body, lim) + return f, why.Text, err } // sizeMB names a byte limit in the refusal texts: 50 MB for 50 MiB, 3 MB diff --git a/controllers/api/material_edit.go b/controllers/api/material_edit.go index 1580a6b..2746f17 100644 --- a/controllers/api/material_edit.go +++ b/controllers/api/material_edit.go @@ -4,12 +4,15 @@ import ( "bytes" "context" "encoding/json" + "errors" + "io" "net/http" "slices" "strconv" "strings" "git.golem15.com/golem15/summercms/modules/lagoon" + "git.golem15.com/golem15/summercms/modules/lagoon/attach" "git.golem15.com/golem15/summercms/modules/wire" "git.golem15.com/jakub/sm-bm-plugin/models" "gorm.io/gorm" @@ -274,3 +277,189 @@ func (h *Handlers) writeStudentConflict(w http.ResponseWriter, r *http.Request, "data": out, }) } + +// imageLimits are the attach.Store rules of an editor image (D-16): image +// mode sniffs and decodes the bytes and admits only JPEG, PNG, GIF and +// WebP (never SVG), up to MaterialFileMaxBytes. +var imageLimits = attach.Limits{MaxBytes: MaterialFileMaxBytes, Image: true} + +// Tags answers GET /tags: the tag dictionary the editor picks from (A2), +// every row of jz_bm_tags by name, to material managers; anyone else gets a +// 403. +func (h *Handlers) Tags(w http.ResponseWriter, r *http.Request) { + userID, ok := h.caller(w, r) + if !ok { + return + } + db, ok := h.db(w, r) + if !ok { + return + } + manager, err := h.managesMaterials(r.Context(), db, userID) + if err != nil { + writeOpaque500(w) + return + } + if !manager { + writeForbidden(w) + return + } + var tags []models.Tag + if err := db.Order("name, id").Find(&tags).Error; err != nil { + writeOpaque500(w) + return + } + writeData(w, newTagsJSON(tags)) +} + +// UnpublishMaterial answers POST /materials/{id}/unpublish (D-11, "Usuń" in +// the app): the material becomes a draft (published_at NULL), which the +// student API no longer serves, so even its author gets a 404 afterwards. +// Nothing is deleted: a push without --draft or /bmadmin publishes it +// again. The revision stays (D-03). The search document is resynced, not +// removed, because the management search keeps drafts. 204. +func (h *Handlers) UnpublishMaterial(w http.ResponseWriter, r *http.Request) { + userID, ok := h.caller(w, r) + if !ok { + return + } + db, ok := h.db(w, r) + if !ok { + return + } + m, manager, ok := h.editableMaterialForCaller(w, r, db, userID) + if !ok { + return + } + outcome := editAllowed + err := lagoon.Transaction(r.Context(), db, func(_ context.Context, tx *gorm.DB) error { + cur, why, err := h.lockEditableMaterial(tx, userID, m.ID, manager) + if err != nil || why != editAllowed { + outcome = why + return err + } + return tx.Model(&models.Material{}).Where("id = ?", cur.ID).Update("published_at", nil).Error + }) + switch { + case err != nil: + writeOpaque500(w) + case outcome == editGone: + writeNotFound(w) + case outcome == editForbidden: + writeForbidden(w) + default: + h.MaterialIndex.Sync(r.Context(), db, m.ID) + w.WriteHeader(http.StatusNoContent) + } +} + +// writeFileRefusal answers 422 to a refused editor image, with the reason +// (type, size or count) for the editor's message. +func writeFileRefusal(w http.ResponseWriter, why fileRefusal) { + wire.WriteJSON(w, http.StatusUnprocessableEntity, map[string]any{ + "message": why.Text, + "errors": map[string][]string{"file": {why.Text}}, + "reason": why.Reason, + }) +} + +// UploadMaterialImage answers POST /materials/{id}/images (D-16): the first +// multipart part named "file" is stored as a public file of the material +// through storeMaterialUpload with imageLimits, and the file text job is +// queued in the same transaction. One image per request. The revision does +// not change: the body edit that links the image bumps it on save. 201 +// {data: {id, url, name, size, mime}}; a refused file is a 422 with reason +// type, size or count, an over-limit body a 413. +func (h *Handlers) UploadMaterialImage(w http.ResponseWriter, r *http.Request) { + userID, ok := h.caller(w, r) + if !ok { + return + } + db, ok := h.db(w, r) + if !ok { + return + } + bucket, ok := h.bucket(w) + if !ok { + return + } + m, manager, ok := h.editableMaterialForCaller(w, r, db, userID) + if !ok { + return + } + mr, err := r.MultipartReader() + if err != nil { + writeInvalid(w, "Send the image as multipart/form-data in a part named \"file\".") + return + } + errRefused := errors.New("refused") + outcome := editAllowed + var stored *attach.File + var refusal fileRefusal + err = lagoon.Transaction(r.Context(), db, func(ctx context.Context, tx *gorm.DB) error { + cur, why, err := h.lockEditableMaterial(tx, userID, m.ID, manager) + if err != nil || why != editAllowed { + outcome = why + return err + } + for { + part, err := mr.NextPart() + if err == io.EOF { + break + } + if err != nil { + return err + } + if part.FormName() != "file" || part.FileName() == "" { + part.Close() + continue + } + if stored != nil { + part.Close() + refusal = fileRefusal{Reason: "type", Text: "Send one image per request."} + return errRefused + } + f, why, err := storeMaterialUpload(ctx, tx, bucket, cur, part.FileName(), part, imageLimits) + part.Close() + if f != nil { + stored = f + } + if err != nil { + return err + } + if why.Reason != "" { + refusal = why + return errRefused + } + } + if stored == nil { + refusal = fileRefusal{Reason: "type", Text: "No image: send one multipart part named \"file\"."} + return errRefused + } + // The text job joins the transaction: it exists only when the file + // commits, and it syncs the material's document. + h.MaterialIndex.QueueExtract(ctx, tx, cur.ID) + return nil + }) + if err != nil && stored != nil { + // The blobs written are deleted again: the transaction did not commit. + _ = attach.DeleteKeys(context.WithoutCancel(r.Context()), bucket, attach.BlobKeys(*stored)) + } + var tooLarge *http.MaxBytesError + switch { + case errors.Is(err, errRefused): + writeFileRefusal(w, refusal) + case errors.As(err, &tooLarge): + wire.WriteJSON(w, http.StatusRequestEntityTooLarge, map[string]any{"error": true, "message": "Request body too large.", "reason": "size"}) + case err != nil: + writeOpaque500(w) + case outcome == editGone: + writeNotFound(w) + case outcome == editForbidden: + writeForbidden(w) + default: + wire.WriteJSON(w, http.StatusCreated, map[string]any{"data": fileJSON{ + ID: stored.ID, URL: stored.URL(), Name: stored.FileName, Size: stored.FileSize, Mime: stored.ContentType, + }}) + } +} diff --git a/controllers/api/material_edit_test.go b/controllers/api/material_edit_test.go index 142cb99..cdfe069 100644 --- a/controllers/api/material_edit_test.go +++ b/controllers/api/material_edit_test.go @@ -1,8 +1,12 @@ package api import ( + "errors" + "fmt" "strings" "testing" + + "git.golem15.com/golem15/summercms/modules/lagoon/attach" ) const fullEditBody = `{"revision":3,"title":"Notatka","type":"notatka","body":"Tekst","source":null,"url":null,"meeting_id":null,"tags":["lektura"]}` @@ -144,3 +148,37 @@ func TestNormalizeBody(t *testing.T) { } } } + +// TestFileRefusalReasons checks how attach.Store errors become refusals of +// the material file routes: size for ErrTooLarge, type for ErrFileType, +// ErrMIMEType and the image-mode ErrNotImage (no longer a 500), and no +// refusal for other errors. +func TestFileRefusalReasons(t *testing.T) { + cases := []struct { + err error + reason string + }{ + {attach.ErrTooLarge, "size"}, + {attach.ErrFileType, "type"}, + {attach.ErrMIMEType, "type"}, + {attach.ErrNotImage, "type"}, + {fmt.Errorf("store: %w", attach.ErrNotImage), "type"}, + } + for _, tc := range cases { + why, ok := fileRefusalFor(tc.err, "a.png", imageLimits) + if !ok || why.Reason != tc.reason || !strings.HasPrefix(why.Text, "a.png: ") { + t.Errorf("fileRefusalFor(%v) = %+v %v, want reason %s", tc.err, why, ok, tc.reason) + } + } + if why, _ := fileRefusalFor(attach.ErrTooLarge, "a.png", imageLimits); why.Text != "a.png: the file is larger than 50 MB." { + t.Errorf("size text = %q", why.Text) + } + for _, err := range []error{nil, errors.New("disk full")} { + if why, ok := fileRefusalFor(err, "a.png", imageLimits); ok { + t.Errorf("fileRefusalFor(%v) = %+v, want no refusal", err, why) + } + } + if !imageLimits.Image || imageLimits.MaxBytes != MaterialFileMaxBytes { + t.Errorf("imageLimits = %+v", imageLimits) + } +} diff --git a/material_edit_postgres_test.go b/material_edit_postgres_test.go index 0d81937..6765dd7 100644 --- a/material_edit_postgres_test.go +++ b/material_edit_postgres_test.go @@ -1,22 +1,39 @@ package bm_test import ( + "bytes" "encoding/json" + "image" + "image/png" + "mime/multipart" "net/http" + "net/http/httptest" + "strings" "testing" "git.golem15.com/golem15/summercms/modules/surf" "git.golem15.com/jakub/sm-bm-plugin/models" + "gocloud.dev/blob" + _ "gocloud.dev/blob/memblob" ) // TestMaterialEditAgainstDatabase checks the author's student routes // (D-07..D-11, D-16) against Postgres: who may edit (the author; 403 for // another manager or a non-manager; 404 for what the caller cannot see), // the revision check with the server version in the 409, the edit body's -// rules, and that an edit keeps the course and the author. +// rules, that an edit keeps the course and the author, the tag dictionary, +// unpublish to drafts and the editor image upload with its refusals. func TestMaterialEditAgainstDatabase(t *testing.T) { app, plugins, gdb := migrated(t) seedFixture(t, plugins, "testdata/seed.example.json") + bucket, err := blob.OpenBucket(t.Context(), "mem://") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { bucket.Close() }) + if err := app.Publish(bucket); err != nil { + t.Fatal(err) + } h, err := surf.Assemble(app, plugins) if err != nil { t.Fatalf("Assemble: %v", err) @@ -171,4 +188,91 @@ VALUES (?, ?, 'notatka', 'Stara treść', `+published+`, ?, ?) RETURNING id`, co if again := obj(author.want(http.StatusOK, "PUT", path(own), edit(2, nil))["data"]); again["revision"] != float64(2) { t.Errorf("unchanged save revision = %v, want 2", again["revision"]) } + + // The tag dictionary: managers only, by name. + var names []string + for _, tag := range list(author.ok("GET", base+"/tags")) { + names = append(names, tag["name"].(string)) + } + if strings.Join(names, ",") != "Cyber,Lektura,Pokój,Prawo,Wojna" { + t.Errorf("tags = %v, want the dictionary by name", names) + } + plain.want(http.StatusForbidden, "GET", base+"/tags", "") + + // Editor images: one PNG per request, public, no revision bump. + pngBytes := func() []byte { + t.Helper() + var buf bytes.Buffer + if err := png.Encode(&buf, image.NewRGBA(image.Rect(0, 0, 2, 2))); err != nil { + t.Fatal(err) + } + return buf.Bytes() + }() + upload := func(c apiClient, materialID uint, name string, content []byte) (int, map[string]any) { + t.Helper() + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + fw, err := mw.CreateFormFile("file", name) + if err != nil { + t.Fatal(err) + } + fw.Write(content) + mw.Close() + req := httptest.NewRequest("POST", path(materialID)+"/images", &buf) + req.Header.Set("Content-Type", mw.FormDataContentType()) + req.Header.Set("Authorization", "Bearer "+c.token) + rec := httptest.NewRecorder() + c.h.ServeHTTP(rec, req) + var out map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + t.Fatalf("upload %s = %d, not JSON: %s", name, rec.Code, rec.Body.String()) + } + return rec.Code, out + } + code, out := upload(author, own, "obraz.png", pngBytes) + if code != http.StatusCreated { + t.Fatalf("PNG upload = %d %v, want 201", code, out) + } + if img := obj(out["data"]); !strings.HasPrefix(img["url"].(string), "/storage/app/uploads/public/") || img["mime"] != "image/png" || img["name"] != "obraz.png" { + t.Errorf("uploaded image = %v", img) + } + if m := row(own); m.Revision != 2 { + t.Errorf("revision after the upload = %d, want 2", m.Revision) + } + if code, out := upload(author, own, "image.png", []byte("to nie jest obrazek")); code != http.StatusUnprocessableEntity || out["reason"] != "type" { + t.Errorf("text as image.png = %d %v, want 422 reason type", code, out) + } + if code, out := upload(author, own, "rysunek.svg", []byte(``)); code != http.StatusUnprocessableEntity || out["reason"] != "type" { + t.Errorf("SVG = %d %v, want 422 reason type", code, out) + } + if code, out := upload(other, own, "obraz.png", pngBytes); code != http.StatusForbidden { + t.Errorf("another manager's upload = %d %v, want 403", code, out) + } + if code, out := upload(author, othersPrivate, "obraz.png", pngBytes); code != http.StatusNotFound { + t.Errorf("upload to an invisible material = %d %v, want 404", code, out) + } + for i := 1; i < 20; i++ { + if code, out := upload(author, own, "obraz.png", pngBytes); code != http.StatusCreated { + t.Fatalf("upload %d = %d %v, want 201", i+1, code, out) + } + } + if code, out := upload(author, own, "obraz.png", pngBytes); code != http.StatusUnprocessableEntity || out["reason"] != "count" { + t.Errorf("21st file = %d %v, want 422 reason count", code, out) + } + if d := obj(author.ok("GET", path(own))["data"]); d["files_count"] != float64(20) { + t.Errorf("files_count = %v, want 20", d["files_count"]) + } + + // Unpublish: a draft, 404 even for the author afterwards, revision kept. + other.want(http.StatusForbidden, "POST", path(own)+"/unpublish", "") + author.want(http.StatusNotFound, "POST", path(othersPrivate)+"/unpublish", "") + if code, _ := author.send("POST", path(own)+"/unpublish", ""); code != http.StatusNoContent { + t.Fatalf("unpublish = %d, want 204", code) + } + author.want(http.StatusNotFound, "GET", path(own), "") + if m := row(own); m.PublishedAt != nil || m.Revision != 2 || m.AuthorUserID == nil || *m.AuthorUserID != authorID { + t.Errorf("row after unpublish = %+v, want a draft at revision 2 with its author", m) + } + author.want(http.StatusNotFound, "POST", path(own)+"/unpublish", "") + author.want(http.StatusNotFound, "PUT", path(own), edit(2, nil)) } diff --git a/mcp_test.go b/mcp_test.go index 6d9a3a0..84d66db 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -503,6 +503,9 @@ var authorRoutes = []string{ "GET /materials/{id}/access", "PUT /materials/{id}/access", "PUT /materials/{id}", + "GET /tags", + "POST /materials/{id}/unpublish", + "POST /materials/{id}/images", } // TestTokenMirrorRoutes is the invariant of the two student surfaces: every @@ -733,3 +736,34 @@ func TestManageBucketKey(t *testing.T) { t.Errorf("token twin key = %q, want jz.bm.token|u:7", got) } } + +// TestAuthorBucketKeys: the author's material routes are throttled per user, +// each under its own name, so an editor's saves and uploads never use up +// the unpublish budget. +func TestAuthorBucketKeys(t *testing.T) { + buckets := (&bm.Plugin{}).Buckets() + req := httptest.NewRequest(http.MethodPut, "/", nil) + req = req.WithContext(bouncer.WithUser(req.Context(), &bouncer.Principal{ID: 7})) + keys := map[string]bool{} + for name, max := range map[string]int{"jz.bm.material-edit": 30, "jz.bm.material-images": 30, "jz.bm.material-unpublish": 10} { + b, ok := buckets[name] + if !ok { + t.Errorf("%s is not declared", name) + continue + } + if b.Max != max { + t.Errorf("%s Max = %d, want %d", name, b.Max, max) + } + key := b.Key(req) + if key != name+"|u:7" { + t.Errorf("%s key = %q, want %s|u:7", name, key, name) + } + keys[key] = true + if got := b.Key(httptest.NewRequest(http.MethodPut, "/", nil)); !strings.HasPrefix(got, name+"|ip:") { + t.Errorf("%s bare key = %q, want the IP", name, got) + } + } + if len(keys) != 3 { + t.Errorf("keys = %v, want three distinct counters", keys) + } +} diff --git a/plugin_test.go b/plugin_test.go index 9675365..0a7b0cf 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -148,6 +148,9 @@ func TestRoutes(t *testing.T) { "GET /_bm/api/v1/materials/{id}/access", "PUT /_bm/api/v1/materials/{id}/access", "PUT /_bm/api/v1/materials/{id}", + "GET /_bm/api/v1/tags", + "POST /_bm/api/v1/materials/{id}/unpublish", + "POST /_bm/api/v1/materials/{id}/images", "GET /_bm/api/v1/token/dashboard", "GET /_bm/api/v1/token/courses", "GET /_bm/api/v1/token/courses/{id}", diff --git a/routes.go b/routes.go index 78671a3..8b76529 100644 --- a/routes.go +++ b/routes.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "net/http" + "net/netip" "strconv" "time" @@ -54,11 +55,34 @@ const ( // ticket in its path is the only credential (D-12). bucketUploadIP = "throttle:jz.bm.upload-ip" bucketUploadTicket = "throttle:jz.bm.upload-ticket" + // bucketMaterialEdit, bucketMaterialImages and bucketMaterialUnpublish + // throttle the author's routes per user (they run after jwt.auth), each + // with its own counter: the inline throttle:N,M form keys by user only, + // so every inline-throttled route would share one count. + bucketMaterialEdit = "throttle:jz.bm.material-edit" + bucketMaterialImages = "throttle:jz.bm.material-images" + bucketMaterialUnpublish = "throttle:jz.bm.material-unpublish" ) +// perUserBucket is a bucket counted per signed-in user under its own name, +// or per client IP for a request without one. +func perUserBucket(name string, max int, trusted []netip.Prefix) surf.Bucket { + return surf.Bucket{ + Max: max, + Decay: time.Minute, + Key: func(r *http.Request) string { + if u, ok := bouncer.User(r.Context()); ok && u.ID != 0 { + return name + "|u:" + strconv.FormatUint(uint64(u.ID), 10) + } + return name + "|ip:" + surf.ClientIP(r, trusted) + }, + } +} + // Buckets declares the rate limits of the public calendar feed, the // management API, the token twins of the student API, the signed material -// upload, the realtime routes and the BM chat routes. +// upload, the realtime routes, the BM chat routes and the author's material +// routes. func (p *Plugin) Buckets() map[string]surf.Bucket { trusted := surf.TrustedProxies(nil) if p.app != nil { @@ -126,6 +150,9 @@ func (p *Plugin) Buckets() map[string]surf.Bucket { return "jz.bm.token|ip:" + surf.ClientIP(r, trusted) }, }, + "jz.bm.material-edit": perUserBucket("jz.bm.material-edit", 30, trusted), + "jz.bm.material-images": perUserBucket("jz.bm.material-images", 30, trusted), + "jz.bm.material-unpublish": perUserBucket("jz.bm.material-unpublish", 10, trusted), "jz.bm.chat": { Max: 240, Decay: time.Minute, @@ -170,7 +197,12 @@ func (p *Plugin) Routes(r pact.Router) error { // The author's routes (D-07): edit, unpublish and editor images. Like // the access panel they have no token twin; tokens and MCP write // materials through the management API. - g.Put("/materials/{id}", h.EditMaterial, "throttle:30,1") + g.Put("/materials/{id}", h.EditMaterial, bucketMaterialEdit) + g.Where("id", "[0-9]+") + g.Get("/tags", h.Tags) + g.Post("/materials/{id}/unpublish", h.UnpublishMaterial, bucketMaterialUnpublish) + g.Where("id", "[0-9]+") + g.Post("/materials/{id}/images", h.UploadMaterialImage, bucketMaterialImages, "body.limit:"+strconv.Itoa(api.MaterialFilesBodyLimit)) g.Where("id", "[0-9]+") if p.app != nil && p.app.HasPlugin("golem15.chat") { g.Get("/chat", h.Chat, bucketChat)