fix httpauth extraction error handling in middleware (#128)

Cargo.toml

@@ -10,4 +10,5 @@ members = [
 ]
 
 [patch.crates-io]
+actix-cors = { path = "actix-cors" }
 actix-session = { path = "actix-session" }

actix-web-httpauth/CHANGES.md

@@ -1,6 +1,9 @@
 # Changes
 
 ## Unreleased - 2020-xx-xx
+* Correct error handling when extracting auth details from request. [#128]
+
+[#128]: https://github.com/actix/actix-web-httpauth/pull/128
 
 
 ## 0.5.0 - 2020-09-11

actix-web-httpauth/Cargo.toml

@@ -21,10 +21,11 @@ path = "src/lib.rs"
 
 [dependencies]
 actix-web = { version = "3.0.0", default_features = false }
-actix-service = "1.0.6"
+base64 = "0.13"
-futures-util = { version = "0.3", default-features = false }
 bytes = "0.5"
-base64 = "0.12"
+futures-util = { version = "0.3", default-features = false }
 
 [dev-dependencies]
-actix-rt = "1"
+actix-cors = "0.5"
+actix-rt = "1.1.1"
+actix-service = "1.0.6"

actix-web-httpauth/examples/cors.rs

@@ -0,0 +1,33 @@
+use actix_cors::Cors;
+use actix_web::{dev::ServiceRequest, get, App, Error, HttpResponse, HttpServer};
+use actix_web_httpauth::{
+    extractors::bearer::BearerAuth, middleware::HttpAuthentication,
+};
+
+async fn ok_validator(
+    req: ServiceRequest,
+    credentials: BearerAuth,
+) -> Result<ServiceRequest, Error> {
+    eprintln!("{:?}", credentials);
+    Ok(req)
+}
+
+#[get("/")]
+async fn index() -> HttpResponse {
+    HttpResponse::Ok().finish()
+}
+
+#[actix_web::main]
+async fn main() -> std::io::Result<()> {
+    HttpServer::new(move || {
+        App::new()
+            .wrap(HttpAuthentication::bearer(ok_validator))
+            // ensure the CORS middleware is wrapped around the httpauth middleware so it is able to
+            // add headers to error responses
+            .wrap(Cors::permissive())
+            .service(index)
+    })
+    .bind(("127.0.0.1", 8080))?
+    .run()
+    .await
+}

actix-web-httpauth/examples/middleware-closure.rs

@@ -2,7 +2,7 @@ use actix_web::{middleware, web, App, HttpServer};
 
 use actix_web_httpauth::middleware::HttpAuthentication;
 
-#[actix_rt::main]
+#[actix_web::main]
 async fn main() -> std::io::Result<()> {
     HttpServer::new(|| {
         let auth = HttpAuthentication::basic(|req, _credentials| async { Ok(req) });

actix-web-httpauth/examples/middleware.rs

@@ -11,7 +11,7 @@ async fn validator(
     Ok(req)
 }
 
-#[actix_rt::main]
+#[actix_web::main]
 async fn main() -> std::io::Result<()> {
     HttpServer::new(|| {
         let auth = HttpAuthentication::basic(validator);

actix-web-httpauth/src/middleware.rs

@@ -7,24 +7,25 @@ use std::pin::Pin;
 use std::rc::Rc;
 use std::sync::Arc;
 
-use actix_service::{Service, Transform};
+use actix_web::{
-use actix_web::dev::{ServiceRequest, ServiceResponse};
+    dev::{Service, ServiceRequest, ServiceResponse, Transform},
-use actix_web::Error;
+    Error,
-use futures_util::future::{self, FutureExt, LocalBoxFuture, TryFutureExt};
+};
-use futures_util::task::{Context, Poll};
+use futures_util::{
+    future::{self, FutureExt as _, LocalBoxFuture, TryFutureExt as _},
+    ready,
+    task::{Context, Poll},
+};
 
 use crate::extractors::{basic, bearer, AuthExtractor};
 
 /// Middleware for checking HTTP authentication.
 ///
-/// If there is no `Authorization` header in the request,
+/// If there is no `Authorization` header in the request, this middleware returns an error
-/// this middleware returns an error immediately,
+/// immediately, without calling the `F` callback.
-/// without calling the `F` callback.
 ///
-/// Otherwise, it will pass both the request and
+/// Otherwise, it will pass both the request and the parsed credentials into it. In case of
-/// the parsed credentials into it.
+/// successful validation `F` callback is required to return the `ServiceRequest` back.
-/// In case of successful validation `F` callback
-/// is required to return the `ServiceRequest` back.
 #[derive(Debug, Clone)]
 pub struct HttpAuthentication<T, F>
 where
@@ -40,8 +41,7 @@ where
     F: Fn(ServiceRequest, T) -> O,
     O: Future<Output = Result<ServiceRequest, Error>>,
 {
-    /// Construct `HttpAuthentication` middleware
+    /// Construct `HttpAuthentication` middleware with the provided auth extractor `T` and
-    /// with the provided auth extractor `T` and
     /// validation callback `F`.
     pub fn with_fn(process_fn: F) -> HttpAuthentication<T, F> {
         HttpAuthentication {
@@ -56,21 +56,18 @@ where
     F: Fn(ServiceRequest, basic::BasicAuth) -> O,
     O: Future<Output = Result<ServiceRequest, Error>>,
 {
-    /// Construct `HttpAuthentication` middleware for the HTTP "Basic"
+    /// Construct `HttpAuthentication` middleware for the HTTP "Basic" authentication scheme.
-    /// authentication scheme.
     ///
-    /// ## Example
+    /// # Example
     ///
     /// ```
     /// # use actix_web::Error;
     /// # use actix_web::dev::ServiceRequest;
     /// # use actix_web_httpauth::middleware::HttpAuthentication;
     /// # use actix_web_httpauth::extractors::basic::BasicAuth;
-    /// // In this example validator returns immediately,
+    /// // In this example validator returns immediately, but since it is required to return
-    /// // but since it is required to return anything
+    /// // anything that implements `IntoFuture` trait, it can be extended to query database or to
-    /// // that implements `IntoFuture` trait,
+    /// // do something else in a async manner.
-    /// // it can be extended to query database
-    /// // or to do something else in a async manner.
     /// async fn validator(
     ///     req: ServiceRequest,
     ///     credentials: BasicAuth,
@@ -91,10 +88,9 @@ where
     F: Fn(ServiceRequest, bearer::BearerAuth) -> O,
     O: Future<Output = Result<ServiceRequest, Error>>,
 {
-    /// Construct `HttpAuthentication` middleware for the HTTP "Bearer"
+    /// Construct `HttpAuthentication` middleware for the HTTP "Bearer" authentication scheme.
-    /// authentication scheme.
     ///
-    /// ## Example
+    /// # Example
     ///
     /// ```
     /// # use actix_web::Error;
@@ -176,15 +172,22 @@ where
     }
 
     fn call(&mut self, req: Self::Request) -> Self::Future {
-        let process_fn = self.process_fn.clone();
+        let process_fn = Arc::clone(&self.process_fn);
 
         let service = Rc::clone(&self.service);
 
         async move {
-            let (req, credentials) = Extract::<T>::new(req).await?;
+            let (req, credentials) = match Extract::<T>::new(req).await {
+                Ok(req) => req,
+                Err((err, req)) => {
+                    return Ok(req.error_response(err));
+                }
+            };
+
+            // TODO: alter to remove ? operator; an error response is required for downstream
+            // middleware to do their thing (eg. cors adding headers)
             let req = process_fn(req, credentials).await?;
-            // It is important that `borrow_mut()` and `.await` are on
+            // Ensure `borrow_mut()` and `.await` are on separate lines or else a panic occurs.
-            // separate lines, or else a panic occurs.
             let fut = service.borrow_mut().call(req);
             fut.await
         }
@@ -214,7 +217,7 @@ where
     T::Future: 'static,
     T::Error: 'static,
 {
-    type Output = Result<(ServiceRequest, T), Error>;
+    type Output = Result<(ServiceRequest, T), (Error, ServiceRequest)>;
 
     fn poll(mut self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll<Self::Output> {
         if self.f.is_none() {
@@ -227,7 +230,14 @@ where
             .f
             .as_mut()
             .expect("Extraction future should be initialized at this point");
-        let credentials = futures_util::ready!(Future::poll(f.as_mut(), ctx))?;
+
+        let credentials = ready!(f.as_mut().poll(ctx)).map_err(|err| {
+            (
+                err,
+                // returning request allows a proper error response to be created
+                self.req.take().expect("Extract future was polled twice!"),
+            )
+        })?;
 
         let req = self.req.take().expect("Extract future was polled twice!");
         Poll::Ready(Ok((req, credentials)))