From c9c3d254a10999c30f99ae4b776dee180b2d7940 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:12:31 +0000 Subject: [PATCH 1/7] refactor: give workspace-scoped paste routes their own actions open_api_spex finds an operation by controller action, not by route, so the personal routes (/api/v1/pastes) and the workspace routes (/api/v1/workspaces/:workspace_id/pastes) cannot share actions once their specs live in the controller: only one of them has a workspace_id. Route the workspace variants to workspace_* actions that delegate to the personal ones, and spell out the personal routes instead of `resources`. Behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019fnjMN7WpEjQJD2zveakV6 --- .../controllers/apiv1/paste_controller.ex | 10 +++++++ lib/textbin_web/router.ex | 27 +++++++++++++------ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/textbin_web/controllers/apiv1/paste_controller.ex b/lib/textbin_web/controllers/apiv1/paste_controller.ex index 5605117..47c0004 100644 --- a/lib/textbin_web/controllers/apiv1/paste_controller.ex +++ b/lib/textbin_web/controllers/apiv1/paste_controller.ex @@ -17,6 +17,16 @@ defmodule TextbinWeb.ApiV1.PasteController do # :max_paste_bytes. @default_max_paste_bytes 1_048_576 + # Workspace-scoped routes share the personal actions' behaviour; the + # workspace_id path parameter is what selects the scope. + def workspace_index(conn, params), do: index(conn, params) + def workspace_create(conn, params), do: create(conn, params) + def workspace_show(conn, params), do: show(conn, params) + def workspace_delete(conn, params), do: delete(conn, params) + def workspace_deleted(conn, params), do: deleted(conn, params) + def workspace_restore(conn, params), do: restore(conn, params) + def workspace_purge(conn, params), do: purge(conn, params) + def index(conn, params) do with_paste_scope(conn, params, fn paste_scope -> case Pastes.list_pastes_with_access(paste_scope) do diff --git a/lib/textbin_web/router.ex b/lib/textbin_web/router.ex index 3c9e53e..02bdada 100644 --- a/lib/textbin_web/router.ex +++ b/lib/textbin_web/router.ex @@ -46,17 +46,28 @@ defmodule TextbinWeb.Router do post "/workspaces/:workspace_id/recovery", OrganizationController, :recover_workspace # Removed pastes are their own collection so "deleted" never collides # with a paste id; these routes must precede the /:id routes below. - get "/workspaces/:workspace_id/pastes/deleted", PasteController, :deleted - post "/workspaces/:workspace_id/pastes/deleted/:id/restore", PasteController, :restore - delete "/workspaces/:workspace_id/pastes/deleted/:id", PasteController, :purge + # + # Personal-workspace and workspace-scoped routes use separate actions so + # each has its own operation in the OpenAPI document (an operation is + # looked up by controller action, not by route). + get "/workspaces/:workspace_id/pastes/deleted", PasteController, :workspace_deleted + + post "/workspaces/:workspace_id/pastes/deleted/:id/restore", + PasteController, + :workspace_restore + + delete "/workspaces/:workspace_id/pastes/deleted/:id", PasteController, :workspace_purge get "/pastes/deleted", PasteController, :deleted post "/pastes/deleted/:id/restore", PasteController, :restore delete "/pastes/deleted/:id", PasteController, :purge - get "/workspaces/:workspace_id/pastes", PasteController, :index - post "/workspaces/:workspace_id/pastes", PasteController, :create - get "/workspaces/:workspace_id/pastes/:id", PasteController, :show - delete "/workspaces/:workspace_id/pastes/:id", PasteController, :delete - resources "/pastes", PasteController, except: [:new, :edit, :update] + get "/workspaces/:workspace_id/pastes", PasteController, :workspace_index + post "/workspaces/:workspace_id/pastes", PasteController, :workspace_create + get "/workspaces/:workspace_id/pastes/:id", PasteController, :workspace_show + delete "/workspaces/:workspace_id/pastes/:id", PasteController, :workspace_delete + get "/pastes", PasteController, :index + post "/pastes", PasteController, :create + get "/pastes/:id", PasteController, :show + delete "/pastes/:id", PasteController, :delete end # Enable LiveDashboard and Swoosh mailbox preview in development From 7f741b3562deec0483214f5ad76160879d5aedb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:16:06 +0000 Subject: [PATCH 2/7] refactor: declare each API operation next to its controller action The OpenAPI document was a hand-written map in TextbinWeb.ApiSpec, away from the code it describes, so changing an endpoint meant remembering a second file. Each action now declares its operation with OpenApiSpex.ControllerSpecs, directly above the function, and ApiSpec builds the paths from the router. - TextbinWeb.ApiDocs holds the building blocks the annotations share: JSON and error responses, UUID path parameters, the paste query fields, and in_workspace/1, which derives each workspace-scoped paste operation from its personal-workspace spec. - The exported document is unchanged apart from dropping `deprecated: false` and two empty parameter descriptions. - The route test now checks that every API action declares an operation, since an action without one would otherwise drop out of the document silently. - .formatter.exs imports open_api_spex so `operation` and `tags` are formatted without parentheses. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019fnjMN7WpEjQJD2zveakV6 --- .formatter.exs | 2 +- lib/textbin_web/api_docs.ex | 83 +++++ lib/textbin_web/api_spec.ex | 351 +----------------- .../controllers/apiv1/auth_controller.ex | 30 ++ .../apiv1/organization_controller.ex | 45 +++ .../controllers/apiv1/paste_controller.ex | 115 +++++- .../controllers/paste_controller.ex | 27 ++ priv/openapi.json | 26 +- test/textbin_web/api_contract_test.exs | 30 +- 9 files changed, 323 insertions(+), 386 deletions(-) create mode 100644 lib/textbin_web/api_docs.ex diff --git a/.formatter.exs b/.formatter.exs index ef8840c..2b5a714 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -1,5 +1,5 @@ [ - import_deps: [:ecto, :ecto_sql, :phoenix], + import_deps: [:ecto, :ecto_sql, :phoenix, :open_api_spex], subdirectories: ["priv/*/migrations"], plugins: [Phoenix.LiveView.HTMLFormatter], inputs: ["*.{heex,ex,exs}", "{config,lib,test}/**/*.{heex,ex,exs}", "priv/*/seeds.exs"] diff --git a/lib/textbin_web/api_docs.ex b/lib/textbin_web/api_docs.ex new file mode 100644 index 0000000..2974444 --- /dev/null +++ b/lib/textbin_web/api_docs.ex @@ -0,0 +1,83 @@ +defmodule TextbinWeb.ApiDocs do + @moduledoc """ + Building blocks for the `operation` specs that API controllers declare + next to their actions with `OpenApiSpex.ControllerSpecs`. + + Specs are evaluated when the controller compiles, so they cannot call + functions of the controller itself; the shared pieces live here instead. + Request and response schemas live in `TextbinWeb.ApiSpec.Schemas` and are + referenced by title. + """ + + alias OpenApiSpex.{MediaType, RequestBody, Response, Schema} + alias TextbinWeb.ApiSpec.Schemas + + @doc "A JSON response whose body is the named schema." + def response(description, title), do: {description, "application/json", Schemas.ref(title)} + + def error(description), do: response(description, "Error") + + def unauthorized, do: error("Missing, invalid or revoked API token") + + @doc "The 422 every operation with parameters or a body can return." + def invalid_request, + do: response("The parameters or body do not match this operation", "ValidationError") + + def no_content(description), do: %Response{description: description} + + def uuid_path(description) do + [ + in: :path, + required: true, + description: description, + schema: %Schema{type: :string, format: :uuid} + ] + end + + def query(schema, description), + do: [in: :query, required: false, description: description || "", schema: schema] + + def json_body(title) do + %RequestBody{ + required: true, + content: %{"application/json" => %MediaType{schema: Schemas.ref(title)}} + } + end + + @doc """ + Adds the responses every paste operation shares. An operation's own + wording for a status wins. + """ + def with_scope_errors(responses) do + Map.merge(%{401 => unauthorized(), 404 => error("Workspace not found")}, responses) + end + + @doc "The optional paste fields as query parameters, for raw-body uploads." + def paste_field_parameters do + for {name, schema} <- Enum.sort(Schemas.create_paste_fields()), + do: {name, query(schema, schema.description)} + end + + def create_paste_body do + %RequestBody{ + required: true, + content: %{ + "application/json" => %MediaType{schema: Schemas.ref("CreatePasteRequest")}, + "*/*" => %MediaType{schema: %Schema{type: :string, format: :binary}} + } + } + end + + @doc """ + The workspace-scoped variant of a personal-workspace paste operation: the + same behaviour under /workspaces/{workspace_id}, with its own operation id. + """ + def in_workspace(spec) do + workspace_id = {:workspace_id, uuid_path("Workspace id")} + + spec + |> Keyword.update(:parameters, [workspace_id], &[workspace_id | &1]) + |> Keyword.update!(:operation_id, &(&1 <> "InWorkspace")) + |> Keyword.update!(:summary, &String.replace(&1, "your personal workspace", "a workspace")) + end +end diff --git a/lib/textbin_web/api_spec.ex b/lib/textbin_web/api_spec.ex index 19a9158..0ed782e 100644 --- a/lib/textbin_web/api_spec.ex +++ b/lib/textbin_web/api_spec.ex @@ -2,30 +2,17 @@ defmodule TextbinWeb.ApiSpec do @moduledoc """ The OpenAPI document for the v1 API and the raw paste endpoint. - The document is written out by hand rather than derived from controller - annotations, so the whole public contract reads in one place. Two checks - keep it honest: a test fails when a route under /api/v1 has no operation - here (or the reverse), and contract tests cast real responses against the - schemas. `priv/openapi.json` is the exported copy; CI regenerates it and - fails if it differs, so a change to the contract shows up in review. + Each operation is declared next to its action with + `OpenApiSpex.ControllerSpecs`, so a change to an endpoint and its contract + land in the same place; this module assembles them from the router and + adds the shared schemas and security. Request validation and the test-time + response check both read the same operations, and `priv/openapi.json` is + the exported copy, kept current by a test. """ @behaviour OpenApiSpex.OpenApi - alias OpenApiSpex.{ - Components, - Info, - MediaType, - OpenApi, - Operation, - Parameter, - PathItem, - RequestBody, - Response, - Schema, - SecurityScheme, - Server - } + alias OpenApiSpex.{Components, Info, OpenApi, Operation, Paths, SecurityScheme, Server} alias TextbinWeb.ApiSpec.Schemas @@ -46,7 +33,7 @@ defmodule TextbinWeb.ApiSpec do "bearer" => %SecurityScheme{type: "http", scheme: "bearer"} } }, - paths: paths() + paths: Paths.from_router(TextbinWeb.Router) } end @@ -57,326 +44,4 @@ defmodule TextbinWeb.ApiSpec do method in [:get, :post, :put, :patch, :delete], do: {method, path, operation} end - - defp paths do - %{ - "/api/v1/auth/tokens" => %PathItem{post: create_token()}, - "/api/v1/me" => %PathItem{get: get_identity()}, - "/api/v1/me/token" => %PathItem{delete: revoke_current_token()}, - "/api/v1/organizations" => %PathItem{get: list_organizations()}, - "/api/v1/organizations/{id}/workspaces" => %PathItem{get: list_workspaces()}, - "/api/v1/organizations/{id}/audit-events" => %PathItem{get: list_audit_events()}, - "/api/v1/workspaces/{workspace_id}/recovery" => %PathItem{post: recover_workspace()}, - "/api/v1/pastes" => %PathItem{ - get: list_pastes(:personal), - post: create_paste(:personal) - }, - "/api/v1/pastes/{id}" => %PathItem{ - get: get_paste(:personal), - delete: delete_paste(:personal) - }, - "/api/v1/pastes/deleted" => %PathItem{get: list_removed_pastes(:personal)}, - "/api/v1/pastes/deleted/{id}" => %PathItem{delete: purge_paste(:personal)}, - "/api/v1/pastes/deleted/{id}/restore" => %PathItem{post: restore_paste(:personal)}, - "/api/v1/workspaces/{workspace_id}/pastes" => %PathItem{ - get: list_pastes(:workspace), - post: create_paste(:workspace) - }, - "/api/v1/workspaces/{workspace_id}/pastes/{id}" => %PathItem{ - get: get_paste(:workspace), - delete: delete_paste(:workspace) - }, - "/api/v1/workspaces/{workspace_id}/pastes/deleted" => %PathItem{ - get: list_removed_pastes(:workspace) - }, - "/api/v1/workspaces/{workspace_id}/pastes/deleted/{id}" => %PathItem{ - delete: purge_paste(:workspace) - }, - "/api/v1/workspaces/{workspace_id}/pastes/deleted/{id}/restore" => %PathItem{ - post: restore_paste(:workspace) - }, - "/pastes/{id}/raw" => %PathItem{get: get_raw_paste()} - } - end - - ## Authentication - - defp create_token do - %Operation{ - operationId: "createApiToken", - tags: ["Authentication"], - summary: "Create an API token with an email and password", - security: [], - requestBody: json_body("CreateTokenRequest"), - responses: %{ - 201 => json("The token and the account it belongs to", "CreatedTokenResponse"), - 400 => error("Email or password missing"), - 401 => error("Wrong email or password"), - 422 => error("The token could not be created") - } - } - end - - defp get_identity do - %Operation{ - operationId: "getIdentity", - tags: ["Authentication"], - summary: "Show the authenticated account and token", - responses: %{200 => json("The current identity", "IdentityResponse"), 401 => unauthorized()} - } - end - - defp revoke_current_token do - %Operation{ - operationId: "revokeCurrentToken", - tags: ["Authentication"], - summary: "Revoke the token used for this request", - responses: %{204 => empty("Revoked, or already gone"), 401 => unauthorized()} - } - end - - ## Organizations and workspaces - - defp list_organizations do - %Operation{ - operationId: "listOrganizations", - tags: ["Organizations"], - summary: "List the organizations you belong to", - responses: %{200 => json("Organizations", "OrganizationList"), 401 => unauthorized()} - } - end - - defp list_workspaces do - %Operation{ - operationId: "listOrganizationWorkspaces", - tags: ["Organizations"], - summary: "List workspaces you have joined or can join", - parameters: [path(:id, "Organization id")], - responses: %{ - 200 => json("The organization and its workspaces", "WorkspaceList"), - 401 => unauthorized(), - 404 => error("Organization not found") - } - } - end - - defp list_audit_events do - %Operation{ - operationId: "listAuditEvents", - tags: ["Organizations"], - summary: "Page through an organization's audit log", - parameters: [ - path(:id, "Organization id"), - query(:limit, %Schema{type: :integer, minimum: 1}, "Events per page"), - query(:cursor, %Schema{type: :string}, "next_cursor from the previous page") - ], - responses: %{ - 200 => json("A page of audit events", "AuditEventPage"), - 401 => unauthorized(), - 404 => error("Organization not found, or you cannot read its audit log") - } - } - end - - defp recover_workspace do - %Operation{ - operationId: "recoverWorkspaceAccess", - tags: ["Organizations"], - summary: "Make yourself owner of a workspace in an organization you own", - parameters: [path(:workspace_id, "Workspace id")], - responses: %{ - 201 => json("Your new membership", "WorkspaceRecovery"), - 401 => unauthorized(), - 404 => error("Workspace not found, or you cannot recover it") - } - } - end - - ## Pastes - - # Every paste operation exists twice: once for the caller's personal - # workspace, and once scoped to a workspace id. - defp list_pastes(scope) do - %Operation{ - operationId: operation_id("listPastes", scope), - tags: ["Pastes"], - summary: "List pastes in #{scope_name(scope)}", - parameters: scope_parameters(scope), - responses: scoped_responses(%{200 => json("Pastes, newest first", "PasteList")}) - } - end - - defp create_paste(scope) do - fields = - for {name, schema} <- Enum.sort(Schemas.create_paste_fields()) do - query(name, schema, schema.description) - end - - %Operation{ - operationId: operation_id("createPaste", scope), - tags: ["Pastes"], - summary: "Create a paste in #{scope_name(scope)}", - description: - "Send JSON, or send the content itself as the request body (any other content type) with the fields as query parameters.", - parameters: scope_parameters(scope) ++ fields, - requestBody: %RequestBody{ - required: true, - content: %{ - "application/json" => %MediaType{schema: Schemas.ref("CreatePasteRequest")}, - "*/*" => %MediaType{schema: %Schema{type: :string, format: :binary}} - } - }, - responses: - scoped_responses(%{ - 201 => json("The new paste, without its content", "PasteMetadataResponse"), - 400 => error("The request body could not be read"), - 413 => error("The content is larger than the server allows"), - 422 => json("Rejected fields", "ValidationError"), - 503 => error("Paste storage is temporarily unavailable") - }) - } - end - - defp get_paste(scope) do - %Operation{ - operationId: operation_id("getPaste", scope), - tags: ["Pastes"], - summary: "Show a paste with its content", - parameters: scope_parameters(scope) ++ [path(:id, "Paste id")], - responses: - scoped_responses(%{ - 200 => json("The paste", "PasteResponse"), - 400 => error("The id is not a UUID"), - 404 => error("Paste or workspace not found") - }) - } - end - - defp delete_paste(scope) do - %Operation{ - operationId: operation_id("deletePaste", scope), - tags: ["Pastes"], - summary: "Delete a paste; it stays restorable for the workspace's recovery window", - parameters: scope_parameters(scope) ++ [path(:id, "Paste id")], - responses: - scoped_responses(%{ - 204 => empty("Deleted, or nothing to delete"), - 503 => error("The deletion could not be completed") - }) - } - end - - defp list_removed_pastes(scope) do - %Operation{ - operationId: operation_id("listRemovedPastes", scope), - tags: ["Pastes"], - summary: "List deleted and expired pastes that can still be restored", - parameters: scope_parameters(scope), - responses: scoped_responses(%{200 => json("Restorable pastes", "RemovedPasteList")}) - } - end - - defp restore_paste(scope) do - %Operation{ - operationId: operation_id("restorePaste", scope), - tags: ["Pastes"], - summary: "Restore a deleted or expired paste", - parameters: scope_parameters(scope) ++ [path(:id, "Paste id")], - responses: - scoped_responses(%{ - 200 => json("The restored paste, without its content", "PasteMetadataResponse"), - 400 => error("The id is not a UUID"), - 404 => error("No restorable paste you can manage has this id") - }) - } - end - - defp purge_paste(scope) do - %Operation{ - operationId: operation_id("purgePaste", scope), - tags: ["Pastes"], - summary: "Delete a removed paste for good", - parameters: scope_parameters(scope) ++ [path(:id, "Paste id")], - responses: - scoped_responses(%{ - 204 => empty("Deleted for good"), - 400 => error("The id is not a UUID"), - 404 => error("No removed paste you can manage has this id"), - 503 => error("The stored content could not be deleted; try again") - }) - } - end - - defp get_raw_paste do - %Operation{ - operationId: "getRawPaste", - tags: ["Pastes"], - summary: "Download a paste's exact stored bytes", - description: - "Uses the browser session or the paste's link, not a bearer token. Text that is safe to show inline is served with its content type; anything else downloads.", - security: [], - parameters: [ - path(:id, "Paste id"), - query(:download, %Schema{type: :string}, "Present to download even safe text") - ], - responses: %{ - 200 => %Response{ - description: "The stored bytes", - content: %{"*/*" => %MediaType{schema: %Schema{type: :string, format: :binary}}} - }, - 404 => %Response{ - description: "Not found, or not shared with you", - content: %{"text/plain" => %MediaType{schema: %Schema{type: :string}}} - } - } - } - end - - ## Helpers - - defp operation_id(name, :personal), do: name - defp operation_id(name, :workspace), do: name <> "InWorkspace" - - defp scope_name(:personal), do: "your personal workspace" - defp scope_name(:workspace), do: "a workspace" - - defp scope_parameters(:personal), do: [] - defp scope_parameters(:workspace), do: [path(:workspace_id, "Workspace id")] - - # Responses every paste operation shares; an operation's own 404 wording wins. - defp scoped_responses(responses) do - Map.merge(%{401 => unauthorized(), 404 => error("Workspace not found")}, responses) - end - - defp path(name, description) do - %Parameter{ - name: name, - in: :path, - required: true, - description: description, - schema: %Schema{type: :string, format: :uuid} - } - end - - defp query(name, schema, description) do - %Parameter{name: name, in: :query, required: false, description: description, schema: schema} - end - - defp json_body(title) do - %RequestBody{ - required: true, - content: %{"application/json" => %MediaType{schema: Schemas.ref(title)}} - } - end - - defp json(description, title) do - %Response{ - description: description, - content: %{"application/json" => %MediaType{schema: Schemas.ref(title)}} - } - end - - defp error(description), do: json(description, "Error") - defp unauthorized, do: error("Missing, invalid or revoked API token") - defp empty(description), do: %Response{description: description} end diff --git a/lib/textbin_web/controllers/apiv1/auth_controller.ex b/lib/textbin_web/controllers/apiv1/auth_controller.ex index cff03e4..b3fbf94 100644 --- a/lib/textbin_web/controllers/apiv1/auth_controller.ex +++ b/lib/textbin_web/controllers/apiv1/auth_controller.ex @@ -1,9 +1,26 @@ defmodule TextbinWeb.ApiV1.AuthController do use TextbinWeb, :controller + use OpenApiSpex.ControllerSpecs + + import TextbinWeb.ApiDocs alias Textbin.Accounts alias Textbin.Accounts.{Scope, User, UserToken} + tags ["Authentication"] + + operation :create, + operation_id: "createApiToken", + summary: "Create an API token with an email and password", + security: [], + request_body: json_body("CreateTokenRequest"), + responses: %{ + 201 => response("The token and the account it belongs to", "CreatedTokenResponse"), + 400 => error("Email or password missing"), + 401 => error("Wrong email or password"), + 422 => error("The token could not be created") + } + def create(conn, %{"email" => email, "password" => password} = params) when is_binary(email) and is_binary(password) do case Accounts.get_user_by_email_and_password(email, password) do @@ -18,11 +35,24 @@ defmodule TextbinWeb.ApiV1.AuthController do |> json(%{errors: %{detail: "Email and password are required"}}) end + operation :show, + operation_id: "getIdentity", + summary: "Show the authenticated account and token", + responses: %{ + 200 => response("The current identity", "IdentityResponse"), + 401 => unauthorized() + } + def show(%{assigns: %{current_scope: %Scope{user: %User{} = user}}} = conn, _params) do user_token = conn.assigns.current_api_token json(conn, %{data: identity_data(user, user_token)}) end + operation :delete, + operation_id: "revokeCurrentToken", + summary: "Revoke the token used for this request", + responses: %{204 => no_content("Revoked, or already gone"), 401 => unauthorized()} + def delete( %{assigns: %{current_scope: %Scope{user: %User{} = user}, current_api_token: user_token}} = conn, diff --git a/lib/textbin_web/controllers/apiv1/organization_controller.ex b/lib/textbin_web/controllers/apiv1/organization_controller.ex index 5bf3892..f81573b 100644 --- a/lib/textbin_web/controllers/apiv1/organization_controller.ex +++ b/lib/textbin_web/controllers/apiv1/organization_controller.ex @@ -1,15 +1,36 @@ defmodule TextbinWeb.ApiV1.OrganizationController do use TextbinWeb, :controller + use OpenApiSpex.ControllerSpecs + import TextbinWeb.ApiDocs + + alias OpenApiSpex.Schema alias Textbin.Accounts.Scope alias Textbin.Organizations + tags ["Organizations"] + + operation :index, + operation_id: "listOrganizations", + summary: "List the organizations you belong to", + responses: %{200 => response("Organizations", "OrganizationList"), 401 => unauthorized()} + def index(conn, _params) do with_api_scope(conn, fn scope -> render(conn, :index, organizations: Organizations.list_available_organizations(scope)) end) end + operation :workspaces, + operation_id: "listOrganizationWorkspaces", + summary: "List workspaces you have joined or can join", + parameters: [id: uuid_path("Organization id")], + responses: %{ + 200 => response("The organization and its workspaces", "WorkspaceList"), + 401 => unauthorized(), + 404 => error("Organization not found") + } + def workspaces(conn, %{"id" => id}) do with_api_scope(conn, fn scope -> with %{} = organization <- Organizations.get_available_organization(scope, id), @@ -24,6 +45,20 @@ defmodule TextbinWeb.ApiV1.OrganizationController do end) end + operation :audit_events, + operation_id: "listAuditEvents", + summary: "Page through an organization's audit log", + parameters: [ + id: uuid_path("Organization id"), + limit: query(%Schema{type: :integer, minimum: 1}, "Events per page"), + cursor: query(%Schema{type: :string}, "next_cursor from the previous page") + ], + responses: %{ + 200 => response("A page of audit events", "AuditEventPage"), + 401 => unauthorized(), + 404 => error("Organization not found, or you cannot read its audit log") + } + def audit_events(conn, %{"id" => id}) do with_api_scope(conn, fn scope -> with %{} = organization <- Organizations.get_available_organization(scope, id), @@ -42,6 +77,16 @@ defmodule TextbinWeb.ApiV1.OrganizationController do end) end + operation :recover_workspace, + operation_id: "recoverWorkspaceAccess", + summary: "Make yourself owner of a workspace in an organization you own", + parameters: [workspace_id: uuid_path("Workspace id")], + responses: %{ + 201 => response("Your new membership", "WorkspaceRecovery"), + 401 => unauthorized(), + 404 => error("Workspace not found, or you cannot recover it") + } + def recover_workspace(conn, %{"workspace_id" => workspace_id}) do with_api_scope(conn, fn scope -> case Organizations.recover_workspace_access(scope, workspace_id) do diff --git a/lib/textbin_web/controllers/apiv1/paste_controller.ex b/lib/textbin_web/controllers/apiv1/paste_controller.ex index 47c0004..f9e6883 100644 --- a/lib/textbin_web/controllers/apiv1/paste_controller.ex +++ b/lib/textbin_web/controllers/apiv1/paste_controller.ex @@ -1,5 +1,8 @@ defmodule TextbinWeb.ApiV1.PasteController do use TextbinWeb, :controller + use OpenApiSpex.ControllerSpecs + + import TextbinWeb.ApiDocs alias Textbin.Accounts.Scope alias Textbin.Organizations @@ -17,16 +20,112 @@ defmodule TextbinWeb.ApiV1.PasteController do # :max_paste_bytes. @default_max_paste_bytes 1_048_576 - # Workspace-scoped routes share the personal actions' behaviour; the - # workspace_id path parameter is what selects the scope. + tags ["Pastes"] + + # Every paste operation exists twice: for the caller's personal workspace, + # and under /workspaces/{workspace_id}. The workspace_* actions share the + # personal actions' behaviour and spec; the workspace_id path parameter is + # what selects the scope. + @index_spec [ + operation_id: "listPastes", + summary: "List pastes in your personal workspace", + responses: with_scope_errors(%{200 => response("Pastes, newest first", "PasteList")}) + ] + + @create_spec [ + operation_id: "createPaste", + summary: "Create a paste in your personal workspace", + description: + "Send JSON, or send the content itself as the request body (any other content type) with the fields as query parameters.", + parameters: paste_field_parameters(), + request_body: create_paste_body(), + responses: + with_scope_errors(%{ + 201 => response("The new paste, without its content", "PasteMetadataResponse"), + 400 => error("The request body could not be read"), + 413 => error("The content is larger than the server allows"), + 422 => response("Rejected fields", "ValidationError"), + 503 => error("Paste storage is temporarily unavailable") + }) + ] + + @show_spec [ + operation_id: "getPaste", + summary: "Show a paste with its content", + parameters: [id: uuid_path("Paste id")], + responses: + with_scope_errors(%{ + 200 => response("The paste", "PasteResponse"), + 400 => error("The id is not a UUID"), + 404 => error("Paste or workspace not found") + }) + ] + + @delete_spec [ + operation_id: "deletePaste", + summary: "Delete a paste; it stays restorable for the workspace's recovery window", + parameters: [id: uuid_path("Paste id")], + responses: + with_scope_errors(%{ + 204 => no_content("Deleted, or nothing to delete"), + 503 => error("The deletion could not be completed") + }) + ] + + @deleted_spec [ + operation_id: "listRemovedPastes", + summary: "List deleted and expired pastes that can still be restored", + responses: with_scope_errors(%{200 => response("Restorable pastes", "RemovedPasteList")}) + ] + + @restore_spec [ + operation_id: "restorePaste", + summary: "Restore a deleted or expired paste", + parameters: [id: uuid_path("Paste id")], + responses: + with_scope_errors(%{ + 200 => response("The restored paste, without its content", "PasteMetadataResponse"), + 400 => error("The id is not a UUID"), + 404 => error("No restorable paste you can manage has this id") + }) + ] + + @purge_spec [ + operation_id: "purgePaste", + summary: "Delete a removed paste for good", + parameters: [id: uuid_path("Paste id")], + responses: + with_scope_errors(%{ + 204 => no_content("Deleted for good"), + 400 => error("The id is not a UUID"), + 404 => error("No removed paste you can manage has this id"), + 503 => error("The stored content could not be deleted; try again") + }) + ] + + operation :workspace_index, in_workspace(@index_spec) def workspace_index(conn, params), do: index(conn, params) + + operation :workspace_create, in_workspace(@create_spec) def workspace_create(conn, params), do: create(conn, params) + + operation :workspace_show, in_workspace(@show_spec) def workspace_show(conn, params), do: show(conn, params) + + operation :workspace_delete, in_workspace(@delete_spec) def workspace_delete(conn, params), do: delete(conn, params) + + operation :workspace_deleted, in_workspace(@deleted_spec) def workspace_deleted(conn, params), do: deleted(conn, params) + + operation :workspace_restore, in_workspace(@restore_spec) def workspace_restore(conn, params), do: restore(conn, params) + + operation :workspace_purge, in_workspace(@purge_spec) def workspace_purge(conn, params), do: purge(conn, params) + operation :index, @index_spec + def index(conn, params) do with_paste_scope(conn, params, fn paste_scope -> case Pastes.list_pastes_with_access(paste_scope) do @@ -46,6 +145,8 @@ defmodule TextbinWeb.ApiV1.PasteController do # Treat create as "store the request body as paste data" while still accepting # common JSON client shapes. This keeps CLI usage simple without making API # clients wrap data unless they want to. + operation :create, @create_spec + def create(conn, params) do request_params = Map.drop(params, Map.keys(conn.path_params)) @@ -314,6 +415,8 @@ defmodule TextbinWeb.ApiV1.PasteController do end end + operation :show, @show_spec + def show(conn, %{"id" => id} = params) do with_paste_scope(conn, params, &show_scoped_paste(conn, &1, id)) end @@ -332,6 +435,8 @@ defmodule TextbinWeb.ApiV1.PasteController do end end + operation :delete, @delete_spec + def delete(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn current_scope -> result = @@ -360,6 +465,8 @@ defmodule TextbinWeb.ApiV1.PasteController do defp delete_scoped_paste(scope, paste, _params), do: Pastes.delete_personal_paste(scope, paste) + operation :deleted, @deleted_spec + def deleted(conn, params) do with_paste_scope(conn, params, fn scope -> render(conn, :deleted, @@ -374,6 +481,8 @@ defmodule TextbinWeb.ApiV1.PasteController do # Restoring and deleting for good answer 404 alike for an unknown id, a paste # the caller cannot manage, and one whose recovery window has passed, so the # response never confirms another member's paste exists. + operation :restore, @restore_spec + def restore(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn scope -> with {:ok, paste_id} <- Ecto.UUID.cast(id), @@ -390,6 +499,8 @@ defmodule TextbinWeb.ApiV1.PasteController do end) end + operation :purge, @purge_spec + def purge(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn scope -> with {:ok, paste_id} <- Ecto.UUID.cast(id), diff --git a/lib/textbin_web/controllers/paste_controller.ex b/lib/textbin_web/controllers/paste_controller.ex index 42eb62a..9a7812f 100644 --- a/lib/textbin_web/controllers/paste_controller.ex +++ b/lib/textbin_web/controllers/paste_controller.ex @@ -1,10 +1,37 @@ defmodule TextbinWeb.PasteController do use TextbinWeb, :controller + use OpenApiSpex.ControllerSpecs + import TextbinWeb.ApiDocs + + alias OpenApiSpex.{MediaType, Response, Schema} alias Textbin.Pastes alias Textbin.Pastes.ContentType alias Textbin.Pastes.Paste + tags ["Pastes"] + + operation :raw, + operation_id: "getRawPaste", + summary: "Download a paste's exact stored bytes", + description: + "Uses the browser session or the paste's link, not a bearer token. Text that is safe to show inline is served with its content type; anything else downloads.", + security: [], + parameters: [ + id: uuid_path("Paste id"), + download: query(%Schema{type: :string}, "Present to download even safe text") + ], + responses: %{ + 200 => %Response{ + description: "The stored bytes", + content: %{"*/*" => %MediaType{schema: %Schema{type: :string, format: :binary}}} + }, + 404 => %Response{ + description: "Not found, or not shared with you", + content: %{"text/plain" => %MediaType{schema: %Schema{type: :string}}} + } + } + def raw(conn, %{"id" => id} = params) do case Pastes.get_shared_paste(conn.assigns.current_scope, id) do %Paste{} = paste -> diff --git a/priv/openapi.json b/priv/openapi.json index 10eb36c..710651d 100644 --- a/priv/openapi.json +++ b/priv/openapi.json @@ -968,7 +968,6 @@ "/api/v1/auth/tokens": { "post": { "callbacks": {}, - "deprecated": false, "operationId": "createApiToken", "parameters": [], "requestBody": { @@ -1033,7 +1032,6 @@ "/api/v1/me": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "getIdentity", "parameters": [], "responses": { @@ -1067,7 +1065,6 @@ "/api/v1/me/token": { "delete": { "callbacks": {}, - "deprecated": false, "operationId": "revokeCurrentToken", "parameters": [], "responses": { @@ -1094,7 +1091,6 @@ "/api/v1/organizations": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listOrganizations", "parameters": [], "responses": { @@ -1128,7 +1124,6 @@ "/api/v1/organizations/{id}/audit-events": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listAuditEvents", "parameters": [ { @@ -1202,7 +1197,6 @@ "/api/v1/organizations/{id}/workspaces": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listOrganizationWorkspaces", "parameters": [ { @@ -1257,7 +1251,6 @@ "/api/v1/pastes": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listPastes", "parameters": [], "responses": { @@ -1299,7 +1292,6 @@ }, "post": { "callbacks": {}, - "deprecated": false, "description": "Send JSON, or send the content itself as the request body (any other content type) with the fields as query parameters.", "operationId": "createPaste", "parameters": [ @@ -1319,6 +1311,7 @@ } }, { + "description": "", "in": "query", "name": "content_type", "required": false, @@ -1350,6 +1343,7 @@ } }, { + "description": "", "in": "query", "name": "syntax_highlight", "required": false, @@ -1493,7 +1487,6 @@ "/api/v1/pastes/deleted": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listRemovedPastes", "parameters": [], "responses": { @@ -1537,7 +1530,6 @@ "/api/v1/pastes/deleted/{id}": { "delete": { "callbacks": {}, - "deprecated": false, "operationId": "purgePaste", "parameters": [ { @@ -1605,7 +1597,6 @@ "/api/v1/pastes/deleted/{id}/restore": { "post": { "callbacks": {}, - "deprecated": false, "operationId": "restorePaste", "parameters": [ { @@ -1670,7 +1661,6 @@ "/api/v1/pastes/{id}": { "delete": { "callbacks": {}, - "deprecated": false, "operationId": "deletePaste", "parameters": [ { @@ -1726,7 +1716,6 @@ }, "get": { "callbacks": {}, - "deprecated": false, "operationId": "getPaste", "parameters": [ { @@ -1791,7 +1780,6 @@ "/api/v1/workspaces/{workspace_id}/pastes": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listPastesInWorkspace", "parameters": [ { @@ -1844,7 +1832,6 @@ }, "post": { "callbacks": {}, - "deprecated": false, "description": "Send JSON, or send the content itself as the request body (any other content type) with the fields as query parameters.", "operationId": "createPasteInWorkspace", "parameters": [ @@ -1874,6 +1861,7 @@ } }, { + "description": "", "in": "query", "name": "content_type", "required": false, @@ -1905,6 +1893,7 @@ } }, { + "description": "", "in": "query", "name": "syntax_highlight", "required": false, @@ -2048,7 +2037,6 @@ "/api/v1/workspaces/{workspace_id}/pastes/deleted": { "get": { "callbacks": {}, - "deprecated": false, "operationId": "listRemovedPastesInWorkspace", "parameters": [ { @@ -2103,7 +2091,6 @@ "/api/v1/workspaces/{workspace_id}/pastes/deleted/{id}": { "delete": { "callbacks": {}, - "deprecated": false, "operationId": "purgePasteInWorkspace", "parameters": [ { @@ -2181,7 +2168,6 @@ "/api/v1/workspaces/{workspace_id}/pastes/deleted/{id}/restore": { "post": { "callbacks": {}, - "deprecated": false, "operationId": "restorePasteInWorkspace", "parameters": [ { @@ -2256,7 +2242,6 @@ "/api/v1/workspaces/{workspace_id}/pastes/{id}": { "delete": { "callbacks": {}, - "deprecated": false, "operationId": "deletePasteInWorkspace", "parameters": [ { @@ -2322,7 +2307,6 @@ }, "get": { "callbacks": {}, - "deprecated": false, "operationId": "getPasteInWorkspace", "parameters": [ { @@ -2397,7 +2381,6 @@ "/api/v1/workspaces/{workspace_id}/recovery": { "post": { "callbacks": {}, - "deprecated": false, "operationId": "recoverWorkspaceAccess", "parameters": [ { @@ -2452,7 +2435,6 @@ "/pastes/{id}/raw": { "get": { "callbacks": {}, - "deprecated": false, "description": "Uses the browser session or the paste's link, not a bearer token. Text that is safe to show inline is served with its content type; anything else downloads.", "operationId": "getRawPaste", "parameters": [ diff --git a/test/textbin_web/api_contract_test.exs b/test/textbin_web/api_contract_test.exs index 9da411e..27f1994 100644 --- a/test/textbin_web/api_contract_test.exs +++ b/test/textbin_web/api_contract_test.exs @@ -1,8 +1,8 @@ defmodule TextbinWeb.ApiContractTest do @moduledoc """ - Keeps `TextbinWeb.ApiSpec` and the server in step: the document covers - every API route, the exported `priv/openapi.json` is current, and each - operation's real responses match their documented schemas. + Keeps `TextbinWeb.ApiSpec` and the server in step: every API action + declares its operation, the exported `priv/openapi.json` is current, and + each operation's real responses match their documented schemas. """ use TextbinWeb.ConnCase, async: false @@ -19,21 +19,15 @@ defmodule TextbinWeb.ApiContractTest do @exported_spec Path.expand("../../priv/openapi.json", __DIR__) describe "the document" do - test "documents every API route and nothing else" do - routed = - for route <- Phoenix.Router.routes(TextbinWeb.Router), - String.starts_with?(route.path, "/api/") or route.path == "/pastes/:id/raw", - into: MapSet.new(), - do: {route.verb, Regex.replace(~r/:(\w+)/, route.path, "{\\1}")} - - documented = - MapSet.new(ApiSpec.operations(), fn {method, path, _operation} -> {method, path} end) - - assert MapSet.difference(routed, documented) == MapSet.new(), - "routes missing from TextbinWeb.ApiSpec" - - assert MapSet.difference(documented, routed) == MapSet.new(), - "operations in TextbinWeb.ApiSpec with no route" + test "every API route's action declares its operation" do + # The document is built from the router, so an action without an + # `operation` would silently drop out of it instead of failing. + for route <- Phoenix.Router.routes(TextbinWeb.Router), + String.starts_with?(route.path, "/api/") or route.path == "/pastes/:id/raw" do + assert function_exported?(route.plug, :open_api_operation, 1) and + match?(%OpenApiSpex.Operation{}, route.plug.open_api_operation(route.plug_opts)), + "#{inspect(route.plug)}.#{route.plug_opts} (#{route.verb} #{route.path}) has no `operation` spec" + end end test "gives each operation a unique id and resolves every schema reference" do From c9abcd23338168d908e518800b196bfd3fed7d72 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:23:38 +0000 Subject: [PATCH 3/7] feat!: reject API requests that do not match the OpenAPI document Each API controller now runs TextbinWeb.ApiRequestValidation, which checks path parameters, query parameters and JSON bodies against the operation the action declares, and rejects anything else with a 422 in the existing ValidationError shape ({"errors": {"field": ["message"]}}). An undocumented parameter no longer works, so the document and the accepted input cannot drift apart. - The router's :api pipeline puts TextbinWeb.ApiSpec on the conn. - Request schemas are closed (additionalProperties: false), and an operation that declares no query parameters accepts none. - Raw paste uploads keep their content type, which is stored with the paste: open_api_spex matches bodies by exact content type, so a body the operation only lists as */* skips body validation. Its query parameters are still checked. - Every validated operation documents its 422. Breaking changes (the API is pre-1.0): - Request bodies must be JSON. The {"paste": {...}} wrapper and bare JSON string bodies are rejected; send {"data": ...}. - A malformed UUID in the path is a 422 instead of a 400 (or a 204 for delete). The controllers' own UUID checks are gone. - POST /api/v1/auth/tokens answers 422 for a missing email or password (was 400), for a non-string name (was silently defaulted), and renders token-creation failures as field errors. Tests send JSON through a json_conn/1 helper and expect the new statuses. The CLI was checked end to end against a local server: whoami, create with title and reference, show, delete, deleted, restore and permanent delete all pass, and bad ids or unknown parameters come back as readable field errors. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019fnjMN7WpEjQJD2zveakV6 --- lib/textbin_web/api_docs.ex | 14 +- lib/textbin_web/api_request_validation.ex | 103 ++++++++ lib/textbin_web/api_spec/schemas.ex | 6 +- .../controllers/apiv1/auth_controller.ex | 27 +-- .../apiv1/organization_controller.ex | 34 +-- .../controllers/apiv1/paste_controller.ex | 121 ++++------ lib/textbin_web/router.ex | 2 + priv/openapi.json | 224 +++++++++++++----- test/support/conn_case.ex | 9 + test/textbin_web/api_contract_test.exs | 16 +- .../apiv1/auth_controller_test.exs | 23 +- .../apiv1/organization_controller_test.exs | 17 +- .../apiv1/paste_controller_test.exs | 64 +++-- 13 files changed, 434 insertions(+), 226 deletions(-) create mode 100644 lib/textbin_web/api_request_validation.ex diff --git a/lib/textbin_web/api_docs.ex b/lib/textbin_web/api_docs.ex index 2974444..b2a57c6 100644 --- a/lib/textbin_web/api_docs.ex +++ b/lib/textbin_web/api_docs.ex @@ -45,11 +45,19 @@ defmodule TextbinWeb.ApiDocs do end @doc """ - Adds the responses every paste operation shares. An operation's own - wording for a status wins. + Adds the responses every authenticated, validated operation shares: a + missing token, and a request that does not match the operation. An + operation's own wording for a status wins. """ + def with_common_errors(responses) do + Map.merge(%{401 => unauthorized(), 422 => invalid_request()}, responses) + end + + @doc "`with_common_errors/1` plus the 404 for a workspace the caller cannot reach." def with_scope_errors(responses) do - Map.merge(%{401 => unauthorized(), 404 => error("Workspace not found")}, responses) + responses + |> Map.put_new(404, error("Workspace not found")) + |> with_common_errors() end @doc "The optional paste fields as query parameters, for raw-body uploads." diff --git a/lib/textbin_web/api_request_validation.ex b/lib/textbin_web/api_request_validation.ex new file mode 100644 index 0000000..381364e --- /dev/null +++ b/lib/textbin_web/api_request_validation.ex @@ -0,0 +1,103 @@ +defmodule TextbinWeb.ApiRequestValidation do + @moduledoc """ + Rejects API requests that do not match the operation the action declares + with `operation`, before the action runs. + + The operation is the contract: path and query parameters, and a JSON body, + are validated against it, and anything it does not describe is rejected + with a 422 in the `ValidationError` shape. That keeps the document and the + accepted input from drifting apart, because an undocumented parameter + simply does not work. + + Raw uploads are the one exception to body validation. Paste creation + accepts the content itself with any content type (the type is stored with + the paste), and open_api_spex only matches request bodies by exact content + type, so a body whose type the operation lists only as `*/*` skips body + validation; its query parameters are still checked. + + Validation leaves `conn.params` as the strings the action already expects. + """ + + @behaviour Plug + + import Plug.Conn + + alias OpenApiSpex.Cast.Error + alias OpenApiSpex.Operation + alias OpenApiSpex.Plug.PutApiSpec + alias OpenApiSpex.RequestBody + + @impl Plug + def init(opts), do: opts + + @impl Plug + def call(%{private: %{phoenix_controller: controller, phoenix_action: action}} = conn, _opts) do + {spec, _operation_lookup} = PutApiSpec.get_spec_and_operation_lookup(conn) + content_type = request_content_type(conn) + operation = controller.open_api_operation(action) |> for_content_type(content_type) + + errors = + case OpenApiSpex.cast_and_validate(spec, operation, conn, content_type, + replace_params: false + ) do + {:ok, _conn} -> [] + {:error, errors} -> errors + end + + case errors ++ undeclared_query_errors(conn, operation) do + [] -> conn + errors -> reject(conn, errors) + end + end + + defp request_content_type(conn) do + case get_req_header(conn, "content-type") do + [value | _rest] -> value |> String.split(";") |> hd() |> String.trim() |> String.downcase() + [] -> nil + end + end + + defp for_content_type( + %Operation{requestBody: %RequestBody{content: content}} = operation, + content_type + ) + when is_binary(content_type) do + if Map.has_key?(content, content_type) or not Map.has_key?(content, "*/*"), + do: operation, + else: %{operation | requestBody: nil} + end + + defp for_content_type(operation, _content_type), do: operation + + # open_api_spex only checks query parameters for operations that declare + # some; an operation that declares none should accept none either. + defp undeclared_query_errors(conn, %Operation{parameters: parameters}) do + if Enum.any?(parameters, &(&1.in == :query)) do + [] + else + conn = fetch_query_params(conn) + + for name <- Map.keys(conn.query_params) do + %Error{reason: :unexpected_field, name: name, path: [name]} + end + end + end + + defp reject(conn, errors) do + body = %{ + errors: + Enum.group_by(errors, &field/1, fn error -> + error |> Error.message() |> String.trim() + end) + } + + conn + |> put_status(:unprocessable_entity) + |> Phoenix.Controller.json(body) + |> halt() + end + + defp field(%Error{path: [_ | _] = path}), do: Enum.map_join(path, ".", &to_string/1) + defp field(%Error{name: name}) when not is_nil(name), do: to_string(name) + defp field(%Error{}), do: "request" +end diff --git a/lib/textbin_web/api_spec/schemas.ex b/lib/textbin_web/api_spec/schemas.ex index ca7543e..600bd0d 100644 --- a/lib/textbin_web/api_spec/schemas.ex +++ b/lib/textbin_web/api_spec/schemas.ex @@ -5,8 +5,8 @@ defmodule TextbinWeb.ApiSpec.Schemas do Response objects set `additionalProperties: false` on purpose: the contract tests cast real responses against these schemas, so a field added to a JSON view without being documented here fails the build instead of drifting - silently. Request objects stay open because the server ignores unknown - fields. + silently. Request objects are closed too: request validation rejects any + field the document does not describe. """ alias OpenApiSpex.{Reference, Schema} @@ -73,6 +73,7 @@ defmodule TextbinWeb.ApiSpec.Schemas do %Schema{ title: "CreateTokenRequest", type: :object, + additionalProperties: false, required: [:email, :password], properties: %{ email: %Schema{type: :string, format: :email}, @@ -259,6 +260,7 @@ defmodule TextbinWeb.ApiSpec.Schemas do "JSON form of a new paste. To upload raw bytes instead, send them as the request body with any other content type and pass these fields as query parameters.", type: :object, required: [:data], + additionalProperties: false, properties: Map.merge(create_paste_fields(), %{ data: %Schema{type: :string, description: "The paste content"} diff --git a/lib/textbin_web/controllers/apiv1/auth_controller.ex b/lib/textbin_web/controllers/apiv1/auth_controller.ex index b3fbf94..3cb10f8 100644 --- a/lib/textbin_web/controllers/apiv1/auth_controller.ex +++ b/lib/textbin_web/controllers/apiv1/auth_controller.ex @@ -9,6 +9,8 @@ defmodule TextbinWeb.ApiV1.AuthController do tags ["Authentication"] + plug TextbinWeb.ApiRequestValidation + operation :create, operation_id: "createApiToken", summary: "Create an API token with an email and password", @@ -16,32 +18,22 @@ defmodule TextbinWeb.ApiV1.AuthController do request_body: json_body("CreateTokenRequest"), responses: %{ 201 => response("The token and the account it belongs to", "CreatedTokenResponse"), - 400 => error("Email or password missing"), 401 => error("Wrong email or password"), - 422 => error("The token could not be created") + 422 => invalid_request() } - def create(conn, %{"email" => email, "password" => password} = params) - when is_binary(email) and is_binary(password) do + # The body has been validated against CreateTokenRequest. + def create(conn, %{"email" => email, "password" => password} = params) do case Accounts.get_user_by_email_and_password(email, password) do %User{} = user -> create_token(conn, user, params) nil -> render_invalid_credentials(conn) end end - def create(conn, _params) do - conn - |> put_status(:bad_request) - |> json(%{errors: %{detail: "Email and password are required"}}) - end - operation :show, operation_id: "getIdentity", summary: "Show the authenticated account and token", - responses: %{ - 200 => response("The current identity", "IdentityResponse"), - 401 => unauthorized() - } + responses: with_common_errors(%{200 => response("The current identity", "IdentityResponse")}) def show(%{assigns: %{current_scope: %Scope{user: %User{} = user}}} = conn, _params) do user_token = conn.assigns.current_api_token @@ -51,7 +43,7 @@ defmodule TextbinWeb.ApiV1.AuthController do operation :delete, operation_id: "revokeCurrentToken", summary: "Revoke the token used for this request", - responses: %{204 => no_content("Revoked, or already gone"), 401 => unauthorized()} + responses: with_common_errors(%{204 => no_content("Revoked, or already gone")}) def delete( %{assigns: %{current_scope: %Scope{user: %User{} = user}, current_api_token: user_token}} = @@ -77,10 +69,11 @@ defmodule TextbinWeb.ApiV1.AuthController do |> put_status(:created) |> json(%{data: Map.put(identity_data(user, user_token), :api_token, token)}) - {:error, _changeset} -> + {:error, changeset} -> conn |> put_status(:unprocessable_entity) - |> json(%{errors: %{detail: "Could not create API token"}}) + |> put_view(json: TextbinWeb.ChangesetJSON) + |> render(:error, changeset: changeset) end end diff --git a/lib/textbin_web/controllers/apiv1/organization_controller.ex b/lib/textbin_web/controllers/apiv1/organization_controller.ex index f81573b..03a2d7a 100644 --- a/lib/textbin_web/controllers/apiv1/organization_controller.ex +++ b/lib/textbin_web/controllers/apiv1/organization_controller.ex @@ -10,10 +10,12 @@ defmodule TextbinWeb.ApiV1.OrganizationController do tags ["Organizations"] + plug TextbinWeb.ApiRequestValidation + operation :index, operation_id: "listOrganizations", summary: "List the organizations you belong to", - responses: %{200 => response("Organizations", "OrganizationList"), 401 => unauthorized()} + responses: with_common_errors(%{200 => response("Organizations", "OrganizationList")}) def index(conn, _params) do with_api_scope(conn, fn scope -> @@ -25,11 +27,11 @@ defmodule TextbinWeb.ApiV1.OrganizationController do operation_id: "listOrganizationWorkspaces", summary: "List workspaces you have joined or can join", parameters: [id: uuid_path("Organization id")], - responses: %{ - 200 => response("The organization and its workspaces", "WorkspaceList"), - 401 => unauthorized(), - 404 => error("Organization not found") - } + responses: + with_common_errors(%{ + 200 => response("The organization and its workspaces", "WorkspaceList"), + 404 => error("Organization not found") + }) def workspaces(conn, %{"id" => id}) do with_api_scope(conn, fn scope -> @@ -53,11 +55,11 @@ defmodule TextbinWeb.ApiV1.OrganizationController do limit: query(%Schema{type: :integer, minimum: 1}, "Events per page"), cursor: query(%Schema{type: :string}, "next_cursor from the previous page") ], - responses: %{ - 200 => response("A page of audit events", "AuditEventPage"), - 401 => unauthorized(), - 404 => error("Organization not found, or you cannot read its audit log") - } + responses: + with_common_errors(%{ + 200 => response("A page of audit events", "AuditEventPage"), + 404 => error("Organization not found, or you cannot read its audit log") + }) def audit_events(conn, %{"id" => id}) do with_api_scope(conn, fn scope -> @@ -81,11 +83,11 @@ defmodule TextbinWeb.ApiV1.OrganizationController do operation_id: "recoverWorkspaceAccess", summary: "Make yourself owner of a workspace in an organization you own", parameters: [workspace_id: uuid_path("Workspace id")], - responses: %{ - 201 => response("Your new membership", "WorkspaceRecovery"), - 401 => unauthorized(), - 404 => error("Workspace not found, or you cannot recover it") - } + responses: + with_common_errors(%{ + 201 => response("Your new membership", "WorkspaceRecovery"), + 404 => error("Workspace not found, or you cannot recover it") + }) def recover_workspace(conn, %{"workspace_id" => workspace_id}) do with_api_scope(conn, fn scope -> diff --git a/lib/textbin_web/controllers/apiv1/paste_controller.ex b/lib/textbin_web/controllers/apiv1/paste_controller.ex index f9e6883..c62fd94 100644 --- a/lib/textbin_web/controllers/apiv1/paste_controller.ex +++ b/lib/textbin_web/controllers/apiv1/paste_controller.ex @@ -22,6 +22,8 @@ defmodule TextbinWeb.ApiV1.PasteController do tags ["Pastes"] + plug TextbinWeb.ApiRequestValidation + # Every paste operation exists twice: for the caller's personal workspace, # and under /workspaces/{workspace_id}. The workspace_* actions share the # personal actions' behaviour and spec; the workspace_id path parameter is @@ -44,7 +46,6 @@ defmodule TextbinWeb.ApiV1.PasteController do 201 => response("The new paste, without its content", "PasteMetadataResponse"), 400 => error("The request body could not be read"), 413 => error("The content is larger than the server allows"), - 422 => response("Rejected fields", "ValidationError"), 503 => error("Paste storage is temporarily unavailable") }) ] @@ -56,7 +57,6 @@ defmodule TextbinWeb.ApiV1.PasteController do responses: with_scope_errors(%{ 200 => response("The paste", "PasteResponse"), - 400 => error("The id is not a UUID"), 404 => error("Paste or workspace not found") }) ] @@ -85,7 +85,6 @@ defmodule TextbinWeb.ApiV1.PasteController do responses: with_scope_errors(%{ 200 => response("The restored paste, without its content", "PasteMetadataResponse"), - 400 => error("The id is not a UUID"), 404 => error("No restorable paste you can manage has this id") }) ] @@ -97,7 +96,6 @@ defmodule TextbinWeb.ApiV1.PasteController do responses: with_scope_errors(%{ 204 => no_content("Deleted for good"), - 400 => error("The id is not a UUID"), 404 => error("No removed paste you can manage has this id"), 503 => error("The stored content could not be deleted; try again") }) @@ -172,47 +170,27 @@ defmodule TextbinWeb.ApiV1.PasteController do end end - # Preferred JSON shape for API clients that already send objects. + # A JSON body has been validated against CreatePasteRequest, so it is an + # object with string data. defp paste_attrs(conn, %{"data" => data} = params) when is_binary(data) do {:ok, {:data, build_paste_attrs(data, params)}, conn} end - # Keep Phoenix generator-style wrapped params working for conventional clients - # and tests. - defp paste_attrs(conn, %{"paste" => %{"data" => data} = params}) when is_binary(data) do - {:ok, {:data, build_paste_attrs(data, params)}, conn} - end - - # Plug puts non-object JSON bodies, such as `"hello"`, under "_json". This is - # useful for callers that want JSON content negotiation without an object - # envelope. - defp paste_attrs(conn, %{"_json" => data} = params) when is_binary(data) do - {:ok, {:data, build_paste_attrs(data, params)}, conn} - end - - # Non-JSON uploads keep the body unread after Plug.Parsers, so streamed - # CLI/stdin data can be consumed here. + # Any other content type is the paste itself, left unread by Plug.Parsers so + # streamed CLI/stdin data can be spooled here. Its fields arrive as query + # parameters, already validated. defp paste_attrs(conn, params) do - metadata_keys = - ~w(title reference_url syntax_highlight content_type audience visibility expires_in ttl) + case read_request_body(conn) do + {:ok, path, metadata, conn} -> + paste_attrs = + params + |> build_paste_attrs() + |> Map.put_new("content_type", request_content_type(conn)) - if map_size(params) == 0 or Enum.any?(metadata_keys, &Map.has_key?(params, &1)) do - case read_request_body(conn) do - {:ok, path, metadata, conn} -> - paste_attrs = - params - |> build_paste_attrs() - |> Map.put_new("content_type", request_content_type(conn)) + {:ok, {:file, path, metadata, paste_attrs}, conn} - {:ok, {:file, path, metadata, paste_attrs}, conn} - - {:error, reason, conn} -> - {:error, reason, conn} - end - else - # Let the changeset produce the canonical "data can't be blank" error for - # unsupported payload shapes instead of inventing a separate controller error. - {:ok, {:data, %{"data" => nil}}, conn} + {:error, reason, conn} -> + {:error, reason, conn} end end @@ -421,17 +399,18 @@ defmodule TextbinWeb.ApiV1.PasteController do with_paste_scope(conn, params, &show_scoped_paste(conn, &1, id)) end + # Paste ids are UUIDs here: ApiRequestValidation rejects anything else. defp show_scoped_paste(conn, current_scope, id) do - with {:ok, paste_id} <- Ecto.UUID.cast(id), - %{} = paste <- Pastes.get_paste(current_scope, paste_id) do - render(conn, :show, - paste: paste, - organization_id: current_scope.organization.id, - workspace_id: current_scope.workspace.id - ) - else - :error -> render_invalid_paste_id(conn) - nil -> render_paste_not_found(conn) + case Pastes.get_paste(current_scope, id) do + %{} = paste -> + render(conn, :show, + paste: paste, + organization_id: current_scope.organization.id, + workspace_id: current_scope.workspace.id + ) + + nil -> + render_paste_not_found(conn) end end @@ -439,12 +418,7 @@ defmodule TextbinWeb.ApiV1.PasteController do def delete(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn current_scope -> - result = - with {:ok, paste_id} <- Ecto.UUID.cast(id) do - delete_scoped_paste(current_scope, %Paste{id: paste_id}, params) - end - - case result do + case delete_scoped_paste(current_scope, %Paste{id: id}, params) do {:error, :not_found} -> send_resp(conn, :no_content, "") @@ -485,16 +459,16 @@ defmodule TextbinWeb.ApiV1.PasteController do def restore(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn scope -> - with {:ok, paste_id} <- Ecto.UUID.cast(id), - {:ok, paste} <- Pastes.restore_paste(scope, paste_id) do - render(conn, :restored, - paste: paste, - organization_id: scope.organization.id, - workspace_id: scope.workspace.id - ) - else - :error -> render_invalid_paste_id(conn) - {:error, _reason} -> render_removed_paste_not_found(conn) + case Pastes.restore_paste(scope, id) do + {:ok, paste} -> + render(conn, :restored, + paste: paste, + organization_id: scope.organization.id, + workspace_id: scope.workspace.id + ) + + {:error, _reason} -> + render_removed_paste_not_found(conn) end end) end @@ -503,12 +477,9 @@ defmodule TextbinWeb.ApiV1.PasteController do def purge(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn scope -> - with {:ok, paste_id} <- Ecto.UUID.cast(id), - {:ok, _paste} <- Pastes.purge_paste(scope, paste_id) do - send_resp(conn, :no_content, "") - else - :error -> - render_invalid_paste_id(conn) + case Pastes.purge_paste(scope, id) do + {:ok, _paste} -> + send_resp(conn, :no_content, "") {:error, :not_found} -> render_removed_paste_not_found(conn) @@ -576,12 +547,6 @@ defmodule TextbinWeb.ApiV1.PasteController do defp then_chmod(:ok, path), do: File.chmod(path, 0o700) defp then_chmod({:error, reason}, _path), do: {:error, reason} - defp render_invalid_paste_id(conn) do - conn - |> put_status(:bad_request) - |> json(%{errors: %{detail: "Paste id must be a valid UUID"}}) - end - defp render_paste_not_found(conn) do conn |> put_status(:not_found) @@ -598,9 +563,7 @@ defmodule TextbinWeb.ApiV1.PasteController do end defp maybe_alias_legacy_visibility_errors(changeset, params) do - params = Map.get(params, "paste", params) - - if is_map(params) and Map.has_key?(params, "visibility") and + if Map.has_key?(params, "visibility") and not Map.has_key?(params, "audience") do errors = Enum.map(changeset.errors, fn diff --git a/lib/textbin_web/router.ex b/lib/textbin_web/router.ex index 02bdada..3f9bfbf 100644 --- a/lib/textbin_web/router.ex +++ b/lib/textbin_web/router.ex @@ -16,6 +16,8 @@ defmodule TextbinWeb.Router do pipeline :api do plug :accepts, ["json"] plug :fetch_current_scope_for_api_token + # API controllers validate each request against this document. + plug OpenApiSpex.Plug.PutApiSpec, module: TextbinWeb.ApiSpec end pipeline :require_authenticated_api do diff --git a/priv/openapi.json b/priv/openapi.json index 710651d..46a90c9 100644 --- a/priv/openapi.json +++ b/priv/openapi.json @@ -69,6 +69,7 @@ "type": "object" }, "CreatePasteRequest": { + "additionalProperties": false, "description": "JSON form of a new paste. To upload raw bytes instead, send them as the request body with any other content type and pass these fields as query parameters.", "properties": { "audience": { @@ -129,6 +130,7 @@ "type": "object" }, "CreateTokenRequest": { + "additionalProperties": false, "properties": { "email": { "format": "email", @@ -991,16 +993,6 @@ }, "description": "The token and the account it belongs to" }, - "400": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "Email or password missing" - }, "401": { "content": { "application/json": { @@ -1015,11 +1007,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "The token could not be created" + "description": "The parameters or body do not match this operation" } }, "security": [], @@ -1054,6 +1046,16 @@ } }, "description": "Missing, invalid or revoked API token" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "Show the authenticated account and token", @@ -1080,6 +1082,16 @@ } }, "description": "Missing, invalid or revoked API token" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "Revoke the token used for this request", @@ -1113,6 +1125,16 @@ } }, "description": "Missing, invalid or revoked API token" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "List the organizations you belong to", @@ -1186,6 +1208,16 @@ } }, "description": "Organization not found, or you cannot read its audit log" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "Page through an organization's audit log", @@ -1240,6 +1272,16 @@ } }, "description": "Organization not found" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "List workspaces you have joined or can join", @@ -1283,6 +1325,16 @@ } }, "description": "Workspace not found" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "List pastes in your personal workspace", @@ -1465,7 +1517,7 @@ } } }, - "description": "Rejected fields" + "description": "The parameters or body do not match this operation" }, "503": { "content": { @@ -1519,6 +1571,16 @@ } }, "description": "Workspace not found" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "List deleted and expired pastes that can still be restored", @@ -1547,7 +1609,7 @@ "204": { "description": "Deleted for good" }, - "400": { + "401": { "content": { "application/json": { "schema": { @@ -1555,9 +1617,9 @@ } } }, - "description": "The id is not a UUID" + "description": "Missing, invalid or revoked API token" }, - "401": { + "404": { "content": { "application/json": { "schema": { @@ -1565,17 +1627,17 @@ } } }, - "description": "Missing, invalid or revoked API token" + "description": "No removed paste you can manage has this id" }, - "404": { + "422": { "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "No removed paste you can manage has this id" + "description": "The parameters or body do not match this operation" }, "503": { "content": { @@ -1621,7 +1683,7 @@ }, "description": "The restored paste, without its content" }, - "400": { + "401": { "content": { "application/json": { "schema": { @@ -1629,9 +1691,9 @@ } } }, - "description": "The id is not a UUID" + "description": "Missing, invalid or revoked API token" }, - "401": { + "404": { "content": { "application/json": { "schema": { @@ -1639,17 +1701,17 @@ } } }, - "description": "Missing, invalid or revoked API token" + "description": "No restorable paste you can manage has this id" }, - "404": { + "422": { "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "No restorable paste you can manage has this id" + "description": "The parameters or body do not match this operation" } }, "summary": "Restore a deleted or expired paste", @@ -1698,6 +1760,16 @@ }, "description": "Workspace not found" }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" + }, "503": { "content": { "application/json": { @@ -1740,7 +1812,7 @@ }, "description": "The paste" }, - "400": { + "401": { "content": { "application/json": { "schema": { @@ -1748,9 +1820,9 @@ } } }, - "description": "The id is not a UUID" + "description": "Missing, invalid or revoked API token" }, - "401": { + "404": { "content": { "application/json": { "schema": { @@ -1758,17 +1830,17 @@ } } }, - "description": "Missing, invalid or revoked API token" + "description": "Paste or workspace not found" }, - "404": { + "422": { "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "Paste or workspace not found" + "description": "The parameters or body do not match this operation" } }, "summary": "Show a paste with its content", @@ -1823,6 +1895,16 @@ } }, "description": "Workspace not found" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "List pastes in a workspace", @@ -2015,7 +2097,7 @@ } } }, - "description": "Rejected fields" + "description": "The parameters or body do not match this operation" }, "503": { "content": { @@ -2080,6 +2162,16 @@ } }, "description": "Workspace not found" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "List deleted and expired pastes that can still be restored", @@ -2118,7 +2210,7 @@ "204": { "description": "Deleted for good" }, - "400": { + "401": { "content": { "application/json": { "schema": { @@ -2126,9 +2218,9 @@ } } }, - "description": "The id is not a UUID" + "description": "Missing, invalid or revoked API token" }, - "401": { + "404": { "content": { "application/json": { "schema": { @@ -2136,17 +2228,17 @@ } } }, - "description": "Missing, invalid or revoked API token" + "description": "No removed paste you can manage has this id" }, - "404": { + "422": { "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "No removed paste you can manage has this id" + "description": "The parameters or body do not match this operation" }, "503": { "content": { @@ -2202,7 +2294,7 @@ }, "description": "The restored paste, without its content" }, - "400": { + "401": { "content": { "application/json": { "schema": { @@ -2210,9 +2302,9 @@ } } }, - "description": "The id is not a UUID" + "description": "Missing, invalid or revoked API token" }, - "401": { + "404": { "content": { "application/json": { "schema": { @@ -2220,17 +2312,17 @@ } } }, - "description": "Missing, invalid or revoked API token" + "description": "No restorable paste you can manage has this id" }, - "404": { + "422": { "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "No restorable paste you can manage has this id" + "description": "The parameters or body do not match this operation" } }, "summary": "Restore a deleted or expired paste", @@ -2289,6 +2381,16 @@ }, "description": "Workspace not found" }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" + }, "503": { "content": { "application/json": { @@ -2341,7 +2443,7 @@ }, "description": "The paste" }, - "400": { + "401": { "content": { "application/json": { "schema": { @@ -2349,9 +2451,9 @@ } } }, - "description": "The id is not a UUID" + "description": "Missing, invalid or revoked API token" }, - "401": { + "404": { "content": { "application/json": { "schema": { @@ -2359,17 +2461,17 @@ } } }, - "description": "Missing, invalid or revoked API token" + "description": "Paste or workspace not found" }, - "404": { + "422": { "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Error" + "$ref": "#/components/schemas/ValidationError" } } }, - "description": "Paste or workspace not found" + "description": "The parameters or body do not match this operation" } }, "summary": "Show a paste with its content", @@ -2424,6 +2526,16 @@ } }, "description": "Workspace not found, or you cannot recover it" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationError" + } + } + }, + "description": "The parameters or body do not match this operation" } }, "summary": "Make yourself owner of a workspace in an organization you own", diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 25aa0d4..5b8f659 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -36,6 +36,15 @@ defmodule TextbinWeb.ConnCase do {:ok, conn: Phoenix.ConnTest.build_conn()} end + @doc """ + A connection that sends JSON, the only request body the API accepts. + Phoenix.ConnTest would otherwise send map params as multipart form data, + which API request validation rejects. + """ + def json_conn(conn \\ Phoenix.ConnTest.build_conn()) do + Plug.Conn.put_req_header(conn, "content-type", "application/json") + end + @doc """ Setup helper that registers and logs in users. diff --git a/test/textbin_web/api_contract_test.exs b/test/textbin_web/api_contract_test.exs index 27f1994..51c6146 100644 --- a/test/textbin_web/api_contract_test.exs +++ b/test/textbin_web/api_contract_test.exs @@ -62,22 +62,22 @@ defmodule TextbinWeb.ApiContractTest do test "createApiToken" do user = user_fixture() - build_conn() + json_conn() |> post(~p"/api/v1/auth/tokens", %{email: user.email, password: valid_user_password()}) |> assert_api_response("createApiToken") - build_conn() + json_conn() |> post(~p"/api/v1/auth/tokens", %{email: user.email, password: "wrong password"}) |> assert_api_response("createApiToken") - build_conn() + json_conn() |> post(~p"/api/v1/auth/tokens", %{}) |> assert_api_response("createApiToken") end test "getIdentity and revokeCurrentToken", %{conn: conn} do conn |> get(~p"/api/v1/me") |> assert_api_response("getIdentity") - build_conn() |> get(~p"/api/v1/me") |> assert_api_response("getIdentity") + json_conn() |> get(~p"/api/v1/me") |> assert_api_response("getIdentity") conn |> delete(~p"/api/v1/me/token") |> assert_api_response("revokeCurrentToken") end end @@ -203,7 +203,7 @@ defmodule TextbinWeb.ApiContractTest do conn |> get(~p"/api/v1/pastes/not-a-uuid") |> assert_api_response("getPaste") conn |> get(~p"/api/v1/pastes/#{Ecto.UUID.generate()}") |> assert_api_response("getPaste") conn |> delete(~p"/api/v1/pastes/#{id}") |> assert_api_response("deletePaste") - build_conn() |> get(~p"/api/v1/pastes") |> assert_api_response("listPastes") + json_conn() |> get(~p"/api/v1/pastes") |> assert_api_response("listPastes") end test "list, restore and purge removed pastes", %{conn: conn, scope: scope} do @@ -229,9 +229,9 @@ defmodule TextbinWeb.ApiContractTest do test "getRawPaste", %{scope: scope} do {:ok, paste} = Pastes.create_paste(scope, %{data: "shared", audience: "unlisted"}) - build_conn() |> get(~p"/pastes/#{paste.id}/raw") |> assert_api_response("getRawPaste") + json_conn() |> get(~p"/pastes/#{paste.id}/raw") |> assert_api_response("getRawPaste") - build_conn() + json_conn() |> get(~p"/pastes/#{Ecto.UUID.generate()}/raw") |> assert_api_response("getRawPaste") end @@ -289,7 +289,7 @@ defmodule TextbinWeb.ApiContractTest do %{ user: user, scope: user_scope_fixture(user), - conn: put_req_header(build_conn(), "authorization", "Bearer #{token}") + conn: put_req_header(json_conn(), "authorization", "Bearer #{token}") } end diff --git a/test/textbin_web/controllers/apiv1/auth_controller_test.exs b/test/textbin_web/controllers/apiv1/auth_controller_test.exs index fcd7bc3..097eba8 100644 --- a/test/textbin_web/controllers/apiv1/auth_controller_test.exs +++ b/test/textbin_web/controllers/apiv1/auth_controller_test.exs @@ -7,6 +7,9 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do import Textbin.AccountsFixtures + # The API accepts JSON bodies only. + setup %{conn: conn}, do: %{conn: json_conn(conn)} + setup do user = user_fixture() |> set_password() %{user: user} @@ -48,11 +51,10 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do test "requires email and password", %{conn: conn} do conn = post(conn, ~p"/api/v1/auth/tokens", %{}) - assert %{"errors" => %{"detail" => "Email and password are required"}} = - json_response(conn, 400) + assert %{"errors" => %{"email" => [_], "password" => [_]}} = json_response(conn, 422) end - test "uses the default token name for malformed names", %{conn: conn, user: user} do + test "rejects a name that is not a string", %{conn: conn, user: user} do conn = post(conn, ~p"/api/v1/auth/tokens", %{ email: user.email, @@ -60,6 +62,13 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do name: %{unexpected: "value"} }) + assert %{"errors" => %{"name" => [_]}} = json_response(conn, 422) + end + + test "uses the default token name when none is given", %{conn: conn, user: user} do + conn = + post(conn, ~p"/api/v1/auth/tokens", %{email: user.email, password: valid_user_password()}) + assert %{"token" => %{"name" => "Textbin CLI"}} = json_response(conn, 201)["data"] end end @@ -69,7 +78,7 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do {:ok, {token, user_token}} = Accounts.create_user_api_token(user, %{"name" => "CLI"}) %{ - conn: put_req_header(conn, "authorization", "Bearer #{token}"), + conn: put_req_header(json_conn(conn), "authorization", "Bearer #{token}"), token: token, user_token: user_token } @@ -94,7 +103,7 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do end test "requires an API token" do - conn = get(build_conn(), ~p"/api/v1/me") + conn = get(json_conn(), ~p"/api/v1/me") assert %{"errors" => %{"detail" => "API token required"}} = json_response(conn, 401) end @@ -106,7 +115,7 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do refute Repo.get(UserToken, user_token.id) conn = - build_conn() + json_conn() |> put_req_header("authorization", "Bearer #{token}") |> get(~p"/api/v1/me") @@ -115,7 +124,7 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do test "concurrent revocation is idempotent", %{user: user, user_token: user_token} do revoke = fn -> - build_conn() + json_conn() |> Plug.Conn.assign(:current_scope, Accounts.Scope.for_user(user)) |> Plug.Conn.assign(:current_api_token, user_token) |> TextbinWeb.ApiV1.AuthController.delete(%{}) diff --git a/test/textbin_web/controllers/apiv1/organization_controller_test.exs b/test/textbin_web/controllers/apiv1/organization_controller_test.exs index 8912586..aafc372 100644 --- a/test/textbin_web/controllers/apiv1/organization_controller_test.exs +++ b/test/textbin_web/controllers/apiv1/organization_controller_test.exs @@ -8,6 +8,9 @@ defmodule TextbinWeb.ApiV1.OrganizationControllerTest do import Textbin.AccountsFixtures + # The API accepts JSON bodies only. + setup %{conn: conn}, do: %{conn: json_conn(conn)} + setup do user = user_fixture() scope = user_scope_fixture(user) @@ -89,10 +92,16 @@ defmodule TextbinWeb.ApiV1.OrganizationControllerTest do assert %{"errors" => %{"detail" => "Organization not found"}} = json_response(conn, 404) end - test "returns not found for an invalid organization id", %{conn: conn} do + test "rejects an organization id that is not a UUID", %{conn: conn} do conn = get(conn, ~p"/api/v1/organizations/not-a-uuid/workspaces") - assert %{"errors" => %{"detail" => "Organization not found"}} = json_response(conn, 404) + assert %{"errors" => %{"id" => [_message]}} = json_response(conn, 422) + end + + test "rejects query parameters the operation does not declare", %{conn: conn} do + conn = get(conn, ~p"/api/v1/organizations?unexpected=1") + + assert %{"errors" => %{"unexpected" => [_message]}} = json_response(conn, 422) end test "organization owners recover private workspace ownership and read the audit event", @@ -221,13 +230,13 @@ defmodule TextbinWeb.ApiV1.OrganizationControllerTest do end test "requires an API token" do - conn = get(build_conn(), ~p"/api/v1/organizations") + conn = get(json_conn(), ~p"/api/v1/organizations") assert %{"errors" => %{"detail" => "API token required"}} = json_response(conn, 401) end defp api_conn(user) do {:ok, {token, _user_token}} = Accounts.create_user_api_token(user, %{"name" => "API"}) - put_req_header(build_conn(), "authorization", "Bearer #{token}") + put_req_header(json_conn(), "authorization", "Bearer #{token}") end end diff --git a/test/textbin_web/controllers/apiv1/paste_controller_test.exs b/test/textbin_web/controllers/apiv1/paste_controller_test.exs index 1b2a968..71df261 100644 --- a/test/textbin_web/controllers/apiv1/paste_controller_test.exs +++ b/test/textbin_web/controllers/apiv1/paste_controller_test.exs @@ -22,7 +22,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do {:ok, {token, user_token}} = Accounts.create_user_api_token(user, %{"name" => "CLI"}) %{ - conn: put_req_header(conn, "authorization", "Bearer #{token}"), + conn: put_req_header(json_conn(conn), "authorization", "Bearer #{token}"), scope: scope, user_token: user_token } @@ -45,7 +45,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do end test "requires an API token", %{conn: _conn} do - conn = get(build_conn(), ~p"/api/v1/pastes") + conn = get(json_conn(), ~p"/api/v1/pastes") assert %{"errors" => %{"detail" => "API token required"}} = json_response(conn, 401) end @@ -53,7 +53,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do describe "create paste" do test "renders paste when data is valid", %{conn: conn, scope: scope} do - conn = post(conn, ~p"/api/v1/pastes", paste: @create_attrs) + conn = post(conn, ~p"/api/v1/pastes", @create_attrs) personal_organization = Organizations.get_personal_organization!(scope.user) personal_workspace = Organizations.get_personal_default_workspace!(scope.user) @@ -132,17 +132,12 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do assert_stored_paste(scope, id, "some data", "elixir") end - test "renders paste from JSON string body", %{conn: conn, scope: scope} do - conn = - conn - |> put_req_header("content-type", "application/json") - |> post(~p"/api/v1/pastes", Jason.encode!("json string data")) + test "rejects JSON that is not a paste object", %{conn: conn} do + for body <- [Jason.encode!("json string data"), Jason.encode!(%{paste: %{data: "x"}})] do + response_conn = post(conn, ~p"/api/v1/pastes", body) - assert %{"id" => id} = response_data = json_response(conn, 201)["data"] - assert response_data["syntax_highlight"] == "plain" - assert is_nil(response_data["expires_at"]) - refute Map.has_key?(response_data, "data") - assert_stored_paste(scope, id, "json string data", "plain") + assert %{"errors" => %{}} = json_response(response_conn, 422) + end end test "renders paste from raw request body", %{conn: conn, scope: scope} do @@ -313,7 +308,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do {:ok, _user} = Accounts.update_user_paste_defaults(scope.user, %{default_paste_ttl: "30d"}) - conn = post(conn, ~p"/api/v1/pastes", paste: @create_attrs) + conn = post(conn, ~p"/api/v1/pastes", @create_attrs) assert %{"id" => id, "expires_at" => expires_at} = json_response(conn, 201)["data"] assert_millisecond_timestamp(expires_at) @@ -339,7 +334,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do end test "accepts a valid API bearer token", %{conn: conn, scope: scope, user_token: user_token} do - conn = post(conn, ~p"/api/v1/pastes", paste: @create_attrs) + conn = post(conn, ~p"/api/v1/pastes", @create_attrs) assert %{"id" => id} = json_response(conn, 201)["data"] assert_stored_paste(scope, id, "some data", "elixir") @@ -348,31 +343,31 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do test "rejects an invalid API bearer token" do conn = - build_conn() + json_conn() |> put_req_header("authorization", "Bearer txb_invalid") - |> post(~p"/api/v1/pastes", paste: @create_attrs) + |> post(~p"/api/v1/pastes", @create_attrs) assert %{"errors" => %{"detail" => "Invalid API token"}} = json_response(conn, 401) end test "rejects a malformed authorization header" do conn = - build_conn() + json_conn() |> put_req_header("authorization", "Token invalid") - |> post(~p"/api/v1/pastes", paste: @create_attrs) + |> post(~p"/api/v1/pastes", @create_attrs) assert %{"errors" => %{"detail" => "Invalid authorization header"}} = json_response(conn, 401) end test "renders errors when data is invalid", %{conn: conn} do - conn = post(conn, ~p"/api/v1/pastes", paste: @invalid_attrs) + conn = post(conn, ~p"/api/v1/pastes", @invalid_attrs) assert %{"data" => [_]} = json_response(conn, 422)["errors"] end test "renders errors when expiration is invalid", %{conn: conn} do - conn = post(conn, ~p"/api/v1/pastes", paste: Map.put(@create_attrs, :expires_in, "forever")) + conn = post(conn, ~p"/api/v1/pastes", Map.put(@create_attrs, :expires_in, "forever")) assert %{"expires_at" => ["must use one of: never, 10m, 1h, 6h, 12h, 1d, 7d, 30d"]} = json_response(conn, 422)["errors"] @@ -381,7 +376,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do test "renders errors when visibility is invalid", %{conn: conn} do conn = post(conn, ~p"/api/v1/pastes", Map.put(@create_attrs, :visibility, "secret")) - assert %{"visibility" => ["is invalid"]} = json_response(conn, 422)["errors"] + assert %{"visibility" => [_message]} = json_response(conn, 422)["errors"] end test "rejects JSON data over the configured size limit", %{conn: conn} do @@ -485,14 +480,14 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do end describe "show paste" do - test "returns bad request when id is not a UUID", %{conn: conn} do + test "rejects an id that is not a UUID", %{conn: conn} do conn = get(conn, ~p"/api/v1/pastes/not-a-uuid") assert %{ "errors" => %{ - "detail" => "Paste id must be a valid UUID" + "id" => [_message] } - } = json_response(conn, 400) + } = json_response(conn, 422) end test "returns not found when paste does not exist", %{conn: conn} do @@ -608,7 +603,7 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do assert Pastes.get_paste(scope, active.id) assert Repo.get(Paste, other.id) - assert conn |> post(~p"/api/v1/pastes/deleted/not-a-uuid/restore") |> json_response(400) + assert conn |> post(~p"/api/v1/pastes/deleted/not-a-uuid/restore") |> json_response(422) end test "workspace routes act on that workspace's removed pastes", %{ @@ -711,10 +706,10 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do assert Repo.get(Paste, paste.id) end - test "returns no content when id is not a UUID", %{conn: conn} do + test "rejects an id that is not a UUID", %{conn: conn} do conn = delete(conn, ~p"/api/v1/pastes/not-a-uuid") - assert response(conn, 204) == "" + assert %{"errors" => %{"id" => [_message]}} = json_response(conn, 422) end test "returns no content when paste does not exist", %{conn: conn} do @@ -895,12 +890,13 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do outsider_scope = user_scope_fixture() outsider_workspace = Organizations.get_personal_default_workspace!(outsider_scope.user) - for workspace_id <- ["not-a-uuid", outsider_workspace.id] do - response_conn = get(conn, ~p"/api/v1/workspaces/#{workspace_id}/pastes") + response_conn = get(conn, ~p"/api/v1/workspaces/#{outsider_workspace.id}/pastes") - assert %{"errors" => %{"detail" => "Workspace not found"}} = - json_response(response_conn, 404) - end + assert %{"errors" => %{"detail" => "Workspace not found"}} = + json_response(response_conn, 404) + + response_conn = get(conn, ~p"/api/v1/workspaces/not-a-uuid/pastes") + assert %{"errors" => %{"workspace_id" => [_message]}} = json_response(response_conn, 422) end end @@ -950,6 +946,6 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do defp api_conn(user) do {:ok, {token, _user_token}} = Accounts.create_user_api_token(user, %{"name" => "API"}) - put_req_header(build_conn(), "authorization", "Bearer #{token}") + put_req_header(json_conn(), "authorization", "Bearer #{token}") end end From 587dacd38471d91282cbe5fa2515b1dd57709168 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:32:42 +0000 Subject: [PATCH 4/7] fix: stop documenting API responses the server cannot send Checking which documented responses the test suite actually produces turned up two that cannot happen: - "Workspace not found" (404) on the personal-workspace paste operations. The personal workspace always exists; the 404 now belongs to the workspace-scoped variants only, added by in_workspace/1. - deletePaste's 503. Since the soft-delete lifecycle, deleting only marks a paste removed and never touches storage, so there is no storage failure to report. The controller branch that rendered it is gone too. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019fnjMN7WpEjQJD2zveakV6 --- lib/textbin_web/api_docs.ex | 10 ++-- .../controllers/apiv1/paste_controller.ex | 20 ++----- priv/openapi.json | 60 ------------------- 3 files changed, 9 insertions(+), 81 deletions(-) diff --git a/lib/textbin_web/api_docs.ex b/lib/textbin_web/api_docs.ex index b2a57c6..3f6c183 100644 --- a/lib/textbin_web/api_docs.ex +++ b/lib/textbin_web/api_docs.ex @@ -53,12 +53,9 @@ defmodule TextbinWeb.ApiDocs do Map.merge(%{401 => unauthorized(), 422 => invalid_request()}, responses) end - @doc "`with_common_errors/1` plus the 404 for a workspace the caller cannot reach." - def with_scope_errors(responses) do - responses - |> Map.put_new(404, error("Workspace not found")) - |> with_common_errors() - end + # Paste operations share these. The personal workspace always exists, so + # the "Workspace not found" 404 is added by in_workspace/1 only. + def with_scope_errors(responses), do: with_common_errors(responses) @doc "The optional paste fields as query parameters, for raw-body uploads." def paste_field_parameters do @@ -85,6 +82,7 @@ defmodule TextbinWeb.ApiDocs do spec |> Keyword.update(:parameters, [workspace_id], &[workspace_id | &1]) + |> Keyword.update!(:responses, &Map.put_new(&1, 404, error("Workspace not found"))) |> Keyword.update!(:operation_id, &(&1 <> "InWorkspace")) |> Keyword.update!(:summary, &String.replace(&1, "your personal workspace", "a workspace")) end diff --git a/lib/textbin_web/controllers/apiv1/paste_controller.ex b/lib/textbin_web/controllers/apiv1/paste_controller.ex index c62fd94..94a3190 100644 --- a/lib/textbin_web/controllers/apiv1/paste_controller.ex +++ b/lib/textbin_web/controllers/apiv1/paste_controller.ex @@ -65,11 +65,7 @@ defmodule TextbinWeb.ApiV1.PasteController do operation_id: "deletePaste", summary: "Delete a paste; it stays restorable for the workspace's recovery window", parameters: [id: uuid_path("Paste id")], - responses: - with_scope_errors(%{ - 204 => no_content("Deleted, or nothing to delete"), - 503 => error("The deletion could not be completed") - }) + responses: with_scope_errors(%{204 => no_content("Deleted, or nothing to delete")}) ] @deleted_spec [ @@ -418,17 +414,11 @@ defmodule TextbinWeb.ApiV1.PasteController do def delete(conn, %{"id" => id} = params) do with_paste_scope(conn, params, fn current_scope -> + # Deleting only marks the paste removed; stored content stays until + # the purge, so there is no storage failure to report here. case delete_scoped_paste(current_scope, %Paste{id: id}, params) do - {:error, :not_found} -> - send_resp(conn, :no_content, "") - - {:error, _reason} -> - conn - |> put_status(:service_unavailable) - |> json(%{errors: %{detail: "Paste deletion could not be completed"}}) - - _result -> - send_resp(conn, :no_content, "") + {:ok, _paste} -> send_resp(conn, :no_content, "") + {:error, :not_found} -> send_resp(conn, :no_content, "") end end) end diff --git a/priv/openapi.json b/priv/openapi.json index 46a90c9..7e29886 100644 --- a/priv/openapi.json +++ b/priv/openapi.json @@ -1316,16 +1316,6 @@ }, "description": "Missing, invalid or revoked API token" }, - "404": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "Workspace not found" - }, "422": { "content": { "application/json": { @@ -1489,16 +1479,6 @@ }, "description": "Missing, invalid or revoked API token" }, - "404": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "Workspace not found" - }, "413": { "content": { "application/json": { @@ -1562,16 +1542,6 @@ }, "description": "Missing, invalid or revoked API token" }, - "404": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "Workspace not found" - }, "422": { "content": { "application/json": { @@ -1750,16 +1720,6 @@ }, "description": "Missing, invalid or revoked API token" }, - "404": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "Workspace not found" - }, "422": { "content": { "application/json": { @@ -1769,16 +1729,6 @@ } }, "description": "The parameters or body do not match this operation" - }, - "503": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "The deletion could not be completed" } }, "summary": "Delete a paste; it stays restorable for the workspace's recovery window", @@ -2390,16 +2340,6 @@ } }, "description": "The parameters or body do not match this operation" - }, - "503": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Error" - } - } - }, - "description": "The deletion could not be completed" } }, "summary": "Delete a paste; it stays restorable for the workspace's recovery window", From b6b1a9227c74e8655e0b6993cf4b5d67d56dc13a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 14:32:43 +0000 Subject: [PATCH 5/7] test: check every API response in every test against its operation Contract tests used to cover only the requests one test file made, so a changed JSON view or a new status elsewhere drifted until someone noticed. Now every API request any test makes is checked. - TextbinWeb.ApiResponseCheck sits in the :api pipeline and hands each response to a checker just before it is sent. It is a no-op unless config names a checker (compile-time; only config/test.exs does), so production is unaffected. Being in the pipeline, it also sees the 401s the pipeline sends itself. - TextbinWeb.ApiContract (test support) finds the route's operation and raises unless the status is documented and the body validates against its schema (wire types first, then closed objects). The first failure is kept and re-raised, so the report names the field or status, not the 500 page Phoenix renders afterwards. - Each checked response is recorded. After a full `mix test`, the suite fails if a documented response was never produced, so stale statuses cannot linger in the document; a run narrowed to files or tags only skips this. A workspace-scoped operation shares coverage with its personal one, since they run the same action. One response Plug.Test cannot produce (a request body the socket fails to deliver) is exempted with its reason, and a stale exemption fails too. - ApiContractTest now produces what every operation shares, across the whole API: 401 without a token, 422 for an undeclared query parameter, and each documented 404 for unknown ids, plus purge's 503 when storage cannot delete. The per-operation response tests it had are gone; the feature tests' own requests now do that job. - AGENTS.md says how API changes work now. Checked by breaking things on purpose: an extra field in PasteJSON fails 23 paste tests with "Unexpected field: surprise", a 410 instead of 404 fails with "getPaste returned 410, which its operation does not document", and a documented 409 nothing sends fails the full run with exit status 1. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019fnjMN7WpEjQJD2zveakV6 --- AGENTS.md | 1 + config/test.exs | 4 + lib/textbin_web/api_response_check.ex | 31 +++ lib/textbin_web/router.ex | 1 + test/support/api_contract.ex | 198 ++++++++++++---- test/test_helper.exs | 5 + test/textbin_web/api_contract_test.exs | 302 ++++++------------------- 7 files changed, 265 insertions(+), 277 deletions(-) create mode 100644 lib/textbin_web/api_response_check.ex diff --git a/AGENTS.md b/AGENTS.md index 255599f..595db0b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,7 @@ This is a web application written using the Phoenix web framework. - Use Conventional Commits style for pull request titles, such as `feat: add workspace membership controls` - When Jujutsu is available for the workspace, use `jj` to record changes with detailed explanations; use Git commits only when `jj` is unavailable - Add concise comments or module/function docs that explain intent, domain boundaries, security decisions, or non-obvious tradeoffs. Avoid comments that merely restate what the code does +- The API contract lives next to the code: each API controller action declares its OpenAPI operation with `operation` (helpers in `TextbinWeb.ApiDocs`, schemas in `TextbinWeb.ApiSpec.Schemas`). Requests that do not match it are rejected, every API response in the test suite is checked against it, and a full `mix test` fails if a documented response is never produced. After changing an operation, run `mix openapi.export` and commit `priv/openapi.json` - Use the already included and available `:req` (`Req`) library for HTTP requests, **avoid** `:httpoison`, `:tesla`, and `:httpc`. Req is included by default and is the preferred HTTP client for Phoenix apps ### Phoenix v1.8 guidelines diff --git a/config/test.exs b/config/test.exs index 71c8a70..e8f82f6 100644 --- a/config/test.exs +++ b/config/test.exs @@ -40,6 +40,10 @@ config :textbin, Textbin.Mailer, adapter: Swoosh.Adapters.Test config :textbin, Textbin.Storage, adapter: Textbin.Storage.Memory +# Every API response a test produces is checked against the operation its +# action declares (see TextbinWeb.ApiResponseCheck). +config :textbin, :api_response_check, TextbinWeb.ApiContract + # Cleanup is exercised explicitly so the global worker does not race SQL sandbox tests. config :textbin, expiration_cleanup_enabled: false diff --git a/lib/textbin_web/api_response_check.ex b/lib/textbin_web/api_response_check.ex new file mode 100644 index 0000000..8a478e7 --- /dev/null +++ b/lib/textbin_web/api_response_check.ex @@ -0,0 +1,31 @@ +defmodule TextbinWeb.ApiResponseCheck do + @moduledoc """ + Hands every API response to a checker just before it is sent, so the test + suite can hold each one to the operation its action declares. + + It does nothing unless `config :textbin, :api_response_check` names a + checker module, which only the test environment does; the value is read at + compile time, so production carries no cost. The checker gets the conn in a + `register_before_send/2` callback and raises on a mismatch, which fails + whichever test made the request. + + It sits in the `:api` pipeline rather than the controllers so responses + from the pipeline itself, such as a missing token's 401, are checked too. + """ + + @behaviour Plug + + @checker Application.compile_env(:textbin, :api_response_check) + + @impl Plug + def init(opts), do: opts + + if @checker do + @impl Plug + def call(conn, _opts), + do: Plug.Conn.register_before_send(conn, fn conn -> @checker.check_response!(conn) end) + else + @impl Plug + def call(conn, _opts), do: conn + end +end diff --git a/lib/textbin_web/router.ex b/lib/textbin_web/router.ex index 3f9bfbf..436eaa7 100644 --- a/lib/textbin_web/router.ex +++ b/lib/textbin_web/router.ex @@ -14,6 +14,7 @@ defmodule TextbinWeb.Router do end pipeline :api do + plug TextbinWeb.ApiResponseCheck plug :accepts, ["json"] plug :fetch_current_scope_for_api_token # API controllers validate each request against this document. diff --git a/test/support/api_contract.ex b/test/support/api_contract.ex index b6c8cae..74c7482 100644 --- a/test/support/api_contract.ex +++ b/test/support/api_contract.ex @@ -1,60 +1,178 @@ defmodule TextbinWeb.ApiContract do @moduledoc """ - Checks a real API response against the operation it claims to implement in - `TextbinWeb.ApiSpec`: the status must be documented for that operation, and - a JSON body must validate without type coercion. Response schemas are closed, so an - undocumented field fails as surely as a missing one. + Holds every API response the test suite produces to the operation its + action declares in `TextbinWeb.ApiSpec`. + + `TextbinWeb.ApiResponseCheck` calls `check_response!/1` just before each + API response is sent (test config enables it), so any test that makes an + API request also checks the contract. The status must be one the operation + documents, and a JSON body must validate against its schema without type + coercion. Response schemas are closed, so an undocumented field fails as + surely as a missing one. + + Each checked response is recorded, and after a full run the suite fails if + a documented response was never produced, so the document cannot keep + statuses or operations the server no longer has. """ - import ExUnit.Assertions - - alias OpenApiSpex.{MediaType, Reference, Response, Schema} + alias OpenApiSpex.{MediaType, Operation, Reference, Response, Schema} alias TextbinWeb.ApiSpec - def assert_api_response(%Plug.Conn{} = conn, operation_id) do - spec = spec() - operation = operation!(operation_id) + @coverage __MODULE__.Coverage + @pending_failure {__MODULE__, :pending_failure} + + # Documented responses Plug.Test cannot produce, with the reason. Keep this + # short: an entry that stops being documented fails the check as well. + @not_producible_in_tests %{ + {"createPaste", 400} => + "a request body the server fails to read (socket timeout or a client that disconnects mid-upload)" + } + + @doc "Raises when `conn`'s response does not match its action's operation." + def check_response!(%Plug.Conn{} = conn) do + # A failed check raises inside the response, so Phoenix renders a 500 + # page, which comes back through here; report the original failure. + if message = Process.delete(@pending_failure), do: raise(message) + + # Responses from the pipeline, such as a missing token's 401, are sent + # before Phoenix records the controller, so look the route up instead. + case Phoenix.Router.route_info(TextbinWeb.Router, conn.method, conn.request_path, conn.host) do + %{plug: controller, plug_opts: action} -> check_operation!(conn, controller, action) + # No route, so no operation to hold the response to. + :error -> :ok + end + + conn + end + + defp check_operation!(conn, controller, action) do + case controller.open_api_operation(action) do + %Operation{} = operation -> + record(operation.operationId, conn.status) + verify!(conn, operation) + + _undocumented -> + raise "#{inspect(controller)}.#{action} has no `operation` spec" + end + end + + defp verify!(conn, operation) do + id = operation.operationId response = Map.get(operation.responses, conn.status) || - flunk( - "#{operation_id} returned #{conn.status}, which the spec does not document " <> - "(documented: #{operation.responses |> Map.keys() |> Enum.sort() |> inspect()})" + fail!( + "#{id} returned #{conn.status}, which its operation does not document " <> + "(documented: #{operation.responses |> Map.keys() |> Enum.sort() |> inspect()}). " <> + "Body: #{conn.resp_body}" ) case response do %Response{content: %{"application/json" => %MediaType{schema: schema}}} -> - body = Jason.decode!(conn.resp_body) + verify_body!(id, conn, schema) - # Casting enforces closed objects and formats but accepts quoted - # booleans/numbers. Validate the original wire types first; this API - # expects property keys to match the decoded JSON's string keys. - case Schema.validate(json_keys(schema), body, json_keys(spec.components.schemas)) do - :ok -> :ok - {:error, detail} -> flunk("#{operation_id} #{conn.status}: #{detail}") - end + %Response{content: nil} -> + if conn.resp_body != "", + do: fail!("#{id} #{conn.status} is documented without a body but sent one") - case OpenApiSpex.cast_value(body, schema, spec) do - {:ok, _value} -> - :ok + %Response{} -> + :ok + end + end - {:error, errors} -> - details = Enum.map_join(errors, "\n", &" #{&1}") + defp verify_body!(id, conn, schema) do + spec = spec() + body = Jason.decode!(conn.resp_body) - flunk( - "#{operation_id} #{conn.status} response does not match the spec:\n#{details}\n\n" <> - "Body: #{conn.resp_body}" - ) - end + # Casting enforces closed objects and formats but accepts quoted + # booleans and numbers, so validate the wire types first. + with {:error, detail} <- + Schema.validate(json_keys(schema), body, json_keys(spec.components.schemas)) do + fail!("#{id} #{conn.status}: #{detail}\n\nBody: #{conn.resp_body}") + end - %Response{content: nil} -> - assert conn.resp_body == "", "#{operation_id} #{conn.status} should have no body" + with {:error, errors} <- OpenApiSpex.cast_value(body, schema, spec) do + details = Enum.map_join(errors, "\n", &" #{&1}") - %Response{} -> - :ok + fail!( + "#{id} #{conn.status} response does not match its operation:\n#{details}\n\n" <> + "Body: #{conn.resp_body}" + ) end - conn + :ok + end + + defp fail!(message) do + Process.put(@pending_failure, message) + raise message + end + + ## Coverage + + def start_coverage, do: :ets.new(@coverage, [:named_table, :public, :set]) + + # A workspace-scoped paste operation runs its personal operation's action, + # so the pair shares coverage: a status counts once either one produces it. + defp record(operation_id, status) do + if :ets.whereis(@coverage) != :undefined, + do: :ets.insert(@coverage, {{coverage_key(operation_id), status}}) + end + + defp coverage_key(operation_id), do: String.replace_suffix(operation_id, "InWorkspace", "") + + @doc """ + Documented API responses no test produced. Only the API pipeline is + checked, so the raw paste route is left out. + """ + def uncovered_responses do + documented = + for {_method, "/api/" <> _ = _path, operation} <- ApiSpec.operations(), + status <- Map.keys(operation.responses), + into: MapSet.new(), + do: {coverage_key(operation.operationId), status} + + missing = + for {key, status} = response <- documented, + not :ets.member(@coverage, response), + not Map.has_key?(@not_producible_in_tests, response), + do: "#{key} #{status}" + + stale_exemptions = + for {key, status} = response <- Map.keys(@not_producible_in_tests), + not MapSet.member?(documented, response), + do: "#{key} #{status} (exempted in TextbinWeb.ApiContract but no longer documented)" + + missing ++ stale_exemptions + end + + @doc """ + After a full run, fails the suite when a documented response was never + produced. A run narrowed to some files or tags would always miss some, so + it only reports then. + """ + def report_coverage(argv) do + missing = Enum.sort(uncovered_responses()) + + if missing != [] and full_run?(argv), do: fail_suite(missing) + end + + defp fail_suite(missing) do + message = + "Documented API responses no test produced:\n" <> + Enum.map_join(missing, "\n", &" #{&1}") <> + "\nAdd a test that produces each one, or remove it from the operation." + + IO.puts(:stderr, IO.ANSI.format([:red, message])) + # ExUnit has already counted failures; this turns the run red regardless. + System.at_exit(fn _status -> System.halt(1) end) + end + + defp full_run?(argv) do + not Enum.any?(argv, fn arg -> + arg in ~w(--only --failed --stale) or String.starts_with?(arg, "test/") or + String.ends_with?(arg, ".exs") or arg =~ ~r/\.exs:\d+$/ + end) end defp json_keys(%Schema{} = schema) do @@ -76,13 +194,7 @@ defmodule TextbinWeb.ApiContract do defp json_keys(list) when is_list(list), do: Enum.map(list, &json_keys/1) defp json_keys(value), do: value - defp operation!(operation_id) do - Enum.find_value(ApiSpec.operations(), fn {_method, _path, operation} -> - operation.operationId == operation_id && operation - end) || flunk("No operation #{operation_id} in TextbinWeb.ApiSpec") - end - - # Building the spec is pure but not free; every assertion shares one copy. + # Building the spec is pure but not free; every check shares one copy. defp spec do case :persistent_term.get({__MODULE__, :spec}, nil) do nil -> diff --git a/test/test_helper.exs b/test/test_helper.exs index a6d8e2e..ab51c78 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -1,2 +1,7 @@ ExUnit.start() Ecto.Adapters.SQL.Sandbox.mode(Textbin.Repo, :manual) + +# Every API response is checked against its operation as tests run; after a +# full run, documented responses no test produced fail the suite. +TextbinWeb.ApiContract.start_coverage() +ExUnit.after_suite(fn _result -> TextbinWeb.ApiContract.report_coverage(System.argv()) end) diff --git a/test/textbin_web/api_contract_test.exs b/test/textbin_web/api_contract_test.exs index 51c6146..28e4909 100644 --- a/test/textbin_web/api_contract_test.exs +++ b/test/textbin_web/api_contract_test.exs @@ -1,19 +1,22 @@ defmodule TextbinWeb.ApiContractTest do @moduledoc """ Keeps `TextbinWeb.ApiSpec` and the server in step: every API action - declares its operation, the exported `priv/openapi.json` is current, and - each operation's real responses match their documented schemas. + declares its operation and `priv/openapi.json` is current. + + Responses themselves are checked in every test by `TextbinWeb.ApiContract`. + The tests below produce the responses every operation shares (401, 422 and + the documented 404s) across the whole API, plus the ones no feature test + reaches, so that the suite's coverage check passes. """ use TextbinWeb.ConnCase, async: false - import TextbinWeb.ApiContract import Textbin.AccountsFixtures - alias OpenApiSpex.Reference + alias OpenApiSpex.{MediaType, Reference, RequestBody} alias Textbin.Accounts - alias Textbin.Organizations alias Textbin.Pastes + alias Textbin.Storage alias TextbinWeb.ApiSpec @exported_spec Path.expand("../../priv/openapi.json", __DIR__) @@ -56,260 +59,91 @@ defmodule TextbinWeb.ApiContractTest do end end - describe "authentication" do - setup :api_user - - test "createApiToken" do - user = user_fixture() - - json_conn() - |> post(~p"/api/v1/auth/tokens", %{email: user.email, password: valid_user_password()}) - |> assert_api_response("createApiToken") - - json_conn() - |> post(~p"/api/v1/auth/tokens", %{email: user.email, password: "wrong password"}) - |> assert_api_response("createApiToken") - - json_conn() - |> post(~p"/api/v1/auth/tokens", %{}) - |> assert_api_response("createApiToken") - end - - test "getIdentity and revokeCurrentToken", %{conn: conn} do - conn |> get(~p"/api/v1/me") |> assert_api_response("getIdentity") - json_conn() |> get(~p"/api/v1/me") |> assert_api_response("getIdentity") - conn |> delete(~p"/api/v1/me/token") |> assert_api_response("revokeCurrentToken") - end - end - - describe "organizations" do - setup [:api_user, :team_organization] - - test "rejects quoted booleans, missing fields and undocumented fields", context do - response = - get(context.conn, ~p"/api/v1/organizations/#{context.organization.id}/workspaces") - - body = json_response(response, 200) - [workspace | rest] = body["data"] + describe "every operation" do + setup :api_token - for invalid <- [ - Map.put(workspace, "is_default", "false"), - Map.delete(workspace, "is_default"), - Map.put(workspace, "surprise", true) - ] do - malformed = %{response | resp_body: Jason.encode!(%{body | "data" => [invalid | rest]})} - - assert_raise ExUnit.AssertionError, fn -> - assert_api_response(malformed, "listOrganizationWorkspaces") - end + test "answers a request without a token with 401" do + for {method, path, operation} <- api_operations(), operation.security != [] do + conn = request(json_conn(), method, path, operation) + assert conn.status == 401, "#{operation.operationId} answered #{conn.status}" end end - test "listOrganizations and listOrganizationWorkspaces", context do - context.conn |> get(~p"/api/v1/organizations") |> assert_api_response("listOrganizations") - - context.conn - |> get(~p"/api/v1/organizations/#{context.organization.id}/workspaces") - |> assert_api_response("listOrganizationWorkspaces") - - context.conn - |> get(~p"/api/v1/organizations/#{Ecto.UUID.generate()}/workspaces") - |> assert_api_response("listOrganizationWorkspaces") - end - - test "listAuditEvents", context do - context.conn - |> get(~p"/api/v1/organizations/#{context.organization.id}/audit-events?limit=1") - |> assert_api_response("listAuditEvents") - end - - test "recoverWorkspaceAccess", context do - member = user_fixture() - - {:ok, _memberships} = - Organizations.add_organization_member(context.scope, context.organization, member) - - {:ok, workspace} = - Organizations.create_workspace(context.scope, context.organization, %{ - name: "Contract recovery", - slug: "contract-recovery", - visibility: "private" - }) - - {:ok, _membership} = - Organizations.add_workspace_member(context.scope, workspace, member, "owner") - - {:ok, _membership} = Organizations.leave_workspace(context.scope, workspace) - - context.conn - |> post(~p"/api/v1/workspaces/#{workspace.id}/recovery") - |> assert_api_response("recoverWorkspaceAccess") - - context.conn - |> post(~p"/api/v1/workspaces/#{Ecto.UUID.generate()}/recovery") - |> assert_api_response("recoverWorkspaceAccess") - end - end - - describe "pastes in the personal workspace" do - setup :api_user - - test "rejects incomplete and mixed paste content representations", %{conn: conn, scope: scope} do - {:ok, paste} = Pastes.create_paste(scope, %{data: "text"}) - response = get(conn, ~p"/api/v1/pastes/#{paste.id}") - body = json_response(response, 200) - metadata = Map.delete(body["data"], "data") - - for fields <- [ - %{"data" => nil}, - %{"data" => nil, "data_base64" => "AP8BAg=="}, - %{"data" => nil, "data_encoding" => "base64"}, - %{"data" => "text", "data_base64" => "AP8BAg==", "data_encoding" => "base64"} - ] do - malformed = %{ - response - | resp_body: Jason.encode!(%{"data" => Map.merge(metadata, fields)}) - } - - assert_raise ExUnit.AssertionError, fn -> assert_api_response(malformed, "getPaste") end + test "rejects query parameters it does not declare with 422", %{token: token} do + for {method, path, operation} <- api_operations() do + conn = request(authorized(token), method, path <> "?undeclared=1", operation) + assert conn.status == 422, "#{operation.operationId} answered #{conn.status}" end end - test "create, list, show and delete", %{conn: conn} do - created = - conn - |> post(~p"/api/v1/pastes", %{ - data: "hello", - title: "Greeting", - reference_url: "https://example.com/ticket/1", - expires_in: "1d" - }) - |> assert_api_response("createPaste") - - id = json_response(created, 201)["data"]["id"] - - binary = - conn - |> put_req_header("content-type", "application/octet-stream") - |> post(~p"/api/v1/pastes?title=Bytes", <<0, 255, 1, 2>>) - |> assert_api_response("createPaste") - - binary_id = json_response(binary, 201)["data"]["id"] - conn |> get(~p"/api/v1/pastes/#{binary_id}") |> assert_api_response("getPaste") - - conn |> post(~p"/api/v1/pastes", %{data: ""}) |> assert_api_response("createPaste") - conn |> get(~p"/api/v1/pastes") |> assert_api_response("listPastes") - conn |> get(~p"/api/v1/pastes/#{id}") |> assert_api_response("getPaste") - conn |> get(~p"/api/v1/pastes/not-a-uuid") |> assert_api_response("getPaste") - conn |> get(~p"/api/v1/pastes/#{Ecto.UUID.generate()}") |> assert_api_response("getPaste") - conn |> delete(~p"/api/v1/pastes/#{id}") |> assert_api_response("deletePaste") - json_conn() |> get(~p"/api/v1/pastes") |> assert_api_response("listPastes") - end - - test "list, restore and purge removed pastes", %{conn: conn, scope: scope} do - {:ok, paste} = Pastes.create_paste(scope, %{data: "gone for now"}) - {:ok, _removed} = Pastes.delete_paste(scope, paste) - - conn |> get(~p"/api/v1/pastes/deleted") |> assert_api_response("listRemovedPastes") - - conn - |> post(~p"/api/v1/pastes/deleted/#{paste.id}/restore") - |> assert_api_response("restorePaste") - - conn - |> post(~p"/api/v1/pastes/deleted/#{paste.id}/restore") - |> assert_api_response("restorePaste") - - {:ok, _removed} = Pastes.delete_paste(scope, paste) - conn |> delete(~p"/api/v1/pastes/deleted/#{paste.id}") |> assert_api_response("purgePaste") - conn |> delete(~p"/api/v1/pastes/deleted/#{paste.id}") |> assert_api_response("purgePaste") - conn |> delete(~p"/api/v1/pastes/deleted/not-a-uuid") |> assert_api_response("purgePaste") - end - - test "getRawPaste", %{scope: scope} do - {:ok, paste} = Pastes.create_paste(scope, %{data: "shared", audience: "unlisted"}) - - json_conn() |> get(~p"/pastes/#{paste.id}/raw") |> assert_api_response("getRawPaste") - - json_conn() - |> get(~p"/pastes/#{Ecto.UUID.generate()}/raw") - |> assert_api_response("getRawPaste") + test "answers unknown ids with each documented 404", %{token: token} do + for {method, path, operation} <- api_operations(), Map.has_key?(operation.responses, 404) do + conn = request(authorized(token), method, path, operation) + assert conn.status == 404, "#{operation.operationId} answered #{conn.status}" + end end end - describe "pastes in a named workspace" do - setup [:api_user, :team_organization] - - test "create, list, show and delete", %{conn: conn, workspace: workspace} do - created = - conn - |> post(~p"/api/v1/workspaces/#{workspace.id}/pastes", %{data: "team note"}) - |> assert_api_response("createPasteInWorkspace") - - id = json_response(created, 201)["data"]["id"] - - conn - |> get(~p"/api/v1/workspaces/#{workspace.id}/pastes") - |> assert_api_response("listPastesInWorkspace") - - conn - |> get(~p"/api/v1/workspaces/#{Ecto.UUID.generate()}/pastes") - |> assert_api_response("listPastesInWorkspace") - - conn - |> get(~p"/api/v1/workspaces/#{workspace.id}/pastes/#{id}") - |> assert_api_response("getPasteInWorkspace") - - conn - |> delete(~p"/api/v1/workspaces/#{workspace.id}/pastes/#{id}") - |> assert_api_response("deletePasteInWorkspace") - - conn - |> get(~p"/api/v1/workspaces/#{workspace.id}/pastes/deleted") - |> assert_api_response("listRemovedPastesInWorkspace") + describe "responses no feature test reaches" do + setup :api_token + + test "purgePaste answers 503 when stored content cannot be deleted", %{ + token: token, + user: user + } do + scope = user_scope_fixture(user) + # Larger than the inline limit, so the content lives in storage. + data = String.duplicate("a", Application.fetch_env!(:textbin, :inline_paste_bytes) + 1) + {:ok, paste} = Pastes.create_paste(scope, %{data: data}) + assert paste.storage_key + {:ok, _removed} = Pastes.delete_paste(scope, paste) - conn - |> post(~p"/api/v1/workspaces/#{workspace.id}/pastes/deleted/#{id}/restore") - |> assert_api_response("restorePasteInWorkspace") + delegate = Application.fetch_env!(:textbin, Storage) + on_exit(fn -> Application.put_env(:textbin, Storage, delegate) end) - conn - |> delete(~p"/api/v1/workspaces/#{workspace.id}/pastes/#{id}") - |> assert_api_response("deletePasteInWorkspace") + Application.put_env(:textbin, Storage, + adapter: Textbin.FailingDeleteStorage, + opts: [test_pid: self(), delegate: delegate] + ) - conn - |> delete(~p"/api/v1/workspaces/#{workspace.id}/pastes/deleted/#{id}") - |> assert_api_response("purgePasteInWorkspace") + conn = delete(authorized(token), ~p"/api/v1/pastes/deleted/#{paste.id}") + assert %{"errors" => %{"detail" => _detail}} = json_response(conn, 503) end end - defp api_user(_context) do + defp api_token(_context) do user = user_fixture() {:ok, {token, _user_token}} = Accounts.create_user_api_token(user, %{"name" => "Contract"}) + %{token: token, user: user} + end + + defp authorized(token), do: put_req_header(json_conn(), "authorization", "Bearer #{token}") - %{ - user: user, - scope: user_scope_fixture(user), - conn: put_req_header(json_conn(), "authorization", "Bearer #{token}") - } + defp api_operations do + for {method, "/api/" <> _ = path, operation} <- ApiSpec.operations(), + do: {method, path, operation} end - defp team_organization(%{scope: scope}) do - {:ok, organization} = - Organizations.create_organization(scope, %{ - name: "Contract organization", - slug: "contract-#{System.unique_integer([:positive])}" - }) + # Fills every path parameter with an id nothing has, and sends the smallest + # valid JSON body when the operation takes one, so the only thing wrong with + # the request is what each test varies. + defp request(conn, method, path, operation) do + path = Regex.replace(~r/\{\w+\}/, path, fn _ -> Ecto.UUID.generate() end) + dispatch(conn, @endpoint, method, path, body(operation)) + end - {:ok, workspace} = - Organizations.create_workspace(scope, organization, %{ - name: "Contract workspace", - slug: "contract-workspace", - visibility: "open" - }) + defp body(%{requestBody: %RequestBody{content: %{"application/json" => %MediaType{} = media}}}) do + case media.schema do + %Reference{"$ref": "#/components/schemas/CreatePasteRequest"} -> + %{data: "contract"} - %{organization: organization, workspace: workspace} + %Reference{"$ref": "#/components/schemas/CreateTokenRequest"} -> + %{email: "x@example.com", password: "x"} + end end + defp body(_operation), do: nil + defp references(%Reference{"$ref": ref}), do: [ref] defp references(%_struct{} = struct), do: struct |> Map.from_struct() |> references() defp references(map) when is_map(map), do: Enum.flat_map(Map.values(map), &references/1) From 27034631594cb89e5922fb17431229442195667b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 15:08:33 +0000 Subject: [PATCH 6/7] test: stream a multi-chunk raw upload past request validation Raw paste uploads are the one request body ApiRequestValidation does not validate, because open_api_spex matches bodies by exact content type. The controller then reads and spools the body itself. Existing tests stopped at about 8 KB, a single read, so nothing showed that a body several read chunks long still arrives unread and whole once validation has run. This sends 300 KB of random bytes with validated query fields and checks the stored bytes and fields. The same path was checked by hand against a local server with disk storage: an 800 KB pipe from the CLI, a 300 KB binary file (@path), and a chunked curl upload all stored byte-identical content; an oversized stream got 413 and an undeclared query field 422. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019fnjMN7WpEjQJD2zveakV6 --- .../apiv1/paste_controller_test.exs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/textbin_web/controllers/apiv1/paste_controller_test.exs b/test/textbin_web/controllers/apiv1/paste_controller_test.exs index 71df261..6eef5dc 100644 --- a/test/textbin_web/controllers/apiv1/paste_controller_test.exs +++ b/test/textbin_web/controllers/apiv1/paste_controller_test.exs @@ -193,6 +193,25 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do end end + # Raw uploads skip body validation (open_api_spex matches bodies by exact + # content type), so the controller must still find the body unread and + # spool all of it, past several read chunks, with validated query fields. + test "streams a multi-chunk raw body past request validation", %{conn: conn, scope: scope} do + body = :crypto.strong_rand_bytes(300_000) + + response = + conn + |> put_req_header("content-type", "application/octet-stream") + |> post(~p"/api/v1/pastes?title=Streamed&syntax_highlight=elixir", body) + |> json_response(201) + + assert %{"title" => "Streamed", "syntax_highlight" => "elixir"} = response["data"] + + assert Pastes.get_paste!(scope, response["data"]["id"]) + |> Pastes.load_data() + |> Map.fetch!(:data) == body + end + test "renders paste from raw request body with syntax highlight", %{conn: conn, scope: scope} do conn = conn From b6fe12168e3749d1cfbee73924f638f5e3a210c1 Mon Sep 17 00:00:00 2001 From: "Darwin D. Wu" Date: Sun, 11 Oct 2026 08:19:10 -0700 Subject: [PATCH 7/7] fix: enforce API request boundaries before controller consumption Reject _json wrappers and undeclared parsed/raw bodies before actions run. Derive paste scope and deletion dispatch only from path parameters. Leave non-JSON paste upload media unread ahead of standard parsers, then spool exact bytes with query-only metadata. Regressions reproduce validation/controller mismatches and verify unchanged lifecycle persistence plus exact upload bytes across personal/workspace routes. 581 tests, full status coverage and Credo pass; OpenAPI export is unchanged. Oracle signed off. --- lib/textbin_web/api_request_validation.ex | 34 ++++++++- .../controllers/apiv1/paste_controller.ex | 27 ++++--- lib/textbin_web/endpoint.ex | 2 +- lib/textbin_web/raw_paste_parser.ex | 38 ++++++++++ test/textbin_web/api_contract_test.exs | 19 +++++ .../apiv1/auth_controller_test.exs | 17 +++++ .../apiv1/paste_controller_test.exs | 75 ++++++++++++++++++- 7 files changed, 197 insertions(+), 15 deletions(-) create mode 100644 lib/textbin_web/raw_paste_parser.ex diff --git a/lib/textbin_web/api_request_validation.ex b/lib/textbin_web/api_request_validation.ex index 381364e..92bab3d 100644 --- a/lib/textbin_web/api_request_validation.ex +++ b/lib/textbin_web/api_request_validation.ex @@ -36,6 +36,8 @@ defmodule TextbinWeb.ApiRequestValidation do content_type = request_content_type(conn) operation = controller.open_api_operation(action) |> for_content_type(content_type) + {conn, body_errors} = body_boundary(conn, operation) + errors = case OpenApiSpex.cast_and_validate(spec, operation, conn, content_type, replace_params: false @@ -44,7 +46,7 @@ defmodule TextbinWeb.ApiRequestValidation do {:error, errors} -> errors end - case errors ++ undeclared_query_errors(conn, operation) do + case body_errors ++ errors ++ undeclared_query_errors(conn, operation) do [] -> conn errors -> reject(conn, errors) end @@ -69,6 +71,36 @@ defmodule TextbinWeb.ApiRequestValidation do defp for_content_type(operation, _content_type), do: operation + # Operation2 unwraps _json even when it was supplied as a real object key. + # Our JSON contracts require flat objects, never Plug's scalar wrapper. + defp body_boundary(%{body_params: %{"_json" => _}} = conn, %Operation{ + requestBody: %RequestBody{} + }), + do: {conn, [unexpected_body("_json")]} + + defp body_boundary(conn, %Operation{requestBody: nil}) do + cond do + conn.private[:raw_paste_upload] -> + {conn, []} + + map_size(conn.body_params) > 0 -> + {conn, [unexpected_body("body")]} + + true -> + # Content-Type alone does not imply a body. Probe only one byte when + # parsers left it unread, rejecting chunked/raw bodies without buffering. + case read_body(conn, length: 1, read_length: 1) do + {:ok, "", conn} -> {conn, []} + {_state, _body, conn} -> {conn, [unexpected_body("body")]} + {:error, _reason} -> {conn, [unexpected_body("body")]} + end + end + end + + defp body_boundary(conn, _operation), do: {conn, []} + + defp unexpected_body(name), do: %Error{reason: :unexpected_field, name: name, path: [name]} + # open_api_spex only checks query parameters for operations that declare # some; an operation that declares none should accept none either. defp undeclared_query_errors(conn, %Operation{parameters: parameters}) do diff --git a/lib/textbin_web/controllers/apiv1/paste_controller.ex b/lib/textbin_web/controllers/apiv1/paste_controller.ex index 94a3190..ccf6ee5 100644 --- a/lib/textbin_web/controllers/apiv1/paste_controller.ex +++ b/lib/textbin_web/controllers/apiv1/paste_controller.ex @@ -136,13 +136,12 @@ defmodule TextbinWeb.ApiV1.PasteController do end) end - # Treat create as "store the request body as paste data" while still accepting - # common JSON client shapes. This keeps CLI usage simple without making API - # clients wrap data unless they want to. + # Exact application/json is a validated object; every other upload media + # type is left unread by RawPasteParser and spooled as the paste itself. operation :create, @create_spec def create(conn, params) do - request_params = Map.drop(params, Map.keys(conn.path_params)) + request_params = Map.merge(conn.query_params, conn.body_params) case paste_attrs(conn, request_params) do {:ok, {:data, paste_attrs}, conn} -> @@ -166,16 +165,21 @@ defmodule TextbinWeb.ApiV1.PasteController do end end + # Any other content type is the paste itself, left unread by Plug.Parsers so + # streamed CLI/stdin data can be spooled here. Its fields arrive as query + # parameters, already validated. + defp paste_attrs(%{private: %{raw_paste_upload: true}} = conn, _params), + do: raw_paste_attrs(conn, conn.query_params) + # A JSON body has been validated against CreatePasteRequest, so it is an # object with string data. defp paste_attrs(conn, %{"data" => data} = params) when is_binary(data) do {:ok, {:data, build_paste_attrs(data, params)}, conn} end - # Any other content type is the paste itself, left unread by Plug.Parsers so - # streamed CLI/stdin data can be spooled here. Its fields arrive as query - # parameters, already validated. - defp paste_attrs(conn, params) do + defp paste_attrs(conn, _params), do: raw_paste_attrs(conn, conn.query_params) + + defp raw_paste_attrs(conn, params) do case read_request_body(conn) do {:ok, path, metadata, conn} -> paste_attrs = @@ -416,7 +420,7 @@ defmodule TextbinWeb.ApiV1.PasteController do with_paste_scope(conn, params, fn current_scope -> # Deleting only marks the paste removed; stored content stays until # the purge, so there is no storage failure to report here. - case delete_scoped_paste(current_scope, %Paste{id: id}, params) do + case delete_scoped_paste(current_scope, %Paste{id: id}, conn.path_params) do {:ok, _paste} -> send_resp(conn, :no_content, "") {:error, :not_found} -> send_resp(conn, :no_content, "") end @@ -575,9 +579,10 @@ defmodule TextbinWeb.ApiV1.PasteController do Application.get_env(:textbin, :upload_tmp_dir, System.tmp_dir!()) end - defp with_paste_scope(conn, params, fun) do + defp with_paste_scope(conn, _params, fun) do with_api_scope(conn, fn scope -> - case resolve_paste_scope(scope, params) do + # Scope belongs to the route, never a body/query parameter with the same name. + case resolve_paste_scope(scope, conn.path_params) do {:ok, paste_scope} -> fun.(paste_scope) {:error, :not_found} -> render_workspace_not_found(conn) end diff --git a/lib/textbin_web/endpoint.ex b/lib/textbin_web/endpoint.ex index 456e30d..860751a 100644 --- a/lib/textbin_web/endpoint.ex +++ b/lib/textbin_web/endpoint.ex @@ -43,7 +43,7 @@ defmodule TextbinWeb.Endpoint do plug Plug.Telemetry, event_prefix: [:phoenix, :endpoint] plug Plug.Parsers, - parsers: [:urlencoded, :multipart, :json], + parsers: [TextbinWeb.RawPasteParser, :urlencoded, :multipart, :json], pass: ["*/*"], json_decoder: Phoenix.json_library() diff --git a/lib/textbin_web/raw_paste_parser.ex b/lib/textbin_web/raw_paste_parser.ex new file mode 100644 index 0000000..39d6e33 --- /dev/null +++ b/lib/textbin_web/raw_paste_parser.ex @@ -0,0 +1,38 @@ +defmodule TextbinWeb.RawPasteParser do + @moduledoc """ + Leaves non-JSON API paste uploads unread for the bounded upload spooler. + Runs before the standard parsers so form, multipart and +json media types + remain exact paste bytes rather than becoming controller parameters. + Browser forms and exact application/json API bodies use the usual parsers. + """ + + @behaviour Plug.Parsers + + @impl true + def init(opts), do: opts + + @impl true + def parse(conn, "application", "json", _headers, _opts), do: {:next, conn} + + def parse( + %{method: "POST", path_info: ["api", "v1", "pastes"]} = conn, + _type, + _subtype, + _headers, + _opts + ), + do: raw(conn) + + def parse( + %{method: "POST", path_info: ["api", "v1", "workspaces", _id, "pastes"]} = conn, + _type, + _subtype, + _headers, + _opts + ), + do: raw(conn) + + def parse(conn, _type, _subtype, _headers, _opts), do: {:next, conn} + + defp raw(conn), do: {:ok, %{}, Plug.Conn.put_private(conn, :raw_paste_upload, true)} +end diff --git a/test/textbin_web/api_contract_test.exs b/test/textbin_web/api_contract_test.exs index 28e4909..5295de9 100644 --- a/test/textbin_web/api_contract_test.exs +++ b/test/textbin_web/api_contract_test.exs @@ -76,6 +76,25 @@ defmodule TextbinWeb.ApiContractTest do end end + test "bodyless mutation operations reject both JSON and unread raw bodies", %{token: token} do + for {method, path, operation} <- api_operations(), + method in [:post, :delete], + is_nil(operation.requestBody), + {content_type, bytes} <- [ + {"application/json", Jason.encode!(%{workspace_id: Ecto.UUID.generate()})}, + {"application/octet-stream", "undeclared body"} + ] do + path = Regex.replace(~r/\{\w+\}/, path, fn _ -> Ecto.UUID.generate() end) + + conn = + authorized(token) + |> put_req_header("content-type", content_type) + |> dispatch(@endpoint, method, path, bytes) + + assert conn.status == 422, "#{operation.operationId} answered #{conn.status}" + end + end + test "answers unknown ids with each documented 404", %{token: token} do for {method, path, operation} <- api_operations(), Map.has_key?(operation.responses, 404) do conn = request(authorized(token), method, path, operation) diff --git a/test/textbin_web/controllers/apiv1/auth_controller_test.exs b/test/textbin_web/controllers/apiv1/auth_controller_test.exs index 097eba8..4f94dc6 100644 --- a/test/textbin_web/controllers/apiv1/auth_controller_test.exs +++ b/test/textbin_web/controllers/apiv1/auth_controller_test.exs @@ -16,6 +16,23 @@ defmodule TextbinWeb.ApiV1.AuthControllerTest do end describe "create token" do + test "rejects _json wrappers rather than validating different credentials", %{ + conn: conn, + user: user + } do + credentials = %{email: user.email, password: valid_user_password()} + + for body <- [ + %{_json: credentials}, + Map.merge(credentials, %{_json: credentials, surprise: true}), + %{_json: credentials, email: 12, password: false} + ] do + assert conn + |> post(~p"/api/v1/auth/tokens", Jason.encode!(body)) + |> json_response(422) + end + end + test "creates and returns a named token from valid credentials", %{conn: conn, user: user} do conn = post(conn, ~p"/api/v1/auth/tokens", %{ diff --git a/test/textbin_web/controllers/apiv1/paste_controller_test.exs b/test/textbin_web/controllers/apiv1/paste_controller_test.exs index 71df261..b94435c 100644 --- a/test/textbin_web/controllers/apiv1/paste_controller_test.exs +++ b/test/textbin_web/controllers/apiv1/paste_controller_test.exs @@ -132,12 +132,58 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do assert_stored_paste(scope, id, "some data", "elixir") end - test "rejects JSON that is not a paste object", %{conn: conn} do - for body <- [Jason.encode!("json string data"), Jason.encode!(%{paste: %{data: "x"}})] do + test "rejects JSON that is not a paste object", %{conn: conn, scope: scope} do + for body <- [ + Jason.encode!("json string data"), + Jason.encode!(%{paste: %{data: "x"}}), + Jason.encode!(%{_json: %{data: "validated"}}), + Jason.encode!(%{_json: %{data: "validated"}, data: "stored", surprise: true}), + Jason.encode!(%{_json: %{data: "validated"}, data: 12}) + ] do response_conn = post(conn, ~p"/api/v1/pastes", body) assert %{"errors" => %{}} = json_response(response_conn, 422) end + + assert Pastes.list_pastes(scope) == [] + end + + test "raw media types preserve exact bytes and take metadata only from the query", %{ + conn: conn, + scope: scope + } do + {:ok, organization} = + Organizations.create_organization(scope, %{name: "Raw team", slug: "raw-team"}) + + [workspace] = organization.workspaces + {:ok, team_scope} = Organizations.resolve_workspace_scope(scope, workspace) + + for {path, paste_scope} <- [ + {~p"/api/v1/pastes", scope}, + {~p"/api/v1/workspaces/#{workspace.id}/pastes", team_scope} + ], + {content_type, bytes, stored_type} <- [ + {"application/x-www-form-urlencoded", "data=hello&title=body&audience=public", + "application/x-www-form-urlencoded"}, + {"application/vnd.example+json", ~s({"data":"hello","surprise":true}), + "application/vnd.example+json"}, + {"application/vnd.example+json", <<0, 255, 1, 2>>, "application/octet-stream"}, + {"multipart/form-data; boundary=review", + "--review\r\nContent-Disposition: form-data; name=\"data\"\r\n\r\nhello\r\n--review--\r\n", + "multipart/form-data"} + ] do + response = + conn + |> put_req_header("content-type", content_type) + |> post(path <> "?title=Query&audience=workspace", bytes) + + assert %{"data" => %{"id" => id}} = json_response(response, 201) + paste = Pastes.get_paste!(paste_scope, id) + assert paste.data == bytes + assert paste.title == "Query" + assert paste.audience == "workspace" + assert paste.content_type == stored_type + end end test "renders paste from raw request body", %{conn: conn, scope: scope} do @@ -529,6 +575,31 @@ defmodule TextbinWeb.ApiV1.PasteControllerTest do end describe "removed pastes" do + test "personal lifecycle routes reject body-injected workspace scopes", %{ + conn: conn, + scope: scope + } do + {:ok, organization} = + Organizations.create_organization(scope, %{name: "Body team", slug: "body-team"}) + + [workspace] = organization.workspaces + {:ok, team_scope} = Organizations.resolve_workspace_scope(scope, workspace) + {:ok, active} = Pastes.create_paste(team_scope, %{data: "active team paste"}) + {:ok, removed} = Pastes.create_paste(team_scope, %{data: "removed team paste"}) + {:ok, _} = Pastes.delete_paste(team_scope, removed) + body = %{workspace_id: workspace.id} + + assert conn |> delete(~p"/api/v1/pastes/#{active.id}", body) |> json_response(422) + + assert conn + |> post(~p"/api/v1/pastes/deleted/#{removed.id}/restore", body) + |> json_response(422) + + assert conn |> delete(~p"/api/v1/pastes/deleted/#{removed.id}", body) |> json_response(422) + assert Pastes.get_paste!(team_scope, active.id).data == "active team paste" + assert Repo.get!(Paste, removed.id).removal_reason == "deleted" + end + test "lists deleted and expired pastes with their reason and purge date", %{ conn: conn, scope: scope