From 085ee4c394fd0a39adb2dbcd0f029bee0b14aadf Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Fri, 19 Jul 2019 14:10:23 +0600 Subject: [PATCH] remove ClonableService usage --- CHANGES.md | 4 +- Cargo.toml | 4 +- src/session.rs | 372 ++++++++++++++++++++++++++++--------------------- 3 files changed, 222 insertions(+), 158 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 7dc8a36a9..ee7e0799a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,8 @@ # Changes -## 0.6.1 (2019-07-11) +## 0.6.1 (2019-07-19) + +* remove ClonableService usage * added comprehensive tests for session workflow diff --git a/Cargo.toml b/Cargo.toml index 434f6ea26..e87513d2a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,7 @@ time = "0.1.42" # actix web session actix-web = { version = "1.0.3", optional = true } -actix-utils = { version = "0.4.2", optional = true } +actix-utils = { version = "0.4.5", optional = true } actix-service = { version = "0.4.1", optional = true } actix-session = { version = "0.2.0", optional = true } rand = { version = "0.7.0", optional = true } @@ -50,7 +50,7 @@ serde = { version = "1.0.94", optional = true , features = ["derive"]} serde_json = { version = "1.0.40", optional = true } env_logger = "0.6.2" actix-http-test = "0.2.2" -actix-http = "0.2.5" +actix-http = "0.2.6" [dev-dependencies] env_logger = "0.6" diff --git a/src/session.rs b/src/session.rs index 2de0b2d87..a395f0fec 100644 --- a/src/session.rs +++ b/src/session.rs @@ -1,8 +1,9 @@ +use std::cell::RefCell; use std::{collections::HashMap, iter, rc::Rc}; + use actix::prelude::*; use actix_service::{Service, Transform}; use actix_session::{Session, SessionStatus}; -use actix_utils::cloneable::CloneableService; use actix_web::cookie::{Cookie, CookieJar, Key, SameSite}; use actix_web::dev::{ServiceRequest, ServiceResponse}; use actix_web::http::header::{self, HeaderValue}; @@ -103,7 +104,7 @@ where fn new_transform(&self, service: S) -> Self::Future { ok(RedisSessionMiddleware { - service: CloneableService::new(service), + service: Rc::new(RefCell::new(service)), inner: self.0.clone(), }) } @@ -111,7 +112,7 @@ where /// Cookie session middleware pub struct RedisSessionMiddleware { - service: CloneableService, + service: Rc>, inner: Rc, } @@ -128,7 +129,7 @@ where type Future = Box>; fn poll_ready(&mut self) -> Poll<(), Self::Error> { - self.service.poll_ready() + self.service.borrow_mut().poll_ready() } fn call(&mut self, mut req: ServiceRequest) -> Self::Future { @@ -145,53 +146,50 @@ where srv.call(req).and_then(move |mut res| { match Session::get_changes(&mut res) { - (SessionStatus::Unchanged, None) => - Either::A(Either::A(Either::A(ok(res)))), - (SessionStatus::Unchanged, Some(state)) => - Either::A(Either::A(Either::B( - if value.is_none(){ // implies the session is new - Either::A( - inner.update(res, state, value) - ) - } else { - Either::B( - ok(res) - ) - } - ))), - (SessionStatus::Changed, Some(state)) => - Either::A(Either::B(Either::A(inner.update(res, state, value)))), + (SessionStatus::Unchanged, None) => { + Either::A(Either::A(Either::A(ok(res)))) + } + (SessionStatus::Unchanged, Some(state)) => { + Either::A(Either::A(Either::B(if value.is_none() { + // implies the session is new + Either::A(inner.update(res, state, value)) + } else { + Either::B(ok(res)) + }))) + } + (SessionStatus::Changed, Some(state)) => { + Either::A(Either::B(Either::A(inner.update(res, state, value)))) + } (SessionStatus::Purged, Some(_)) => { - if let Some(val) = value { - Either::A(Either::B(Either::B(Either::A( - inner.clear_cache(val) - .and_then(move |_| - match inner.remove_cookie(&mut res){ - Ok(_) => Either::A(ok(res)), - Err(_err) => Either::B(err( - error::ErrorInternalServerError(_err))) - }) - )))) - } else { - Either::A(Either::B(Either::B(Either::B( - err(error::ErrorInternalServerError("unexpected")) - )))) - } - }, - (SessionStatus::Renewed, Some(state)) => { + if let Some(val) = value { + Either::A(Either::B(Either::B(Either::A( + inner.clear_cache(val).and_then(move |_| { + match inner.remove_cookie(&mut res) { + Ok(_) => Either::A(ok(res)), + Err(_err) => Either::B(err( + error::ErrorInternalServerError(_err), + )), + } + }), + )))) + } else { + Either::A(Either::B(Either::B(Either::B(err( + error::ErrorInternalServerError("unexpected"), + ))))) + } + } + (SessionStatus::Renewed, Some(state)) => { if let Some(val) = value { Either::B(Either::A( - inner.clear_cache(val) - .and_then(move |_| - inner.update(res, state, None)) + inner + .clear_cache(val) + .and_then(move |_| inner.update(res, state, None)), )) } else { - Either::B(Either::B( - inner.update(res, state, None) - )) + Either::B(Either::B(inner.update(res, state, None))) } - }, - (_, None) => unreachable!() + } + (_, None) => unreachable!(), } }) })) @@ -329,97 +327,102 @@ impl Inner { } } - /// removes cache entry - fn clear_cache(&self, key: String) - -> impl Future { + /// removes cache entry + fn clear_cache(&self, key: String) -> impl Future { self.addr - .send(Command(resp_array!["DEL", key])) - .map_err(Error::from) - .and_then(|res| { - match res { - // redis responds with number of deleted records - Ok(RespValue::Integer(x)) if x > 0 => Ok(()), - _ => Err(error::ErrorInternalServerError( - "failed to remove session from cache")) - } - }) - } + .send(Command(resp_array!["DEL", key])) + .map_err(Error::from) + .and_then(|res| { + match res { + // redis responds with number of deleted records + Ok(RespValue::Integer(x)) if x > 0 => Ok(()), + _ => Err(error::ErrorInternalServerError( + "failed to remove session from cache", + )), + } + }) + } /// invalidates session cookie - fn remove_cookie(&self, res: &mut ServiceResponse) - -> Result<(), Error> { + fn remove_cookie(&self, res: &mut ServiceResponse) -> Result<(), Error> { let mut cookie = Cookie::named(self.name.clone()); cookie.set_value(""); cookie.set_max_age(Duration::seconds(0)); cookie.set_expires(time::now() - Duration::days(365)); let val = HeaderValue::from_str(&cookie.to_string()) - .map_err(|err| error::ErrorInternalServerError(err))?; + .map_err(|err| error::ErrorInternalServerError(err))?; res.headers_mut().append(header::SET_COOKIE, val); Ok(()) } } - #[cfg(test)] mod test { use super::*; - use actix_http::{HttpService, httpmessage::HttpMessage}; - use actix_http_test::{TestServer, block_on}; + use actix_http::{httpmessage::HttpMessage, HttpService}; + use actix_http_test::{block_on, TestServer}; use actix_session::Session; - use actix_web::{middleware, web, App, HttpResponse, HttpServer, Result, - web::{resource, get, post}}; + use actix_web::{ + middleware, web, + web::{get, post, resource}, + App, HttpResponse, HttpServer, Result, + }; use serde::{Deserialize, Serialize}; use serde_json::json; use time; - #[derive(Serialize, Deserialize, Debug, PartialEq)] pub struct IndexResponse { user_id: Option, - counter: i32 + counter: i32, } fn index(session: Session) -> Result { let user_id: Option = session.get::("user_id").unwrap(); - let counter: i32 = session.get::("counter") - .unwrap_or(Some(0)) - .unwrap_or(0); + let counter: i32 = session + .get::("counter") + .unwrap_or(Some(0)) + .unwrap_or(0); - Ok(HttpResponse::Ok().json(IndexResponse{user_id, counter})) + Ok(HttpResponse::Ok().json(IndexResponse { user_id, counter })) } - fn do_something(session: Session) -> Result { let user_id: Option = session.get::("user_id").unwrap(); - let counter: i32 = session.get::("counter") - .unwrap_or(Some(0)) - .map_or(1, |inner| inner + 1); + let counter: i32 = session + .get::("counter") + .unwrap_or(Some(0)) + .map_or(1, |inner| inner + 1); session.set("counter", counter)?; - Ok(HttpResponse::Ok().json(IndexResponse{user_id, counter})) + Ok(HttpResponse::Ok().json(IndexResponse { user_id, counter })) } #[derive(Deserialize)] struct Identity { - user_id: String + user_id: String, } fn login(user_id: web::Json, session: Session) -> Result { let id = user_id.into_inner().user_id; session.set("user_id", &id)?; session.renew(); - let counter: i32 = session.get::("counter") - .unwrap_or(Some(0)) - .unwrap_or(0); + let counter: i32 = session + .get::("counter") + .unwrap_or(Some(0)) + .unwrap_or(0); - Ok(HttpResponse::Ok().json(IndexResponse{user_id: Some(id), counter})) + Ok(HttpResponse::Ok().json(IndexResponse { + user_id: Some(id), + counter, + })) } fn logout(session: Session) -> Result { let id: Option = session.get("user_id")?; - if let Some(x) = id{ + if let Some(x) = id { session.purge(); Ok(format!("Logged out: {}", x).into()) } else { @@ -429,17 +432,17 @@ mod test { #[test] fn test_workflow() { - // Step 1: GET index + // Step 1: GET index // - set-cookie actix-session will be in response (session cookie #1) // - response should be: {"counter": 0, "user_id": None} // Step 2: GET index, including session cookie #1 in request // - set-cookie will *not* be in response // - response should be: {"counter": 0, "user_id": None} // Step 3: POST to do_something, including session cookie #1 in request - // - adds new session state in redis: {"counter": 1} + // - adds new session state in redis: {"counter": 1} // - response should be: {"counter": 1, "user_id": None} // Step 4: POST again to do_something, including session cookie #1 in request - // - updates session state in redis: {"counter": 2} + // - updates session state in redis: {"counter": 2} // - response should be: {"counter": 2, "user_id": None} // Step 5: POST to login, including session cookie #1 in request // - set-cookie actix-session will be in response (session cookie #2) @@ -459,86 +462,124 @@ mod test { // - set-cookie actix-session will be in response (session cookie #3) // - response should be: {"counter": 0, "user_id": None} - let mut srv = - TestServer::new(|| { - HttpService::new( - App::new() - .wrap(RedisSession::new("127.0.0.1:6379", &[0; 32]) - .cookie_name("test-session")) - .wrap(middleware::Logger::default()) - .service(resource("/").route(get().to(index))) - .service(resource("/do_something").route(post().to(do_something))) - .service(resource("/login").route(post().to(login))) - .service(resource("/logout").route(post().to(logout))) - ) - }); + let mut srv = TestServer::new(|| { + HttpService::new( + App::new() + .wrap( + RedisSession::new("127.0.0.1:6379", &[0; 32]) + .cookie_name("test-session"), + ) + .wrap(middleware::Logger::default()) + .service(resource("/").route(get().to(index))) + .service(resource("/do_something").route(post().to(do_something))) + .service(resource("/login").route(post().to(login))) + .service(resource("/logout").route(post().to(logout))), + ) + }); - - // Step 1: GET index + // Step 1: GET index // - set-cookie actix-session will be in response (session cookie #1) // - response should be: {"counter": 0, "user_id": None} let req_1a = srv.get("/").send(); let mut resp_1 = srv.block_on(req_1a).unwrap(); - let cookie_1 = resp_1.cookies().unwrap().clone() - .into_iter().find(|c| c.name() == "test-session") - .unwrap(); + let cookie_1 = resp_1 + .cookies() + .unwrap() + .clone() + .into_iter() + .find(|c| c.name() == "test-session") + .unwrap(); let result_1 = block_on(resp_1.json::()).unwrap(); - assert_eq!(result_1, IndexResponse{user_id: None, counter: 0}); - + assert_eq!( + result_1, + IndexResponse { + user_id: None, + counter: 0 + } + ); // Step 2: GET index, including session cookie #1 in request // - set-cookie will *not* be in response // - response should be: {"counter": 0, "user_id": None} let req_2 = srv.get("/").cookie(cookie_1.clone()).send(); let resp_2 = srv.block_on(req_2).unwrap(); - let cookie_2 = resp_2.cookies().unwrap().clone() - .into_iter().find(|c| c.name() == "test-session"); + let cookie_2 = resp_2 + .cookies() + .unwrap() + .clone() + .into_iter() + .find(|c| c.name() == "test-session"); assert_eq!(cookie_2, None); - // Step 3: POST to do_something, including session cookie #1 in request - // - adds new session state in redis: {"counter": 1} + // - adds new session state in redis: {"counter": 1} // - response should be: {"counter": 1, "user_id": None} let req_3 = srv.post("/do_something").cookie(cookie_1.clone()).send(); let mut resp_3 = srv.block_on(req_3).unwrap(); let result_3 = block_on(resp_3.json::()).unwrap(); - assert_eq!(result_3, IndexResponse{user_id: None, counter: 1}); - + assert_eq!( + result_3, + IndexResponse { + user_id: None, + counter: 1 + } + ); // Step 4: POST again to do_something, including session cookie #1 in request - // - updates session state in redis: {"counter": 2} + // - updates session state in redis: {"counter": 2} // - response should be: {"counter": 2, "user_id": None} let req_4 = srv.post("/do_something").cookie(cookie_1.clone()).send(); let mut resp_4 = srv.block_on(req_4).unwrap(); let result_4 = block_on(resp_4.json::()).unwrap(); - assert_eq!(result_4, IndexResponse{user_id: None, counter: 2}); - + assert_eq!( + result_4, + IndexResponse { + user_id: None, + counter: 2 + } + ); // Step 5: POST to login, including session cookie #1 in request // - set-cookie actix-session will be in response (session cookie #2) // - updates session state in redis: {"counter": 2, "user_id": "ferris"} - let req_5 = srv.post("/login") - .cookie(cookie_1.clone()) - .send_json(&json!({"user_id": "ferris"})); + let req_5 = srv + .post("/login") + .cookie(cookie_1.clone()) + .send_json(&json!({"user_id": "ferris"})); let mut resp_5 = srv.block_on(req_5).unwrap(); - let cookie_2 = resp_5.cookies().unwrap().clone() - .into_iter().find(|c| c.name() == "test-session") - .unwrap(); - assert_eq!(true, cookie_1.value().to_string() != cookie_2.value().to_string()); + let cookie_2 = resp_5 + .cookies() + .unwrap() + .clone() + .into_iter() + .find(|c| c.name() == "test-session") + .unwrap(); + assert_eq!( + true, + cookie_1.value().to_string() != cookie_2.value().to_string() + ); let result_5 = block_on(resp_5.json::()).unwrap(); - assert_eq!(result_5, IndexResponse{user_id: Some("ferris".into()), counter: 2}); - + assert_eq!( + result_5, + IndexResponse { + user_id: Some("ferris".into()), + counter: 2 + } + ); // Step 6: GET index, including session cookie #2 in request // - response should be: {"counter": 2, "user_id": "ferris"} - let req_6 = srv.get("/") - .cookie(cookie_2.clone()) - .send(); + let req_6 = srv.get("/").cookie(cookie_2.clone()).send(); let mut resp_6 = srv.block_on(req_6).unwrap(); let result_6 = block_on(resp_6.json::()).unwrap(); - assert_eq!(result_6, IndexResponse{user_id: Some("ferris".into()), counter: 2}); - + assert_eq!( + result_6, + IndexResponse { + user_id: Some("ferris".into()), + counter: 2 + } + ); // Step 7: POST again to do_something, including session cookie #2 in request // - updates session state in redis: {"counter": 3, "user_id": "ferris"} @@ -546,50 +587,71 @@ mod test { let req_7 = srv.post("/do_something").cookie(cookie_2.clone()).send(); let mut resp_7 = srv.block_on(req_7).unwrap(); let result_7 = block_on(resp_7.json::()).unwrap(); - assert_eq!(result_7, IndexResponse{user_id: Some("ferris".into()), counter: 3}); - + assert_eq!( + result_7, + IndexResponse { + user_id: Some("ferris".into()), + counter: 3 + } + ); // Step 8: GET index, including session cookie #1 in request // - set-cookie actix-session will be in response (session cookie #3) // - response should be: {"counter": 0, "user_id": None} - let req_8 = srv.get("/") - .cookie(cookie_1.clone()) - .send(); + let req_8 = srv.get("/").cookie(cookie_1.clone()).send(); let mut resp_8 = srv.block_on(req_8).unwrap(); - let cookie_3 = resp_8.cookies().unwrap().clone() - .into_iter().find(|c| c.name() == "test-session") - .unwrap(); + let cookie_3 = resp_8 + .cookies() + .unwrap() + .clone() + .into_iter() + .find(|c| c.name() == "test-session") + .unwrap(); let result_8 = block_on(resp_8.json::()).unwrap(); - assert_eq!(result_8, IndexResponse{user_id: None, counter: 0}); + assert_eq!( + result_8, + IndexResponse { + user_id: None, + counter: 0 + } + ); assert!(cookie_3.value().to_string() != cookie_2.value().to_string()); - // Step 9: POST to logout, including session cookie #2 // - set-cookie actix-session will be in response with session cookie #2 // invalidation logic - let req_9 = srv.post("/logout") - .cookie(cookie_2.clone()) - .send(); + let req_9 = srv.post("/logout").cookie(cookie_2.clone()).send(); let resp_9 = srv.block_on(req_9).unwrap(); - let cookie_4 = resp_9.cookies().unwrap().clone() - .into_iter().find(|c| c.name() == "test-session") - .unwrap(); + let cookie_4 = resp_9 + .cookies() + .unwrap() + .clone() + .into_iter() + .find(|c| c.name() == "test-session") + .unwrap(); assert!(&time::now().tm_year != &cookie_4.expires().map(|t| t.tm_year).unwrap()); - // Step 10: GET index, including session cookie #2 in request // - set-cookie actix-session will be in response (session cookie #3) // - response should be: {"counter": 0, "user_id": None} - let req_10 = srv.get("/") - .cookie(cookie_2.clone()) - .send(); + let req_10 = srv.get("/").cookie(cookie_2.clone()).send(); let mut resp_10 = srv.block_on(req_10).unwrap(); let result_10 = block_on(resp_10.json::()).unwrap(); - assert_eq!(result_10, IndexResponse{user_id: None, counter: 0}); - - let cookie_5 = resp_10.cookies().unwrap().clone() - .into_iter().find(|c| c.name() == "test-session") - .unwrap(); + assert_eq!( + result_10, + IndexResponse { + user_id: None, + counter: 0 + } + ); + + let cookie_5 = resp_10 + .cookies() + .unwrap() + .clone() + .into_iter() + .find(|c| c.name() == "test-session") + .unwrap(); assert!(cookie_5.value().to_string() != cookie_2.value().to_string()); } }