pr feedback

elpiel · elpiel · commit 872edbc2ee01 · 2019-11-12T10:44:24.000+02:00
diff --git a/sentry/src/lib.rs b/sentry/src/lib.rs
@@ -39,7 +39,7 @@ pub mod event_reducer;
 
 pub struct Application<A: Adapter> {
     adapter: A,
-    logger: slog::Logger,
+    logger: Logger,
     redis: SharedConnection,
     _clustered: bool,
     port: u16,
@@ -73,14 +73,21 @@ impl<A: Adapter + 'static> Application<A> {
         let make_service = make_service_fn(move |_| {
             let adapter_config = (self.adapter.clone(), self.config.clone());
             let redis = self.redis.clone();
+            let logger = self.logger.clone();
             async move {
                 Ok::<_, Error>(service_fn(move |req| {
                     let adapter_config = adapter_config.clone();
                     let redis = redis.clone();
+                    let logger = logger.clone();
                     async move {
                         Ok::<_, Error>(
-                            handle_routing(req, (&adapter_config.0, &adapter_config.1), redis)
-                                .await,
+                            handle_routing(
+                                req,
+                                (&adapter_config.0, &adapter_config.1),
+                                redis,
+                                &logger,
+                            )
+                            .await,
                         )
                     }
                 }))
@@ -114,6 +121,7 @@ async fn handle_routing(
     req: Request<Body>,
     (adapter, config): (&impl Adapter, &Config),
     redis: SharedConnection,
+    logger: &Logger,
 ) -> Response<Body> {
     let headers = match cors(&req) {
         Some(Cors::Simple(headers)) => headers,
@@ -124,7 +132,11 @@ async fn handle_routing(
 
     let req = match auth::for_request(req, adapter, redis.clone()).await {
         Ok(req) => req,
-        Err(response_error) => return map_response_error(response_error),
+        Err(error) => {
+            error!(&logger, "{}", &error; "module" => "middleware-auth");
+
+            return map_response_error(ResponseError::BadRequest(error));
+        }
     };
 
     let mut response = match (req.uri().path(), req.method()) {
@@ -155,8 +167,8 @@ pub fn not_found() -> Response<Body> {
     response
 }
 
-pub fn bad_request(error: Box<dyn std::error::Error>) -> Response<Body> {
-    let body = Body::from(format!("Bad Request: {}", error));
+pub fn bad_request(_: Box<dyn std::error::Error>) -> Response<Body> {
+    let body = Body::from("Bad Request: try again later");
     let mut response = Response::new(body);
     let status = response.status_mut();
     *status = StatusCode::BAD_REQUEST;
diff --git a/sentry/src/middleware/auth.rs b/sentry/src/middleware/auth.rs
@@ -1,18 +1,20 @@
+use std::error;
+
 use hyper::header::AUTHORIZATION;
 use hyper::{Body, Request};
 use redis::aio::SharedConnection;
 
 use primitives::adapter::{Adapter, Session as AdapterSession};
 
-use crate::{ResponseError, Session};
+use crate::Session;
 
 /// Check `Authorization` header for `Bearer` scheme with `Adapter::session_from_token`.
 /// If the `Adapter` fails to create an `AdapterSession`, `ResponseError::BadRequest` will be returned.
 pub(crate) async fn for_request(
     mut req: Request<Body>,
     adapter: &impl Adapter,
     redis: SharedConnection,
-) -> Result<Request<Body>, ResponseError> {
+) -> Result<Request<Body>, Box<dyn error::Error>> {
     let authorization = req.headers().get(AUTHORIZATION);
 
     let prefix = "Bearer ";
@@ -36,16 +38,8 @@ pub(crate) async fn for_request(
             .arg(token)
             .query_async::<_, Option<String>>(&mut redis.clone())
             .await?
-            .and_then(|session_str| {
-                match serde_json::from_str::<AdapterSession>(&session_str) {
-                    Ok(session) => Some(session),
-                    Err(serde_error) => {
-                        // log message instead
-                        println!("{}", serde_error);
-                        None
-                    }
-                }
-            }) {
+            .and_then(|session_str| serde_json::from_str::<AdapterSession>(&session_str).ok())
+        {
             Some(adapter_session) => adapter_session,
             None => {
                 // If there was a problem with the Session or the Token, this will error
@@ -85,14 +79,17 @@ fn get_request_ip(req: &Request<Body>) -> Option<String> {
 
 #[cfg(test)]
 mod test {
-    use super::*;
-    use crate::db::redis_connection;
-    use adapter::DummyAdapter;
     use hyper::Request;
+
+    use adapter::DummyAdapter;
     use primitives::adapter::DummyAdapterOptions;
     use primitives::config::configuration;
     use primitives::util::tests::prep_db::{AUTH, IDS};
 
+    use crate::db::redis_connection;
+
+    use super::*;
+
     async fn setup() -> (DummyAdapter, SharedConnection) {
         let adapter_options = DummyAdapterOptions {
             dummy_identity: IDS["leader"].clone(),
@@ -143,9 +140,9 @@ mod test {
             .body(Body::empty())
             .unwrap();
         match for_request(non_existent_token_req, &dummy_adapter, redis).await {
-            Err(ResponseError::BadRequest(error)) => {
+            Err(error) => {
                 assert!(error.to_string().contains("no session token for this auth: wrong-token"), "Wrong error received");
-            },
+            }
             _ => panic!("We shouldn't get a success response nor a different Error than BadRequest for this call"),
         };
     }
