From cbfd5d94ee9ed70fb47dabfb221785d4625fe6de Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Wed, 18 Nov 2020 15:08:03 +0000 Subject: [PATCH] fix httpauth extraction error handling in middleware (#128) --- Cargo.toml | 1 + actix-web-httpauth/CHANGES.md | 3 + actix-web-httpauth/Cargo.toml | 9 +-- actix-web-httpauth/examples/cors.rs | 33 +++++++++ .../examples/middleware-closure.rs | 2 +- actix-web-httpauth/examples/middleware.rs | 2 +- actix-web-httpauth/src/middleware.rs | 72 +++++++++++-------- 7 files changed, 85 insertions(+), 37 deletions(-) create mode 100644 actix-web-httpauth/examples/cors.rs diff --git a/Cargo.toml b/Cargo.toml index fe1f02214..8d78c6939 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,4 +10,5 @@ members = [ ] [patch.crates-io] +actix-cors = { path = "actix-cors" } actix-session = { path = "actix-session" } diff --git a/actix-web-httpauth/CHANGES.md b/actix-web-httpauth/CHANGES.md index 0c7e9c6cc..0baeab777 100644 --- a/actix-web-httpauth/CHANGES.md +++ b/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 diff --git a/actix-web-httpauth/Cargo.toml b/actix-web-httpauth/Cargo.toml index f2499e956..b0f93cece 100644 --- a/actix-web-httpauth/Cargo.toml +++ b/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" -futures-util = { version = "0.3", default-features = false } +base64 = "0.13" 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" diff --git a/actix-web-httpauth/examples/cors.rs b/actix-web-httpauth/examples/cors.rs new file mode 100644 index 000000000..ac6226127 --- /dev/null +++ b/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 { + 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 +} diff --git a/actix-web-httpauth/examples/middleware-closure.rs b/actix-web-httpauth/examples/middleware-closure.rs index e26a11478..0935dc605 100644 --- a/actix-web-httpauth/examples/middleware-closure.rs +++ b/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) }); diff --git a/actix-web-httpauth/examples/middleware.rs b/actix-web-httpauth/examples/middleware.rs index 672e08d90..f1ad351fe 100644 --- a/actix-web-httpauth/examples/middleware.rs +++ b/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); diff --git a/actix-web-httpauth/src/middleware.rs b/actix-web-httpauth/src/middleware.rs index d0db6ea5c..8aac191ab 100644 --- a/actix-web-httpauth/src/middleware.rs +++ b/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::dev::{ServiceRequest, ServiceResponse}; -use actix_web::Error; -use futures_util::future::{self, FutureExt, LocalBoxFuture, TryFutureExt}; -use futures_util::task::{Context, Poll}; +use actix_web::{ + dev::{Service, ServiceRequest, ServiceResponse, Transform}, + Error, +}; +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, -/// this middleware returns an error immediately, -/// without calling the `F` callback. +/// If there is no `Authorization` header in the request, this middleware returns an error +/// immediately, without calling the `F` callback. /// -/// Otherwise, it will pass both the request and -/// the parsed credentials into it. -/// In case of successful validation `F` callback -/// is required to return the `ServiceRequest` back. +/// Otherwise, it will pass both the request and the parsed credentials into it. In case of +/// successful validation `F` callback is required to return the `ServiceRequest` back. #[derive(Debug, Clone)] pub struct HttpAuthentication where @@ -40,8 +41,7 @@ where F: Fn(ServiceRequest, T) -> O, O: Future>, { - /// Construct `HttpAuthentication` middleware - /// with the provided auth extractor `T` and + /// Construct `HttpAuthentication` middleware with the provided auth extractor `T` and /// validation callback `F`. pub fn with_fn(process_fn: F) -> HttpAuthentication { HttpAuthentication { @@ -56,21 +56,18 @@ where F: Fn(ServiceRequest, basic::BasicAuth) -> O, O: Future>, { - /// Construct `HttpAuthentication` middleware for the HTTP "Basic" - /// authentication scheme. + /// Construct `HttpAuthentication` middleware for the HTTP "Basic" 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, - /// // but since it is required to return anything - /// // that implements `IntoFuture` trait, - /// // it can be extended to query database - /// // or to do something else in a async manner. + /// // In this example validator returns immediately, but since it is required to return + /// // anything that implements `IntoFuture` trait, 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>, { - /// Construct `HttpAuthentication` middleware for the HTTP "Bearer" - /// authentication scheme. + /// Construct `HttpAuthentication` middleware for the HTTP "Bearer" 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::::new(req).await?; + let (req, credentials) = match Extract::::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 - // separate lines, or else a panic occurs. + // Ensure `borrow_mut()` and `.await` are on 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 { 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)))