diff --git a/classes/uploads.go b/classes/uploads.go new file mode 100644 index 0000000..608dbc7 --- /dev/null +++ b/classes/uploads.go @@ -0,0 +1,49 @@ +package classes + +import ( + "context" + "errors" + "time" + + "git.golem15.com/golem15/summercms/modules/cooler" +) + +// UploadTTL is how long an upload URL is valid. +const UploadTTL = 10 * time.Minute + +var ( + // ErrUploadTicket is every refused upload ticket. + ErrUploadTicket = errors.New("bm: invalid or expired upload ticket") + // ErrUploadKeyMissing means no key to sign upload tickets with. + ErrUploadKeyMissing = errors.New("bm: upload key is not configured") +) + +// UploadTicket is what a signed upload URL grants. +type UploadTicket struct { + Kind string + Target uint + User uint + Expires time.Time + Nonce string +} + +// UploadKey is the signing key from the app key. +func UploadKey(appKey string) []byte { return nil } + +// SignUpload signs t. +func SignUpload(key []byte, t UploadTicket) (string, error) { return "", nil } + +// VerifyUpload checks a ticket. +func VerifyUpload(key []byte, ticket string, now time.Time) (UploadTicket, error) { + return UploadTicket{}, nil +} + +// IssueUpload signs and registers a ticket. +func IssueUpload(ctx context.Context, cache cooler.Cache, key []byte, kind string, target, user uint, now time.Time) (string, time.Time, error) { + return "", time.Time{}, nil +} + +// RedeemUpload verifies and consumes a ticket. +func RedeemUpload(ctx context.Context, cache cooler.Cache, key []byte, ticket string, now time.Time) (UploadTicket, error) { + return UploadTicket{}, nil +} diff --git a/classes/uploads_test.go b/classes/uploads_test.go new file mode 100644 index 0000000..c207c11 --- /dev/null +++ b/classes/uploads_test.go @@ -0,0 +1,164 @@ +package classes_test + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/base64" + "errors" + "strconv" + "strings" + "testing" + "time" + + "git.golem15.com/golem15/summercms/modules/cooler" + "git.golem15.com/jakub/sm-bm-plugin/classes" +) + +// signWith builds a ticket for payload with an explicit label, as another +// plugin (or a forger who knows the format) would. +func signWith(key []byte, label, payload string) string { + mac := hmac.New(sha256.New, key) + mac.Write([]byte(label + payload)) + return base64.RawURLEncoding.EncodeToString([]byte(payload)) + "." + base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) +} + +func memCache(t *testing.T) cooler.Cache { + t.Helper() + c, err := cooler.NewMemoryCache("test:", 100) + if err != nil { + t.Fatal(err) + } + return c +} + +// TestUploadTicket (D-10, D-12) covers the signed one-time upload tickets: +// the HMAC under the jz.bm.upload.v1: label, the expiry and the single use. +func TestUploadTicket(t *testing.T) { + key := classes.UploadKey(" app-key ") + now := time.Date(2026, 10, 28, 14, 0, 0, 0, time.UTC) + nonce := strings.Repeat("ab", 16) + tk := classes.UploadTicket{Kind: "material", Target: 12, User: 7, Expires: now.Add(classes.UploadTTL), Nonce: nonce} + payload := "material|12|7|" + strconvInt(now.Add(classes.UploadTTL).Unix()) + "|" + nonce + + t.Run("key", func(t *testing.T) { + if string(key) != "app-key" { + t.Errorf("UploadKey trimmed = %q, want app-key", key) + } + if classes.UploadKey(" ") != nil || classes.UploadKey("") != nil { + t.Error("an empty app key is not nil") + } + if classes.UploadTTL != 10*time.Minute { + t.Errorf("UploadTTL = %v, want 10m", classes.UploadTTL) + } + }) + + t.Run("round trip", func(t *testing.T) { + signed, err := classes.SignUpload(key, tk) + if err != nil { + t.Fatal(err) + } + if signed != signWith(key, "jz.bm.upload.v1:", payload) { + t.Errorf("ticket %q is not base64url(payload).base64url(HMAC(label+payload))", signed) + } + got, err := classes.VerifyUpload(key, signed, now) + if err != nil { + t.Fatalf("VerifyUpload: %v", err) + } + if got.Kind != "material" || got.Target != 12 || got.User != 7 || !got.Expires.Equal(tk.Expires) || got.Nonce != nonce { + t.Errorf("verified %+v, want %+v", got, tk) + } + }) + + t.Run("refused", func(t *testing.T) { + signed := signWith(key, "jz.bm.upload.v1:", payload) + head, mac, _ := strings.Cut(signed, ".") + raw, _ := base64.RawURLEncoding.DecodeString(head) + raw[len("material|1")] = '3' + macRaw, _ := base64.RawURLEncoding.DecodeString(mac) + macRaw[0] ^= 1 + cases := map[string]string{ + "empty": "", + "no dot": head, + "two dots": signed + ".x", + "bad base64 payload": "!!!." + mac, + "bad base64 mac": head + ".!!!", + "changed payload": base64.RawURLEncoding.EncodeToString(raw) + "." + mac, + "changed mac": head + "." + base64.RawURLEncoding.EncodeToString(macRaw), + "other key": signWith([]byte("other-key"), "jz.bm.upload.v1:", payload), + "other label": signWith(key, "golem15.presentations.upload.v1:", payload), + "no label": signWith(key, "", payload), + "four fields": signWith(key, "jz.bm.upload.v1:", "material|12|7|"+strconvInt(tk.Expires.Unix())), + "six fields": signWith(key, "jz.bm.upload.v1:", payload+"|x"), + "zero target": signWith(key, "jz.bm.upload.v1:", "material|0|7|"+strconvInt(tk.Expires.Unix())+"|"+nonce), + "short nonce": signWith(key, "jz.bm.upload.v1:", "material|12|7|"+strconvInt(tk.Expires.Unix())+"|abcd"), + "far expiry": signWith(key, "jz.bm.upload.v1:", "material|12|7|"+strconvInt(now.Add(24*time.Hour).Unix())+"|"+nonce), + } + for name, ticket := range cases { + if _, err := classes.VerifyUpload(key, ticket, now); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("%s: err %v, want ErrUploadTicket", name, err) + } + } + if _, err := classes.VerifyUpload(key, signed, tk.Expires); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("expired: err %v, want ErrUploadTicket", err) + } + if _, err := classes.VerifyUpload(nil, signed, now); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("no key: err %v, want ErrUploadTicket", err) + } + }) + + t.Run("issue and redeem once", func(t *testing.T) { + ctx := context.Background() + cache := memCache(t) + if _, _, err := classes.IssueUpload(ctx, cache, nil, "material", 12, 7, now); !errors.Is(err, classes.ErrUploadKeyMissing) { + t.Errorf("empty key: err %v, want ErrUploadKeyMissing", err) + } + ticket, expires, err := classes.IssueUpload(ctx, cache, key, "material", 12, 7, now) + if err != nil { + t.Fatalf("IssueUpload: %v", err) + } + if !expires.Equal(now.Add(classes.UploadTTL)) { + t.Errorf("expires %v, want %v", expires, now.Add(classes.UploadTTL)) + } + if strings.Contains(ticket, string(key)) { + t.Error("the ticket carries the key") + } + got, err := classes.RedeemUpload(ctx, cache, key, ticket, now.Add(time.Minute)) + if err != nil { + t.Fatalf("first RedeemUpload: %v", err) + } + if got.Kind != "material" || got.Target != 12 || got.User != 7 || len(got.Nonce) != 32 { + t.Errorf("redeemed %+v", got) + } + if _, err := classes.RedeemUpload(ctx, cache, key, ticket, now.Add(time.Minute)); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("second RedeemUpload: err %v, want ErrUploadTicket", err) + } + other, _, err := classes.IssueUpload(ctx, cache, key, "material", 12, 7, now) + if err != nil { + t.Fatal(err) + } + if other == ticket { + t.Error("two issued tickets are equal (no fresh nonce)") + } + }) + + t.Run("restart fails closed", func(t *testing.T) { + ctx := context.Background() + ticket, _, err := classes.IssueUpload(ctx, memCache(t), key, "material", 12, 7, now) + if err != nil { + t.Fatal(err) + } + if _, err := classes.RedeemUpload(ctx, memCache(t), key, ticket, now); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("ticket without its cache entry: err %v, want ErrUploadTicket", err) + } + signed, _ := classes.SignUpload(key, tk) + if _, err := classes.RedeemUpload(ctx, memCache(t), key, signed, now); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("signed but never issued: err %v, want ErrUploadTicket", err) + } + if _, err := classes.RedeemUpload(ctx, nil, key, signed, now); !errors.Is(err, classes.ErrUploadTicket) { + t.Errorf("no cache: err %v, want ErrUploadTicket", err) + } + }) +} + +func strconvInt(n int64) string { return strconv.FormatInt(n, 10) } diff --git a/controllers/api/api.go b/controllers/api/api.go index be14103..48351e6 100644 --- a/controllers/api/api.go +++ b/controllers/api/api.go @@ -14,6 +14,7 @@ import ( chatclasses "git.golem15.com/golem15/sm-chat-plugin/classes" chanclasses "git.golem15.com/golem15/sm-chatchannels-plugin/classes" "git.golem15.com/golem15/summercms/modules/bouncer" + "git.golem15.com/golem15/summercms/modules/cooler" "git.golem15.com/golem15/summercms/modules/phrasebook" "git.golem15.com/golem15/summercms/modules/wire" "gocloud.dev/blob" @@ -66,6 +67,12 @@ type Handlers struct { // ChatBots is whether golem15.chatbots is active, so its is_bot flags // can be read. ChatBots bool + // UploadKey returns the key the signed material upload URLs are signed + // with (app.key), or nil when none is set: then no URL is issued. + UploadKey func() []byte + // Cache returns the application cache that holds the issued upload + // tickets until they are used, or nil while none is available. + Cache func() cooler.Cache } func (h *Handlers) now() time.Time { diff --git a/controllers/api/uploads.go b/controllers/api/uploads.go index 1453f8e..3410022 100644 --- a/controllers/api/uploads.go +++ b/controllers/api/uploads.go @@ -11,6 +11,7 @@ import ( "git.golem15.com/golem15/summercms/modules/lagoon" "git.golem15.com/golem15/summercms/modules/lagoon/attach" + "git.golem15.com/golem15/summercms/modules/wire" "gorm.io/gorm" ) @@ -140,3 +141,16 @@ func (h *Handlers) AttachMaterialInline(w http.ResponseWriter, r *http.Request) } h.writeMaterialDetailWith(w, r, db, m.ID, http.StatusCreated, map[string]any{"warning": MaterialFileWarning}) } + +// UploadPath is the path of the signed upload route. +const UploadPath = "/_bm/api/v1/uploads/" + +// CreateMaterialUpload is not implemented yet. +func (h *Handlers) CreateMaterialUpload(w http.ResponseWriter, r *http.Request) { + wire.WriteJSON(w, http.StatusNotImplemented, map[string]any{"error": true}) +} + +// SignedMaterialUpload is not implemented yet. +func (h *Handlers) SignedMaterialUpload(w http.ResponseWriter, r *http.Request) { + wire.WriteJSON(w, http.StatusNotImplemented, map[string]any{"error": true}) +} diff --git a/controllers/api/uploads_test.go b/controllers/api/uploads_test.go index 6c24276..c441af7 100644 --- a/controllers/api/uploads_test.go +++ b/controllers/api/uploads_test.go @@ -1,12 +1,16 @@ package api_test import ( + "context" "encoding/base64" "net/http" "strings" "testing" + "time" "git.golem15.com/golem15/summercms/modules/bouncer" + "git.golem15.com/golem15/summercms/modules/cooler" + "git.golem15.com/jakub/sm-bm-plugin/classes" "git.golem15.com/jakub/sm-bm-plugin/controllers/api" "gorm.io/gorm" ) @@ -88,3 +92,78 @@ func TestAttachMaterialInlineNoDatabase(t *testing.T) { t.Error("the database was looked up for an invalid body") } } + +// TestSignedMaterialUploadRefusesBadTickets (D-12): a malformed, forged, +// never issued, expired, wrong-kind or used ticket gets the same 403 before +// the database is looked up. +func TestSignedMaterialUploadRefusesBadTickets(t *testing.T) { + key := []byte("app-key") + cache, err := cooler.NewMemoryCache("test:", 100) + if err != nil { + t.Fatal(err) + } + dbLooked := false + h := &api.Handlers{ + DB: func() *gorm.DB { dbLooked = true; return nil }, + UploadKey: func() []byte { return key }, + Cache: func() cooler.Cache { return cache }, + } + now := time.Now() + forged, _, err := classes.IssueUpload(context.Background(), cache, []byte("other-key"), "material", 1, 7, now) + if err != nil { + t.Fatal(err) + } + unissued, err := classes.SignUpload(key, classes.UploadTicket{Kind: "material", Target: 1, User: 7, Expires: now.Add(time.Minute), Nonce: strings.Repeat("0f", 16)}) + if err != nil { + t.Fatal(err) + } + expired, _, err := classes.IssueUpload(context.Background(), cache, key, "material", 1, 7, now.Add(-classes.UploadTTL-time.Second)) + if err != nil { + t.Fatal(err) + } + deck, _, err := classes.IssueUpload(context.Background(), cache, key, "deck", 1, 7, now) + if err != nil { + t.Fatal(err) + } + refused := map[string]string{ + "empty": "", + "garbage": "not-a-ticket", + "two parts": "abc.def", + "forged": forged, + "unissued": unissued, + "expired": expired, + "wrong kind": deck, + } + for name, ticket := range refused { + code, body := serve(t, h.SignedMaterialUpload, http.MethodPost, "/", nil, map[string]string{"ticket": ticket}) + if code != http.StatusForbidden || body["error"] != true || body["message"] != "Invalid or expired upload URL." { + t.Errorf("%s: %d %v, want 403 Invalid or expired upload URL.", name, code, body) + } + } + if dbLooked { + t.Error("the database was looked up for a refused ticket") + } + + valid, _, err := classes.IssueUpload(context.Background(), cache, key, "material", 1, 7, now) + if err != nil { + t.Fatal(err) + } + if code, body := serve(t, h.SignedMaterialUpload, http.MethodPost, "/", nil, map[string]string{"ticket": valid}); code != http.StatusServiceUnavailable { + t.Errorf("valid ticket without a database = %d %v, want 503", code, body) + } + if code, body := serve(t, h.SignedMaterialUpload, http.MethodPost, "/", nil, map[string]string{"ticket": valid}); code != http.StatusForbidden { + t.Errorf("used ticket = %d %v, want 403", code, body) + } +} + +// TestCreateMaterialUploadNeedsCaller: no signed-in user is a 401; a caller +// without a database is a 503. +func TestCreateMaterialUploadNeedsCaller(t *testing.T) { + h := noDB() + if code, body := serve(t, h.CreateMaterialUpload, http.MethodPost, "/", nil, map[string]string{"id": "1"}); code != http.StatusUnauthorized || body["error"] != true { + t.Errorf("without a caller = %d %v, want 401", code, body) + } + if code, body := serve(t, h.CreateMaterialUpload, http.MethodPost, "/", &bouncer.Principal{ID: 7}, map[string]string{"id": "1"}); code != http.StatusServiceUnavailable { + t.Errorf("without a database = %d %v, want 503", code, body) + } +} diff --git a/mcp_test.go b/mcp_test.go index 86e2f2f..96e7e16 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -3,6 +3,8 @@ package bm_test import ( "bytes" "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "net/http" "net/http/httptest" @@ -57,6 +59,7 @@ var pinnedTools = []string{ "bm_manage_create_class_session", "bm_manage_create_course", "bm_manage_create_deadline", + "bm_manage_create_material_upload", "bm_manage_create_meeting", "bm_manage_create_reading", "bm_manage_delete_class_session", @@ -374,12 +377,13 @@ func testManageTools(t *testing.T, tools []pact.MCPTool) { // manageToolCount is the number of management tools: 28 record and // material tools plus the material file tools. -const manageToolCount = 30 +const manageToolCount = 31 // fileBodyKeys are the body keys of each material file tool, exactly. var fileBodyKeys = map[string]map[string]bool{ - "manage_attach_material_file": {"name": false, "mime": true, "content_base64": false}, - "manage_detach_material_file": {}, + "manage_attach_material_file": {"name": false, "mime": true, "content_base64": false}, + "manage_detach_material_file": {}, + "manage_create_material_upload": {}, } // testFileTool (D-10, D-11, D-17) checks one material file tool: its body @@ -414,6 +418,13 @@ func testFileTool(t *testing.T, tool pact.MCPTool, fields map[string]bool) { if !idPath { t.Errorf("%s: no id path field", tool.Name) } + if tool.Name == "manage_create_material_upload" { + for _, s := range []string{"10 minutes", "curl", "50 MB", "once"} { + if !strings.Contains(tool.Description, s) { + t.Errorf("%s: description does not mention %q", tool.Name, s) + } + } + } if tool.Name == "manage_detach_material_file" { if tool.Method != http.MethodDelete || (tool.Destructive != nil && !*tool.Destructive) { t.Errorf("%s is not destructive (method %s)", tool.Name, tool.Method) @@ -628,6 +639,23 @@ func TestManageBucketKey(t *testing.T) { if got := manage.Key(httptest.NewRequest(http.MethodGet, "/", nil)); !strings.HasPrefix(got, "jz.bm.manage.ip|") { t.Errorf("bare key = %q, want the IP", got) } + ip, ok := buckets["jz.bm.upload-ip"] + if !ok || ip.Max != 10 { + t.Errorf("jz.bm.upload-ip declared %v with Max %d, want 10", ok, ip.Max) + } else if got := ip.Key(httptest.NewRequest(http.MethodPost, "/", nil)); !strings.HasPrefix(got, "jz.bm.upload.ip|") { + t.Errorf("upload IP key = %q", got) + } + ticket, ok := buckets["jz.bm.upload-ticket"] + if !ok || ticket.Max != 3 { + t.Errorf("jz.bm.upload-ticket declared %v with Max %d, want 3", ok, ticket.Max) + } else { + r := httptest.NewRequest(http.MethodPost, "/", nil) + r.SetPathValue("ticket", "secret-ticket.mac") + sum := sha256.Sum256([]byte("secret-ticket.mac")) + if got, want := ticket.Key(r), "jz.bm.upload.ticket|"+hex.EncodeToString(sum[:])[:32]; got != want { + t.Errorf("upload ticket key = %q, want %q", got, want) + } + } token, ok := buckets["jz.bm.token"] if !ok { t.Fatal("jz.bm.token is not declared") diff --git a/plugin_test.go b/plugin_test.go index 285fc1b..4c665d3 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "slices" + "strconv" "strings" "testing" @@ -17,6 +18,7 @@ import ( "git.golem15.com/golem15/summercms/modules/party" "git.golem15.com/golem15/summercms/modules/surf" bm "git.golem15.com/jakub/sm-bm-plugin" + "git.golem15.com/jakub/sm-bm-plugin/controllers/api" ) const ( @@ -140,6 +142,7 @@ func TestRoutes(t *testing.T) { "DELETE /api/v1/bm/materials/{id}", "POST /api/v1/bm/materials/{id}/files", "POST /api/v1/bm/materials/{id}/files/inline", + "POST /api/v1/bm/materials/{id}/files/upload-url", "DELETE /api/v1/bm/materials/{id}/files/{file}", "GET /api/v1/bm/courses", "GET /api/v1/bm/courses/{id}", @@ -167,7 +170,7 @@ func TestRoutes(t *testing.T) { "POST /api/v1/bm/seed", } var got []string - sawFeed := false + sawFeed, sawUploads := false, false for _, rt := range router.Routes() { if rt.PluginID != "jz.bm" { continue @@ -189,6 +192,16 @@ func TestRoutes(t *testing.T) { sawFeed = true continue } + if rt.Method+" "+rt.Pattern == "POST /_bm/api/v1/uploads/{ticket}" { + // The signed material upload: no bearer (the ticket is the + // credential), throttled per IP and per ticket, body capped. + want := []string{"throttle:jz.bm.upload-ip", "throttle:jz.bm.upload-ticket", "body.limit:" + strconv.Itoa(api.MaterialFilesBodyLimit)} + if !slices.Equal(rt.Middleware, want) { + t.Errorf("the signed upload has middleware %v, want %v", rt.Middleware, want) + } + sawUploads = true + continue + } got = append(got, rt.Method+" "+rt.Pattern) if strings.HasPrefix(rt.Pattern, bm.TokenPrefix+"/") { // The token twins of the student API: the user API token guard @@ -210,6 +223,12 @@ func TestRoutes(t *testing.T) { if !sawFeed { t.Error("GET /_bm/calendar/{token} is not registered") } + if !sawUploads { + t.Error("POST /_bm/api/v1/uploads/{ticket} is not registered") + } + if api.UploadPath != bm.APIPrefix+"/uploads/" { + t.Errorf("api.UploadPath = %q, want %q", api.UploadPath, bm.APIPrefix+"/uploads/") + } if !slices.Equal(got, want) { t.Errorf("routes =\n%v\nwant\n%v", got, want) }