diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index dda0969e3..37f0e898f 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -15,9 +15,8 @@ https://github.com/oxidecomputer/dropshot/compare/v0.8.0\...HEAD[Full list of commits] -=== Other notable changes - * https://github.com/oxidecomputer/dropshot/pull/452[#452] Dropshot no longer enables the `slog` cargo features `max_level_trace` and `release_max_level_debug`. Previously, clients were unable to set a release log level of `trace`; now they can. However, clients that did not select their own max log levels will see behavior change from the levels Dropshot was choosing to the default levels of `slog` itself (`debug` for debug builds and `info` for release builds). +* https://github.com/oxidecomputer/dropshot/pull/451[#451] There are now response types to support 302 ("Found"), 303 ("See Other"), and 307 ("Temporary Redirect") HTTP response codes. See `HttpResponseFound`, `HttpResponseSeeOther`, and `HttpResponseTemporaryRedirect`. == 0.8.0 (released 2022-09-09) diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 5f7575ec6..c05abc3ab 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -3,7 +3,7 @@ * Interface for implementing HTTP endpoint handler functions. * * For information about supported endpoint function signatures, argument types, - * extractors, and return types, see the top-level documentation dependencies: () for this crate. + * extractors, and return types, see the top-level documentation for this crate. * As documented there, we support several different sets of function arguments * and return types. * @@ -1436,6 +1436,172 @@ impl From for HttpHandlerResult { } } +/** Describes headers associated with a 300-level response. */ +#[derive(JsonSchema, Serialize)] +#[doc(hidden)] +pub struct RedirectHeaders { + /** HTTP "Location" header */ + /* + * What type should we use to represent header values? + * + * It's tempting to use `http::HeaderValue` here. But in HTTP, header + * values can contain bytes that aren't valid Rust strings. See + * `http::header::HeaderValue`. We could propagate this nonsense all the + * way to the OpenAPI spec, encoding the Location header as, say, + * base64-encoded bytes. This sounds really annoying to consumers. It's + * also a fair bit more work to implement. We'd need to create a separate + * type for this field so that we can impl `Serialize` and `JsonSchema` on + * it, and we'd need to also impl serialization of byte sequences in + * `MapSerializer`. Ugh. + * + * We just use `String`. This might contain values that aren't valid in + * HTTP response headers. But we can at least validate that at runtime, and + * it sure is easier to implement! + */ + location: String, +} + +/** See `http_response_found()` */ +pub type HttpResponseFound = + HttpResponseHeaders; + +/** + * `http_response_found` returns an HTTP 302 "Found" response with no response + * body. + * + * The sole argument will become the value of the `Location` header. This is + * where you want to redirect the client to. + * + * Per MDN and RFC 9110 S15.4.3, you might want to use 307 ("Temporary + * Redirect") or 303 ("See Other") instead. + */ +pub fn http_response_found( + location: String, +) -> Result { + let _ = http::HeaderValue::from_str(&location) + .map_err(|e| http_redirect_error(e, &location))?; + Ok(HttpResponseHeaders::new( + HttpResponseFoundStatus, + RedirectHeaders { location }, + )) +} + +fn http_redirect_error( + error: http::header::InvalidHeaderValue, + location: &str, +) -> HttpError { + HttpError::for_internal_error(format!( + "error encoding redirect URL {:?}: {:#}", + location, error + )) +} + +/** + * This internal type impls HttpCodedResponse. Consumers should use + * `HttpResponseFound` instead, which includes metadata about the `Location` + * header. + */ +#[doc(hidden)] +pub struct HttpResponseFoundStatus; +impl HttpCodedResponse for HttpResponseFoundStatus { + type Body = Empty; + const STATUS_CODE: StatusCode = StatusCode::FOUND; + const DESCRIPTION: &'static str = "redirect (found)"; +} +impl From for HttpHandlerResult { + fn from(_: HttpResponseFoundStatus) -> HttpHandlerResult { + HttpResponseFoundStatus::for_object(Empty) + } +} + +/** See `http_response_see_other()` */ +pub type HttpResponseSeeOther = + HttpResponseHeaders; + +/** + * `http_response_see_other` returns an HTTP 303 "See Other" response with no + * response body. + * + * The sole argument will become the value of the `Location` header. This is + * where you want to redirect the client to. + * + * Use this (as opposed to 307 "Temporary Redirect") when you want the client to + * follow up with a GET, rather than whatever method they used to make the + * current request. This is intended to be used after a PUT or POST to show a + * confirmation page or the like. + */ +pub fn http_response_see_other( + location: String, +) -> Result { + let _ = http::HeaderValue::from_str(&location) + .map_err(|e| http_redirect_error(e, &location))?; + Ok(HttpResponseHeaders::new( + HttpResponseSeeOtherStatus, + RedirectHeaders { location }, + )) +} + +/** + * This internal type impls HttpCodedResponse. Consumers should use + * `HttpResponseSeeOther` instead, which includes metadata about the `Location` + * header. + */ +#[doc(hidden)] +pub struct HttpResponseSeeOtherStatus; +impl HttpCodedResponse for HttpResponseSeeOtherStatus { + type Body = Empty; + const STATUS_CODE: StatusCode = StatusCode::SEE_OTHER; + const DESCRIPTION: &'static str = "redirect (see other)"; +} +impl From for HttpHandlerResult { + fn from(_: HttpResponseSeeOtherStatus) -> HttpHandlerResult { + HttpResponseSeeOtherStatus::for_object(Empty) + } +} + +/** See `http_response_temporary_redirect()` */ +pub type HttpResponseTemporaryRedirect = + HttpResponseHeaders; + +/** + * `http_response_temporary_redirect` represents an HTTP 307 "Temporary + * Redirect" response with no response body. + * + * The sole argument will become the value of the `Location` header. This is + * where you want to redirect the client to. + * + * Use this (as opposed to 303 "See Other") when you want the client to use the + * same request method and body when it makes the follow-up request. + */ +pub fn http_response_temporary_redirect( + location: String, +) -> Result { + let _ = http::HeaderValue::from_str(&location) + .map_err(|e| http_redirect_error(e, &location))?; + Ok(HttpResponseHeaders::new( + HttpResponseTemporaryRedirectStatus, + RedirectHeaders { location }, + )) +} + +/** + * This internal type impls HttpCodedResponse. Consumers should use + * `HttpResponseTemporaryRedirect` instead, which includes metadata about the + * `Location` header. + */ +#[doc(hidden)] +pub struct HttpResponseTemporaryRedirectStatus; +impl HttpCodedResponse for HttpResponseTemporaryRedirectStatus { + type Body = Empty; + const STATUS_CODE: StatusCode = StatusCode::TEMPORARY_REDIRECT; + const DESCRIPTION: &'static str = "redirect (temporary redirect)"; +} +impl From for HttpHandlerResult { + fn from(_: HttpResponseTemporaryRedirectStatus) -> HttpHandlerResult { + HttpResponseTemporaryRedirectStatus::for_object(Empty) + } +} + #[derive(Serialize, JsonSchema)] pub struct NoHeaders {} diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 6d29c4d37..12fd1fb62 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -634,6 +634,9 @@ pub use config::ConfigDropshot; pub use config::ConfigTls; pub use error::HttpError; pub use error::HttpErrorResponseBody; +pub use handler::http_response_found; +pub use handler::http_response_see_other; +pub use handler::http_response_temporary_redirect; pub use handler::Extractor; pub use handler::ExtractorMetadata; pub use handler::FreeformBody; @@ -642,8 +645,11 @@ pub use handler::HttpResponse; pub use handler::HttpResponseAccepted; pub use handler::HttpResponseCreated; pub use handler::HttpResponseDeleted; +pub use handler::HttpResponseFound; pub use handler::HttpResponseHeaders; pub use handler::HttpResponseOk; +pub use handler::HttpResponseSeeOther; +pub use handler::HttpResponseTemporaryRedirect; pub use handler::HttpResponseUpdatedNoContent; pub use handler::NoHeaders; pub use handler::Path; diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index 12a80cf4f..7271bae77 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -57,10 +57,11 @@ pub const TEST_HEADER_2: &str = "x-dropshot-test-header-2"; // List of allowed HTTP headers in responses. // Used to make sure we don't leak headers unexpectedly. -const ALLOWED_HEADERS: [AllowedHeader<'static>; 7] = [ +const ALLOWED_HEADERS: [AllowedHeader<'static>; 8] = [ AllowedHeader::new("content-length"), AllowedHeader::new("content-type"), AllowedHeader::new("date"), + AllowedHeader::new("location"), AllowedHeader::new("x-request-id"), AllowedHeader { name: "transfer-encoding", diff --git a/dropshot/tests/fail/bad_endpoint12.stderr b/dropshot/tests/fail/bad_endpoint12.stderr index b352bb4de..664407b91 100644 --- a/dropshot/tests/fail/bad_endpoint12.stderr +++ b/dropshot/tests/fail/bad_endpoint12.stderr @@ -10,6 +10,9 @@ error[E0277]: the trait bound `String: HttpCodedResponse` is not satisfied HttpResponseDeleted HttpResponseOk HttpResponseUpdatedNoContent + dropshot::handler::HttpResponseFoundStatus + dropshot::handler::HttpResponseSeeOtherStatus + dropshot::handler::HttpResponseTemporaryRedirectStatus = note: required because of the requirements on the impl of `HttpResponse` for `String` note: required because of the requirements on the impl of `ResultTrait` for `Result` --> tests/fail/bad_endpoint12.rs:16:6 diff --git a/dropshot/tests/fail/bad_endpoint4.stderr b/dropshot/tests/fail/bad_endpoint4.stderr index 7476539c0..a45df7829 100644 --- a/dropshot/tests/fail/bad_endpoint4.stderr +++ b/dropshot/tests/fail/bad_endpoint4.stderr @@ -13,7 +13,7 @@ error[E0277]: the trait bound `QueryParams: schemars::JsonSchema` is not satisfi (T0, T1, T2, T3) (T0, T1, T2, T3, T4) (T0, T1, T2, T3, T4, T5) - and 144 others + and 145 others note: required by a bound in `dropshot::Query` --> src/handler.rs | diff --git a/dropshot/tests/fail/bad_endpoint7.stderr b/dropshot/tests/fail/bad_endpoint7.stderr index eb4e973a7..fe36e3fa6 100644 --- a/dropshot/tests/fail/bad_endpoint7.stderr +++ b/dropshot/tests/fail/bad_endpoint7.stderr @@ -13,7 +13,7 @@ error[E0277]: the trait bound `Ret: serde::ser::Serialize` is not satisfied (T0, T1, T2, T3) (T0, T1, T2, T3, T4) (T0, T1, T2, T3, T4, T5) - and 219 others + and 220 others = note: required because of the requirements on the impl of `dropshot::handler::HttpResponseContent` for `Ret` note: required by a bound in `HttpResponseOk` --> src/handler.rs diff --git a/dropshot/tests/test_demo.rs b/dropshot/tests/test_demo.rs index 5fd57c26b..aa628a37e 100644 --- a/dropshot/tests/test_demo.rs +++ b/dropshot/tests/test_demo.rs @@ -17,6 +17,9 @@ use dropshot::channel; use dropshot::endpoint; +use dropshot::http_response_found; +use dropshot::http_response_see_other; +use dropshot::http_response_temporary_redirect; use dropshot::test_util::object_delete; use dropshot::test_util::read_json; use dropshot::test_util::read_string; @@ -25,8 +28,11 @@ use dropshot::test_util::TEST_HEADER_2; use dropshot::ApiDescription; use dropshot::HttpError; use dropshot::HttpResponseDeleted; +use dropshot::HttpResponseFound; use dropshot::HttpResponseHeaders; use dropshot::HttpResponseOk; +use dropshot::HttpResponseSeeOther; +use dropshot::HttpResponseTemporaryRedirect; use dropshot::HttpResponseUpdatedNoContent; use dropshot::Path; use dropshot::Query; @@ -68,6 +74,10 @@ fn demo_api() -> ApiDescription { api.register(demo_handler_untyped_body).unwrap(); api.register(demo_handler_delete).unwrap(); api.register(demo_handler_headers).unwrap(); + api.register(demo_handler_302_bogus).unwrap(); + api.register(demo_handler_302_found).unwrap(); + api.register(demo_handler_303_see_other).unwrap(); + api.register(demo_handler_307_temporary_redirect).unwrap(); api.register(demo_handler_websocket).unwrap(); /* @@ -743,6 +753,105 @@ async fn test_header_request() { assert_eq!(headers, vec!["hi", "howdy"]); } +/* + * Test 302 "Found" response with an invalid header value + */ +#[tokio::test] +async fn test_302_bogus() { + let api = demo_api(); + let testctx = common::test_setup("test_302_bogus", api); + let error = testctx + .client_testctx + .make_request_error( + Method::GET, + "/testing/302_bogus", + StatusCode::INTERNAL_SERVER_ERROR, + ) + .await; + assert_eq!(error.message, "Internal Server Error"); +} + +/* + * Test 302 "Found" response + */ +#[tokio::test] +async fn test_302_found() { + let api = demo_api(); + let testctx = common::test_setup("test_302_found", api); + let mut response = testctx + .client_testctx + .make_request( + Method::GET, + "/testing/302_found", + None as Option<()>, + StatusCode::FOUND, + ) + .await + .expect("expected success"); + let headers = response + .headers() + .get_all(http::header::LOCATION) + .iter() + .map(|v| v.to_str().unwrap()) + .collect::>(); + assert_eq!(headers, vec!["/path1"]); + assert_eq!(read_string(&mut response).await, ""); +} + +/* + * Test 303 "See Other" response + */ +#[tokio::test] +async fn test_303_see_other() { + let api = demo_api(); + let testctx = common::test_setup("test_303_see_other", api); + let mut response = testctx + .client_testctx + .make_request( + Method::GET, + "/testing/303_see_other", + None as Option<()>, + StatusCode::SEE_OTHER, + ) + .await + .expect("expected success"); + let headers = response + .headers() + .get_all(http::header::LOCATION) + .iter() + .map(|v| v.to_str().unwrap()) + .collect::>(); + assert_eq!(headers, vec!["/path2"]); + assert_eq!(read_string(&mut response).await, ""); +} + +/* + * Test 307 "Temporary Redirect" response + */ +#[tokio::test] +async fn test_307_temporary_redirect() { + let api = demo_api(); + let testctx = common::test_setup("test_307_temporary_redirect", api); + let mut response = testctx + .client_testctx + .make_request( + Method::GET, + "/testing/307_temporary_redirect", + None as Option<()>, + StatusCode::TEMPORARY_REDIRECT, + ) + .await + .expect("expected success"); + let headers = response + .headers() + .get_all(http::header::LOCATION) + .iter() + .map(|v| v.to_str().unwrap()) + .collect::>(); + assert_eq!(headers, vec!["/path3"]); + assert_eq!(read_string(&mut response).await, ""); +} + /* * The "test_demo_websocket" handler upgrades to a websocket and exchanges * greetings with the client. @@ -964,6 +1073,46 @@ async fn demo_handler_headers( Ok(response) } +#[endpoint { + method = GET, + path = "/testing/302_bogus", +}] +async fn demo_handler_302_bogus( + _rqctx: RequestCtx, +) -> Result { + http_response_found(String::from("\x10")) +} + +#[endpoint { + method = GET, + path = "/testing/302_found", +}] +async fn demo_handler_302_found( + _rqctx: RequestCtx, +) -> Result { + Ok(http_response_found(String::from("/path1")).unwrap()) +} + +#[endpoint { + method = GET, + path = "/testing/303_see_other", +}] +async fn demo_handler_303_see_other( + _rqctx: RequestCtx, +) -> Result { + Ok(http_response_see_other(String::from("/path2")).unwrap()) +} + +#[endpoint { + method = GET, + path = "/testing/307_temporary_redirect", +}] +async fn demo_handler_307_temporary_redirect( + _rqctx: RequestCtx, +) -> Result { + Ok(http_response_temporary_redirect(String::from("/path3")).unwrap()) +} + #[channel { protocol = WEBSOCKETS, path = "/testing/websocket" diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index c164672de..98e209d01 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -285,6 +285,93 @@ } } }, + "/test/302_found": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler21", + "responses": { + "302": { + "description": "redirect (found)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/test/303_see_other": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler22", + "responses": { + "303": { + "description": "redirect (see other)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/test/307_temporary_redirect": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler23", + "responses": { + "307": { + "description": "redirect (temporary redirect)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/test/camera": { "post": { "tags": [ diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index 2ecaa39f8..3f7ce94e9 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -1,11 +1,13 @@ // Copyright 2022 Oxide Computer Company use dropshot::{ - endpoint, ApiDescription, FreeformBody, HttpError, HttpResponseAccepted, - HttpResponseCreated, HttpResponseDeleted, HttpResponseHeaders, - HttpResponseOk, HttpResponseUpdatedNoContent, PaginationParams, Path, - Query, RequestContext, ResultsPage, TagConfig, TagDetails, TypedBody, - UntypedBody, + endpoint, http_response_found, http_response_see_other, + http_response_temporary_redirect, ApiDescription, FreeformBody, HttpError, + HttpResponseAccepted, HttpResponseCreated, HttpResponseDeleted, + HttpResponseFound, HttpResponseHeaders, HttpResponseOk, + HttpResponseSeeOther, HttpResponseTemporaryRedirect, + HttpResponseUpdatedNoContent, PaginationParams, Path, Query, + RequestContext, ResultsPage, TagConfig, TagDetails, TypedBody, UntypedBody, }; use hyper::Body; use schemars::JsonSchema; @@ -420,6 +422,39 @@ async fn handler20( Ok(HttpResponseCreated(Response {})) } +#[endpoint { + method = GET, + path = "/test/302_found", + tags = [ "it"], +}] +async fn handler21( + _rqctx: Arc>, +) -> Result { + Ok(http_response_found(String::from("/path1")).unwrap()) +} + +#[endpoint { + method = GET, + path = "/test/303_see_other", + tags = [ "it"], +}] +async fn handler22( + _rqctx: Arc>, +) -> Result { + Ok(http_response_see_other(String::from("/path2")).unwrap()) +} + +#[endpoint { + method = GET, + path = "/test/307_temporary_redirect", + tags = [ "it"], +}] +async fn handler23( + _rqctx: Arc>, +) -> Result { + Ok(http_response_temporary_redirect(String::from("/path3")).unwrap()) +} + fn make_api( maybe_tag_config: Option, ) -> Result, String> { @@ -449,6 +484,9 @@ fn make_api( api.register(handler18)?; api.register(handler19)?; api.register(handler20)?; + api.register(handler21)?; + api.register(handler22)?; + api.register(handler23)?; Ok(api) } diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index 1f141dbc2..b29b35fe9 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -293,6 +293,93 @@ } } }, + "/test/302_found": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler21", + "responses": { + "302": { + "description": "redirect (found)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/test/303_see_other": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler22", + "responses": { + "303": { + "description": "redirect (see other)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/test/307_temporary_redirect": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler23", + "responses": { + "307": { + "description": "redirect (temporary redirect)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/test/camera": { "post": { "tags": [