404 response not returned when expected

[pjenvey]

test_storage::test_that_404_responses_have_a_json_body currently fails, expecting a 404 at "/nonexistent/url"

[philbooth]

Fwiw, I spent a bit of time trying to fix this today, but failed to bend Actix to my will. Essentially I was hoping to use App::default_resource like so:

diff --git a/src/server/mod.rs b/src/server/mod.rs
index 0530100..fb60c6e 100644
--- a/src/server/mod.rs
+++ b/src/server/mod.rs
@@ -74,7 +74,11 @@ pub fn build_app(state: ServerState) -> App<ServerState> {
         .middleware(middleware::WeaveTimestamp)
         .middleware(middleware::DbTransaction)
         .middleware(middleware::PreConditionCheck)
-        .configure(|app| init_routes!(Cors::for_app(app)).register())
+        .configure(|app| {
+            init_routes!(Cors::for_app(app))
+                .register()
+                .default_resource(|r| r.f(handlers::not_found))
+        })
 }

 pub struct Server {}
diff --git a/src/web/handlers.rs b/src/web/handlers.rs
index 685f831..060579e 100644
--- a/src/web/handlers.rs
+++ b/src/web/handlers.rs
@@ -1,7 +1,7 @@
 //! API Handlers
 use std::collections::HashMap;

-use actix_web::{http::StatusCode, FutureResponse, HttpResponse, State};
+use actix_web::{http::StatusCode, FutureResponse, HttpRequest, HttpResponse, State};
 use futures::future::{self, Either, Future};
 use serde::Serialize;

@@ -368,3 +368,9 @@ pub fn get_configuration(
 ) -> FutureResponse<HttpResponse> {
     Box::new(future::result(Ok(HttpResponse::Ok().json(&*state.limits))))
 }
+
+pub fn not_found(req: &HttpRequest<ServerState>) -> HttpResponse {
+    req.build_response(StatusCode::NOT_FOUND)
+        .content_type("application/json")
+        .body("0")
+}

Not sure if the cors stuff is affecting it or maybe I've misunderstood the docs/example. Anyway, just recording what I tried, I can come back to this later (want to try and achieve something tangible before today's meeting, lol).

Branch: pb/115

[pjenvey]

@philbooth tried out this branch against the e2e test w/ RUST_LOG=actix_web=debug: which spit out actix_web::pipeline: Error occured during request handling: missing field `uid` caused by HawkIdentifier::from_request in the PreconditionCheck response.

So errors from middlewares short circuit the request handling.

_2019-06-04 edit: RUSTLOG -> RUST_LOG_

[jrconlin]

Looking at latest master:

GET /some/url => 404 GET /1.5/some/url => 401

Is this still a valid bug? Do we want to do a resource check before authorization (I'd think we'd want to know the request is valid before we do a resource check)?

[pjenvey]

The test still fails (still a valid issue) but it's probably the least important of the failing e2e tests.

Let's hold off on this one for now, as it's dependent on actix-web's middleware behavior which significantly changes in 1.0.

The current middlewares could either be more careful to avoid error responses in cases like this or possibly they could check the request to determine if the route had a specific match (if no match, don't do anything, we're 404ing anyway).

..but let's wait for 1.0 to have a chance to settle

[pjenvey]

This was fixed during the actix-web 1.0 upgrade #126