Skip to content

fix(session): fix double serialisation of values when inserting data into session state #366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ guide/build/
.DS_Store

Server.toml

.idea
2 changes: 2 additions & 0 deletions actix-identity/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Replace the usages of `HashMap` with `serde::Map`.

## 0.6.0

- Add `error` module.
Expand Down
1 change: 1 addition & 0 deletions actix-identity/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ actix-web = { version = "4", default-features = false, features = ["cookies", "s
derive_more = "0.99.7"
futures-core = "0.3.7"
serde = { version = "1", features = ["derive"] }
serde_json = "1"
tracing = { version = "0.1.30", default-features = false, features = ["log"] }

[dev-dependencies]
Expand Down
15 changes: 11 additions & 4 deletions actix-identity/src/identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use actix_web::{
http::StatusCode,
Error, FromRequest, HttpMessage, HttpRequest, HttpResponse,
};
use serde_json::Value;

use crate::{
config::LogoutBehaviour,
Expand Down Expand Up @@ -153,13 +154,17 @@ impl Identity {
/// ```
pub fn login(ext: &Extensions, id: String) -> Result<Self, LoginError> {
let inner = IdentityInner::extract(ext);
inner.session.insert(ID_KEY, id)?;
inner.session.insert(ID_KEY, Value::from(id));
let now = OffsetDateTime::now_utc().unix_timestamp();
if inner.is_login_deadline_enabled {
inner.session.insert(LOGIN_UNIX_TIMESTAMP_KEY, now)?;
inner
.session
.insert(LOGIN_UNIX_TIMESTAMP_KEY, Value::from(now));
}
if inner.is_visit_deadline_enabled {
inner.session.insert(LAST_VISIT_UNIX_TIMESTAMP_KEY, now)?;
inner
.session
.insert(LAST_VISIT_UNIX_TIMESTAMP_KEY, Value::from(now));
}
inner.session.renew();
Ok(Self(inner))
Expand Down Expand Up @@ -230,7 +235,9 @@ impl Identity {

pub(crate) fn set_last_visited_at(&self) -> Result<(), LoginError> {
let now = OffsetDateTime::now_utc().unix_timestamp();
self.0.session.insert(LAST_VISIT_UNIX_TIMESTAMP_KEY, now)?;
self.0
.session
.insert(LAST_VISIT_UNIX_TIMESTAMP_KEY, Value::from(now));
Ok(())
}
}
Expand Down
3 changes: 2 additions & 1 deletion actix-identity/tests/integration/test_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use actix_identity::{config::IdentityMiddlewareBuilder, Identity, IdentityMiddle
use actix_session::{Session, SessionStatus};
use actix_web::{web, App, HttpMessage, HttpRequest, HttpResponse, HttpServer};
use serde::{Deserialize, Serialize};
use serde_json::Value;

use crate::fixtures::session_middleware;

Expand Down Expand Up @@ -136,7 +137,7 @@ async fn increment(session: Session, user: Option<Identity>) -> HttpResponse {
.get::<i32>("counter")
.unwrap_or(Some(0))
.map_or(1, |inner| inner + 1);
session.insert("counter", counter).unwrap();
session.insert("counter", Value::from(counter));

HttpResponse::Ok().json(&EndpointResponse {
user_id,
Expand Down
3 changes: 3 additions & 0 deletions actix-session/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

- Replace the usages of `HashMap` with `serde::Map`.
- Fix double serialization when inserting values into the session.

## 0.8.0

- Set secure attribute when adding a session removal cookie.
Expand Down
3 changes: 2 additions & 1 deletion actix-session/examples/authentication.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use actix_web::{
middleware, web, App, Error, HttpResponse, HttpServer, Responder,
};
use serde::{Deserialize, Serialize};
use serde_json::Value;

#[derive(Deserialize)]
struct Credentials {
Expand Down Expand Up @@ -54,7 +55,7 @@ async fn login(
let credentials = credentials.into_inner();

match User::authenticate(credentials) {
Ok(user) => session.insert("user_id", user.id).unwrap(),
Ok(user) => session.insert("user_id", Value::from(user.id)),
Err(err) => return Err(InternalError::from_response("", err).into()),
};

Expand Down
5 changes: 3 additions & 2 deletions actix-session/examples/basic.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use actix_session::{storage::RedisActorSessionStore, Session, SessionMiddleware};
use actix_web::{cookie::Key, middleware, web, App, Error, HttpRequest, HttpServer, Responder};
use serde_json::Value;

/// simple handler
async fn index(req: HttpRequest, session: Session) -> Result<impl Responder, Error> {
Expand All @@ -8,9 +9,9 @@ async fn index(req: HttpRequest, session: Session) -> Result<impl Responder, Err
// session
if let Some(count) = session.get::<i32>("counter")? {
println!("SESSION value: {count}");
session.insert("counter", count + 1)?;
session.insert("counter", Value::from(count + 1));
} else {
session.insert("counter", 1)?;
session.insert("counter", Value::from(1));
}

Ok("Welcome!")
Expand Down
17 changes: 9 additions & 8 deletions actix-session/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,17 @@
//!
//! ```no_run
//! use actix_web::Error;
//! use serde_json::Value;
//! use actix_session::Session;
//!
//! fn index(session: Session) -> Result<&'static str, Error> {
//! // access the session state
//! if let Some(count) = session.get::<i32>("counter")? {
//! println!("SESSION value: {}", count);
//! // modify the session state
//! session.insert("counter", count + 1)?;
//! session.insert("counter", Value::from(count + 1));
//! } else {
//! session.insert("counter", 1)?;
//! session.insert("counter", Value::from(1));
//! }
//!
//! Ok("Welcome!")
Expand Down Expand Up @@ -207,7 +208,7 @@ pub mod test_helpers {
App, HttpResponse, Result,
};
use serde::{Deserialize, Serialize};
use serde_json::json;
use serde_json::{json, Value};

use crate::{
config::{CookieContentSecurity, PersistentSession, TtlExtensionPolicy},
Expand Down Expand Up @@ -238,7 +239,7 @@ pub mod test_helpers {
.build(),
)
.service(web::resource("/").to(|ses: Session| async move {
let _ = ses.insert("counter", 100);
ses.insert("counter", Value::from(100));
"test"
}))
.service(web::resource("/test/").to(|ses: Session| async move {
Expand Down Expand Up @@ -286,7 +287,7 @@ pub mod test_helpers {
.build(),
)
.service(web::resource("/").to(|ses: Session| async move {
let _ = ses.insert("counter", 100);
ses.insert("counter", Value::from(100));
"test"
}))
.service(web::resource("/test/").to(|| async move { "no-changes-in-session" })),
Expand Down Expand Up @@ -328,7 +329,7 @@ pub mod test_helpers {
.build(),
)
.service(web::resource("/").to(|ses: Session| async move {
let _ = ses.insert("counter", 100);
ses.insert("counter", Value::from(100));
"test"
}))
.service(web::resource("/test/").to(|| async move { "no-changes-in-session" })),
Expand Down Expand Up @@ -674,7 +675,7 @@ pub mod test_helpers {
.get::<i32>("counter")
.unwrap_or(Some(0))
.map_or(1, |inner| inner + 1);
session.insert("counter", counter)?;
session.insert("counter", Value::from(counter));

Ok(HttpResponse::Ok().json(&IndexResponse { user_id, counter }))
}
Expand All @@ -693,7 +694,7 @@ pub mod test_helpers {

async fn login(user_id: web::Json<Identity>, session: Session) -> Result<HttpResponse> {
let id = user_id.into_inner().user_id;
session.insert("user_id", &id)?;
session.insert("user_id", Value::from(id.clone()));
session.renew();

let counter: i32 = session
Expand Down
11 changes: 6 additions & 5 deletions actix-session/src/middleware.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{collections::HashMap, convert::TryInto, fmt, future::Future, pin::Pin, rc::Rc};
use std::{convert::TryInto, fmt, future::Future, pin::Pin, rc::Rc};

use actix_utils::future::{ready, Ready};
use actix_web::{
Expand All @@ -9,6 +9,7 @@ use actix_web::{
HttpResponse,
};
use anyhow::Context;
use serde_json::{Map, Value};

use crate::{
config::{
Expand Down Expand Up @@ -360,7 +361,7 @@ fn extract_session_key(req: &ServiceRequest, config: &CookieConfiguration) -> Op
async fn load_session_state<Store: SessionStore>(
session_key: Option<SessionKey>,
storage_backend: &Store,
) -> Result<(Option<SessionKey>, HashMap<String, String>), actix_web::Error> {
) -> Result<(Option<SessionKey>, Map<String, Value>), actix_web::Error> {
if let Some(session_key) = session_key {
match storage_backend.load(&session_key).await {
Ok(state) => {
Expand All @@ -378,7 +379,7 @@ async fn load_session_state<Store: SessionStore>(
empty session."
);

Ok((None, HashMap::new()))
Ok((None, Map::new()))
}
}

Expand All @@ -390,14 +391,14 @@ async fn load_session_state<Store: SessionStore>(
"Invalid session state, creating a new empty session."
);

Ok((Some(session_key), HashMap::new()))
Ok((Some(session_key), Map::new()))
}

LoadError::Other(err) => Err(e500(err)),
},
}
} else {
Ok((None, HashMap::new()))
Ok((None, Map::new()))
}
}

Expand Down
Loading