diff --git a/server/AGENTS.md b/server/AGENTS.md index 7a43c3f5..1744c948 100644 --- a/server/AGENTS.md +++ b/server/AGENTS.md @@ -19,9 +19,9 @@ auth-realtime: [auth-realtime/AGENTS.md](auth-realtime/AGENTS.md). (`OXYNOTE_AUTH_REALTIME_BACKEND_URL`). - `datagen/`: separate Go module synthesising demo Postgres/MariaDB content. -Go builds go through goreleaser: `make build` in `server/core` and `datagen` -runs `goreleaser release --snapshot --clean`, producing `bin/` and the -`ghcr.io/oxynote/{core,datagen}:dev` images the dev compose stack consumes. +Go builds go through goreleaser (`make build` in `server/core` and +`datagen`), producing `bin/` and the `ghcr.io/oxynote/{core,datagen}:dev` +images the dev compose stack consumes. datagen shares core's test/lint workflow with its own copy of the golangci-lint profile; its `pgdemo`/`mariademo` suites start throwaway containers. @@ -55,11 +55,9 @@ GitHub and Slack callback URLs point at `:8080/core/api/apps/...`. request is validated against `GET /api/internal/mcp/session` (JWKS + consent row, so revocation 401s immediately) and scoped by `documents:read`/`documents:write`/`data-sources:read`. -- `/api/ws` topics (`wetsocks/wsserver`): `change@document-tree`, - `change@tag-tree`, - `change@documents.{documentId}.comments|metadata|reviewers|maintainers|tags|hooks`, - `post@slack.messages`, `creation@notifications`, `ping@version`. Binders - are `Handler.BindXxx` under `internal/server/internal/...`. +- `/api/ws` topics (`wetsocks/wsserver`) are registered in `router.go` as + `@`; binders are `Handler.BindXxx` under + `internal/server/internal/...`. The README lists the public routes; update it when changing handlers. diff --git a/server/core/AGENTS.md b/server/core/AGENTS.md index d39709b4..6d1a8f4c 100644 --- a/server/core/AGENTS.md +++ b/server/core/AGENTS.md @@ -1,9 +1,9 @@ # AGENTS.md — Go standards Mandatory for all Go code in `server/core` and `datagen`; this file wins -over surrounding code for new and modified code. Shared helpers live in -`pkg/`; supervisors come from `github.com/jellydator/xync`. Where this file -names an exemplar file, copy its shape. +over surrounding code for new and modified code. Supervisors come from +`github.com/jellydator/xync`. Where this file names an exemplar file, copy +its shape. ```sh make build # goreleaser snapshot -> bin/ + ghcr.io/oxynote/core:dev @@ -30,17 +30,17 @@ make lint # golangci-lint run --fix; check-lint = verify only **Comments.** Every comment ends with a period (godot). Doc comments on everything, unexported included: `// .`; interface methods use `should`, implementations indicative. `// NOCOV: -.` is the first line of a branch that genuinely cannot be -reproduced; it is a reviewer covenant, not a tool directive, and never -excuses a testable path. -`// NOTE: .` for design rationale. `//nolint: // `, reason mandatory (deferred Rollback/Close: `errcheck // error -provides no meaningful info`). Comments never narrate the next line, justify -a diff, or point at design docs or agent-instruction files. +.` heads a branch that genuinely cannot be reproduced; it +never excuses a testable path. `// NOTE: .` for design rationale. +`//nolint: // `, reason mandatory (deferred +Rollback/Close: `errcheck // error provides no meaningful info`). Comments +never narrate the next line, justify a diff, or point at design docs or +agent-instruction files. **Globals.** Every unexported package-level var/const is `_`-prefixed and doc-commented (`var` for values tests shorten). Exported globals: `Err*` -sentinels only. `init()` is banned except for build-info parsing. +sentinels and consts another package reads. `init()` is banned except for +build-info parsing. Compile-time interface assertions (`var _ Iface = &Impl{}`) only when a human asks: interfaces are consumer-declared, so the assertion imports the consumer and risks a cycle. @@ -48,9 +48,8 @@ consumer and risks a cycle. **Naming.** Receivers are 1–3 letters from the type's initials, consistent per type. Shorts: `cfg`, `opts`, `inp`, `supv`, `res`, `evt`; secondary errors `cerr`/`rerr`/`perr`. Acronyms stay uppercase (`httpClient`, `ID`), -except `Ws` in compounds. `any`, never `interface{}`. Unused parameters are -`_`. An exported identifier used only inside its package becomes unexported; -an unused one is deleted. +except `Ws` in compounds. Unused parameters are `_`. An exported identifier +used only inside its package becomes unexported; an unused one is deleted. **Shape (wsl).** Guard clauses, no `else` after a returning `if`. `if err := f(); err != nil {` when `err` is not reused. Blank line before every `return` @@ -58,9 +57,8 @@ f(); err != nil {` when `err` is not reused. Blank line before every `return` `Lock`/`defer Unlock` pair. Multi-argument calls one parameter per line. Named returns only so a `defer` can observe or replace the result; naked returns are banned. `defer cancel()`/`defer mu.Unlock()`/`defer -resp.Body.Close()` immediately after the acquiring call. Imports: one stdlib -block, then one block of everything else; alias collisions as lowercase -concatenations (`hookMan`), mocks as `Mock`. +resp.Body.Close()` immediately after the acquiring call. Import alias +collisions are lowercase concatenations (`hookMan`), mocks `Mock`. ## API design @@ -81,7 +79,7 @@ resource-owning type has `Close() error`. configurable package, next to its constructor, every field doc-commented with units and zero-value behaviour. Options nest to mirror the dependency tree and `validate()` cascades (exported `Validate()` when a parent cascades -into it). Config is environment only, read once in `main`. +into it). **Structs.** Service structs: unexported fields ordered `log *slog.Logger`, dependencies, each mutex directly above the fields it guards (blank lines @@ -189,8 +187,7 @@ slice; always a deterministic tiebreaker after the user sort. IDs are generated in the domain layer, no `RETURNING`. Soft deletes set `deleted_at`; hard deletes happen in `Cleanup*` sweeps. A sqlhooks error hook maps constraint violations → user errors; an empty result is a bare -`sql.ErrNoRows`, so callers use `errutil.IsNotFound`. Migrations are -`migrations/NNN_snake_case.sql`, embedded with `//go:embed`. +`sql.ErrNoRows`, so callers use `errutil.IsNotFound`. ## HTTP & WebSocket @@ -205,6 +202,9 @@ hook maps constraint violations → user errors; an empty result is a bare `FetchX`/`FetchXs` (not `ListX`), action verbs (`DuplicateDocument`), `CreateX`/`UpdateX`/`DeleteX`; `Handle` only where the route has no entity (`HandleChat`). +- Handler packages hold no domain rules: a parser, allow-list, cap or key + layout lives in the domain package it describes; a handler decodes, + transacts, calls the domain and responds. - A session handler's first statement is `auth.RequireSession(h.log, w, r)`. Decoding only through `DecodeJSON`, `DecodeForm`, `ParseQuery`, `ExtractTargetID`; responding only through `Respond` and `RespondError`, @@ -244,8 +244,8 @@ for known third-party leaks. Serialization types get `Test_X_UnmarshalJSON`, before the result assertions. - Case names are sentence case without punctuation (`"Successful - creation"`, `"Context cancelled"`); error paths name the collaborator - verbatim (`"Error returned by Tx.Commit"`). No `name` field. + creation"`); error paths name the collaborator verbatim (`"Error returned + by Tx.Commit"`). No `name` field. - Case fields are PascalCase, inputs and collaborators first (`DB`, `Tx`, `Inp`, `JSON`, `Context`), expectations (`Result`, `Err`, `Checks`, `RespCode`, `RespJSON`) last. @@ -270,11 +270,11 @@ nondeterministic types; `assert.JSONEq` for JSON; decimals as strings. **Mocks.** `//go:generate ../../scripts/codegen/mock -t internal DB db` (in-package `DBMock`), `-t external` (importable `_mock/` package, `mock.DB`), `-t both`. All use `-stub`, so `&DBMock{}` is a valid don't-care -collaborator that still records calls. `_mock/` holds only generated code; -third-party mocks go in `internal/_mock`. Configure by struct literal with -only the needed funcs; repeated shapes are local `stub*` closures -parameterized by the errors to return. Assert calls only through recorders -(`db.CreateItemCalls()`); test `On*` APIs by invoking the captured callback. +collaborator that still records calls. Third-party mocks go in +`internal/_mock`. Configure by struct literal with only the needed funcs; +repeated shapes are local `stub*` closures parameterized by the errors to +return. Assert calls only through recorders (`db.CreateItemCalls()`); test +`On*` APIs by invoking the captured callback. **check/checks combinator.** For functions touching several mocks, a `Checks []check` field replaces per-field expectations: `type check @@ -291,15 +291,14 @@ counts. Exemplar: `internal/assistant/tools/document_test.go`. **Database-layer tests** (`internal/db/db_test.go`): `TestMain` starts one gnomock Postgres per package run; `prepTempDB(t)` gives each subtest its own -migrated database, which is what makes `t.Parallel()` safe. Tables are maps -of case constructors `func(*testing.T, *DB) tcase`. Round times to -microseconds before insert; verify by re-querying through the production -select builders. +migrated database. Tables are maps of case constructors `func(*testing.T, +*DB) tcase`. Round times to microseconds before insert; verify by +re-querying through the production select builders. **HTTP handlers.** Invoke methods directly, no router: `httptest.NewRequest` against `http://test.com/`, chi params via -`testutil.AddChiCtx` (an `OmitID bool` case field drives the missing-param -path). Assert `rec.Code` and `assert.JSONEq` against verbatim envelopes; +`testutil.AddChiCtx`. Assert `rec.Code` and `assert.JSONEq` against verbatim +envelopes; 204s assert an empty body. Auth middleware is tested through real components over mocked stores. WS binder tests stub `OnFirstSubFunc`/`OnLastUnsubFunc` to invoke immediately, fire the captured callback, assert @@ -318,6 +317,4 @@ Logging is asserted, not mocked: `slog.NewTextHandler` over a `bytes.Buffer` logs don't matter. Use `context.Background()`, not `t.Context()`. **Hygiene.** Golden files in `testdata/`. Package-level fixtures are -`_`-prefixed. `t.Helper()` in named helpers. White-box construction is -normal. Giant test functions are fine; prefer one exhaustive function over -splitting a target's cases. +`_`-prefixed. `t.Helper()` in named helpers. diff --git a/server/core/internal/assistant/tools/search.go b/server/core/internal/assistant/tools/search.go index 7d666cc0..40381077 100644 --- a/server/core/internal/assistant/tools/search.go +++ b/server/core/internal/assistant/tools/search.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" ) @@ -35,6 +36,10 @@ func (a searchDocumentsArgs) Validate() error { return errRequired("query") } + if err := search.ValidateQuery(a.Query); err != nil { + return fmt.Errorf("query must be at most %d characters; search for the key terms instead", search.MaxQueryLength) + } + return nil } diff --git a/server/core/internal/assistant/tools/search_test.go b/server/core/internal/assistant/tools/search_test.go index 24f7f3f8..e0df1bd7 100644 --- a/server/core/internal/assistant/tools/search_test.go +++ b/server/core/internal/assistant/tools/search_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "log/slog" + "strings" "testing" "github.com/oxynote/oxynote/server/core/internal/document" @@ -98,6 +99,12 @@ func Test_searchDocuments_Execute(t *testing.T) { Args: `{}`, Err: assert.AnError, }, + "Query over the cap": { + Searcher: hitSearcher(nil), + DB: &DBMock{}, + Args: `{"query":"` + strings.Repeat("a", search.MaxQueryLength+1) + `"}`, + Err: assert.AnError, + }, "Error returned by search.SearchDocumentBlocks": { Searcher: &SearcherMock{ SearchDocumentBlocksFunc: func(context.Context, string, string, int) ([]search.Block, error) { diff --git a/server/core/internal/assistant/tools/tool.go b/server/core/internal/assistant/tools/tool.go index de6f6c85..3ee20ae6 100644 --- a/server/core/internal/assistant/tools/tool.go +++ b/server/core/internal/assistant/tools/tool.go @@ -60,6 +60,27 @@ func (info Info) Schema() map[string]any { return out } +// Available tool access constants. +const ( + // AccessNone is held by no surface outside this process: the tool + // addresses conversation state only the assistant has. + AccessNone Access = "none" + + // AccessRead reads the organization's documents. + AccessRead Access = "read" + + // AccessWrite mutates the organization's documents. + AccessWrite Access = "write" + + // AccessDataSource reads one of the organization's outbound + // data-source connections, which is not the same permission as + // reading its documents. + AccessDataSource Access = "data-source" +) + +// Access is the permission a surface must hold to offer a tool. +type Access string + // Traits are the facts about a tool that decide how the assistant // treats it. A tool states them in one place in its own file rather // than being interrogated for them from several. @@ -102,6 +123,35 @@ type Traits struct { Internal bool } +// DestroysContent reports whether a write loses work the caller did not +// name: an outright removal, or an overwrite that takes nested blocks, +// uids, comments and hooks with it. Both are what a client deciding +// whether to confirm first wants to know. +func (t Traits) DestroysContent() bool { + return t.Destructive || t.Overwrites +} + +// OpenWorld reports whether the tool reaches outside the organization's +// own documents: a data-source tool reaches whatever the connection +// points at. +func (t Traits) OpenWorld() bool { + return t.DataSource +} + +// Access reports the permission a surface must hold to offer the tool. +func (t Traits) Access() Access { + switch { + case t.Internal: + return AccessNone + case t.DataSource: + return AccessDataSource + case t.Write: + return AccessWrite + default: + return AccessRead + } +} + // Tool is a tool the model can call. What a tool is — whether it // mutates, whether it survives an approve-all, whether it belongs // outside this process — it states in Traits rather than in the set of diff --git a/server/core/internal/assistant/tools/tool_test.go b/server/core/internal/assistant/tools/tool_test.go index 463a7da2..7ea60217 100644 --- a/server/core/internal/assistant/tools/tool_test.go +++ b/server/core/internal/assistant/tools/tool_test.go @@ -25,6 +25,55 @@ func Test_plainTraits_Traits(t *testing.T) { assert.Equal(t, Traits{}, plainTraits{}.Traits()) } +func Test_Traits_DestroysContent(t *testing.T) { + cc := map[string]struct { + Traits Traits + Result bool + }{ + "Read": {}, + "Plain write": {Traits: Traits{Write: true}}, + "Destructive": {Traits: Traits{Write: true, Destructive: true}, Result: true}, + "Overwriting": {Traits: Traits{Write: true, Overwrites: true}, Result: true}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, c.Traits.DestroysContent()) + }) + } +} + +func Test_Traits_OpenWorld(t *testing.T) { + t.Parallel() + + assert.False(t, Traits{}.OpenWorld()) + assert.True(t, Traits{DataSource: true}.OpenWorld()) +} + +func Test_Traits_Access(t *testing.T) { + cc := map[string]struct { + Traits Traits + Result Access + }{ + "Read": {Result: AccessRead}, + "Write": {Traits: Traits{Write: true}, Result: AccessWrite}, + "Data source": {Traits: Traits{DataSource: true}, Result: AccessDataSource}, + "Internal": {Traits: Traits{Internal: true}, Result: AccessNone}, + "Internal write": {Traits: Traits{Write: true, Internal: true}, Result: AccessNone}, + "Data source over write": {Traits: Traits{Write: true, DataSource: true}, Result: AccessDataSource}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, c.Traits.Access()) + }) + } +} + func Test_plainTitle_Title(t *testing.T) { t.Parallel() diff --git a/server/core/internal/datasource/datasource.go b/server/core/internal/datasource/datasource.go index 7b8f95f2..85531784 100644 --- a/server/core/internal/datasource/datasource.go +++ b/server/core/internal/datasource/datasource.go @@ -6,6 +6,7 @@ import ( "time" "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/datasource/demo" "github.com/oxynote/oxynote/server/core/internal/datasource/processor" "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" @@ -72,6 +73,17 @@ func NewDataSource(ci CreateInput, organizationID string) *DataSource { } } +// NewDemoDataSource creates the demo Prometheus data source a fresh +// organization starts with. +func NewDemoDataSource(organizationID string) *DataSource { + return NewDataSource(CreateInput{ + Type: TypePrometheus, + Name: "Demo", + URL: demo.URL, + Credentials: processor.NewCredentials([]byte(`{}`)), + }, organizationID) +} + // Info returns a subset of data source fields for external use. func (ds *DataSource) Info() Info { return Info{ diff --git a/server/core/internal/datasource/datasource_test.go b/server/core/internal/datasource/datasource_test.go index 92859ab1..f315f0b9 100644 --- a/server/core/internal/datasource/datasource_test.go +++ b/server/core/internal/datasource/datasource_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/datasource/demo" "github.com/oxynote/oxynote/server/core/internal/datasource/processor" "github.com/oxynote/oxynote/server/core/pkg/cryptoutil" "github.com/oxynote/oxynote/server/core/pkg/testutil" @@ -56,6 +57,20 @@ func Test_NewDataSource(t *testing.T) { assert.False(t, ds.UpdatedAt.Valid) } +func Test_NewDemoDataSource(t *testing.T) { + t.Parallel() + + ds := NewDemoDataSource("org-1") + + require.NotNil(t, ds) + assert.False(t, ds.ID.IsNil()) + assert.Equal(t, "org-1", ds.OrganizationID) + assert.Equal(t, "Demo", ds.Name) + assert.Equal(t, TypePrometheus, ds.Type) + assert.Equal(t, demo.URL, ds.URL) + assert.Equal(t, processor.NewCredentials([]byte(`{}`)), ds.Credentials) +} + func Test_DataSource_Info(t *testing.T) { t.Parallel() diff --git a/server/core/internal/datasource/processor/query.go b/server/core/internal/datasource/processor/query.go index 41406c3d..c358c81f 100644 --- a/server/core/internal/datasource/processor/query.go +++ b/server/core/internal/datasource/processor/query.go @@ -8,6 +8,10 @@ import ( "github.com/oxynote/oxynote/server/core/pkg/errutil" ) +// ErrInvalidChartType is returned when a chart type is not one the +// processors draw. +var ErrInvalidChartType = errutil.New(http.StatusBadRequest, "chart_type.invalid", "Invalid chart type. Must be one of: line, bar, gauge.") + // ChartType represents the type of chart visualization. type ChartType string @@ -32,6 +36,16 @@ func (ct ChartType) IsValid() bool { } } +// Validate reports whether the chart type is one the processors draw; +// the empty one is not. +func (ct ChartType) Validate() error { + if !ct.IsValid() { + return ErrInvalidChartType + } + + return nil +} + // UnmarshalText parses a chart type, refusing anything but the known // kinds so a bad value is reported where it is decoded. func (ct *ChartType) UnmarshalText(text []byte) error { diff --git a/server/core/internal/datasource/processor/query_test.go b/server/core/internal/datasource/processor/query_test.go index 8e152f41..f1c45f83 100644 --- a/server/core/internal/datasource/processor/query_test.go +++ b/server/core/internal/datasource/processor/query_test.go @@ -75,6 +75,27 @@ func Test_ChartType_IsValid(t *testing.T) { } } +func Test_ChartType_Validate(t *testing.T) { + cc := map[string]struct { + ChartType ChartType + Err error + }{ + "Line chart": {ChartType: ChartTypeLine}, + "Bar chart": {ChartType: ChartTypeBar}, + "Gauge chart": {ChartType: ChartTypeGauge}, + "Empty chart type": {Err: ErrInvalidChartType}, + "Unknown chart type": {ChartType: ChartType("bogus"), Err: ErrInvalidChartType}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, c.ChartType.Validate()) + }) + } +} + func Test_QueryResult_HasData(t *testing.T) { t.Parallel() diff --git a/server/core/internal/datasource/query.go b/server/core/internal/datasource/query.go new file mode 100644 index 00000000..b316b5c2 --- /dev/null +++ b/server/core/internal/datasource/query.go @@ -0,0 +1,83 @@ +package datasource + +import ( + "context" + "net/http" + + "github.com/oxynote/oxynote/server/core/internal/datasource/processor" + "github.com/oxynote/oxynote/server/core/pkg/errutil" +) + +// ErrTypeNotSupported is returned when a data source speaks something +// the generic query cannot run. +var ErrTypeNotSupported = errutil.New(http.StatusBadRequest, "data_source.type_not_supported", "Generic query is not supported for this data source type.") + +// Query runs q against the data source the runner operates and returns +// the result in the unified shape, whatever the source speaks. A query +// that answered nothing is normalised to QueryStatusNoData here, so no +// caller has to tell a nil result from an empty one. +func Query( + ctx context.Context, + r Runner, + q string, + tr processor.TimeRange, + ct processor.ChartType, +) (*processor.QueryResult, error) { + switch r.Type() { + case TypePrometheus: + client, err := r.Prometheus(ctx) + if err != nil { + return nil, err + } + + res, err := client.QueryRange(ctx, q, tr) + if err != nil { + return nil, err + } + + if res == nil { + return noData(), nil + } + + return res.Transform(ct), nil + case TypePostgreSQL: + client, err := r.PostgreSQL(ctx) + if err != nil { + return nil, err + } + + res, err := client.Query(ctx, q, tr) + if err != nil { + return nil, err + } + + if res == nil { + return noData(), nil + } + + return res.Transform(ct), nil + case TypeMariaDB, TypeMySQL: + client, err := r.MySQL(ctx) + if err != nil { + return nil, err + } + + res, err := client.Query(ctx, q, tr) + if err != nil { + return nil, err + } + + if res == nil { + return noData(), nil + } + + return res.Transform(ct), nil + default: + return nil, ErrTypeNotSupported + } +} + +// noData is the result of a query the source answered nothing to. +func noData() *processor.QueryResult { + return &processor.QueryResult{Status: processor.QueryStatusNoData} +} diff --git a/server/core/internal/datasource/query_test.go b/server/core/internal/datasource/query_test.go new file mode 100644 index 00000000..40244fba --- /dev/null +++ b/server/core/internal/datasource/query_test.go @@ -0,0 +1,183 @@ +package datasource + +import ( + "context" + "testing" + "time" + + "github.com/oxynote/oxynote/server/core/internal/datasource/processor" + "github.com/oxynote/oxynote/server/core/pkg/testutil" + "github.com/prometheus/common/model" + "github.com/stretchr/testify/assert" +) + +// stubRunner is a runner of one type whose clients answer the query with +// fixed values. The clients embed their interface, so only the query +// method exists; any other call is a test bug. +type stubRunner struct { + Runner + + typ Type + err error + prometheus *processor.PrometheusQueryResult + postgreSQL *processor.PostgreSQLQueryResult + mySQL *processor.MySQLQueryResult +} + +func (s stubRunner) Type() Type { return s.typ } + +func (s stubRunner) Prometheus(context.Context) (Prometheus, error) { + return stubPrometheus{res: s.prometheus, err: s.err}, nil +} + +func (s stubRunner) PostgreSQL(context.Context) (PostgreSQL, error) { + return stubPostgreSQL{res: s.postgreSQL, err: s.err}, nil +} + +func (s stubRunner) MySQL(context.Context) (MySQL, error) { + return stubMySQL{res: s.mySQL, err: s.err}, nil +} + +// refusingRunner is a runner that hands out no client at all. +type refusingRunner struct { + Runner + + typ Type +} + +func (r refusingRunner) Type() Type { return r.typ } + +func (refusingRunner) Prometheus(context.Context) (Prometheus, error) { return nil, assert.AnError } + +func (refusingRunner) PostgreSQL(context.Context) (PostgreSQL, error) { return nil, assert.AnError } + +func (refusingRunner) MySQL(context.Context) (MySQL, error) { return nil, assert.AnError } + +type stubPrometheus struct { + Prometheus + + res *processor.PrometheusQueryResult + err error +} + +func (s stubPrometheus) QueryRange(context.Context, string, processor.TimeRange) (*processor.PrometheusQueryResult, error) { + return s.res, s.err +} + +type stubPostgreSQL struct { + PostgreSQL + + res *processor.PostgreSQLQueryResult + err error +} + +func (s stubPostgreSQL) Query(context.Context, string, processor.TimeRange) (*processor.PostgreSQLQueryResult, error) { + return s.res, s.err +} + +type stubMySQL struct { + MySQL + + res *processor.MySQLQueryResult + err error +} + +func (s stubMySQL) Query(context.Context, string, processor.TimeRange) (*processor.MySQLQueryResult, error) { + return s.res, s.err +} + +func Test_Query(t *testing.T) { + rows := &processor.PostgreSQLQueryResult{ + Columns: []string{"time", "value"}, + Rows: [][]any{{1700000000.0, 10.0}}, + } + + cc := map[string]struct { + Runner Runner + Result *processor.QueryResult + Err error + }{ + "Unsupported type": { + Runner: stubRunner{typ: Type("bogus")}, + Err: ErrTypeNotSupported, + }, + "Error returned by Runner.Prometheus": { + Runner: refusingRunner{typ: TypePrometheus}, + Err: assert.AnError, + }, + "Error returned by Runner.PostgreSQL": { + Runner: refusingRunner{typ: TypePostgreSQL}, + Err: assert.AnError, + }, + "Error returned by Runner.MySQL": { + Runner: refusingRunner{typ: TypeMySQL}, + Err: assert.AnError, + }, + "Error returned by Prometheus.QueryRange": { + Runner: stubRunner{typ: TypePrometheus, err: assert.AnError}, + Err: assert.AnError, + }, + "Error returned by PostgreSQL.Query": { + Runner: stubRunner{typ: TypePostgreSQL, err: assert.AnError}, + Err: assert.AnError, + }, + "Error returned by MySQL.Query": { + Runner: stubRunner{typ: TypeMariaDB, err: assert.AnError}, + Err: assert.AnError, + }, + "Prometheus answered nothing": { + Runner: stubRunner{typ: TypePrometheus}, + Result: &processor.QueryResult{Status: processor.QueryStatusNoData}, + }, + "PostgreSQL answered nothing": { + Runner: stubRunner{typ: TypePostgreSQL}, + Result: &processor.QueryResult{Status: processor.QueryStatusNoData}, + }, + "MySQL answered nothing": { + Runner: stubRunner{typ: TypeMySQL}, + Result: &processor.QueryResult{Status: processor.QueryStatusNoData}, + }, + "Prometheus result transformed": { + Runner: stubRunner{typ: TypePrometheus, prometheus: &processor.PrometheusQueryResult{ + Type: model.ValMatrix, + Result: model.Matrix{ + &model.SampleStream{ + Metric: model.Metric{"job": "a"}, + Values: []model.SamplePair{{Timestamp: model.Time(1700000000000), Value: 10}}, + }, + }, + }}, + Result: &processor.QueryResult{ + Status: processor.QueryStatusOK, + Data: []processor.QueryResultSeries{ + {Labels: map[string]string{"job": "a"}, Metrics: [][2]any{{int64(1700000000), 10.0}}}, + }, + }, + }, + "PostgreSQL result transformed": { + Runner: stubRunner{typ: TypePostgreSQL, postgreSQL: rows}, + Result: (&processor.PostgreSQLQueryResult{Columns: rows.Columns, Rows: rows.Rows}).Transform(processor.ChartTypeLine), + }, + "MySQL result transformed": { + Runner: stubRunner{typ: TypeMySQL, mySQL: &processor.MySQLQueryResult{Columns: rows.Columns, Rows: rows.Rows}}, + Result: (&processor.MySQLQueryResult{Columns: rows.Columns, Rows: rows.Rows}).Transform(processor.ChartTypeLine), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + tr := processor.TimeRange{From: time.Unix(1700000000, 0), To: time.Unix(1700003600, 0)} + + res, err := Query(context.Background(), c.Runner, "up", tr, processor.ChartTypeLine) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + assert.Equal(t, c.Result, res) + }) + } +} diff --git a/server/core/internal/datasource/simulation/simulation.go b/server/core/internal/datasource/simulation/simulation.go index fb10b0ba..3f22cfe0 100644 --- a/server/core/internal/datasource/simulation/simulation.go +++ b/server/core/internal/datasource/simulation/simulation.go @@ -209,8 +209,11 @@ func (c *Checker) checkMetricActive( now := timeutil.Now() tr := processor.TimeRange{From: now.Add(-probeSpan(block)), To: now} + // the probe transforms as a gauge because that shape accepts every + // result a source can answer with, scalar, vector and matrix alike, + // and only asks whether anything came back, not how it would be drawn. for _, q := range extractQueries(block) { - res, qerr := c.query(ctx, *ds, q, tr) + res, qerr := datasourceCore.Query(ctx, c.runners.Runner(*ds), q, tr, processor.ChartTypeGauge) if qerr != nil { c.log.DebugContext(ctx, "the block's query did not answer", "error", qerr) @@ -249,61 +252,6 @@ func (c *Checker) fetchDataSource( return c.db.FetchDataSource(ctx, id, organizationID) } -// query runs one query against the data source and returns it in the -// unified shape, whatever the source speaks. It transforms as a gauge -// because that shape accepts every result a source can answer with — -// scalar, vector and matrix alike — and the probe only asks whether -// anything came back, not how it would be drawn. -func (c *Checker) query( - ctx context.Context, - ds datasourceCore.DataSource, - q string, - tr processor.TimeRange, -) (*processor.QueryResult, error) { - runner := c.runners.Runner(ds) - - switch ds.Type { - case datasourceCore.TypePrometheus: - client, err := runner.Prometheus(ctx) - if err != nil { - return nil, err - } - - res, err := client.QueryRange(ctx, q, tr) - if err != nil || res == nil { - return nil, err - } - - return res.Transform(processor.ChartTypeGauge), nil - case datasourceCore.TypePostgreSQL: - client, err := runner.PostgreSQL(ctx) - if err != nil { - return nil, err - } - - res, err := client.Query(ctx, q, tr) - if err != nil || res == nil { - return nil, err - } - - return res.Transform(processor.ChartTypeGauge), nil - case datasourceCore.TypeMariaDB, datasourceCore.TypeMySQL: - client, err := runner.MySQL(ctx) - if err != nil { - return nil, err - } - - res, err := client.Query(ctx, q, tr) - if err != nil || res == nil { - return nil, err - } - - return res.Transform(processor.ChartTypeGauge), nil - default: - return nil, fmt.Errorf("data source type %q cannot be queried", ds.Type) - } -} - // clear switches the block's simulation off on the live document. The // preset stays, so the block remembers which series it drew. func (c *Checker) clear( diff --git a/server/core/internal/document/AGENTS.md b/server/core/internal/document/AGENTS.md index 5fbb4f66..78ee6617 100644 --- a/server/core/internal/document/AGENTS.md +++ b/server/core/internal/document/AGENTS.md @@ -8,9 +8,8 @@ Cross-service storage rules (Hocuspocus, Yjs) live in ## Branches - **The main branch is identified by its `default` flag, not its name**: the - tree join and `FetchMainBranchContent` key on it, and - `UpdateDocumentBranch` refuses to rename it. `NewDocument` and `Duplicate` - set `Default: true`; forks must not. + tree join keys on it, and `Document.AllowsBranchRename` refuses to rename + it. `NewDocument` and `Duplicate` set `Default: true`; forks must not. - **Maintainers accumulate.** The `maintainers` field on a branch update is who edited in that persist; `UpsertDocumentMaintainers` only adds. There is no removal path. diff --git a/server/core/internal/document/branch.go b/server/core/internal/document/branch.go new file mode 100644 index 00000000..b8b98aad --- /dev/null +++ b/server/core/internal/document/branch.go @@ -0,0 +1,60 @@ +package document + +import ( + "net/http" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/pkg/errutil" + "github.com/rs/xid" +) + +var ( + // ErrDefaultBranchRename is returned when a rename targets the default + // branch. + ErrDefaultBranchRename = errutil.New(http.StatusBadRequest, "document.default_branch_rename", "the default branch cannot be renamed") + + // ErrBranchSelfMerge is returned when a branch is merged into itself. + ErrBranchSelfMerge = errutil.New(http.StatusBadRequest, "document.branch_self_merge", "cannot merge a branch into itself") + + // ErrDefaultBranchDelete is returned when a delete targets the default + // branch. + ErrDefaultBranchDelete = errutil.New(http.StatusConflict, "document.default_branch", "cannot delete the default branch") + + // ErrLastBranchDelete is returned when a delete targets the only branch + // a document has left. + ErrLastBranchDelete = errutil.New(http.StatusConflict, "document.last_branch", "cannot delete the last branch") +) + +// AllowsBranchRename reports whether the branch may take the given name. +// The default branch is what the tree and content queries return, and its +// name is what the user sees the document under; a rename would leave the +// two out of step. +func (d Document) AllowsBranchRename(name null.String) error { + if d.Default && name.Valid && name.String != d.BranchName { + return ErrDefaultBranchRename + } + + return nil +} + +// AllowsBranchDelete reports whether the branch may be deleted: the +// default branch never is. +func (d Document) AllowsBranchDelete() error { + if d.Default { + return ErrDefaultBranchDelete + } + + return nil +} + +// AllowsMerge reports whether one branch may be merged into another. A +// self-merge would soft-delete the branch's hooks and comments and then +// copy the hooks back from the same, now hook-less, branch, permanently +// destroying both. +func AllowsMerge(fromBranchID, toBranchID xid.ID) error { + if fromBranchID == toBranchID { + return ErrBranchSelfMerge + } + + return nil +} diff --git a/server/core/internal/document/branch_test.go b/server/core/internal/document/branch_test.go new file mode 100644 index 00000000..5d0574e8 --- /dev/null +++ b/server/core/internal/document/branch_test.go @@ -0,0 +1,46 @@ +package document + +import ( + "testing" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/pkg/testutil" + "github.com/rs/xid" +) + +func Test_Document_AllowsBranchRename(t *testing.T) { + cc := map[string]struct { + Doc Document + Name null.String + Err error + }{ + "Default branch renamed": {Doc: Document{Default: true, BranchName: DefaultBranch}, Name: null.StringFrom("Renamed"), Err: ErrDefaultBranchRename}, + "Default branch keeps its name": {Doc: Document{Default: true, BranchName: DefaultBranch}, Name: null.StringFrom(DefaultBranch)}, + "Default branch without a name": {Doc: Document{Default: true, BranchName: DefaultBranch}}, + "Fork renamed": {Doc: Document{BranchName: "feature"}, Name: null.StringFrom("Renamed")}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, c.Doc.AllowsBranchRename(c.Name)) + }) + } +} + +func Test_Document_AllowsBranchDelete(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, ErrDefaultBranchDelete, Document{Default: true}.AllowsBranchDelete()) + testutil.AssertEqualError(t, nil, Document{}.AllowsBranchDelete()) +} + +func Test_AllowsMerge(t *testing.T) { + t.Parallel() + + from, to := xid.New(), xid.New() + + testutil.AssertEqualError(t, ErrBranchSelfMerge, AllowsMerge(from, from)) + testutil.AssertEqualError(t, nil, AllowsMerge(from, to)) +} diff --git a/server/core/internal/document/comment/comment.go b/server/core/internal/document/comment/comment.go index 7cadabc6..9541f69b 100644 --- a/server/core/internal/document/comment/comment.go +++ b/server/core/internal/document/comment/comment.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net/http" + "slices" "time" "github.com/guregu/null/v5" @@ -203,6 +204,41 @@ func (c Comment) Replace() (Comment, bool) { return nc, true } +// Participants returns the users who wrote the comment or a reply to it, +// in that order, each once. +func (c Comment) Participants() []string { + var ids []string + + if c.UserID.Valid { + ids = append(ids, c.UserID.String) + } + + for _, r := range c.Replies { + if r.UserID.Valid && !slices.Contains(ids, r.UserID.String) { + ids = append(ids, r.UserID.String) + } + } + + return ids +} + +// Recipients reduces the candidate user ids to the ones a notification +// about the actor's change goes to: the actor is dropped, and so is +// every repeat. +func Recipients(userIDs []string, actor string) []string { + var out []string + + for _, id := range userIDs { + if id == actor || slices.Contains(out, id) { + continue + } + + out = append(out, id) + } + + return out +} + // NewReply creates a new reply instance with the provided input. func NewReply(inp ReplyInput, commentID xid.ID, userID, organizationID string) Reply { return Reply{ diff --git a/server/core/internal/document/comment/comment_test.go b/server/core/internal/document/comment/comment_test.go index 07810c1a..0464f93d 100644 --- a/server/core/internal/document/comment/comment_test.go +++ b/server/core/internal/document/comment/comment_test.go @@ -138,6 +138,65 @@ func Test_Comment_Replace(t *testing.T) { assert.Equal(t, empty, same) } +func Test_Comment_Participants(t *testing.T) { + cc := map[string]struct { + Comment Comment + Result []string + }{ + "Author alone": { + Comment: Comment{UserID: null.StringFrom("user-1")}, + Result: []string{"user-1"}, + }, + "Author and repliers": { + Comment: Comment{ + UserID: null.StringFrom("user-1"), + Replies: []Reply{ + {UserID: null.StringFrom("user-2")}, + {UserID: null.StringFrom("user-1")}, + {UserID: null.String{}}, + {UserID: null.StringFrom("user-3")}, + {UserID: null.StringFrom("user-2")}, + }, + }, + Result: []string{"user-1", "user-2", "user-3"}, + }, + "Deleted author": { + Comment: Comment{Replies: []Reply{{UserID: null.StringFrom("user-2")}}}, + Result: []string{"user-2"}, + }, + "Nobody": {}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, c.Comment.Participants()) + }) + } +} + +func Test_Recipients(t *testing.T) { + cc := map[string]struct { + UserIDs []string + Actor string + Result []string + }{ + "Actor dropped": {UserIDs: []string{"user-1", "user-2"}, Actor: "user-1", Result: []string{"user-2"}}, + "Repeats dropped": {UserIDs: []string{"user-2", "user-3", "user-2"}, Actor: "user-1", Result: []string{"user-2", "user-3"}}, + "Actor alone": {UserIDs: []string{"user-1"}, Actor: "user-1"}, + "Nobody": {Actor: "user-1"}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, Recipients(c.UserIDs, c.Actor)) + }) + } +} + func Test_NewReply(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/document.go b/server/core/internal/document/document.go index d216d128..271f28bb 100644 --- a/server/core/internal/document/document.go +++ b/server/core/internal/document/document.go @@ -8,6 +8,7 @@ import ( "fmt" "maps" "math/rand" + "slices" "time" "github.com/guregu/null/v5" @@ -348,6 +349,39 @@ type UpdateInput struct { Branch string `json:"branch"` } +// AddsMaintainers reports whether the update names anyone the stored +// maintainers do not include. The update carries the editors of one +// persist, so the set is only ever added to: diffing it against the +// stored maintainers would drop everyone who happens not to be editing +// right now. +func (ui UpdateInput) AddsMaintainers(stored []string) bool { + for _, maintainer := range ui.Maintainers { + if !slices.Contains(stored, maintainer) { + return true + } + } + + return false +} + +// NewWelcomeDocument creates the document a fresh organization starts +// with: the getting-started content, its charts reading the given data +// source or dropped when there is none. +func NewWelcomeDocument(organizationID, createdBy string, dataSourceID null.Value[xid.ID]) (Document, error) { + content, err := InitialDocumentContent(dataSourceID) + if err != nil { + return Document{}, err + } + + doc := NewDocument(CreateInput{ + Name: "Welcome to Oxynote!", + Icon: "mingcute:flag-4-fill", + }, organizationID, createdBy) + doc.Content = content + + return doc, nil +} + // InitialDocumentContent returns the initial content for a new document. func InitialDocumentContent(dataSourceID null.Value[xid.ID]) (RootBlock, error) { var drb RootBlock diff --git a/server/core/internal/document/document_test.go b/server/core/internal/document/document_test.go index 86a47b3a..0a2cb8d5 100644 --- a/server/core/internal/document/document_test.go +++ b/server/core/internal/document/document_test.go @@ -408,6 +408,42 @@ func Test_NewDocumentContent(t *testing.T) { assert.Len(t, uid, 21) } +func Test_UpdateInput_AddsMaintainers(t *testing.T) { + cc := map[string]struct { + Maintainers []string + Stored []string + Result bool + }{ + "New maintainer": {Maintainers: []string{"u1", "u2"}, Stored: []string{"u1"}, Result: true}, + "Known maintainers only": {Maintainers: []string{"u1"}, Stored: []string{"u1", "u2"}}, + "No maintainers": {Stored: []string{"u1"}}, + "Nobody stored yet": {Maintainers: []string{"u1"}, Result: true}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, UpdateInput{Maintainers: c.Maintainers}.AddsMaintainers(c.Stored)) + }) + } +} + +func Test_NewWelcomeDocument(t *testing.T) { + t.Parallel() + + doc, err := NewWelcomeDocument("org1", "u1", null.Value[xid.ID]{}) + require.NoError(t, err) + + assert.Equal(t, "Welcome to Oxynote!", doc.DocumentName) + assert.Equal(t, "mingcute:flag-4-fill", doc.Icon) + assert.Equal(t, "org1", doc.OrganizationID) + assert.Equal(t, null.StringFrom("u1"), doc.CreatedBy) + assert.True(t, doc.Default) + assert.Equal(t, BlockNodeDoc, doc.Content.Type) + assert.NotEmpty(t, doc.Content.Content) +} + func Test_InitialDocumentContent(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/file/file.go b/server/core/internal/document/file/file.go index 8fce4dea..c2d05cd2 100644 --- a/server/core/internal/document/file/file.go +++ b/server/core/internal/document/file/file.go @@ -7,6 +7,7 @@ import ( "path" "strings" "time" + "unicode/utf8" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/pkg/timeutil" @@ -16,6 +17,13 @@ import ( // _folderFormat is the storage folder format for a document's files. const _folderFormat = "organizations/%s/documents/%s/files" +// _maxNameLength caps the name recorded for an upload, in runes. +const _maxNameLength = 255 + +// _fallbackName names an upload whose multipart part carried no usable +// name. +const _fallbackName = "file" + // Folder returns the storage folder holding the given document's files. func Folder(organizationID string, documentID xid.ID) string { return fmt.Sprintf(_folderFormat, organizationID, documentID) @@ -155,3 +163,25 @@ func (f File) Disposition() string { func (f File) Orphaned() bool { return !f.DocumentID.Valid || !f.OrganizationID.Valid } + +// CleanName reduces the name a multipart part carried to a bare file +// name: a browser may send a path, Windows ones with backslashes, and +// the part may carry no name at all. +func CleanName(raw string) string { + name := path.Base(strings.ReplaceAll(raw, "\\", "/")) + name = strings.TrimSpace(name) + + if name == "." || name == "/" { + name = "" + } + + if utf8.RuneCountInString(name) > _maxNameLength { + name = string([]rune(name)[:_maxNameLength]) + } + + if name == "" { + return _fallbackName + } + + return name +} diff --git a/server/core/internal/document/file/file_test.go b/server/core/internal/document/file/file_test.go index ba76c4c4..ec3105f5 100644 --- a/server/core/internal/document/file/file_test.go +++ b/server/core/internal/document/file/file_test.go @@ -1,6 +1,7 @@ package file import ( + "strings" "testing" "github.com/guregu/null/v5" @@ -153,6 +154,34 @@ func Test_Key(t *testing.T) { ) } +func Test_CleanName(t *testing.T) { + cc := map[string]struct { + Raw string + Result string + }{ + "Plain name": {Raw: "notes.zip", Result: "notes.zip"}, + "Unix path": {Raw: "/home/me/notes.zip", Result: "notes.zip"}, + "Windows path": {Raw: `C:\Users\me\notes.zip`, Result: "notes.zip"}, + "Surrounding whitespace": {Raw: " notes.zip ", Result: "notes.zip"}, + "Empty name": {Raw: "", Result: "file"}, + "Whitespace only": {Raw: " ", Result: "file"}, + "Dot": {Raw: ".", Result: "file"}, + "Root": {Raw: "/", Result: "file"}, + "Non-ASCII name": {Raw: "résumé.pdf", Result: "résumé.pdf"}, + "Name over the rune cap": {Raw: strings.Repeat("é", 300), Result: strings.Repeat("é", 255)}, + "Name at the rune cap": {Raw: strings.Repeat("a", 255), Result: strings.Repeat("a", 255)}, + "Path with a long base": {Raw: "dir/" + strings.Repeat("b", 256), Result: strings.Repeat("b", 255)}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, CleanName(c.Raw)) + }) + } +} + func Test_File_Orphaned(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/node.go b/server/core/internal/document/node.go index 628bf91e..f7b39278 100644 --- a/server/core/internal/document/node.go +++ b/server/core/internal/document/node.go @@ -17,10 +17,10 @@ import ( // router mounts the file routes on this very format. const FilePathFormat = "/api/documents/%s/files/%s" -// FileIDLength is the exact length of a file id: a 21-character nanoid. -// The id's alphabet includes the dash, so a "-" segment is -// split by length, never at the first dash. -const FileIDLength = 21 +// FileIDLength is the exact length of a file id, a nanoid. The id's +// alphabet includes the dash, so a "-" segment is split by +// length, never at the first dash. +const FileIDLength = strutil.NanoIDLength // FilePath returns the URL path a document's file is served under: the // file route with the id followed by a dash and the file name, so the @@ -30,6 +30,24 @@ func FilePath(documentID xid.ID, id, name string) string { return fmt.Sprintf(FilePathFormat, documentID, id+"-"+url.PathEscape(name)) } +// ParseFileRef cuts a "-" route segment, as FilePath +// builds it, back into its id and name, reporting whether the segment +// has that shape. The id must be a nanoid: its charset shuts out the +// path separators and dot segments that would otherwise let an id +// escape its storage folder once joined into an object key. +func ParseFileRef(ref string) (id, name string, ok bool) { + if len(ref) <= FileIDLength || ref[FileIDLength] != '-' { + return "", "", false + } + + id = ref[:FileIDLength] + if !strutil.IsNanoID(id) { + return "", "", false + } + + return id, ref[FileIDLength+1:], true +} + // RootBlock represents a block in a document. type RootBlock struct { // Type is the type of the root block (always BlockNodeText's @@ -376,12 +394,11 @@ func (d *duplication) rewriteFileRef(attrs, newAttrs Attributes) { return } - if len(rest) <= FileIDLength || rest[FileIDLength] != '-' { + oldID, name, ok := ParseFileRef(rest) + if !ok { return } - oldID, name := rest[:FileIDLength], rest[FileIDLength+1:] - newID, ok := d.files[oldID] if !ok { newID = strutil.NanoID() diff --git a/server/core/internal/document/node_test.go b/server/core/internal/document/node_test.go index c597e08e..dc785513 100644 --- a/server/core/internal/document/node_test.go +++ b/server/core/internal/document/node_test.go @@ -293,6 +293,36 @@ func Test_FilePath(t *testing.T) { } } +func Test_ParseFileRef(t *testing.T) { + cc := map[string]struct { + Ref string + ID string + Name string + OK bool + }{ + "Id and name": {Ref: "f1xxxxxxxxxxxxxxxxxxx-notes.zip", ID: "f1xxxxxxxxxxxxxxxxxxx", Name: "notes.zip", OK: true}, + "Name with dashes": {Ref: "f1xxxxxxxxxxxxxxxxxxx-a-b-c.zip", ID: "f1xxxxxxxxxxxxxxxxxxx", Name: "a-b-c.zip", OK: true}, + "Id with dashes": {Ref: "f1-x_-xxxxxxxxxxxxxxx-notes.zip", ID: "f1-x_-xxxxxxxxxxxxxxx", Name: "notes.zip", OK: true}, + "Empty name": {Ref: "f1xxxxxxxxxxxxxxxxxxx-", ID: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, + "No dash after the id": {Ref: "f1xxxxxxxxxxxxxxxxxxxxnotes.zip"}, + "Id alone": {Ref: "f1xxxxxxxxxxxxxxxxxxx"}, + "Short": {Ref: "f1-notes.zip"}, + "Id with the wrong charset": {Ref: "f1.................xx-notes.zip"}, + "Empty": {Ref: ""}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + id, name, ok := ParseFileRef(c.Ref) + assert.Equal(t, c.OK, ok) + assert.Equal(t, c.ID, id) + assert.Equal(t, c.Name, name) + }) + } +} + func Test_RootBlock_Duplicate(t *testing.T) { t.Parallel() diff --git a/server/core/internal/email/email.go b/server/core/internal/email/email.go index fcd8855d..9f134739 100644 --- a/server/core/internal/email/email.go +++ b/server/core/internal/email/email.go @@ -238,98 +238,24 @@ func (s *Sender) deliver(ctx context.Context, msg *mail.Msg, toEmail, subject st ) } -// SendEmailVerification sends an email regarding new email -// verification with the token, embedded into a full URL, to the -// specified email address. -func (s *Sender) SendEmailVerification(eml, link string) { - s.send( - eml, - "Verify your new email address", - TemplateEmailVerification, - map[string]string{ - _linkKey: link, - }, - ) -} +// Send sends the given template to the recipient in d, rendered with the +// values d carries. Delivery runs on its own and never reports back: the +// only error is a template that cannot be sent. +func (s *Sender) Send(tmpl Template, d Data) error { + if err := tmpl.Validate(); err != nil { + return err + } -// SendEmailChangeConfirmation sends an email asking the current address to -// approve a change of address, with the approval link. -func (s *Sender) SendEmailChangeConfirmation(eml, link string) { - s.send( - eml, - "Approve your email address change", - TemplateEmailChangeConfirmation, - map[string]string{ - _linkKey: link, - }, - ) -} + sp := _specs[tmpl] -// SendOrganizationInvitation sends an email regarding -// organization invitation with the token, embedded into a full URL, to the -// specified email address. -func (s *Sender) SendOrganizationInvitation(eml, org, link string) { - s.send( - eml, - fmt.Sprintf("Join %s on Oxynote", org), - TemplateOrganizationInvitation, - map[string]string{ - _linkKey: link, - "organization": org, - }, - ) -} + subject := sp.subject + if sp.subjectFrom != nil { + subject = sp.subjectFrom(d) + } -// SendUserDeletionConfirmation sends an email to confirm account deletion -// with a verification link. -func (s *Sender) SendUserDeletionConfirmation(eml, link string) { - s.send( - eml, - "Confirm your account deletion", - TemplateUserDeletion, - map[string]string{ - _linkKey: link, - }, - ) -} + s.send(d.Email, subject, tmpl, sp.args(d)) -// SendPasswordReset sends an email with a password reset link to the -// specified email address. -func (s *Sender) SendPasswordReset(eml, link string) { - s.send( - eml, - "Reset your password", - TemplatePasswordReset, - map[string]string{ - _linkKey: link, - }, - ) -} - -// SendSignupVerification sends the initial account-activation email -// with a verification link to a freshly signed-up email address. -func (s *Sender) SendSignupVerification(eml, link string) { - s.send( - eml, - "Confirm your email address", - TemplateSignupVerification, - map[string]string{ - _linkKey: link, - }, - ) -} - -// SendAccountExists notifies the owner of an existing account that a -// signup was attempted with their email address, with a login link. -func (s *Sender) SendAccountExists(eml, link string) { - s.send( - eml, - "You already have an Oxynote account", - TemplateAccountExists, - map[string]string{ - _linkKey: link, - }, - ) + return nil } // client is an interface that handles communication with the SMTP diff --git a/server/core/internal/email/email_test.go b/server/core/internal/email/email_test.go index a54a867a..c92a5136 100644 --- a/server/core/internal/email/email_test.go +++ b/server/core/internal/email/email_test.go @@ -221,119 +221,93 @@ func Test_Sender_send(t *testing.T) { } } -func Test_Sender_SendEmailVerification(t *testing.T) { - t.Parallel() +func Test_Sender_Send(t *testing.T) { + cc := map[string]struct { + Template Template + Data Data + Subject string + Contains []string + Err error + }{ + "Unknown template": { + Template: Template("nonexistent"), + Data: Data{Email: "user@example.com"}, + Err: ErrInvalidTemplate, + }, + "Email verification": { + Template: TemplateEmailVerification, + Data: Data{Email: "user@example.com", Link: "https://example.com/verify"}, + Subject: "Verify your new email address", + Contains: []string{"https://example.com/verify"}, + }, + "Email change confirmation": { + Template: TemplateEmailChangeConfirmation, + Data: Data{Email: "user@example.com", Link: "https://example.com/approve"}, + Subject: "Approve your email address change", + Contains: []string{"https://example.com/approve"}, + }, + "Organization invitation": { + Template: TemplateOrganizationInvitation, + Data: Data{Email: "user@example.com", Organization: "Acme", Link: "https://example.com/join"}, + Subject: "Join Acme on Oxynote", + Contains: []string{"https://example.com/join", "Acme"}, + }, + "User deletion confirmation": { + Template: TemplateUserDeletion, + Data: Data{Email: "user@example.com", Link: "https://example.com/delete"}, + Subject: "Confirm your account deletion", + Contains: []string{"https://example.com/delete"}, + }, + "Password reset": { + Template: TemplatePasswordReset, + Data: Data{Email: "user@example.com", Link: "https://example.com/reset"}, + Subject: "Reset your password", + Contains: []string{"https://example.com/reset"}, + }, + "Signup verification": { + Template: TemplateSignupVerification, + Data: Data{Email: "user@example.com", Link: "https://example.com/activate"}, + Subject: "Confirm your email address", + Contains: []string{"https://example.com/activate"}, + }, + "Account exists": { + Template: TemplateAccountExists, + Data: Data{Email: "user@example.com", Link: "https://example.com/login"}, + Subject: "You already have an Oxynote account", + Contains: []string{"https://example.com/login"}, + }, + } - client := &clientMock{} - s := stubSender(client) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - s.SendEmailVerification("user@example.com", "https://example.com/verify") + client := &clientMock{} + s := stubSender(client) - s.supv.Wait() + err := s.Send(c.Template, c.Data) - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"Verify your new email address"}, msg.GetGenHeader(mail.HeaderSubject)) - assert.Contains(t, msgBody(t, msg), "https://example.com/verify") -} + s.supv.Wait() -func Test_Sender_SendEmailChangeConfirmation(t *testing.T) { - t.Parallel() + testutil.AssertEqualError(t, c.Err, err) - client := &clientMock{} - s := stubSender(client) + if err != nil { + assert.Empty(t, client.DialAndSendWithContextCalls()) - s.SendEmailChangeConfirmation("user@example.com", "https://example.com/approve") + return + } - s.supv.Wait() + msg := sentMsg(t, client) + assert.Equal(t, c.Data.Email, msgTo(t, msg)) + assert.Equal(t, []string{c.Subject}, msg.GetGenHeader(mail.HeaderSubject)) - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"Approve your email address change"}, msg.GetGenHeader(mail.HeaderSubject)) - assert.Contains(t, msgBody(t, msg), "https://example.com/approve") -} + body := msgBody(t, msg) -func Test_Sender_SendOrganizationInvitation(t *testing.T) { - t.Parallel() - - client := &clientMock{} - s := stubSender(client) - - s.SendOrganizationInvitation("user@example.com", "Acme", "https://example.com/join") - - s.supv.Wait() - - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"Join Acme on Oxynote"}, msg.GetGenHeader(mail.HeaderSubject)) - - body := msgBody(t, msg) - assert.Contains(t, body, "https://example.com/join") - assert.Contains(t, body, "Acme") -} - -func Test_Sender_SendUserDeletionConfirmation(t *testing.T) { - t.Parallel() - - client := &clientMock{} - s := stubSender(client) - - s.SendUserDeletionConfirmation("user@example.com", "https://example.com/delete") - - s.supv.Wait() - - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"Confirm your account deletion"}, msg.GetGenHeader(mail.HeaderSubject)) - assert.Contains(t, msgBody(t, msg), "https://example.com/delete") -} - -func Test_Sender_SendPasswordReset(t *testing.T) { - t.Parallel() - - client := &clientMock{} - s := stubSender(client) - - s.SendPasswordReset("user@example.com", "https://example.com/reset") - - s.supv.Wait() - - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"Reset your password"}, msg.GetGenHeader(mail.HeaderSubject)) - assert.Contains(t, msgBody(t, msg), "https://example.com/reset") -} - -func Test_Sender_SendSignupVerification(t *testing.T) { - t.Parallel() - - client := &clientMock{} - s := stubSender(client) - - s.SendSignupVerification("user@example.com", "https://example.com/activate") - - s.supv.Wait() - - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"Confirm your email address"}, msg.GetGenHeader(mail.HeaderSubject)) - assert.Contains(t, msgBody(t, msg), "https://example.com/activate") -} - -func Test_Sender_SendAccountExists(t *testing.T) { - t.Parallel() - - client := &clientMock{} - s := stubSender(client) - - s.SendAccountExists("user@example.com", "https://example.com/login") - - s.supv.Wait() - - msg := sentMsg(t, client) - assert.Equal(t, "user@example.com", msgTo(t, msg)) - assert.Equal(t, []string{"You already have an Oxynote account"}, msg.GetGenHeader(mail.HeaderSubject)) - assert.Contains(t, msgBody(t, msg), "https://example.com/login") + for _, sub := range c.Contains { + assert.Contains(t, body, sub) + } + }) + } } func Test_Sender_Close(t *testing.T) { @@ -350,8 +324,7 @@ func Test_Sender_Close(t *testing.T) { } s := stubSender(client) - s.SendPasswordReset("user@example.com", "https://example.com/reset") - + require.NoError(t, s.Send(TemplatePasswordReset, Data{Email: "user@example.com", Link: "https://example.com/reset"})) require.NoError(t, s.Close()) // Close must drain the in-flight delivery before cancelling the diff --git a/server/core/internal/email/templates.go b/server/core/internal/email/templates.go index 0b81cf50..f77322ce 100644 --- a/server/core/internal/email/templates.go +++ b/server/core/internal/email/templates.go @@ -4,8 +4,11 @@ import ( "embed" "fmt" "html" + "net/http" "strings" texttemplate "text/template" + + "github.com/oxynote/oxynote/server/core/pkg/errutil" ) // All available email template constants. @@ -38,9 +41,48 @@ const ( TemplateSignupVerification Template = "signup_verification" ) -// Template identifies an email template. Its value matches the base -// name of the embedded HTML file the template is rendered from. -type Template string +// ErrInvalidTemplate is returned when the requested email template is +// not recognized. +var ErrInvalidTemplate = errutil.New(http.StatusBadRequest, "email.invalid_template", "Invalid email template.") + +// _specs maps every template to the subject line it is sent under and +// the arguments its HTML file reads. A template absent from the map +// cannot be sent. +var _specs = map[Template]spec{ + TemplateEmailVerification: { + subject: "Verify your new email address", + args: linkArgs, + }, + TemplateEmailChangeConfirmation: { + subject: "Approve your email address change", + args: linkArgs, + }, + TemplateOrganizationInvitation: { + subjectFrom: func(d Data) string { return fmt.Sprintf("Join %s on Oxynote", d.Organization) }, + args: func(d Data) map[string]string { + return map[string]string{ + _linkKey: d.Link, + "organization": d.Organization, + } + }, + }, + TemplateUserDeletion: { + subject: "Confirm your account deletion", + args: linkArgs, + }, + TemplatePasswordReset: { + subject: "Reset your password", + args: linkArgs, + }, + TemplateSignupVerification: { + subject: "Confirm your email address", + args: linkArgs, + }, + TemplateAccountExists: { + subject: "You already have an Oxynote account", + args: linkArgs, + }, +} //go:embed templates/*.html var _templateFS embed.FS @@ -64,6 +106,53 @@ var _templates = texttemplate.Must( texttemplate.ParseFS(_templateFS, "templates/*.html"), ) +// Template identifies an email template. Its value matches the base +// name of the embedded HTML file the template is rendered from. +type Template string + +// Validate reports whether the template is one that can be sent. +func (t Template) Validate() error { + if _, ok := _specs[t]; !ok { + return ErrInvalidTemplate + } + + return nil +} + +// Data carries what a template is rendered with: the recipient and the +// values the templates read. A template ignores the fields it has no +// use for. +type Data struct { + // Email is the recipient address. + Email string `json:"email"` + + // Organization is the name of the organization an invitation is to. + Organization string `json:"organization"` + + // Link is the action URL the email carries. + Link string `json:"link"` +} + +// spec describes how a template is sent: its subject line and the +// arguments its HTML file reads. +type spec struct { + // subject is the subject line, when it is the same for every send. + subject string + + // subjectFrom builds the subject line from the data, for a template + // whose subject names something in it. It takes precedence over + // subject. + subjectFrom func(Data) string + + // args builds the template arguments. + args func(Data) map[string]string +} + +// linkArgs builds the arguments of a template that reads only the link. +func linkArgs(d Data) map[string]string { + return map[string]string{_linkKey: d.Link} +} + // render executes the specified template with the provided arguments, // HTML-escaping each argument value, and returns the resulting HTML // body. diff --git a/server/core/internal/email/templates_test.go b/server/core/internal/email/templates_test.go index 7028275f..f6939445 100644 --- a/server/core/internal/email/templates_test.go +++ b/server/core/internal/email/templates_test.go @@ -8,6 +8,31 @@ import ( "github.com/stretchr/testify/require" ) +func Test_Template_Validate(t *testing.T) { + cc := map[string]struct { + Template Template + Err error + }{ + "Email verification": {Template: TemplateEmailVerification}, + "Email change confirmation": {Template: TemplateEmailChangeConfirmation}, + "Organization invitation": {Template: TemplateOrganizationInvitation}, + "User deletion": {Template: TemplateUserDeletion}, + "Password reset": {Template: TemplatePasswordReset}, + "Signup verification": {Template: TemplateSignupVerification}, + "Account exists": {Template: TemplateAccountExists}, + "Unknown template": {Template: Template("nonexistent"), Err: ErrInvalidTemplate}, + "Empty template": {Err: ErrInvalidTemplate}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, c.Template.Validate()) + }) + } +} + func Test_render(t *testing.T) { cc := map[string]struct { Template Template diff --git a/server/core/internal/org/org.go b/server/core/internal/org/org.go new file mode 100644 index 00000000..bb646d60 --- /dev/null +++ b/server/core/internal/org/org.go @@ -0,0 +1,20 @@ +// Package org defines what an organization owns outside of Postgres: the +// storage layout of its logo and the URL the logo is served under. +package org + +import "fmt" + +// _logoFolderFormat is the storage folder format for an organization's +// logo. +const _logoFolderFormat = "organizations/%s/logo" + +// LogoPath is the URL path an organization's logo is served under. The +// router mounts the logo routes on this very path, and the value is stored +// on the organization row, so the two must not drift apart. +const LogoPath = "/api/organizations/logo" + +// LogoFolder returns the storage folder holding the given organization's +// logo, which is stored under the organization's own id. +func LogoFolder(organizationID string) string { + return fmt.Sprintf(_logoFolderFormat, organizationID) +} diff --git a/server/core/internal/org/org_test.go b/server/core/internal/org/org_test.go new file mode 100644 index 00000000..11575861 --- /dev/null +++ b/server/core/internal/org/org_test.go @@ -0,0 +1,18 @@ +package org + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} + +func Test_LogoFolder(t *testing.T) { + t.Parallel() + + assert.Equal(t, "organizations/org1/logo", LogoFolder("org1")) +} diff --git a/server/core/internal/search/query.go b/server/core/internal/search/query.go index 2a7a8c69..c4866904 100644 --- a/server/core/internal/search/query.go +++ b/server/core/internal/search/query.go @@ -4,15 +4,23 @@ import ( "context" "encoding/json" "fmt" + "net/http" "time" + "unicode/utf8" "github.com/blevesearch/bleve/v2" bleveSearch "github.com/blevesearch/bleve/v2/search" "github.com/blevesearch/bleve/v2/search/query" + "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/rs/xid" ) const ( + // MaxQueryLength caps a search query in characters. Every term + // expands to a dictionary walk inside the process, so the query size + // bounds the work one request can ask for. + MaxQueryLength = 200 + // _searchResultLimit caps the number of hits the search endpoint // returns. _searchResultLimit = 30 @@ -45,6 +53,19 @@ const ( _maxTermSearchers = 1024 ) +// ErrInvalidQuery is returned when a search query is empty or over +// MaxQueryLength. +var ErrInvalidQuery = errutil.New(http.StatusBadRequest, "document.invalid_search_query", "invalid search query") + +// ValidateQuery reports whether the query is one the index will run. +func ValidateQuery(q string) error { + if q == "" || utf8.RuneCountInString(q) > MaxQueryLength { + return ErrInvalidQuery + } + + return nil +} + // SearchDocuments searches the organization's entries and returns them as // the search endpoint's JSON: the matched terms wrapped in mark tags and // the text cropped to a fragment around the best match. @@ -72,6 +93,10 @@ func (i *Index) SearchDocumentBlocks(ctx context.Context, organizationID, q stri // search runs the query and decodes the hits. func (i *Index) search(ctx context.Context, organizationID, q string, limit int, highlight bool) ([]Block, error) { + if err := ValidateQuery(q); err != nil { + return nil, err + } + ctx, cancel := context.WithTimeout(ctx, _searchTimeout) defer cancel() diff --git a/server/core/internal/search/query_test.go b/server/core/internal/search/query_test.go index 5e8d7861..ee84d3eb 100644 --- a/server/core/internal/search/query_test.go +++ b/server/core/internal/search/query_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/blevesearch/bleve/v2/search/query" + "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/rs/xid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -141,6 +142,26 @@ func Test_Index_SearchDocumentBlocks(t *testing.T) { assert.Nil(t, blocks) } +func Test_ValidateQuery(t *testing.T) { + cc := map[string]struct { + Query string + Err error + }{ + "Empty query": {Err: ErrInvalidQuery}, + "Overlong query": {Query: strings.Repeat("a", MaxQueryLength+1), Err: ErrInvalidQuery}, + "Query at the cap": {Query: strings.Repeat("é", MaxQueryLength)}, + "Plain query": {Query: "deploy"}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, ValidateQuery(c.Query)) + }) + } +} + func Test_Index_search(t *testing.T) { t.Parallel() @@ -152,7 +173,14 @@ func Test_Index_search(t *testing.T) { Limit int Highlight bool Check func(t *testing.T, blocks []Block) + Err error }{ + "Overlong query": { + OrganizationID: "org-1", + Query: strings.Repeat("a", MaxQueryLength+1), + Limit: 10, + Err: ErrInvalidQuery, + }, "Stemming matches another form of the word": { OrganizationID: "org-1", Query: "deploying", @@ -330,7 +358,11 @@ func Test_Index_search(t *testing.T) { t.Parallel() blocks, err := idx.search(context.Background(), c.OrganizationID, c.Query, c.Limit, c.Highlight) - require.NoError(t, err) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } c.Check(t, blocks) }) diff --git a/server/core/internal/server/internal/auth/auth.go b/server/core/internal/server/internal/auth/auth.go index b19d7872..f55ad051 100644 --- a/server/core/internal/server/internal/auth/auth.go +++ b/server/core/internal/server/internal/auth/auth.go @@ -126,6 +126,16 @@ func RequireSession(log *slog.Logger, w http.ResponseWriter, r *http.Request) (S return session, true } +// HasActiveOrganization reports whether a validated identity may reach +// the handlers: a user together with the organization their work is +// scoped to. A session without an active organization (e.g. right after +// signup, before one is selected) must not: every query is scoped by the +// organization id, and a write would create rows under an empty one. The +// cookie session path and the MCP bearer verifier both ask it. +func HasActiveOrganization(userID, organizationID string) bool { + return userID != "" && organizationID != "" +} + // getSession forwards the request to the Better Auth server to retrieve session information. func getSession(ctx context.Context, client *http.Client, url string, cookies []*http.Cookie) (Session, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) @@ -161,11 +171,7 @@ func getSession(ctx context.Context, client *http.Client, url string, cookies [] return Session{}, err } - // a session without an active organization (e.g. right after signup, - // before one is selected) must not reach the handlers: every query is - // scoped by the organization id, and a write would create rows under an - // empty one. The MCP bearer verifier applies the same rule. - if payload.Session.UserID == "" || payload.Session.ActiveOrganizationID == "" { + if !HasActiveOrganization(payload.Session.UserID, payload.Session.ActiveOrganizationID) { return Session{}, httpserver.ErrNotAuthenticated } diff --git a/server/core/internal/server/internal/auth/auth_test.go b/server/core/internal/server/internal/auth/auth_test.go index 1e8e5a87..e406be81 100644 --- a/server/core/internal/server/internal/auth/auth_test.go +++ b/server/core/internal/server/internal/auth/auth_test.go @@ -185,6 +185,27 @@ func Test_Middleware(t *testing.T) { } } +func Test_HasActiveOrganization(t *testing.T) { + cc := map[string]struct { + UserID string + OrganizationID string + Result bool + }{ + "User and organization": {UserID: "u1", OrganizationID: "org1", Result: true}, + "Empty user": {OrganizationID: "org1"}, + "Empty organization": {UserID: "u1"}, + "Both empty": {}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, HasActiveOrganization(c.UserID, c.OrganizationID)) + }) + } +} + func Test_FilterOrganization(t *testing.T) { t.Parallel() diff --git a/server/core/internal/server/internal/datasource/http_test.go b/server/core/internal/server/internal/datasource/http_test.go index 824e97b4..297ecbff 100644 --- a/server/core/internal/server/internal/datasource/http_test.go +++ b/server/core/internal/server/internal/datasource/http_test.go @@ -247,6 +247,9 @@ func newRunnerMock(clients *clientMocks) *runnerMock { m := &runnerMock{clientMocks: clients} m.runner = &datasourceMock.Runner{ + TypeFunc: func() datasourceCore.Type { + return m.dataSources[len(m.dataSources)-1].Type + }, TestConnectionFunc: func(context.Context) (processor.ConnectionStatus, error) { return clients.status, clients.statusErr }, diff --git a/server/core/internal/server/internal/datasource/query.go b/server/core/internal/server/internal/datasource/query.go index 91617149..c5d4afbb 100644 --- a/server/core/internal/server/internal/datasource/query.go +++ b/server/core/internal/server/internal/datasource/query.go @@ -6,7 +6,6 @@ import ( datasourceCore "github.com/oxynote/oxynote/server/core/internal/datasource" "github.com/oxynote/oxynote/server/core/internal/datasource/processor" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" - "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" ) @@ -37,103 +36,22 @@ func (h *Handler) QueryDataSource(w http.ResponseWriter, r *http.Request) { } ct := processor.ChartType(r.URL.Query().Get("chartType")) - if !ct.IsValid() { - httpserver.RespondError(h.log, w, errutil.New(http.StatusBadRequest, "chart_type.invalid", "Invalid chart type. Must be one of: line, bar, gauge.")) + if err = ct.Validate(); err != nil { + httpserver.RespondError(h.log, w, err) return } - switch ds.Type { - case datasourceCore.TypePrometheus: - h.queryPrometheusGeneric(w, r, ds, query, ct) - case datasourceCore.TypePostgreSQL: - h.queryPostgreSQLGeneric(w, r, ds, query, ct) - case datasourceCore.TypeMariaDB, datasourceCore.TypeMySQL: - h.queryMySQLGeneric(w, r, ds, query, ct) - default: - httpserver.RespondError(h.log, w, errutil.New(http.StatusBadRequest, "data_source.type_not_supported", "Generic query is not supported for this data source type.")) - } -} - -// queryPrometheusGeneric executes a Prometheus query and returns a unified QueryResult. -func (h *Handler) queryPrometheusGeneric(w http.ResponseWriter, r *http.Request, ds *datasourceCore.DataSource, query string, ct processor.ChartType) { tr, err := processor.ParseTimeRange(false, r.URL.Query()) if err != nil { httpserver.RespondError(h.log, w, err) return } - client, err := h.runners.Runner(*ds).Prometheus(r.Context()) + res, err := datasourceCore.Query(r.Context(), h.runners.Runner(*ds), query, *tr, ct) if err != nil { httpserver.RespondError(h.log, w, err) return } - result, err := client.QueryRange(r.Context(), query, *tr) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if result == nil { - httpserver.Respond(h.log, w, &processor.QueryResult{Status: processor.QueryStatusNoData}, http.StatusOK) - return - } - - httpserver.Respond(h.log, w, result.Transform(ct), http.StatusOK) -} - -// queryMySQLGeneric executes a MySQL query and returns a unified QueryResult. -func (h *Handler) queryMySQLGeneric(w http.ResponseWriter, r *http.Request, ds *datasourceCore.DataSource, query string, ct processor.ChartType) { - tr, err := processor.ParseTimeRange(false, r.URL.Query()) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - client, err := h.runners.Runner(*ds).MySQL(r.Context()) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - result, err := client.Query(r.Context(), query, *tr) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if result == nil { - httpserver.Respond(h.log, w, &processor.QueryResult{Status: processor.QueryStatusNoData}, http.StatusOK) - return - } - - httpserver.Respond(h.log, w, result.Transform(ct), http.StatusOK) -} - -// queryPostgreSQLGeneric executes a PostgreSQL query and returns a unified QueryResult. -func (h *Handler) queryPostgreSQLGeneric(w http.ResponseWriter, r *http.Request, ds *datasourceCore.DataSource, query string, ct processor.ChartType) { - tr, err := processor.ParseTimeRange(false, r.URL.Query()) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - client, err := h.runners.Runner(*ds).PostgreSQL(r.Context()) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - result, err := client.Query(r.Context(), query, *tr) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if result == nil { - httpserver.Respond(h.log, w, &processor.QueryResult{Status: processor.QueryStatusNoData}, http.StatusOK) - return - } - - httpserver.Respond(h.log, w, result.Transform(ct), http.StatusOK) + httpserver.Respond(h.log, w, res, http.StatusOK) } diff --git a/server/core/internal/server/internal/datasource/query_test.go b/server/core/internal/server/internal/datasource/query_test.go index 1f7c3739..49b7b090 100644 --- a/server/core/internal/server/internal/datasource/query_test.go +++ b/server/core/internal/server/internal/datasource/query_test.go @@ -119,6 +119,15 @@ func Test_Handler_QueryDataSource(t *testing.T) { wasExecutorQueryCalled(0, 0, 0), ), }, + "Invalid time range": { + DB: stubDB(stubDataSource(datasourceCore.TypePrometheus), nil), + Target: "http://test.com/?q=up&chartType=line_chart&from=bogus", + ID: _testID.String(), + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"from.invalid","message":"From parameter must be a valid RFC3339 timestamp."}`), + wasExecutorQueryCalled(0, 0, 0), + ), + }, "Unsupported data source type": { DB: stubDB(stubDataSource(datasourceCore.Type("bogus")), nil), Target: _testGenericQueryTarget, @@ -128,6 +137,46 @@ func Test_Handler_QueryDataSource(t *testing.T) { wasExecutorQueryCalled(0, 0, 0), ), }, + "Error returned by the query": { + DB: stubDB(stubDataSource(datasourceCore.TypePrometheus), nil), + Clients: stubPrometheusQueryClients("", nil, assert.AnError), + Target: _testGenericQueryTarget, + ID: _testID.String(), + Checks: checks( + hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), + wasExecutorQueryCalled(1, 0, 0), + ), + }, + "A connection the data source refused": { + DB: stubDB(stubDataSource(datasourceCore.TypePrometheus), nil), + Clients: stubPrometheusQueryClients(processor.ConnectionStatusUnreachable, nil, nil), + Target: _testGenericQueryTarget, + ID: _testID.String(), + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"data_source.unreachable","message":"The data source is unreachable."}`), + wasExecutorQueryCalled(0, 0, 0), + ), + }, + "Successful query": { + DB: stubDB(stubDataSource(datasourceCore.TypePrometheus), nil), + Clients: stubPrometheusQueryClients(processor.ConnectionStatusSuccess, &processor.PrometheusQueryResult{ + Type: model.ValMatrix, + Result: model.Matrix{ + &model.SampleStream{ + Metric: model.Metric{"job": "a"}, + Values: []model.SamplePair{ + {Timestamp: model.Time(1700000000000), Value: 10}, + }, + }, + }, + }, nil), + Target: _testGenericQueryTarget, + ID: _testID.String(), + Checks: checks( + hasResp(http.StatusOK, `{"status":"ok","data":[{"labels":{"job":"a"},"metrics":[[1700000000,10]]}]}`), + wasExecutorQueryCalled(1, 0, 0), + ), + }, "Prometheus dispatch": { DB: stubDB(stubDataSource(datasourceCore.TypePrometheus), nil), Clients: stubPrometheusQueryClients(processor.ConnectionStatusSuccess, nil, nil), @@ -187,214 +236,3 @@ func Test_Handler_QueryDataSource(t *testing.T) { }) } } - -func Test_Handler_queryPrometheusGeneric(t *testing.T) { - cc := map[string]struct { - DB *DBMock - Clients *clientMocks - Target string - Checks []check - }{ - "Invalid time range": { - Target: "http://test.com/?from=bogus", - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"from.invalid","message":"From parameter must be a valid RFC3339 timestamp."}`), - ), - }, - "Error returned by executor.PrometheusQuery": { - Clients: stubPrometheusQueryClients("", nil, assert.AnError), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "A connection the data source refused": { - Clients: stubPrometheusQueryClients(processor.ConnectionStatusUnreachable, nil, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"data_source.unreachable","message":"The data source is unreachable."}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "Nil result returns no-data": { - Clients: stubPrometheusQueryClients(processor.ConnectionStatusSuccess, nil, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusOK, `{"status":"no-data"}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "Successful query": { - Clients: stubPrometheusQueryClients(processor.ConnectionStatusSuccess, &processor.PrometheusQueryResult{ - Type: model.ValMatrix, - Result: model.Matrix{ - &model.SampleStream{ - Metric: model.Metric{"job": "a"}, - Values: []model.SamplePair{ - {Timestamp: model.Time(1700000000000), Value: 10}, - }, - }, - }, - }, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusOK, `{"status":"ok","data":[{"labels":{"job":"a"},"metrics":[[1700000000,10]]}]}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - hdl, db, exec, logs := prepHandler(c.DB, c.Clients) - - req := prepRequest("GET", c.Target, "", true, "") - rec := httptest.NewRecorder() - - hdl.queryPrometheusGeneric(rec, req, stubDataSource(datasourceCore.TypePrometheus), "up", processor.ChartTypeLine) - - for _, ch := range c.Checks { - ch(t, db, exec, rec, logs) - } - }) - } -} - -func Test_Handler_queryMySQLGeneric(t *testing.T) { - cc := map[string]struct { - DB *DBMock - Clients *clientMocks - Target string - Checks []check - }{ - "Invalid time range": { - Target: "http://test.com/?from=bogus", - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"from.invalid","message":"From parameter must be a valid RFC3339 timestamp."}`), - ), - }, - "Error returned by executor.MySQLQuery": { - Clients: stubMySQLQueryClients("", nil, assert.AnError), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "A connection the data source refused": { - Clients: stubMySQLQueryClients(processor.ConnectionStatusUnreachable, nil, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"data_source.unreachable","message":"The data source is unreachable."}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "Nil result returns no-data": { - Clients: stubMySQLQueryClients(processor.ConnectionStatusSuccess, nil, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusOK, `{"status":"no-data"}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "Successful query": { - Clients: stubMySQLQueryClients(processor.ConnectionStatusSuccess, &processor.MySQLQueryResult{ - Columns: []string{"time", "value"}, - Rows: [][]any{{int64(1700000000), float64(10)}}, - }, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusOK, `{"status":"ok","data":[{"labels":{},"metrics":[[1700000000,10]]}]}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - hdl, db, exec, logs := prepHandler(c.DB, c.Clients) - - req := prepRequest("GET", c.Target, "", true, "") - rec := httptest.NewRecorder() - - hdl.queryMySQLGeneric(rec, req, stubDataSource(datasourceCore.TypeMySQL), "SELECT 1", processor.ChartTypeLine) - - for _, ch := range c.Checks { - ch(t, db, exec, rec, logs) - } - }) - } -} - -func Test_Handler_queryPostgreSQLGeneric(t *testing.T) { - cc := map[string]struct { - DB *DBMock - Clients *clientMocks - Target string - Checks []check - }{ - "Invalid time range": { - Target: "http://test.com/?from=bogus", - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"from.invalid","message":"From parameter must be a valid RFC3339 timestamp."}`), - ), - }, - "Error returned by executor.PostgreSQLQuery": { - Clients: stubPostgreSQLQueryClients("", nil, assert.AnError), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "A connection the data source refused": { - Clients: stubPostgreSQLQueryClients(processor.ConnectionStatusUnreachable, nil, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"data_source.unreachable","message":"The data source is unreachable."}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "Nil result returns no-data": { - Clients: stubPostgreSQLQueryClients(processor.ConnectionStatusSuccess, nil, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusOK, `{"status":"no-data"}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - "Successful query": { - Clients: stubPostgreSQLQueryClients(processor.ConnectionStatusSuccess, &processor.PostgreSQLQueryResult{ - Columns: []string{"time", "value"}, - Rows: [][]any{{1700000000.0, 10.0}}, - }, nil), - Target: _testQueryTarget, - Checks: checks( - hasResp(http.StatusOK, `{"status":"ok","data":[{"labels":{},"metrics":[[1700000000,10]]}]}`), - wasDBUpdateDataSourceCalled(0), - ), - }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - hdl, db, exec, logs := prepHandler(c.DB, c.Clients) - - req := prepRequest("GET", c.Target, "", true, "") - rec := httptest.NewRecorder() - - hdl.queryPostgreSQLGeneric(rec, req, stubDataSource(datasourceCore.TypePostgreSQL), "SELECT 1", processor.ChartTypeLine) - - for _, ch := range c.Checks { - ch(t, db, exec, rec, logs) - } - }) - } -} diff --git a/server/core/internal/server/internal/document/branch_http.go b/server/core/internal/server/internal/document/branch_http.go index 7654c43c..ae425122 100644 --- a/server/core/internal/server/internal/document/branch_http.go +++ b/server/core/internal/server/internal/document/branch_http.go @@ -5,7 +5,6 @@ import ( "errors" "log/slog" "net/http" - "slices" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/webchange" @@ -14,7 +13,6 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" - "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/oxynote/server/core/pkg/logutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" @@ -61,10 +59,8 @@ func (h *Handler) UpdateDocumentBranch(w http.ResponseWriter, r *http.Request) { return } - // the main branch is found by name in the tree and content queries, and - // a rename would make the document look nameless and contentless. - if doc.Default && ui.Name.Valid && ui.Name.String != doc.BranchName { - httpserver.RespondError(h.log, w, ErrDefaultBranchRename) + if err := doc.AllowsBranchRename(ui.Name); err != nil { + httpserver.RespondError(h.log, w, err) return } @@ -233,17 +229,7 @@ func (h *Handler) UpdateDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http. return } - // the update carries the editors of one persist, so the set is only - // ever added to. Diffing it against the stored maintainers would drop - // everyone who happens not to be editing right now. - var maintainersAdded bool - - for _, maintainer := range ui.Maintainers { - if !slices.Contains(maintainers, maintainer) { - maintainersAdded = true - break - } - } + maintainersAdded := ui.AddsMaintainers(maintainers) if maintainersAdded { if err = tx.UpsertDocumentMaintainers( @@ -312,11 +298,8 @@ func (h *Handler) MergeBranches(w http.ResponseWriter, r *http.Request) { return } - // a self-merge would soft-delete the branch's hooks and comments and - // then copy the hooks back from the same, now hook-less, branch — - // permanently destroying both. - if inp.FromBranchID == inp.ToBranchID { - httpserver.RespondError(h.log, w, errutil.New(http.StatusBadRequest, "document.branch_self_merge", "cannot merge a branch into itself")) + if err = documentCore.AllowsMerge(inp.FromBranchID, inp.ToBranchID); err != nil { + httpserver.RespondError(h.log, w, err) return } @@ -568,8 +551,8 @@ func (h *Handler) DeleteDocumentBranch(w http.ResponseWriter, r *http.Request) { return } - if branchDoc.Default { - httpserver.RespondError(h.log, w, errutil.New(http.StatusConflict, "document.default_branch", "cannot delete the default branch")) + if err = branchDoc.AllowsBranchDelete(); err != nil { + httpserver.RespondError(h.log, w, err) return } @@ -580,7 +563,7 @@ func (h *Handler) DeleteDocumentBranch(w http.ResponseWriter, r *http.Request) { } if count <= 1 { - httpserver.RespondError(h.log, w, errutil.New(http.StatusConflict, "document.last_branch", "cannot delete the last branch")) + httpserver.RespondError(h.log, w, documentCore.ErrLastBranchDelete) return } diff --git a/server/core/internal/server/internal/document/comment/http.go b/server/core/internal/server/internal/document/comment/http.go index 5d2a76b7..386be892 100644 --- a/server/core/internal/server/internal/document/comment/http.go +++ b/server/core/internal/server/internal/document/comment/http.go @@ -5,7 +5,6 @@ import ( "context" "log/slog" "net/http" - "slices" "github.com/oxynote/oxynote/server/core/internal/document" commentCore "github.com/oxynote/oxynote/server/core/internal/document/comment" @@ -124,17 +123,7 @@ func (h *Handler) CreateDocumentComment(w http.ResponseWriter, r *http.Request) maintainers = nil } - var userIDs []string - - for _, userID := range maintainers { - if userID == session.UserID { - continue - } - - if !slices.Contains(userIDs, userID) { - userIDs = append(userIDs, userID) - } - } + userIDs := commentCore.Recipients(maintainers, session.UserID) if len(userIDs) > 0 { h.notifPub.PublishNotifications( @@ -219,22 +208,6 @@ func (h *Handler) CreateDocumentCommentReply(w http.ResponseWriter, r *http.Requ }) } - var userIDs []string - - if c.UserID.Valid && !slices.Contains(userIDs, c.UserID.String) && c.UserID.String != session.UserID { - userIDs = append(userIDs, c.UserID.String) - } - - for _, r := range c.Replies { - if !r.UserID.Valid || r.UserID.String == session.UserID { - continue - } - - if !slices.Contains(userIDs, r.UserID.String) { - userIDs = append(userIDs, r.UserID.String) - } - } - h.notifPub.PublishNotifications( session.ActiveOrganizationID, notification.NewDocumentNewCommentReplyNotification( @@ -245,7 +218,7 @@ func (h *Handler) CreateDocumentCommentReply(w http.ResponseWriter, r *http.Requ c.AnchorBlockID, c.BranchID, ), - userIDs..., + commentCore.Recipients(c.Participants(), session.UserID)..., ) httpserver.Respond( diff --git a/server/core/internal/server/internal/document/comment/ws.go b/server/core/internal/server/internal/document/comment/ws.go index 9c127aa4..c5779212 100644 --- a/server/core/internal/server/internal/document/comment/ws.go +++ b/server/core/internal/server/internal/document/comment/ws.go @@ -2,17 +2,13 @@ package comment import ( "context" - "time" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/wetsocks/wsserver" "github.com/rs/xid" ) -// _publishTimeout bounds each WebSocket publish triggered by a domain -// callback. -const _publishTimeout = 5 * time.Second - // ChangeType represents the type of comment change. type ChangeType string @@ -39,7 +35,7 @@ type ChangeMessage struct { // BindCommentsChange binds a comment change event to the given topic. func (h *Handler) BindCommentsChange(tpc wsserver.Topic) { h.comments.changeCallback = func(organizationID string, documentId xid.ID, msg ChangeMessage) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany(ctx, msg, auth.FilterOrganizationDocument(organizationID, documentId)) diff --git a/server/core/internal/server/internal/document/document_http.go b/server/core/internal/server/internal/document/document_http.go index 0009ad1f..1114459b 100644 --- a/server/core/internal/server/internal/document/document_http.go +++ b/server/core/internal/server/internal/document/document_http.go @@ -7,7 +7,6 @@ import ( "net/http" "net/url" "path" - "unicode/utf8" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/github" @@ -26,21 +25,10 @@ import ( "github.com/rs/xid" ) -// _searchQueryMaxLength caps a search query in characters. Every term -// expands to a dictionary walk inside the process, so the query size -// bounds the work one request can ask for. -const _searchQueryMaxLength = 200 - -// ErrInvalidSearchQuery is returned when the search query is invalid. -var ErrInvalidSearchQuery = errutil.New(http.StatusBadRequest, "document.invalid_search_query", "invalid search query") - // ErrBranchMismatch is returned when the requested branch does not belong to // the document identified by the request path. var ErrBranchMismatch = errutil.New(http.StatusNotFound, "document.branch_mismatch", "branch does not belong to the document") -// ErrDefaultBranchRename is returned when a rename targets the default branch. -var ErrDefaultBranchRename = errutil.New(http.StatusBadRequest, "document.default_branch_rename", "the default branch cannot be renamed") - // ErrInvalidDocumentParent is returned when a document would be moved under // itself or one of its own descendants. var ErrInvalidDocumentParent = errutil.New(http.StatusBadRequest, "document.invalid_parent", "document cannot be moved under itself or its descendant") @@ -521,18 +509,7 @@ func (h *Handler) SearchDocuments(w http.ResponseWriter, r *http.Request) { return } - q := r.URL.Query().Get("q") - if q == "" || utf8.RuneCountInString(q) > _searchQueryMaxLength { - httpserver.RespondError( - h.log, - w, - ErrInvalidSearchQuery, - ) - - return - } - - data, err := h.searcher.SearchDocuments(r.Context(), session.ActiveOrganizationID, q) + data, err := h.searcher.SearchDocuments(r.Context(), session.ActiveOrganizationID, r.URL.Query().Get("q")) if err != nil { httpserver.RespondError(h.log, w, err) return diff --git a/server/core/internal/server/internal/document/document_http_test.go b/server/core/internal/server/internal/document/document_http_test.go index 885322f8..ea39c571 100644 --- a/server/core/internal/server/internal/document/document_http_test.go +++ b/server/core/internal/server/internal/document/document_http_test.go @@ -1180,13 +1180,12 @@ func Test_Handler_SearchDocuments(t *testing.T) { Query: "?q=test", RespCode: http.StatusUnauthorized, }, - "Empty search query": { - Searcher: &SearcherMock{}, - RespCode: http.StatusBadRequest, - }, - "Overlong search query": { - Searcher: &SearcherMock{}, - Query: "?q=" + strings.Repeat("a", _searchQueryMaxLength+1), + "Invalid search query": { + Searcher: &SearcherMock{ + SearchDocumentsFunc: func(context.Context, string, string) ([]byte, error) { + return nil, search.ErrInvalidQuery + }, + }, RespCode: http.StatusBadRequest, }, "Searcher error": { diff --git a/server/core/internal/server/internal/document/files/http.go b/server/core/internal/server/internal/document/files/http.go index 576f3e99..291ac40b 100644 --- a/server/core/internal/server/internal/document/files/http.go +++ b/server/core/internal/server/internal/document/files/http.go @@ -6,9 +6,6 @@ import ( "log/slog" "net/http" "path" - "regexp" - "strings" - "unicode/utf8" "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" @@ -16,21 +13,10 @@ import ( "github.com/oxynote/oxynote/server/core/internal/storage" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" + "github.com/oxynote/oxynote/server/core/pkg/strutil" "github.com/rs/xid" ) -// _fileIDPattern matches a file id. The charset shuts out the path -// separators and dot segments that would otherwise let an id escape its -// storage folder once joined into an object key. -var _fileIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{21}$`) - -// _maxFileNameLength caps the name recorded for an upload, in runes. -const _maxFileNameLength = 255 - -// _fallbackFileName names an upload whose multipart part carried no -// usable name. -const _fallbackFileName = "file" - // Handler holds dependencies required for document file operations. type Handler struct { log *slog.Logger @@ -76,7 +62,7 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { } fileID := r.URL.Query().Get("id") - if !_fileIDPattern.MatchString(fileID) { + if !strutil.IsNanoID(fileID) { httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) return } @@ -87,7 +73,7 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { return } - policy, ok := uploadPolicy(r.URL.Query().Get("kind")) + policy, ok := storage.PolicyForKind(r.URL.Query().Get("kind")) if !ok { httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) return @@ -108,7 +94,7 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { return } - name := fileName(r.MultipartForm.File["file"][0].Filename) + name := file.CleanName(r.MultipartForm.File["file"][0].Filename) f := file.NewFile( fileID, @@ -164,57 +150,6 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { ) } -// fileID cuts the id off a "-" route segment, reporting -// whether the segment has that shape. -func fileID(ref string) (string, bool) { - if len(ref) <= document.FileIDLength || ref[document.FileIDLength] != '-' { - return "", false - } - - id := ref[:document.FileIDLength] - if !_fileIDPattern.MatchString(id) { - return "", false - } - - return id, true -} - -// uploadPolicy maps the kind of block an upload belongs to onto the -// storage policy it is admitted under, reporting whether the kind is one -// at all. -func uploadPolicy(kind string) (storage.Policy, bool) { - switch kind { - case "image": - return storage.ImagePolicy, true - case "file": - return storage.FilePolicy, true - default: - return storage.Policy{}, false - } -} - -// fileName reduces the name a multipart part carried to a bare file name: -// a browser may send a path, Windows ones with backslashes, and the part -// may carry no name at all. -func fileName(raw string) string { - name := path.Base(strings.ReplaceAll(raw, "\\", "/")) - name = strings.TrimSpace(name) - - if name == "." || name == "/" { - name = "" - } - - if utf8.RuneCountInString(name) > _maxFileNameLength { - name = string([]rune(name)[:_maxFileNameLength]) - } - - if name == "" { - return _fallbackFileName - } - - return name -} - // RetrieveDocumentFile handles the retrieval of a file from a document. // The route's last segment is "-": only the id identifies // the file, the name makes the address read as the file it is. @@ -236,7 +171,7 @@ func (h *Handler) RetrieveDocumentFile(w http.ResponseWriter, r *http.Request) { return } - id, ok := fileID(ref) + id, _, ok := document.ParseFileRef(ref) if !ok { httpserver.RespondError(h.log, w, errutil.ErrNotFound) return diff --git a/server/core/internal/server/internal/document/files/http_test.go b/server/core/internal/server/internal/document/files/http_test.go index a72fab05..5a0201c6 100644 --- a/server/core/internal/server/internal/document/files/http_test.go +++ b/server/core/internal/server/internal/document/files/http_test.go @@ -889,92 +889,3 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { }) } } - -func Test_fileID(t *testing.T) { - cc := map[string]struct { - Ref string - Result string - OK bool - }{ - "Id and name": {Ref: "f1xxxxxxxxxxxxxxxxxxx-notes.zip", Result: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, - "Name with dashes": {Ref: "f1xxxxxxxxxxxxxxxxxxx-a-b-c.zip", Result: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, - "Id with dashes": {Ref: "f1-x_-xxxxxxxxxxxxxxx-notes.zip", Result: "f1-x_-xxxxxxxxxxxxxxx", OK: true}, - "Empty name": {Ref: "f1xxxxxxxxxxxxxxxxxxx-", Result: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, - "No dash after the id": {Ref: "f1xxxxxxxxxxxxxxxxxxxxnotes.zip"}, - "Id alone": {Ref: "f1xxxxxxxxxxxxxxxxxxx"}, - "Short": {Ref: "f1-notes.zip"}, - "Id with the wrong charset": {Ref: "f1.................xx-notes.zip"}, - "Empty": {Ref: ""}, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - res, ok := fileID(c.Ref) - assert.Equal(t, c.OK, ok) - assert.Equal(t, c.Result, res) - }) - } -} - -func Test_uploadPolicy(t *testing.T) { - cc := map[string]struct { - Kind string - Result storage.Policy - OK bool - }{ - "Image kind": { - Kind: "image", - Result: storage.ImagePolicy, - OK: true, - }, - "File kind": { - Kind: "file", - Result: storage.FilePolicy, - OK: true, - }, - "Unknown kind": { - Kind: "video", - }, - "Empty kind": {}, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - res, ok := uploadPolicy(c.Kind) - assert.Equal(t, c.OK, ok) - assert.Equal(t, c.Result, res) - }) - } -} - -func Test_fileName(t *testing.T) { - cc := map[string]struct { - Raw string - Result string - }{ - "Plain name": {Raw: "notes.zip", Result: "notes.zip"}, - "Unix path": {Raw: "/home/me/notes.zip", Result: "notes.zip"}, - "Windows path": {Raw: `C:\Users\me\notes.zip`, Result: "notes.zip"}, - "Surrounding whitespace": {Raw: " notes.zip ", Result: "notes.zip"}, - "Empty name": {Raw: "", Result: "file"}, - "Whitespace only": {Raw: " ", Result: "file"}, - "Dot": {Raw: ".", Result: "file"}, - "Root": {Raw: "/", Result: "file"}, - "Non-ASCII name": {Raw: "résumé.pdf", Result: "résumé.pdf"}, - "Name over the rune cap": {Raw: strings.Repeat("é", 300), Result: strings.Repeat("é", 255)}, - "Name at the rune cap": {Raw: strings.Repeat("a", 255), Result: strings.Repeat("a", 255)}, - "Path with a long base": {Raw: "dir/" + strings.Repeat("b", 256), Result: strings.Repeat("b", 255)}, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - assert.Equal(t, c.Result, fileName(c.Raw)) - }) - } -} diff --git a/server/core/internal/server/internal/document/hook/ws.go b/server/core/internal/server/internal/document/hook/ws.go index 6fbf6d96..6bc9935b 100644 --- a/server/core/internal/server/internal/document/hook/ws.go +++ b/server/core/internal/server/internal/document/hook/ws.go @@ -2,18 +2,14 @@ package hook import ( "context" - "time" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/wetsocks/wsserver" "github.com/rs/xid" ) -// _publishTimeout bounds each WebSocket publish triggered by a domain -// callback. -const _publishTimeout = 5 * time.Second - // HooksChangeMessage represents a change in the hooks one branch of a // document carries. It names the branch so a subscriber showing another // branch of the same document can leave its own list alone. @@ -26,7 +22,7 @@ type HooksChangeMessage struct { // is scoped to one document. func (h *Handler) BindHooksChange(tpc wsserver.Topic) { h.hooks.changeCallback = func(organizationID string, documentID, branchID xid.ID) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany( diff --git a/server/core/internal/server/internal/document/ws.go b/server/core/internal/server/internal/document/ws.go index dd2d5d23..352f94e4 100644 --- a/server/core/internal/server/internal/document/ws.go +++ b/server/core/internal/server/internal/document/ws.go @@ -7,14 +7,11 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/wetsocks/wsserver" "github.com/rs/xid" ) -// _publishTimeout bounds each WebSocket publish triggered by a domain -// callback. -const _publishTimeout = 5 * time.Second - // TreeChangeMessage represents a change in the tree of a document. type TreeChangeMessage struct { // ParentID is the id of the parent document that has changed. @@ -24,7 +21,7 @@ type TreeChangeMessage struct { // BindTreeChange binds a tree change event to the given topic. func (h *Handler) BindTreeChange(tpc wsserver.Topic) { h.tree.changeCallback = func(organizationID string, parentId null.Value[xid.ID]) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany(ctx, TreeChangeMessage{ @@ -85,7 +82,7 @@ func (h *Handler) BindMaintainersChange(tpc wsserver.Topic) { // callback slot it is assigned to. func publishDocumentChange(tpc wsserver.Topic) func(organizationID string, documentID xid.ID) { return func(organizationID string, documentID xid.ID) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany(ctx, struct{}{}, auth.FilterOrganizationDocument(organizationID, documentID)) @@ -128,7 +125,7 @@ func (h *Handler) BindMetadataChange(tpc wsserver.Topic) { }) h.metadata.changeCallback = func(organizationID string, doc documentCore.Document) { - pubCtx, pubCancel := context.WithTimeout(context.Background(), _publishTimeout) + pubCtx, pubCancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer pubCancel() tpc.PublishMany( diff --git a/server/core/internal/server/internal/email/0mock_sender_test.go b/server/core/internal/server/internal/email/0mock_sender_test.go index c754267d..6f108d33 100644 --- a/server/core/internal/server/internal/email/0mock_sender_test.go +++ b/server/core/internal/server/internal/email/0mock_sender_test.go @@ -5,6 +5,8 @@ package email import ( "sync" + + emailCore "github.com/oxynote/oxynote/server/core/internal/email" ) // Ensure, that SenderMock does implement Sender. @@ -17,26 +19,8 @@ var _ Sender = &SenderMock{} // // // make and configure a mocked Sender // mockedSender := &SenderMock{ -// SendAccountExistsFunc: func(eml string, link string) { -// panic("mock out the SendAccountExists method") -// }, -// SendEmailChangeConfirmationFunc: func(eml string, link string) { -// panic("mock out the SendEmailChangeConfirmation method") -// }, -// SendEmailVerificationFunc: func(eml string, link string) { -// panic("mock out the SendEmailVerification method") -// }, -// SendOrganizationInvitationFunc: func(eml string, org string, link string) { -// panic("mock out the SendOrganizationInvitation method") -// }, -// SendPasswordResetFunc: func(eml string, link string) { -// panic("mock out the SendPasswordReset method") -// }, -// SendSignupVerificationFunc: func(eml string, link string) { -// panic("mock out the SendSignupVerification method") -// }, -// SendUserDeletionConfirmationFunc: func(eml string, link string) { -// panic("mock out the SendUserDeletionConfirmation method") +// SendFunc: func(tmpl emailCore.Template, d emailCore.Data) error { +// panic("mock out the Send method") // }, // } // @@ -45,342 +29,57 @@ var _ Sender = &SenderMock{} // // } type SenderMock struct { - // SendAccountExistsFunc mocks the SendAccountExists method. - SendAccountExistsFunc func(eml string, link string) - - // SendEmailChangeConfirmationFunc mocks the SendEmailChangeConfirmation method. - SendEmailChangeConfirmationFunc func(eml string, link string) - - // SendEmailVerificationFunc mocks the SendEmailVerification method. - SendEmailVerificationFunc func(eml string, link string) - - // SendOrganizationInvitationFunc mocks the SendOrganizationInvitation method. - SendOrganizationInvitationFunc func(eml string, org string, link string) - - // SendPasswordResetFunc mocks the SendPasswordReset method. - SendPasswordResetFunc func(eml string, link string) - - // SendSignupVerificationFunc mocks the SendSignupVerification method. - SendSignupVerificationFunc func(eml string, link string) - - // SendUserDeletionConfirmationFunc mocks the SendUserDeletionConfirmation method. - SendUserDeletionConfirmationFunc func(eml string, link string) + // SendFunc mocks the Send method. + SendFunc func(tmpl emailCore.Template, d emailCore.Data) error // calls tracks calls to the methods. calls struct { - // SendAccountExists holds details about calls to the SendAccountExists method. - SendAccountExists []struct { - // Eml is the eml argument value. - Eml string - // Link is the link argument value. - Link string - } - // SendEmailChangeConfirmation holds details about calls to the SendEmailChangeConfirmation method. - SendEmailChangeConfirmation []struct { - // Eml is the eml argument value. - Eml string - // Link is the link argument value. - Link string - } - // SendEmailVerification holds details about calls to the SendEmailVerification method. - SendEmailVerification []struct { - // Eml is the eml argument value. - Eml string - // Link is the link argument value. - Link string - } - // SendOrganizationInvitation holds details about calls to the SendOrganizationInvitation method. - SendOrganizationInvitation []struct { - // Eml is the eml argument value. - Eml string - // Org is the org argument value. - Org string - // Link is the link argument value. - Link string - } - // SendPasswordReset holds details about calls to the SendPasswordReset method. - SendPasswordReset []struct { - // Eml is the eml argument value. - Eml string - // Link is the link argument value. - Link string - } - // SendSignupVerification holds details about calls to the SendSignupVerification method. - SendSignupVerification []struct { - // Eml is the eml argument value. - Eml string - // Link is the link argument value. - Link string - } - // SendUserDeletionConfirmation holds details about calls to the SendUserDeletionConfirmation method. - SendUserDeletionConfirmation []struct { - // Eml is the eml argument value. - Eml string - // Link is the link argument value. - Link string + // Send holds details about calls to the Send method. + Send []struct { + // Tmpl is the tmpl argument value. + Tmpl emailCore.Template + // D is the d argument value. + D emailCore.Data } } - lockSendAccountExists sync.RWMutex - lockSendEmailChangeConfirmation sync.RWMutex - lockSendEmailVerification sync.RWMutex - lockSendOrganizationInvitation sync.RWMutex - lockSendPasswordReset sync.RWMutex - lockSendSignupVerification sync.RWMutex - lockSendUserDeletionConfirmation sync.RWMutex + lockSend sync.RWMutex } -// SendAccountExists calls SendAccountExistsFunc. -func (mock *SenderMock) SendAccountExists(eml string, link string) { +// Send calls SendFunc. +func (mock *SenderMock) Send(tmpl emailCore.Template, d emailCore.Data) error { callInfo := struct { - Eml string - Link string + Tmpl emailCore.Template + D emailCore.Data }{ - Eml: eml, - Link: link, + Tmpl: tmpl, + D: d, } - mock.lockSendAccountExists.Lock() - mock.calls.SendAccountExists = append(mock.calls.SendAccountExists, callInfo) - mock.lockSendAccountExists.Unlock() - if mock.SendAccountExistsFunc == nil { - return + mock.lockSend.Lock() + mock.calls.Send = append(mock.calls.Send, callInfo) + mock.lockSend.Unlock() + if mock.SendFunc == nil { + var ( + errOut error + ) + return errOut } - mock.SendAccountExistsFunc(eml, link) + return mock.SendFunc(tmpl, d) } -// SendAccountExistsCalls gets all the calls that were made to SendAccountExists. +// SendCalls gets all the calls that were made to Send. // Check the length with: // -// len(mockedSender.SendAccountExistsCalls()) -func (mock *SenderMock) SendAccountExistsCalls() []struct { - Eml string - Link string +// len(mockedSender.SendCalls()) +func (mock *SenderMock) SendCalls() []struct { + Tmpl emailCore.Template + D emailCore.Data } { var calls []struct { - Eml string - Link string + Tmpl emailCore.Template + D emailCore.Data } - mock.lockSendAccountExists.RLock() - calls = mock.calls.SendAccountExists - mock.lockSendAccountExists.RUnlock() - return calls -} - -// SendEmailChangeConfirmation calls SendEmailChangeConfirmationFunc. -func (mock *SenderMock) SendEmailChangeConfirmation(eml string, link string) { - callInfo := struct { - Eml string - Link string - }{ - Eml: eml, - Link: link, - } - mock.lockSendEmailChangeConfirmation.Lock() - mock.calls.SendEmailChangeConfirmation = append(mock.calls.SendEmailChangeConfirmation, callInfo) - mock.lockSendEmailChangeConfirmation.Unlock() - if mock.SendEmailChangeConfirmationFunc == nil { - return - } - mock.SendEmailChangeConfirmationFunc(eml, link) -} - -// SendEmailChangeConfirmationCalls gets all the calls that were made to SendEmailChangeConfirmation. -// Check the length with: -// -// len(mockedSender.SendEmailChangeConfirmationCalls()) -func (mock *SenderMock) SendEmailChangeConfirmationCalls() []struct { - Eml string - Link string -} { - var calls []struct { - Eml string - Link string - } - mock.lockSendEmailChangeConfirmation.RLock() - calls = mock.calls.SendEmailChangeConfirmation - mock.lockSendEmailChangeConfirmation.RUnlock() - return calls -} - -// SendEmailVerification calls SendEmailVerificationFunc. -func (mock *SenderMock) SendEmailVerification(eml string, link string) { - callInfo := struct { - Eml string - Link string - }{ - Eml: eml, - Link: link, - } - mock.lockSendEmailVerification.Lock() - mock.calls.SendEmailVerification = append(mock.calls.SendEmailVerification, callInfo) - mock.lockSendEmailVerification.Unlock() - if mock.SendEmailVerificationFunc == nil { - return - } - mock.SendEmailVerificationFunc(eml, link) -} - -// SendEmailVerificationCalls gets all the calls that were made to SendEmailVerification. -// Check the length with: -// -// len(mockedSender.SendEmailVerificationCalls()) -func (mock *SenderMock) SendEmailVerificationCalls() []struct { - Eml string - Link string -} { - var calls []struct { - Eml string - Link string - } - mock.lockSendEmailVerification.RLock() - calls = mock.calls.SendEmailVerification - mock.lockSendEmailVerification.RUnlock() - return calls -} - -// SendOrganizationInvitation calls SendOrganizationInvitationFunc. -func (mock *SenderMock) SendOrganizationInvitation(eml string, org string, link string) { - callInfo := struct { - Eml string - Org string - Link string - }{ - Eml: eml, - Org: org, - Link: link, - } - mock.lockSendOrganizationInvitation.Lock() - mock.calls.SendOrganizationInvitation = append(mock.calls.SendOrganizationInvitation, callInfo) - mock.lockSendOrganizationInvitation.Unlock() - if mock.SendOrganizationInvitationFunc == nil { - return - } - mock.SendOrganizationInvitationFunc(eml, org, link) -} - -// SendOrganizationInvitationCalls gets all the calls that were made to SendOrganizationInvitation. -// Check the length with: -// -// len(mockedSender.SendOrganizationInvitationCalls()) -func (mock *SenderMock) SendOrganizationInvitationCalls() []struct { - Eml string - Org string - Link string -} { - var calls []struct { - Eml string - Org string - Link string - } - mock.lockSendOrganizationInvitation.RLock() - calls = mock.calls.SendOrganizationInvitation - mock.lockSendOrganizationInvitation.RUnlock() - return calls -} - -// SendPasswordReset calls SendPasswordResetFunc. -func (mock *SenderMock) SendPasswordReset(eml string, link string) { - callInfo := struct { - Eml string - Link string - }{ - Eml: eml, - Link: link, - } - mock.lockSendPasswordReset.Lock() - mock.calls.SendPasswordReset = append(mock.calls.SendPasswordReset, callInfo) - mock.lockSendPasswordReset.Unlock() - if mock.SendPasswordResetFunc == nil { - return - } - mock.SendPasswordResetFunc(eml, link) -} - -// SendPasswordResetCalls gets all the calls that were made to SendPasswordReset. -// Check the length with: -// -// len(mockedSender.SendPasswordResetCalls()) -func (mock *SenderMock) SendPasswordResetCalls() []struct { - Eml string - Link string -} { - var calls []struct { - Eml string - Link string - } - mock.lockSendPasswordReset.RLock() - calls = mock.calls.SendPasswordReset - mock.lockSendPasswordReset.RUnlock() - return calls -} - -// SendSignupVerification calls SendSignupVerificationFunc. -func (mock *SenderMock) SendSignupVerification(eml string, link string) { - callInfo := struct { - Eml string - Link string - }{ - Eml: eml, - Link: link, - } - mock.lockSendSignupVerification.Lock() - mock.calls.SendSignupVerification = append(mock.calls.SendSignupVerification, callInfo) - mock.lockSendSignupVerification.Unlock() - if mock.SendSignupVerificationFunc == nil { - return - } - mock.SendSignupVerificationFunc(eml, link) -} - -// SendSignupVerificationCalls gets all the calls that were made to SendSignupVerification. -// Check the length with: -// -// len(mockedSender.SendSignupVerificationCalls()) -func (mock *SenderMock) SendSignupVerificationCalls() []struct { - Eml string - Link string -} { - var calls []struct { - Eml string - Link string - } - mock.lockSendSignupVerification.RLock() - calls = mock.calls.SendSignupVerification - mock.lockSendSignupVerification.RUnlock() - return calls -} - -// SendUserDeletionConfirmation calls SendUserDeletionConfirmationFunc. -func (mock *SenderMock) SendUserDeletionConfirmation(eml string, link string) { - callInfo := struct { - Eml string - Link string - }{ - Eml: eml, - Link: link, - } - mock.lockSendUserDeletionConfirmation.Lock() - mock.calls.SendUserDeletionConfirmation = append(mock.calls.SendUserDeletionConfirmation, callInfo) - mock.lockSendUserDeletionConfirmation.Unlock() - if mock.SendUserDeletionConfirmationFunc == nil { - return - } - mock.SendUserDeletionConfirmationFunc(eml, link) -} - -// SendUserDeletionConfirmationCalls gets all the calls that were made to SendUserDeletionConfirmation. -// Check the length with: -// -// len(mockedSender.SendUserDeletionConfirmationCalls()) -func (mock *SenderMock) SendUserDeletionConfirmationCalls() []struct { - Eml string - Link string -} { - var calls []struct { - Eml string - Link string - } - mock.lockSendUserDeletionConfirmation.RLock() - calls = mock.calls.SendUserDeletionConfirmation - mock.lockSendUserDeletionConfirmation.RUnlock() + mock.lockSend.RLock() + calls = mock.calls.Send + mock.lockSend.RUnlock() return calls } diff --git a/server/core/internal/server/internal/email/http.go b/server/core/internal/server/internal/email/http.go index 63e1711c..7b0cbbef 100644 --- a/server/core/internal/server/internal/email/http.go +++ b/server/core/internal/server/internal/email/http.go @@ -6,14 +6,9 @@ import ( "net/http" emailCore "github.com/oxynote/oxynote/server/core/internal/email" - "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" ) -// ErrInvalidTemplate is returned when the requested email template is -// not recognized. -var ErrInvalidTemplate = errutil.New(http.StatusBadRequest, "email.invalid_template", "Invalid email template.") - // Handler holds dependencies required for email operations. type Handler struct { log *slog.Logger @@ -35,11 +30,7 @@ func NewHandler( func (h *Handler) SendEmail(w http.ResponseWriter, r *http.Request) { var req struct { Template emailCore.Template `json:"template"` - Data struct { - Email string `json:"email"` - Organization string `json:"organization"` - Link string `json:"link"` - } `json:"data"` + Data emailCore.Data `json:"data"` } if err := httpserver.DecodeJSON(r, &req); err != nil { @@ -47,23 +38,8 @@ func (h *Handler) SendEmail(w http.ResponseWriter, r *http.Request) { return } - switch req.Template { - case emailCore.TemplateEmailVerification: - h.sender.SendEmailVerification(req.Data.Email, req.Data.Link) - case emailCore.TemplateEmailChangeConfirmation: - h.sender.SendEmailChangeConfirmation(req.Data.Email, req.Data.Link) - case emailCore.TemplatePasswordReset: - h.sender.SendPasswordReset(req.Data.Email, req.Data.Link) - case emailCore.TemplateAccountExists: - h.sender.SendAccountExists(req.Data.Email, req.Data.Link) - case emailCore.TemplateSignupVerification: - h.sender.SendSignupVerification(req.Data.Email, req.Data.Link) - case emailCore.TemplateOrganizationInvitation: - h.sender.SendOrganizationInvitation(req.Data.Email, req.Data.Organization, req.Data.Link) - case emailCore.TemplateUserDeletion: - h.sender.SendUserDeletionConfirmation(req.Data.Email, req.Data.Link) - default: - httpserver.RespondError(h.log, w, ErrInvalidTemplate) + if err := h.sender.Send(req.Template, req.Data); err != nil { + httpserver.RespondError(h.log, w, err) return } @@ -74,30 +50,8 @@ func (h *Handler) SendEmail(w http.ResponseWriter, r *http.Request) { // //go:generate ../../../../scripts/codegen/mock -t internal Sender type Sender interface { - // SendEmailVerification should send a new-email-address - // verification email. - SendEmailVerification(eml, link string) - - // SendEmailChangeConfirmation should send the email asking the - // current address to approve a change of address. - SendEmailChangeConfirmation(eml, link string) - - // SendPasswordReset should send a password reset email. - SendPasswordReset(eml, link string) - - // SendAccountExists should send an account-exists notification - // email. - SendAccountExists(eml, link string) - - // SendSignupVerification should send the account-activation email - // for a fresh signup. - SendSignupVerification(eml, link string) - - // SendOrganizationInvitation should send an organization invitation - // email. - SendOrganizationInvitation(eml, org, link string) - - // SendUserDeletionConfirmation should send a user deletion - // confirmation email. - SendUserDeletionConfirmation(eml, link string) + // Send should send the template to the recipient in the data, + // rendered with the values it carries, reporting only a template + // that cannot be sent. + Send(tmpl emailCore.Template, d emailCore.Data) error } diff --git a/server/core/internal/server/internal/email/http_test.go b/server/core/internal/server/internal/email/http_test.go index 556eff8f..c859955b 100644 --- a/server/core/internal/server/internal/email/http_test.go +++ b/server/core/internal/server/internal/email/http_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + emailCore "github.com/oxynote/oxynote/server/core/internal/email" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" @@ -45,189 +46,55 @@ func Test_Handler_SendEmail(t *testing.T) { } } - wasNoSendCalled := func() check { + wasSendCalled := func(count int, tmpl emailCore.Template, d emailCore.Data) check { return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - assert.Empty(t, sender.SendEmailVerificationCalls()) - assert.Empty(t, sender.SendEmailChangeConfirmationCalls()) - assert.Empty(t, sender.SendPasswordResetCalls()) - assert.Empty(t, sender.SendAccountExistsCalls()) - assert.Empty(t, sender.SendSignupVerificationCalls()) - assert.Empty(t, sender.SendOrganizationInvitationCalls()) - assert.Empty(t, sender.SendUserDeletionConfirmationCalls()) - } - } - - wasSendEmailVerificationCalled := func(count int, eml, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendEmailVerificationCalls() + ff := sender.SendCalls() require.Len(t, ff, count) if count == 0 { return } - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, link, ff[0].Link) - } - } - - wasSendEmailChangeConfirmationCalled := func(count int, eml, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendEmailChangeConfirmationCalls() - require.Len(t, ff, count) - - if count == 0 { - return - } - - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, link, ff[0].Link) - } - } - - wasSendPasswordResetCalled := func(count int, eml, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendPasswordResetCalls() - require.Len(t, ff, count) - - if count == 0 { - return - } - - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, link, ff[0].Link) - } - } - - wasSendAccountExistsCalled := func(count int, eml, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendAccountExistsCalls() - require.Len(t, ff, count) - - if count == 0 { - return - } - - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, link, ff[0].Link) - } - } - - wasSendSignupVerificationCalled := func(count int, eml, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendSignupVerificationCalls() - require.Len(t, ff, count) - - if count == 0 { - return - } - - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, link, ff[0].Link) - } - } - - wasSendOrganizationInvitationCalled := func(count int, eml, org, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendOrganizationInvitationCalls() - require.Len(t, ff, count) - - if count == 0 { - return - } - - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, org, ff[0].Org) - assert.Equal(t, link, ff[0].Link) - } - } - - wasSendUserDeletionConfirmationCalled := func(count int, eml, link string) check { - return func(t *testing.T, sender *SenderMock, _ *httptest.ResponseRecorder) { - ff := sender.SendUserDeletionConfirmationCalls() - require.Len(t, ff, count) - - if count == 0 { - return - } - - assert.Equal(t, eml, ff[0].Eml) - assert.Equal(t, link, ff[0].Link) + assert.Equal(t, tmpl, ff[0].Tmpl) + assert.Equal(t, d, ff[0].D) } } cc := map[string]struct { + Sender *SenderMock Body string Checks []check }{ "Invalid JSON body": { - Body: "{", + Sender: &SenderMock{}, + Body: "{", Checks: checks( hasResp(http.StatusBadRequest, `{"code":"request.invalid_json","message":"invalid JSON body"}`), - wasNoSendCalled(), + wasSendCalled(0, "", emailCore.Data{}), ), }, - "Unknown template": { + "Error returned by Sender.Send": { + Sender: &SenderMock{ + SendFunc: func(emailCore.Template, emailCore.Data) error { + return emailCore.ErrInvalidTemplate + }, + }, Body: `{"template":"nonexistent","data":{"email":"user@example.com"}}`, Checks: checks( hasResp(http.StatusBadRequest, `{"code":"email.invalid_template","message":"Invalid email template."}`), - wasNoSendCalled(), + wasSendCalled(1, "nonexistent", emailCore.Data{Email: "user@example.com"}), ), }, - "Email verification sent": { - Body: `{"template":"email_verification","data":{"email":"user@example.com","link":"https://example.com/verify"}}`, + "Email sent": { + Sender: &SenderMock{}, + Body: `{"template":"organization_invitation","data":{"email":"user@example.com","organization":"Acme","link":"https://example.com/join"}}`, Checks: checks( hasResp(http.StatusNoContent, ""), - wasSendEmailVerificationCalled(1, "user@example.com", "https://example.com/verify"), - ), - }, - "Email change confirmation sent": { - Body: `{"template":"email_change_confirmation","data":{"email":"user@example.com","link":"https://example.com/approve"}}`, - Checks: checks( - hasResp(http.StatusNoContent, ""), - wasSendEmailChangeConfirmationCalled(1, "user@example.com", "https://example.com/approve"), - ), - }, - "Password reset sent": { - Body: `{"template":"password_reset","data":{"email":"user@example.com","link":"https://example.com/reset"}}`, - Checks: checks( - hasResp(http.StatusNoContent, ""), - wasSendPasswordResetCalled(1, "user@example.com", "https://example.com/reset"), - ), - }, - "Account exists sent": { - Body: `{"template":"account_exists","data":{"email":"user@example.com","link":"https://example.com/login"}}`, - Checks: checks( - hasResp(http.StatusNoContent, ""), - wasSendAccountExistsCalled(1, "user@example.com", "https://example.com/login"), - ), - }, - "Signup verification sent": { - Body: `{"template":"signup_verification","data":{"email":"user@example.com","link":"https://example.com/activate"}}`, - Checks: checks( - hasResp(http.StatusNoContent, ""), - wasSendSignupVerificationCalled(1, "user@example.com", "https://example.com/activate"), - ), - }, - "Organization invitation sent": { - Body: `{"template":"organization_invitation","data":{"email":"user@example.com","organization":"Acme","link":"https://example.com/join"}}`, - Checks: checks( - hasResp(http.StatusNoContent, ""), - wasSendOrganizationInvitationCalled(1, "user@example.com", "Acme", "https://example.com/join"), - ), - }, - "User deletion confirmation sent": { - Body: `{"template":"user_deletion","data":{"email":"user@example.com","link":"https://example.com/delete"}}`, - Checks: checks( - hasResp(http.StatusNoContent, ""), - wasSendUserDeletionConfirmationCalled(1, "user@example.com", "https://example.com/delete"), - ), - }, - "User creation rejected": { - Body: `{"template":"user_creation","data":{"email":"user@example.com"}}`, - Checks: checks( - hasResp(http.StatusBadRequest, `{"code":"email.invalid_template","message":"Invalid email template."}`), - wasNoSendCalled(), + wasSendCalled(1, emailCore.TemplateOrganizationInvitation, emailCore.Data{ + Email: "user@example.com", + Organization: "Acme", + Link: "https://example.com/join", + }), ), }, } @@ -236,11 +103,9 @@ func Test_Handler_SendEmail(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - sender := &SenderMock{} - hdl := Handler{ log: slog.New(slog.DiscardHandler), - sender: sender, + sender: c.Sender, } req := httptest.NewRequest("POST", "http://test.com/", strings.NewReader(c.Body)) @@ -249,7 +114,7 @@ func Test_Handler_SendEmail(t *testing.T) { hdl.SendEmail(rec, req) for _, ch := range c.Checks { - ch(t, sender, rec) + ch(t, c.Sender, rec) } }) } diff --git a/server/core/internal/server/internal/mcp/bearer.go b/server/core/internal/server/internal/mcp/bearer.go index 7a44206d..cbb1f0cf 100644 --- a/server/core/internal/server/internal/mcp/bearer.go +++ b/server/core/internal/server/internal/mcp/bearer.go @@ -9,6 +9,7 @@ import ( "time" sdkauth "github.com/modelcontextprotocol/go-sdk/auth" + "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" "github.com/oxynote/oxynote/server/core/pkg/httpserver" ) @@ -114,7 +115,7 @@ func fetchSession(ctx context.Context, client *http.Client, url, token string) ( return Session{}, err } - if session.UserID == "" || session.OrganizationID == "" { + if !auth.HasActiveOrganization(session.UserID, session.OrganizationID) { return Session{}, fmt.Errorf("%w: incomplete session payload", sdkauth.ErrInvalidToken) } diff --git a/server/core/internal/server/internal/mcp/bridge.go b/server/core/internal/server/internal/mcp/bridge.go index 21d2a174..c51969dc 100644 --- a/server/core/internal/server/internal/mcp/bridge.go +++ b/server/core/internal/server/internal/mcp/bridge.go @@ -8,31 +8,18 @@ import ( "github.com/oxynote/oxynote/server/core/internal/assistant/tools" ) -// _resourceURIPrefix addresses a document in the oxynote resource -// scheme; the id after the prefix is the document's xid. -const _resourceURIPrefix = "oxynote://documents/" - -// _resourceBranchSegment separates a document id from a branch id in a -// resource URI: oxynote://documents/{id}/branches/{branch_id} reads that -// branch, and every resource names one. -const _resourceBranchSegment = "/branches/" - // annotations declares a tool's intent so MCP clients can gate calls: -// reads are read-only, writes say whether they can destroy content. A -// write counts as destructive here when it removes content outright and -// also when it overwrites content the caller did not name — both lose -// work a client would want to confirm first, which is the question the -// hint answers. A document tool works on the organization's own -// documents, so its world is closed; a data-source tool reaches whatever -// the connection points at, which is not. +// reads are read-only, writes say whether they can destroy content, and +// every tool says whether it reaches outside the organization's own +// documents. func annotations(e tools.Entry) *mcp.ToolAnnotations { out := &mcp.ToolAnnotations{ ReadOnlyHint: !e.Write, - OpenWorldHint: new(e.DataSource), + OpenWorldHint: new(e.OpenWorld()), } if e.Write { - out.DestructiveHint = new(e.Destructive || e.Overwrites) + out.DestructiveHint = new(e.DestroysContent()) } return out @@ -63,7 +50,7 @@ func (h *Handler) toolHandler(e tools.Entry) mcp.ToolHandler { // can follow the edit straight to its target. for _, t := range res.Documents { content = append(content, &mcp.ResourceLink{ - URI: _resourceURIPrefix + t.DocumentID.String() + _resourceBranchSegment + t.BranchID.String(), + URI: resourceURI(t.DocumentID, t.BranchID), Name: t.DocumentID.String(), MIMEType: _documentMIMEType, }) diff --git a/server/core/internal/server/internal/mcp/bridge_test.go b/server/core/internal/server/internal/mcp/bridge_test.go index c3c039b0..97933379 100644 --- a/server/core/internal/server/internal/mcp/bridge_test.go +++ b/server/core/internal/server/internal/mcp/bridge_test.go @@ -36,12 +36,18 @@ func Test_annotations(t *testing.T) { cc := map[string]struct { Entry tools.Entry ReadOnly bool + OpenWorld bool Destructive *bool }{ "Read tool": { Entry: tools.Entry{}, ReadOnly: true, }, + "Data-source tool reaches an open world": { + Entry: tools.Entry{Traits: tools.Traits{DataSource: true}}, + ReadOnly: true, + OpenWorld: true, + }, "Additive write tool": { Entry: tools.Entry{Traits: tools.Traits{Write: true}}, Destructive: new(bool), @@ -71,7 +77,7 @@ func Test_annotations(t *testing.T) { assert.Equal(t, c.ReadOnly, got.ReadOnlyHint) assert.Equal(t, c.Destructive, got.DestructiveHint) require.NotNil(t, got.OpenWorldHint) - assert.False(t, *got.OpenWorldHint) + assert.Equal(t, c.OpenWorld, *got.OpenWorldHint) }) } } @@ -115,13 +121,13 @@ func Test_Handler_toolHandler(t *testing.T) { Entry: tools.Entry{Tool: &stubRunner{out: `{"ok":true}`, docs: []tools.Touched{{DocumentID: doc1, BranchID: branch1}}}, Traits: tools.Traits{Write: true}}, Args: `{"document_id":"` + doc1.String() + `"}`, Text: `{"ok":true}`, - LinkURIs: []string{_resourceURIPrefix + doc1.String() + _resourceBranchSegment + branch1.String()}, + LinkURIs: []string{resourceURI(doc1, branch1)}, }, "A call links every document it changed": { Entry: tools.Entry{Tool: &stubRunner{out: `{"ok":true}`, docs: []tools.Touched{{DocumentID: doc1, BranchID: branch1}, {DocumentID: doc2, BranchID: branch2}}}, Traits: tools.Traits{Write: true}}, Args: `{}`, Text: `{"ok":true}`, - LinkURIs: []string{_resourceURIPrefix + doc1.String() + _resourceBranchSegment + branch1.String(), _resourceURIPrefix + doc2.String() + _resourceBranchSegment + branch2.String()}, + LinkURIs: []string{resourceURI(doc1, branch1), resourceURI(doc2, branch2)}, }, "A write that changed nothing has no link to offer": { Entry: tools.Entry{Tool: &stubRunner{out: `{"ok":true}`}, Traits: tools.Traits{Write: true}}, diff --git a/server/core/internal/server/internal/mcp/resource.go b/server/core/internal/server/internal/mcp/resource.go new file mode 100644 index 00000000..ded621e7 --- /dev/null +++ b/server/core/internal/server/internal/mcp/resource.go @@ -0,0 +1,51 @@ +package mcp + +import ( + "strings" + + "github.com/rs/xid" +) + +// _resourceURIPrefix addresses a document in the oxynote resource +// scheme; the id after the prefix is the document's xid. +const _resourceURIPrefix = "oxynote://documents/" + +// _resourceBranchSegment separates a document id from a branch id in a +// resource URI: oxynote://documents/{id}/branches/{branch_id} reads that +// branch, and every resource names one. +const _resourceBranchSegment = "/branches/" + +// _resourceURITemplate is the RFC 6570 template every branch resource +// URI matches. +const _resourceURITemplate = _resourceURIPrefix + "{id}" + _resourceBranchSegment + "{branch_id}" + +// resourceURI addresses one branch of a document in the resource scheme. +func resourceURI(documentID, branchID xid.ID) string { + return _resourceURIPrefix + documentID.String() + _resourceBranchSegment + branchID.String() +} + +// parseResourceURI reads back what resourceURI wrote, reporting whether +// the URI names a document branch at all. +func parseResourceURI(uri string) (documentID, branchID xid.ID, ok bool) { + rest, ok := strings.CutPrefix(uri, _resourceURIPrefix) + if !ok { + return xid.NilID(), xid.NilID(), false + } + + rawDocumentID, rawBranchID, ok := strings.Cut(rest, _resourceBranchSegment) + if !ok { + return xid.NilID(), xid.NilID(), false + } + + documentID, err := xid.FromString(rawDocumentID) + if err != nil { + return xid.NilID(), xid.NilID(), false + } + + branchID, err = xid.FromString(rawBranchID) + if err != nil { + return xid.NilID(), xid.NilID(), false + } + + return documentID, branchID, true +} diff --git a/server/core/internal/server/internal/mcp/resource_test.go b/server/core/internal/server/internal/mcp/resource_test.go new file mode 100644 index 00000000..95747fdb --- /dev/null +++ b/server/core/internal/server/internal/mcp/resource_test.go @@ -0,0 +1,47 @@ +package mcp + +import ( + "testing" + + "github.com/rs/xid" + "github.com/stretchr/testify/assert" +) + +func Test_resourceURI(t *testing.T) { + t.Parallel() + + documentID, branchID := xid.New(), xid.New() + + assert.Equal(t, "oxynote://documents/"+documentID.String()+"/branches/"+branchID.String(), resourceURI(documentID, branchID)) +} + +func Test_parseResourceURI(t *testing.T) { + documentID, branchID := xid.New(), xid.New() + + cc := map[string]struct { + URI string + DocumentID xid.ID + BranchID xid.ID + OK bool + }{ + "Branch URI": {URI: resourceURI(documentID, branchID), DocumentID: documentID, BranchID: branchID, OK: true}, + "URI outside the scheme": {URI: "file:///etc/passwd"}, + "URI without an id": {URI: _resourceURIPrefix}, + "URI without a branch": {URI: _resourceURIPrefix + documentID.String()}, + "Branch URI without a document": {URI: _resourceURIPrefix + _resourceBranchSegment + branchID.String()}, + "Document segment that is not an id": {URI: _resourceURIPrefix + "doc" + _resourceBranchSegment + branchID.String()}, + "Branch segment that is not an id": {URI: _resourceURIPrefix + documentID.String() + _resourceBranchSegment + "draft"}, + "Empty": {}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + gotDocumentID, gotBranchID, ok := parseResourceURI(c.URI) + assert.Equal(t, c.OK, ok) + assert.Equal(t, c.DocumentID, gotDocumentID) + assert.Equal(t, c.BranchID, gotBranchID) + }) + } +} diff --git a/server/core/internal/server/internal/mcp/resources.go b/server/core/internal/server/internal/mcp/resources.go index 40d16559..f98d507d 100644 --- a/server/core/internal/server/internal/mcp/resources.go +++ b/server/core/internal/server/internal/mcp/resources.go @@ -5,12 +5,10 @@ import ( "encoding/json" "errors" "log/slog" - "strings" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/oxynote/oxynote/server/core/internal/assistant/tools" "github.com/oxynote/oxynote/server/core/pkg/errutil" - "github.com/rs/xid" ) // _documentMIMEType is the media type of a document resource body: the @@ -34,7 +32,7 @@ func (h *Handler) addResources( Title: "Oxynote document branch", Description: "One branch of an Oxynote document, by document and branch id: its metadata, branches and blocks in canonical JSON form.", MIMEType: _documentMIMEType, - URITemplate: _resourceURIPrefix + "{id}" + _resourceBranchSegment + "{branch_id}", + URITemplate: _resourceURITemplate, }, read) tree, err := h.db.FetchDocumentTree(ctx, session.OrganizationID) @@ -55,7 +53,7 @@ func (h *Handler) addResources( // reader lands on without picking one. for _, s := range tree.Descendants() { srv.AddResource(&mcp.Resource{ - URI: _resourceURIPrefix + s.ID.String() + _resourceBranchSegment + s.DefaultBranchID.String(), + URI: resourceURI(s.ID, s.DefaultBranchID), Name: s.DocumentName, MIMEType: _documentMIMEType, }, read) @@ -67,24 +65,14 @@ func (h *Handler) addResources( // output shape stay identical to a tool call. func (h *Handler) readDocument(set *tools.Set) mcp.ResourceHandler { return func(ctx context.Context, req *mcp.ReadResourceRequest) (*mcp.ReadResourceResult, error) { - rest, ok := strings.CutPrefix(req.Params.URI, _resourceURIPrefix) - if !ok || rest == "" { - return nil, mcp.ResourceNotFoundError(req.Params.URI) - } - // a resource names a document and one of its branches; anything - // short of that, or a branch segment that is not an id, names - // nothing. - id, branchID, ok := strings.Cut(rest, _resourceBranchSegment) - if !ok || id == "" || branchID == "" { + // short of that names nothing. + id, branchID, ok := parseResourceURI(req.Params.URI) + if !ok { return nil, mcp.ResourceNotFoundError(req.Params.URI) } - if _, err := xid.FromString(branchID); err != nil { - return nil, mcp.ResourceNotFoundError(req.Params.URI) - } - - raw, err := json.Marshal(map[string]string{"document_id": id, "branch_id": branchID}) + raw, err := json.Marshal(map[string]string{"document_id": id.String(), "branch_id": branchID.String()}) if err != nil { // NOCOV: two string fields always marshal. return nil, err diff --git a/server/core/internal/server/internal/mcp/resources_test.go b/server/core/internal/server/internal/mcp/resources_test.go index ccabfeca..78e08395 100644 --- a/server/core/internal/server/internal/mcp/resources_test.go +++ b/server/core/internal/server/internal/mcp/resources_test.go @@ -179,8 +179,13 @@ func Test_Handler_readDocument(t *testing.T) { ToolsDB: stubToolsDB(), Err: mcp.ResourceNotFoundError(_resourceURIPrefix + docID.String()), }, + "Document segment that is not an id": { + URI: _resourceURIPrefix + "doc" + _resourceBranchSegment + _stubDraftBranchID.String(), + ToolsDB: stubToolsDB(), + Err: mcp.ResourceNotFoundError(_resourceURIPrefix + "doc" + _resourceBranchSegment + _stubDraftBranchID.String()), + }, "Internal tool failure passes through": { - URI: _resourceURIPrefix + docID.String() + _resourceBranchSegment + _stubDraftBranchID.String(), + URI: resourceURI(docID, _stubDraftBranchID), ToolsDB: &toolsMock.DB{ FetchDocumentByBranchIDFunc: stubDraftDocument(docID), FetchDocumentBranchesFunc: func(context.Context, xid.ID, string) ([]document.BranchSummary, error) { @@ -191,12 +196,12 @@ func Test_Handler_readDocument(t *testing.T) { Passthru: true, }, "Unknown branch maps to resource not found": { - URI: _resourceURIPrefix + docID.String() + _resourceBranchSegment + unknownBranchID.String(), + URI: resourceURI(docID, unknownBranchID), ToolsDB: &toolsMock.DB{ FetchDocumentByBranchIDFunc: stubDraftDocument(docID), FetchDocumentBranchesFunc: stubBranches, }, - Err: mcp.ResourceNotFoundError(_resourceURIPrefix + docID.String() + _resourceBranchSegment + unknownBranchID.String()), + Err: mcp.ResourceNotFoundError(resourceURI(docID, unknownBranchID)), }, "Branch segment that is not an id": { URI: _resourceURIPrefix + docID.String() + _resourceBranchSegment + "draft", @@ -209,7 +214,7 @@ func Test_Handler_readDocument(t *testing.T) { Err: mcp.ResourceNotFoundError(_resourceURIPrefix + _resourceBranchSegment + _stubDraftBranchID.String()), }, "Successful read": { - URI: _resourceURIPrefix + docID.String() + _resourceBranchSegment + _stubDraftBranchID.String(), + URI: resourceURI(docID, _stubDraftBranchID), ToolsDB: &toolsMock.DB{ FetchDocumentByBranchIDFunc: stubDraftDocument(docID), FetchDocumentBranchesFunc: stubBranches, diff --git a/server/core/internal/server/internal/mcp/server.go b/server/core/internal/server/internal/mcp/server.go index 464fae71..effe7a56 100644 --- a/server/core/internal/server/internal/mcp/server.go +++ b/server/core/internal/server/internal/mcp/server.go @@ -6,9 +6,18 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/oxynote/oxynote/server/core/internal/assistant" + "github.com/oxynote/oxynote/server/core/internal/assistant/tools" "github.com/oxynote/oxynote/server/core/internal/buildinfo" ) +// _scopes is the OAuth scope each kind of tool access answers to. An +// access with no scope here is offered to no client. +var _scopes = map[tools.Access]string{ + tools.AccessRead: ScopeDocumentRead, + tools.AccessWrite: ScopeDocumentWrite, + tools.AccessDataSource: ScopeDataSourceRead, +} + // server builds the MCP server for one request's identity: the tools // the token's scopes allow, plus the organization's documents as // resources. Stateless transport means every request gets a fresh @@ -44,25 +53,12 @@ func (h *Handler) server(r *http.Request) *mcp.Server { set := h.man.ToolSet(session.OrganizationID, session.UserID) - read := slices.Contains(session.Scopes, ScopeDocumentRead) - write := slices.Contains(session.Scopes, ScopeDocumentWrite) - dataSources := slices.Contains(session.Scopes, ScopeDataSourceRead) - for _, e := range set.Entries() { - // an internal tool addresses conversation state this surface has - // none of, so offering it would only invite a call that cannot - // succeed. - if e.Internal { - continue - } - - // a data-source tool reaches outside Oxynote entirely, so it - // answers to its own scope rather than to the document ones. - if e.DataSource { - if !dataSources { - continue - } - } else if e.Write && !write || !e.Write && !read { + // a tool no scope grants is not offered: an internal one addresses + // conversation state this surface has none of, and the rest answer + // to the scope their access maps onto. + scope, ok := _scopes[e.Access()] + if !ok || !slices.Contains(session.Scopes, scope) { continue } @@ -76,7 +72,7 @@ func (h *Handler) server(r *http.Request) *mcp.Server { }, h.toolHandler(e)) } - if read { + if slices.Contains(session.Scopes, ScopeDocumentRead) { h.addResources(r.Context(), srv, session, set) } diff --git a/server/core/internal/server/internal/org/http.go b/server/core/internal/server/internal/org/http.go index 4cd9b740..12e171e9 100644 --- a/server/core/internal/server/internal/org/http.go +++ b/server/core/internal/server/internal/org/http.go @@ -3,7 +3,6 @@ package org import ( "context" - "fmt" "log/slog" "net/http" @@ -11,10 +10,9 @@ import ( "github.com/oxynote/oxynote/server/core/internal/apps/github" "github.com/oxynote/oxynote/server/core/internal/apps/webchange" "github.com/oxynote/oxynote/server/core/internal/datasource" - "github.com/oxynote/oxynote/server/core/internal/datasource/demo" - "github.com/oxynote/oxynote/server/core/internal/datasource/processor" "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/hook" + orgCore "github.com/oxynote/oxynote/server/core/internal/org" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" "github.com/oxynote/oxynote/server/core/internal/storage" @@ -22,24 +20,12 @@ import ( "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/oxynote/server/core/pkg/sqlutil" - "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" ) -// _organizationsLogoFolderFormat is the folder where organization logos are stored. -const _organizationsLogoFolderFormat = "organizations/%s/logo" - // ErrNoOrganizationMembers is returned when an organization has no members. var ErrNoOrganizationMembers = errutil.New(http.StatusBadRequest, "organization.no_members", "organization has no members") -// _seedTags are the tags a fresh organization starts with, in display -// order. The welcome document goes under the first of them. -var _seedTags = []tag.CreateInput{ - {TagName: "Production", Color: "#00a63e"}, - {TagName: "Staging", Color: "#f54a00"}, - {TagName: "Incidents", Color: "#155dfc"}, -} - // Handler holds dependencies required for organization-related operations. type Handler struct { log *slog.Logger @@ -48,10 +34,11 @@ type Handler struct { githubMan *github.Manager webchangeClient *webchange.Client searchTrigger SearchTrigger - logoLocation string + publicURL string } -// NewHandler creates a new handler instance with the provided logger and database. +// NewHandler creates a new handler instance with the provided logger and +// database. publicURL is the origin the Location of a logo is built on. func NewHandler( log *slog.Logger, db DB, @@ -59,7 +46,7 @@ func NewHandler( githubMan *github.Manager, webchangeClient *webchange.Client, searchTrigger SearchTrigger, - logoLocationFormat string, + publicURL string, ) *Handler { return &Handler{ log: log, @@ -68,7 +55,7 @@ func NewHandler( githubMan: githubMan, webchangeClient: webchangeClient, searchTrigger: searchTrigger, - logoLocation: logoLocationFormat, + publicURL: publicURL, } } @@ -95,8 +82,18 @@ func (h *Handler) InitializeOrganization(w http.ResponseWriter, r *http.Request) // failed statement aborts a Postgres transaction, so logging and // carrying on inside one would only move the failure to the commit. // The welcome document drops its charts when the source is missing - // rather than pointing them at an id that was never stored. - dataSourceID := h.insertDemoDataSource(r.Context(), id) + // rather than pointing them at an id that was never stored. The + // failure is logged rather than returned: the demo content is a + // nicety, and an organization without it is still fully initialized. + var dataSourceID null.Value[xid.ID] + + ds := datasource.NewDemoDataSource(id) + + if err = h.db.InsertDataSource(r.Context(), ds); err != nil { + h.log.Error("inserting demo data source", slog.String("error", err.Error())) + } else { + dataSourceID = null.ValueFrom(ds.ID) + } var tx Tx @@ -108,19 +105,12 @@ func (h *Handler) InitializeOrganization(w http.ResponseWriter, r *http.Request) defer tx.Rollback() //nolint:errcheck // error provides no meaningful info - doc := document.NewDocument(document.CreateInput{ - Name: "Welcome to Oxynote!", - Icon: "mingcute:flag-4-fill", - }, id, members[0]) - - content, err := document.InitialDocumentContent(dataSourceID) + doc, err := document.NewWelcomeDocument(id, members[0], dataSourceID) if err != nil { httpserver.RespondError(h.log, w, err) return } - doc.Content = content - if err = tx.InsertDocument(r.Context(), doc); err != nil { httpserver.RespondError(h.log, w, err) return @@ -133,7 +123,7 @@ func (h *Handler) InitializeOrganization(w http.ResponseWriter, r *http.Request) var productionID xid.ID - for i, inp := range _seedTags { + for i, inp := range tag.SeedTags() { t := tag.NewTag(inp, id, members[0]) if i == 0 { @@ -192,7 +182,7 @@ func (h *Handler) UploadOrganizationLogo(w http.ResponseWriter, r *http.Request) return } - logoFolder := fmt.Sprintf(_organizationsLogoFolderFormat, session.ActiveOrganizationID) + logoFolder := orgCore.LogoFolder(session.ActiveOrganizationID) err = h.storer.Upload(r.Context(), logoFolder, session.ActiveOrganizationID, data, contentType) if err != nil { @@ -200,8 +190,7 @@ func (h *Handler) UploadOrganizationLogo(w http.ResponseWriter, r *http.Request) return } - // Append a timestamp to the URL to prevent caching issues. - logoLocation := h.logoLocation + "?v=" + timeutil.Now().Format("20060102150405") + logoLocation := httpserver.CacheBust(h.publicURL + orgCore.LogoPath) err = h.db.UpdateOrganizationLogo(r.Context(), session.ActiveOrganizationID, logoLocation) if err != nil { @@ -231,7 +220,7 @@ func (h *Handler) RetrieveOrganizationLogo(w http.ResponseWriter, r *http.Reques return } - logoFolder := fmt.Sprintf(_organizationsLogoFolderFormat, session.ActiveOrganizationID) + logoFolder := orgCore.LogoFolder(session.ActiveOrganizationID) obj, found, err := h.storer.Retrieve(r.Context(), logoFolder, session.ActiveOrganizationID) if err != nil { @@ -319,7 +308,7 @@ func (h *Handler) TeardownOrganization(w http.ResponseWriter, r *http.Request) { // the logo is the organization's own object; the documents' files are // left to the file manager, which reclaims them once the cascade nulls // their foreign keys. - err = h.storer.Delete(r.Context(), fmt.Sprintf(_organizationsLogoFolderFormat, id), id) + err = h.storer.Delete(r.Context(), orgCore.LogoFolder(id), id) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -333,26 +322,6 @@ func (h *Handler) TeardownOrganization(w http.ResponseWriter, r *http.Request) { ) } -// insertDemoDataSource inserts the demo Prometheus data source and reports -// its id. A failure is logged rather than returned: the demo content is a -// nicety, and an organization without it is still fully initialized. -func (h *Handler) insertDemoDataSource(ctx context.Context, organizationID string) null.Value[xid.ID] { - ds := datasource.NewDataSource(datasource.CreateInput{ - Type: datasource.TypePrometheus, - Name: "Demo", - URL: demo.URL, - Credentials: processor.NewCredentials([]byte(`{}`)), - }, organizationID) - - if err := h.db.InsertDataSource(ctx, ds); err != nil { - h.log.Error("inserting demo data source", slog.String("error", err.Error())) - - return null.Value[xid.ID]{} - } - - return null.ValueFrom(ds.ID) -} - // extractOrganizationParameter extracts the document ID from the request parameters. func (h *Handler) extractOrganizationParameter(r *http.Request) (string, error) { return httpserver.ExtractParam(r, "organizationId") diff --git a/server/core/internal/server/internal/org/http_test.go b/server/core/internal/server/internal/org/http_test.go index b9d36932..54052c1a 100644 --- a/server/core/internal/server/internal/org/http_test.go +++ b/server/core/internal/server/internal/org/http_test.go @@ -107,7 +107,7 @@ func Test_NewHandler(t *testing.T) { assert.Same(t, githubMan, hdl.githubMan) assert.Same(t, webchangeClient, hdl.webchangeClient) assert.Same(t, searchTrigger, hdl.searchTrigger) - assert.Equal(t, "loc", hdl.logoLocation) + assert.Equal(t, "loc", hdl.publicURL) } // assertSeededTags checks that the seeded tags were inserted in their @@ -501,7 +501,7 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { } assert.Equal(t, "org1", ff[0].OrganizationID) - assert.Regexp(t, `^loc\?v=\d{14}$`, ff[0].Logo) + assert.Regexp(t, `^loc/api/organizations/logo\?v=\d{14}$`, ff[0].Logo) } } @@ -598,7 +598,7 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { assert.Equal(t, http.StatusCreated, rec.Code) assert.Zero(t, rec.Body.Len(), rec.Body.String()) - assert.Regexp(t, `^loc\?v=\d{14}$`, rec.Header().Get("Location")) + assert.Regexp(t, `^loc/api/organizations/logo\?v=\d{14}$`, rec.Header().Get("Location")) }, wasUploadCalled(1), wasUpdateLogoCalled(1), @@ -612,10 +612,10 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { t.Parallel() hdl := Handler{ - log: slog.New(slog.DiscardHandler), - db: c.DB, - storer: c.Storer, - logoLocation: "loc", + log: slog.New(slog.DiscardHandler), + db: c.DB, + storer: c.Storer, + publicURL: "loc", } var ( diff --git a/server/core/internal/server/internal/slack/ws.go b/server/core/internal/server/internal/slack/ws.go index e0aa0dd7..3f90314b 100644 --- a/server/core/internal/server/internal/slack/ws.go +++ b/server/core/internal/server/internal/slack/ws.go @@ -2,17 +2,13 @@ package slack import ( "context" - "time" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/wetsocks/wsserver" "github.com/rs/xid" ) -// _publishTimeout bounds each WebSocket publish triggered by a domain -// callback. -const _publishTimeout = 5 * time.Second - // Message represents a new message. type Message struct { // ID is the unique identifier for the message. @@ -22,7 +18,7 @@ type Message struct { // BindPostMessage binds the post message callback to a WebSocket topic. func (h *Handler) BindPostMessage(tpc wsserver.Topic) { h.message.postCallback = func(organizationID string, id xid.ID) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany(ctx, Message{ID: id}, auth.FilterOrganization(organizationID)) diff --git a/server/core/internal/server/internal/tag/ws.go b/server/core/internal/server/internal/tag/ws.go index d891c144..50916d62 100644 --- a/server/core/internal/server/internal/tag/ws.go +++ b/server/core/internal/server/internal/tag/ws.go @@ -2,17 +2,13 @@ package tag import ( "context" - "time" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/wetsocks/wsserver" "github.com/rs/xid" ) -// _publishTimeout bounds each WebSocket publish triggered by a domain -// callback. -const _publishTimeout = 5 * time.Second - // TreeChangeMessage represents a change in the tree of tags. The tree is // small and always fetched whole, so the message carries no payload: it // tells a subscriber to refetch. @@ -29,13 +25,13 @@ type BranchTagsChangeMessage struct { // BindTreeChange binds a tag tree change event to the given topic. func (h *Handler) BindTreeChange(tpc wsserver.Topic) { h.tree.changeCallback = func(organizationID string) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany(ctx, TreeChangeMessage{}, auth.FilterOrganization(organizationID)) } h.tree.userChangeCallback = func(organizationID, userID string) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany(ctx, TreeChangeMessage{}, auth.FilterUser(organizationID, userID)) @@ -46,7 +42,7 @@ func (h *Handler) BindTreeChange(tpc wsserver.Topic) { // topic, which is scoped to one document. func (h *Handler) BindBranchTagsChange(tpc wsserver.Topic) { h.branchTags.changeCallback = func(organizationID string, documentID, branchID xid.ID) { - ctx, cancel := context.WithTimeout(context.Background(), _publishTimeout) + ctx, cancel := context.WithTimeout(context.Background(), httpserver.WSPublishTimeout) defer cancel() tpc.PublishMany( diff --git a/server/core/internal/server/internal/user/http.go b/server/core/internal/server/internal/user/http.go index 07956ef0..3848d9f8 100644 --- a/server/core/internal/server/internal/user/http.go +++ b/server/core/internal/server/internal/user/http.go @@ -3,42 +3,37 @@ package user import ( "context" - "fmt" "log/slog" "net/http" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" "github.com/oxynote/oxynote/server/core/internal/storage" + userCore "github.com/oxynote/oxynote/server/core/internal/user" "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/oxynote/server/core/pkg/sqlutil" - "github.com/oxynote/oxynote/server/core/pkg/timeutil" ) -// _userImageFolder is the folder where user images are stored. It is keyed -// by user only: the avatar lives on the global user row, so an org-scoped -// object would 404 for viewers with a different active organization. -const _userImageFolder = "users/images" - // Handler holds dependencies required for user-related operations. type Handler struct { - log *slog.Logger - db DB - storer Storer - imageLocationFormat string + log *slog.Logger + db DB + storer Storer + publicURL string } -// NewHandler creates a new handler instance with the provided logger and database. +// NewHandler creates a new handler instance with the provided logger and +// database. publicURL is the origin the Location of an image is built on. func NewHandler( log *slog.Logger, db DB, storer Storer, - imageLocationFormat string, + publicURL string, ) *Handler { return &Handler{ - log: log, - db: db, - storer: storer, - imageLocationFormat: imageLocationFormat, + log: log, + db: db, + storer: storer, + publicURL: publicURL, } } @@ -54,7 +49,7 @@ func (h *Handler) RetrieveUserImage(w http.ResponseWriter, r *http.Request) { return } - obj, found, err := h.storer.Retrieve(r.Context(), _userImageFolder, userID) + obj, found, err := h.storer.Retrieve(r.Context(), userCore.ImageFolder, userID) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -97,18 +92,17 @@ func (h *Handler) UploadUserImage(w http.ResponseWriter, r *http.Request) { return } - err = h.storer.Upload(r.Context(), _userImageFolder, session.UserID, data, contentType) + err = h.storer.Upload(r.Context(), userCore.ImageFolder, session.UserID, data, contentType) if err != nil { httpserver.RespondError(h.log, w, err) return } - // Append a timestamp to the URL to prevent caching issues. - imageLocation := fmt.Sprintf(h.imageLocationFormat, session.UserID) + "?v=" + timeutil.Now().Format("20060102150405") + imageLocation := httpserver.CacheBust(h.publicURL + userCore.ImagePath(session.UserID)) err = h.db.UpdateUserImage(r.Context(), session.UserID, imageLocation) if err != nil { - derr := h.storer.Delete(r.Context(), _userImageFolder, session.UserID) + derr := h.storer.Delete(r.Context(), userCore.ImageFolder, session.UserID) if derr != nil { h.log.Error("deleting object after DB failure", slog.String("error", derr.Error())) } diff --git a/server/core/internal/server/internal/user/http_test.go b/server/core/internal/server/internal/user/http_test.go index 8ebc56b6..f0dcba42 100644 --- a/server/core/internal/server/internal/user/http_test.go +++ b/server/core/internal/server/internal/user/http_test.go @@ -60,12 +60,12 @@ func Test_NewHandler(t *testing.T) { db := &DBMock{} storer := &StorerMock{} - hdl := NewHandler(slog.New(slog.DiscardHandler), db, storer, "loc/%s") + hdl := NewHandler(slog.New(slog.DiscardHandler), db, storer, "loc") require.NotNil(t, hdl) assert.NotNil(t, hdl.log) assert.Same(t, db, hdl.db) assert.Same(t, storer, hdl.storer) - assert.Equal(t, "loc/%s", hdl.imageLocationFormat) + assert.Equal(t, "loc", hdl.publicURL) } func Test_Handler_RetrieveUserImage(t *testing.T) { @@ -264,7 +264,7 @@ func Test_Handler_UploadUserImage(t *testing.T) { } assert.Equal(t, "u1", ff[0].UserID) - assert.Regexp(t, `^loc/u1\?v=\d{14}$`, ff[0].Image) + assert.Regexp(t, `^loc/api/users/u1/image\?v=\d{14}$`, ff[0].Image) } } @@ -374,7 +374,7 @@ func Test_Handler_UploadUserImage(t *testing.T) { func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { assert.Equal(t, http.StatusCreated, rec.Code) assert.Zero(t, rec.Body.Len(), rec.Body.String()) - assert.Regexp(t, `^loc/u1\?v=\d{14}$`, rec.Header().Get("Location")) + assert.Regexp(t, `^loc/api/users/u1/image\?v=\d{14}$`, rec.Header().Get("Location")) }, wasUploadCalled(1), wasUpdateUserImageCalled(1), @@ -388,10 +388,10 @@ func Test_Handler_UploadUserImage(t *testing.T) { t.Parallel() hdl := Handler{ - log: slog.New(slog.DiscardHandler), - db: c.DB, - storer: c.Storer, - imageLocationFormat: "loc/%s", + log: slog.New(slog.DiscardHandler), + db: c.DB, + storer: c.Storer, + publicURL: "loc", } var ( diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index 985c71fe..ecc25464 100644 --- a/server/core/internal/server/router.go +++ b/server/core/internal/server/router.go @@ -15,12 +15,6 @@ import ( ) const ( - // _userImageLocationFormat is the URL format for user images. - _userImageLocationFormat = "/api/users/%s/image" - - // _organizationLogoLocation is the URL path for organization logos. - _organizationLogoLocation = "/api/organizations/logo" - // _requestTimeout bounds the processing of a single HTTP request. _requestTimeout = 30 * time.Second @@ -241,6 +235,8 @@ func (s *Server) router() chi.Router { }) }) + // the image and logo routes are mounted on user.ImagePathFormat and + // org.LogoPath: the rows store those paths. r.Route("/users", func(sr chi.Router) { sr.Put("/image", s.handlers.user.UploadUserImage) sr.Get("/{userId}/image", s.handlers.user.RetrieveUserImage) diff --git a/server/core/internal/server/server.go b/server/core/internal/server/server.go index fa8e4966..2939a705 100644 --- a/server/core/internal/server/server.go +++ b/server/core/internal/server/server.go @@ -165,7 +165,7 @@ func NewServer( }, } - srv.handlers.user = user.NewHandler(log, db, storageClient, opts.PublicURL+_userImageLocationFormat) + srv.handlers.user = user.NewHandler(log, db, storageClient, opts.PublicURL) srv.handlers.organization = org.NewHandler( log, db, @@ -173,7 +173,7 @@ func NewServer( githubMan, webchangeClient, searchTrigger, - opts.PublicURL+_organizationLogoLocation, + opts.PublicURL, ) srv.handlers.document = document.NewHandler(log, db, githubMan, webchangeClient, searcher, searchTrigger, notifier, storageClient) srv.handlers.tag = tag.NewHandler(log, db) diff --git a/server/core/internal/storage/storage.go b/server/core/internal/storage/storage.go index 0481b6bd..98b62d36 100644 --- a/server/core/internal/storage/storage.go +++ b/server/core/internal/storage/storage.go @@ -67,6 +67,19 @@ func (p Policy) MaxUploadBytes() int64 { return p.MaxSize + _uploadHeadroom } +// PolicyForKind maps the kind of block an upload belongs to onto the +// policy it is admitted under, reporting whether the kind is one at all. +func PolicyForKind(kind string) (Policy, bool) { + switch kind { + case "image": + return ImagePolicy, true + case "file": + return FilePolicy, true + default: + return Policy{}, false + } +} + // ObjectInfo contains metadata about a retrieved object. type ObjectInfo struct { // Body is the object's data stream. diff --git a/server/core/internal/storage/storage_test.go b/server/core/internal/storage/storage_test.go index 78997516..50ccdd25 100644 --- a/server/core/internal/storage/storage_test.go +++ b/server/core/internal/storage/storage_test.go @@ -32,6 +32,29 @@ func Test_Policy_MaxUploadBytes(t *testing.T) { assert.EqualValues(t, 25*1024*1024+64*1024, FilePolicy.MaxUploadBytes()) } +func Test_PolicyForKind(t *testing.T) { + cc := map[string]struct { + Kind string + Result Policy + OK bool + }{ + "Image kind": {Kind: "image", Result: ImagePolicy, OK: true}, + "File kind": {Kind: "file", Result: FilePolicy, OK: true}, + "Unknown kind": {Kind: "video"}, + "Empty kind": {}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + res, ok := PolicyForKind(c.Kind) + assert.Equal(t, c.OK, ok) + assert.Equal(t, c.Result, res) + }) + } +} + func TestMain(m *testing.M) { goleak.VerifyTestMain(m) } diff --git a/server/core/internal/tag/seed.go b/server/core/internal/tag/seed.go new file mode 100644 index 00000000..55e9e74c --- /dev/null +++ b/server/core/internal/tag/seed.go @@ -0,0 +1,11 @@ +package tag + +// SeedTags returns the tags a fresh organization starts with, in display +// order. The welcome document goes under the first of them. +func SeedTags() []CreateInput { + return []CreateInput{ + {TagName: "Production", Color: ColorHex("green")}, + {TagName: "Staging", Color: ColorHex("orange")}, + {TagName: "Incidents", Color: ColorHex("blue")}, + } +} diff --git a/server/core/internal/tag/seed_test.go b/server/core/internal/tag/seed_test.go new file mode 100644 index 00000000..91fd84ba --- /dev/null +++ b/server/core/internal/tag/seed_test.go @@ -0,0 +1,24 @@ +package tag + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_SeedTags(t *testing.T) { + t.Parallel() + + tags := SeedTags() + require.Len(t, tags, 3) + + names := make([]string, 0, len(tags)) + + for _, inp := range tags { + names = append(names, inp.TagName) + assert.NoError(t, inp.Validate()) + } + + assert.Equal(t, []string{"Production", "Staging", "Incidents"}, names) +} diff --git a/server/core/internal/user/user.go b/server/core/internal/user/user.go new file mode 100644 index 00000000..117bdff7 --- /dev/null +++ b/server/core/internal/user/user.go @@ -0,0 +1,22 @@ +// Package user defines what a user owns outside of Postgres: the storage +// layout of their image and the URL the image is served under. +package user + +import "fmt" + +// ImageFolder is the storage folder holding every user's image, each under +// its user id. It is keyed by user only: the avatar lives on the global +// user row, so an org-scoped object would 404 for viewers with a different +// active organization. +const ImageFolder = "users/images" + +// ImagePathFormat is the URL path format a user's image is served under, +// taking the user id. The router mounts the image routes on this very +// format, and the value is stored on the user row, so the two must not +// drift apart. +const ImagePathFormat = "/api/users/%s/image" + +// ImagePath returns the URL path the given user's image is served under. +func ImagePath(userID string) string { + return fmt.Sprintf(ImagePathFormat, userID) +} diff --git a/server/core/internal/user/user_test.go b/server/core/internal/user/user_test.go new file mode 100644 index 00000000..3e99e0ec --- /dev/null +++ b/server/core/internal/user/user_test.go @@ -0,0 +1,18 @@ +package user + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} + +func Test_ImagePath(t *testing.T) { + t.Parallel() + + assert.Equal(t, "/api/users/u1/image", ImagePath("u1")) +} diff --git a/server/core/pkg/httpserver/handler.go b/server/core/pkg/httpserver/handler.go index 71d620c1..49b6b075 100644 --- a/server/core/pkg/httpserver/handler.go +++ b/server/core/pkg/httpserver/handler.go @@ -17,6 +17,7 @@ import ( "github.com/gorilla/schema" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/logutil" + "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" ) @@ -24,6 +25,10 @@ import ( // that critical reports point at the handler instead of this helper. const _criticalSkipFrames = 2 +// WSPublishTimeout bounds each WebSocket publish triggered by a domain +// callback. +const WSPublishTimeout = 5 * time.Second + var ( // ErrNotAuthenticated is returned when authorization process fails. ErrNotAuthenticated = errutil.New(http.StatusUnauthorized, "account.not_authenticated", "not authenticated") @@ -312,6 +317,13 @@ func ExtractQueryID(r *http.Request, name string) (xid.ID, error) { return id, nil } +// CacheBust appends a version query carrying the current time to a +// location that has no query of its own, so a browser refetches an +// object that was replaced under an unchanged URL. +func CacheBust(location string) string { + return location + "?v=" + timeutil.Now().Format("20060102150405") +} + // Timeout is a middleware that cancels request's context after a given // time period. func Timeout(timeout time.Duration) func(next http.Handler) http.Handler { diff --git a/server/core/pkg/httpserver/handler_test.go b/server/core/pkg/httpserver/handler_test.go index 590195f5..5735939a 100644 --- a/server/core/pkg/httpserver/handler_test.go +++ b/server/core/pkg/httpserver/handler_test.go @@ -521,6 +521,12 @@ func Test_ExtractQueryID(t *testing.T) { assert.NoError(t, err) } +func Test_CacheBust(t *testing.T) { + t.Parallel() + + assert.Regexp(t, `^/api/users/u1/image\?v=\d{14}$`, CacheBust("/api/users/u1/image")) +} + func Test_Timeout(t *testing.T) { Timeout(time.Millisecond)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { <-r.Context().Done() diff --git a/server/core/pkg/strutil/nanoid.go b/server/core/pkg/strutil/nanoid.go index 2ea58a48..b7170560 100644 --- a/server/core/pkg/strutil/nanoid.go +++ b/server/core/pkg/strutil/nanoid.go @@ -1,18 +1,21 @@ package strutil -import "crypto/rand" +import ( + "crypto/rand" + "strings" +) // _nanoidAlphabet is the default alphabet used by nanoid. // This matches the Node.js nanoid default: A-Za-z0-9_-. const _nanoidAlphabet = "useandom-26T198340PX75pxJACKVERYMINDBUSHWOLF_GQZbfghjklqvwyzrict" -// _nanoidLength is the default length of nanoid IDs (21 characters). -const _nanoidLength = 21 +// NanoIDLength is the default length of nanoid IDs (21 characters). +const NanoIDLength = 21 // NanoID generates a nanoid-compatible unique identifier. The output is 1:1 // compatible with the Node.js nanoid() function using default settings. func NanoID() string { - bytes := make([]byte, _nanoidLength) + bytes := make([]byte, NanoIDLength) _, err := rand.Read(bytes) if err != nil { @@ -20,11 +23,27 @@ func NanoID() string { panic(err) } - id := make([]byte, _nanoidLength) + id := make([]byte, NanoIDLength) - for i := range _nanoidLength { + for i := range NanoIDLength { id[i] = _nanoidAlphabet[bytes[i]&63] } return string(id) } + +// IsNanoID reports whether s has the exact shape NanoID produces: the +// default length, every byte from the default alphabet. +func IsNanoID(s string) bool { + if len(s) != NanoIDLength { + return false + } + + for i := range len(s) { + if !strings.ContainsRune(_nanoidAlphabet, rune(s[i])) { + return false + } + } + + return true +} diff --git a/server/core/pkg/strutil/nanoid_test.go b/server/core/pkg/strutil/nanoid_test.go index dfe2feee..2c5d351e 100644 --- a/server/core/pkg/strutil/nanoid_test.go +++ b/server/core/pkg/strutil/nanoid_test.go @@ -15,7 +15,7 @@ func Test_NanoID(t *testing.T) { for range 100 { id := NanoID() - assert.Len(t, id, _nanoidLength) + assert.Len(t, id, NanoIDLength) for _, r := range id { assert.True( @@ -30,3 +30,26 @@ func Test_NanoID(t *testing.T) { assert.Len(t, seen, 100, "generated IDs should be unique") } + +func Test_IsNanoID(t *testing.T) { + cc := map[string]struct { + ID string + Result bool + }{ + "Generated id": {ID: NanoID(), Result: true}, + "Id with dashes": {ID: "f1-x_-xxxxxxxxxxxxxxx", Result: true}, + "Short": {ID: "f1"}, + "Long": {ID: "f1xxxxxxxxxxxxxxxxxxxx"}, + "Wrong charset": {ID: "f1.................xx"}, + "Multi-byte runes": {ID: "é" + "xxxxxxxxxxxxxxxxxxx"}, + "Empty": {}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, IsNanoID(c.ID)) + }) + } +}