From 05daf9376e2680a181e5d09c8c99256ec49c386d Mon Sep 17 00:00:00 2001
From: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
Date: Sat, 11 Sep 2021 18:42:11 +0300
Subject: [PATCH] HttpAuthentication: fix error handling

---
 actix-web-httpauth/src/middleware.rs | 57 ++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/actix-web-httpauth/src/middleware.rs b/actix-web-httpauth/src/middleware.rs
index 983b9ff0ad..860b52517c 100644
--- a/actix-web-httpauth/src/middleware.rs
+++ b/actix-web-httpauth/src/middleware.rs
@@ -121,7 +121,7 @@ where
     S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error> + 'static,
     S::Future: 'static,
     F: Fn(ServiceRequest, T) -> O + 'static,
-    O: Future<Output = Result<ServiceRequest, Error>> + 'static,
+    O: Future<Output = Result<ServiceRequest, Error>>,
     T: AuthExtractor + 'static,
     B: MessageBody + 'static,
     B::Error: StdError,
@@ -156,7 +156,7 @@ where
     S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error> + 'static,
     S::Future: 'static,
     F: Fn(ServiceRequest, T) -> O + 'static,
-    O: Future<Output = Result<ServiceRequest, Error>> + 'static,
+    O: Future<Output = Result<ServiceRequest, Error>>,
     T: AuthExtractor + 'static,
     B: MessageBody + 'static,
     B::Error: StdError,
@@ -173,16 +173,16 @@ where
         let service = Rc::clone(&self.service);
 
         async move {
-            let (req, credentials) = match Extract::<T>::new(req).await {
-                Ok(req) => req,
-                Err((err, req)) => {
-                    return Ok(req.error_response(err));
-                }
+            let (mut req, credentials) = match Extract::<T>::new(req).await {
+                Ok((req, cred)) => (req, cred),
+                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?;
+            let req_cloned = ServiceRequest::from_request(req.parts_mut().0.clone());
+            let req = match process_fn(req, credentials).await {
+                Ok(req) => req,
+                Err(err) => return Ok(req_cloned.error_response(err)),
+            };
 
             service
                 .call(req)
@@ -246,9 +246,42 @@ where
 mod tests {
     use super::*;
     use crate::extractors::bearer::BearerAuth;
-    use actix_service::{into_service, Service};
+    use actix_service::{into_service, Service, Transform};
     use actix_web::error;
-    use actix_web::test::TestRequest;
+    use actix_web::http::{header::*, StatusCode};
+    use actix_web::test::{ok_service, TestRequest};
+
+    #[actix_rt::test]
+    async fn test_middleware_basic() {
+        let mw = HttpAuthentication::basic(|req, cred| async move {
+            match cred.user_id() {
+                id if id == "test_id" => Ok(req),
+                _ => Err(error::ErrorUnauthorized("")),
+            }
+        })
+        .new_transform(ok_service())
+        .await
+        .unwrap();
+
+        let req = TestRequest::default();
+        let resp = mw.call(req.to_srv_request()).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
+        assert_eq!(resp.headers().get(WWW_AUTHENTICATE).unwrap(), "Basic");
+
+        let req = TestRequest::default()
+            .insert_header(("Authorization", "Basic YTo=" /* Wromg ID */));
+        let resp = mw.call(req.to_srv_request()).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
+        assert!(resp.headers().get(WWW_AUTHENTICATE).is_none());
+
+        let req = TestRequest::default().insert_header((
+            "Authorization",
+            "Basic dGVzdF9pZDo=" /* ID for "test_id" */
+        ));
+        let resp = mw.call(req.to_srv_request()).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+        assert!(resp.headers().get(WWW_AUTHENTICATE).is_none());
+    }
 
     /// This is a test for https://github.com/actix/actix-extras/issues/10
     #[actix_rt::test]