Skip to content

Commit 694ce86

Browse files
committed
Removed last_used_* fields from persistent session table.
1 parent 67fd9d3 commit 694ce86

File tree

7 files changed

+80
-140
lines changed

7 files changed

+80
-140
lines changed

migrations/2022-02-21-211645_create_sessions/up.sql

+1-4
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,7 @@ CREATE TABLE persistent_sessions
66
user_id INTEGER NOT NULL,
77
hashed_token bytea NOT NULL,
88
created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
9-
last_used_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
10-
revoked BOOLEAN DEFAULT FALSE NOT NULL,
11-
last_ip_address inet NOT NULL,
12-
last_user_agent VARCHAR NOT NULL
9+
revoked BOOLEAN DEFAULT FALSE NOT NULL
1310
);
1411

1512
COMMENT ON TABLE persistent_sessions IS 'This table contains the hashed tokens for all of the cookie-based persistent sessions';

src/controllers/user/session.rs

+24-19
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ use conduit_cookie::{RequestCookies, RequestSession};
44
use cookie::Cookie;
55
use oauth2::reqwest::http_client;
66
use oauth2::{AuthorizationCode, Scope, TokenResponse};
7+
use thiserror::Error;
78

89
use crate::email::Emails;
910
use crate::github::GithubUser;
11+
use crate::models::persistent_session::ParseSessionCookieError;
1012
use crate::models::persistent_session::SessionCookie;
1113
use crate::models::{NewUser, PersistentSession, User};
1214
use crate::schema::users;
@@ -102,16 +104,8 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
102104
let user = save_user_to_database(&ghuser, token.secret(), &req.app().emails, &*req.db_conn()?)?;
103105

104106
// Setup a persistent session for the newly logged in user.
105-
let user_agent = req
106-
.headers()
107-
.get(header::USER_AGENT)
108-
.and_then(|value| value.to_str().ok())
109-
.map(|value| value.to_string())
110-
.unwrap_or_default();
111-
112107
let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session);
113-
let session = PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent)
114-
.insert(&*req.db_conn()?)?;
108+
let session = PersistentSession::create(user.id, &token).insert(&*req.db_conn()?)?;
115109

116110
let cookie = SessionCookie::new(session.id, token.plaintext().to_string());
117111

@@ -157,25 +151,36 @@ fn save_user_to_database(
157151
})
158152
}
159153

154+
#[derive(Error, Debug, PartialEq)]
155+
pub enum LogoutError {
156+
#[error("No session cookie found.")]
157+
MissingSessionCookie,
158+
#[error("Session cookie had an unexpected format.")]
159+
SessionCookieMalformatted(#[from] ParseSessionCookieError),
160+
#[error("Session is not in the database.")]
161+
SessionNotInDB,
162+
}
163+
160164
/// Handles the `DELETE /api/private/session` route.
161165
pub fn logout(req: &mut dyn RequestExt) -> EndpointResult {
162166
// TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630.
163167
req.session_mut().remove(&"user_id".to_string());
164168

165169
// Remove persistent session from database.
166-
if let Some(session_token) = req
170+
let session_cookie = req
167171
.cookies()
168172
.get(SessionCookie::SESSION_COOKIE_NAME)
169-
.map(|cookie| cookie.value().to_string())
170-
{
171-
req.cookies_mut()
172-
.remove(Cookie::named(SessionCookie::SESSION_COOKIE_NAME));
173+
.ok_or(LogoutError::MissingSessionCookie)?
174+
.value()
175+
.parse::<SessionCookie>()?;
173176

174-
if let Ok(conn) = req.db_conn() {
175-
// TODO(adsnaider): Maybe log errors somehow?
176-
let _ = PersistentSession::revoke_from_token(&conn, &session_token);
177-
}
178-
}
177+
req.cookies_mut()
178+
.remove(Cookie::named(SessionCookie::SESSION_COOKIE_NAME));
179+
180+
let conn = req.db_conn()?;
181+
let mut session = PersistentSession::find(session_cookie.session_id(), &conn)?
182+
.ok_or(LogoutError::SessionNotInDB)?;
183+
session.revoke().update(&conn)?;
179184
Ok(req.json(&true))
180185
}
181186

src/controllers/util.rs

+10-17
Original file line numberDiff line numberDiff line change
@@ -90,23 +90,16 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
9090
.map(|cookie| cookie.value())
9191
.map(|cookie| cookie.parse::<SessionCookie>())
9292
{
93-
let ip_addr = req.remote_addr().ip();
94-
95-
let user_agent = req
96-
.headers()
97-
.get(header::USER_AGENT)
98-
.and_then(|value| value.to_str().ok())
99-
.unwrap_or_default();
100-
101-
if let Some(session) =
102-
PersistentSession::find_and_update(&conn, &session_cookie, ip_addr, user_agent)?
103-
{
104-
let user = User::find(&conn, session.user_id)
105-
.map_err(|e| e.chain(internal("user_id from session not found in the database")))?;
106-
return Ok(AuthenticatedUser {
107-
user,
108-
token_id: None,
109-
});
93+
if let Some(session) = PersistentSession::find(session_cookie.session_id(), &conn)? {
94+
if session.is_authorized(session_cookie.token()) {
95+
let user = User::find(&conn, session.user_id).map_err(|e| {
96+
e.chain(internal("user_id from session not found in the database"))
97+
})?;
98+
return Ok(AuthenticatedUser {
99+
user,
100+
token_id: None,
101+
});
102+
}
110103
}
111104
}
112105

src/models/persistent_session.rs

+44-75
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
use chrono::NaiveDateTime;
22
use cookie::{Cookie, SameSite};
33
use diesel::prelude::*;
4-
use ipnetwork::IpNetwork;
5-
use std::net::IpAddr;
64
use std::num::ParseIntError;
75
use std::str::FromStr;
86
use thiserror::Error;
@@ -28,106 +26,69 @@ pub struct PersistentSession {
2826
pub hashed_token: SecureToken,
2927
/// Datetime the session was created.
3028
pub created_at: NaiveDateTime,
31-
/// Datetime the session was last used.
32-
pub last_used_at: NaiveDateTime,
3329
/// Whether the session is revoked.
3430
pub revoked: bool,
35-
/// Last IP address that used the session.
36-
pub last_ip_address: IpNetwork,
37-
/// Last user agent that used the session.
38-
pub last_user_agent: String,
39-
}
40-
41-
/// Session-related errors.
42-
#[derive(Error, Debug, PartialEq)]
43-
pub enum SessionError {
44-
#[error("token prefix doesn't match session tokens")]
45-
InvalidSessionToken,
46-
#[error("database manipulation error")]
47-
DieselError(#[from] diesel::result::Error),
4831
}
4932

5033
impl PersistentSession {
5134
/// Creates a `NewPersistentSession` that can be inserted into the database.
52-
pub fn create<'a, 'b>(
53-
user_id: i32,
54-
token: &'a SecureToken,
55-
last_ip_address: IpAddr,
56-
last_user_agent: &'b str,
57-
) -> NewPersistentSession<'a, 'b> {
35+
pub fn create<'a>(user_id: i32, token: &'a SecureToken) -> NewPersistentSession<'a> {
5836
NewPersistentSession {
5937
user_id,
6038
hashed_token: token,
61-
last_ip_address: last_ip_address.into(),
62-
last_user_agent,
6339
}
6440
}
6541

66-
/// Finds an unrevoked session that matches `token` from the database and returns it.
42+
/// Finds the session with the ID.
6743
///
6844
/// # Returns
6945
///
70-
/// * `Ok(Some(...))` if a session matches the `token`.
71-
/// * `Ok(None)` if no session matches the `token`.
72-
/// * `Err(...)` for other errors such as invalid tokens or diesel errors.
73-
pub fn find_and_update(
74-
conn: &PgConnection,
75-
session_cookie: &SessionCookie,
76-
ip_address: IpAddr,
77-
user_agent: &str,
78-
) -> Result<Option<Self>, SessionError> {
79-
let hashed_token = SecureToken::parse(SecureTokenKind::Session, &session_cookie.token)
80-
.ok_or(SessionError::InvalidSessionToken)?;
81-
let sessions = persistent_sessions::table
82-
.filter(persistent_sessions::id.eq(session_cookie.id))
83-
.filter(persistent_sessions::revoked.eq(false))
84-
.filter(persistent_sessions::hashed_token.eq(hashed_token));
85-
86-
conn.transaction(|| {
87-
diesel::update(sessions.clone())
88-
.set((
89-
persistent_sessions::last_used_at.eq(diesel::dsl::now),
90-
persistent_sessions::last_ip_address.eq(IpNetwork::from(ip_address)),
91-
persistent_sessions::last_user_agent.eq(user_agent),
92-
))
93-
.get_result(conn)
94-
.optional()
95-
})
96-
.or_else(|_| sessions.first(conn).optional())
97-
.map_err(SessionError::DieselError)
46+
/// * `Ok(Some(...))` if a session matches the id.
47+
/// * `Ok(None)` if no session matches the id.
48+
/// * `Err(...)` for other errors..
49+
pub fn find(id: i64, conn: &PgConnection) -> Result<Option<Self>, diesel::result::Error> {
50+
persistent_sessions::table
51+
.find(id)
52+
.get_result(conn)
53+
.optional()
9854
}
9955

100-
/// Revokes the `token` in the database.
101-
///
102-
/// # Returns
103-
///
104-
/// The number of sessions that were revoked or an error if the `token` isn't valid or there
105-
/// was an issue with the database connection.
106-
pub fn revoke_from_token(conn: &PgConnection, token: &str) -> Result<usize, SessionError> {
107-
let hashed_token = SecureToken::parse(SecureTokenKind::Session, token)
108-
.ok_or(SessionError::InvalidSessionToken)?;
109-
let sessions = persistent_sessions::table
110-
.filter(persistent_sessions::hashed_token.eq(hashed_token))
111-
.filter(persistent_sessions::revoked.eq(false));
112-
113-
diesel::update(sessions)
114-
.set(persistent_sessions::revoked.eq(true))
115-
.execute(conn)
116-
.map_err(SessionError::DieselError)
56+
/// Updates the session in the database.
57+
pub fn update(&self, conn: &PgConnection) -> Result<(), diesel::result::Error> {
58+
diesel::update(persistent_sessions::table.find(self.id))
59+
.set((
60+
persistent_sessions::user_id.eq(&self.user_id),
61+
persistent_sessions::hashed_token.eq(&self.hashed_token),
62+
persistent_sessions::revoked.eq(&self.revoked),
63+
))
64+
.get_result::<Self>(conn)
65+
.map(|_| ())
66+
}
67+
68+
pub fn is_authorized(&self, token: &str) -> bool {
69+
if let Some(hashed_token) = SecureToken::parse(SecureTokenKind::Session, token) {
70+
!self.revoked && self.hashed_token == hashed_token
71+
} else {
72+
false
73+
}
74+
}
75+
76+
/// Revokes the session (needs update).
77+
pub fn revoke(&mut self) -> &mut Self {
78+
self.revoked = true;
79+
self
11780
}
11881
}
11982

12083
/// A new, insertable persistent session.
12184
#[derive(Clone, Debug, PartialEq, Eq, Insertable)]
12285
#[table_name = "persistent_sessions"]
123-
pub struct NewPersistentSession<'a, 'b> {
86+
pub struct NewPersistentSession<'a> {
12487
user_id: i32,
12588
hashed_token: &'a SecureToken,
126-
last_ip_address: IpNetwork,
127-
last_user_agent: &'b str,
12889
}
12990

130-
impl NewPersistentSession<'_, '_> {
91+
impl NewPersistentSession<'_> {
13192
/// Inserts the session into the database.
13293
pub fn insert(self, conn: &PgConnection) -> Result<PersistentSession, diesel::result::Error> {
13394
diesel::insert_into(persistent_sessions::table)
@@ -166,6 +127,14 @@ impl SessionCookie {
166127
.path("/")
167128
.finish()
168129
}
130+
131+
pub fn session_id(&self) -> i64 {
132+
self.id
133+
}
134+
135+
pub fn token(&self) -> &str {
136+
&self.token
137+
}
169138
}
170139

171140
/// Error returned when the session cookie couldn't be parsed.

src/schema.rs

-18
Original file line numberDiff line numberDiff line change
@@ -630,30 +630,12 @@ table! {
630630
///
631631
/// (Automatically generated by Diesel.)
632632
created_at -> Timestamp,
633-
/// The `last_used_at` column of the `persistent_sessions` table.
634-
///
635-
/// Its SQL type is `Timestamp`.
636-
///
637-
/// (Automatically generated by Diesel.)
638-
last_used_at -> Timestamp,
639633
/// The `revoked` column of the `persistent_sessions` table.
640634
///
641635
/// Its SQL type is `Bool`.
642636
///
643637
/// (Automatically generated by Diesel.)
644638
revoked -> Bool,
645-
/// The `last_ip_address` column of the `persistent_sessions` table.
646-
///
647-
/// Its SQL type is `Inet`.
648-
///
649-
/// (Automatically generated by Diesel.)
650-
last_ip_address -> Inet,
651-
/// The `last_user_agent` column of the `persistent_sessions` table.
652-
///
653-
/// Its SQL type is `Varchar`.
654-
///
655-
/// (Automatically generated by Diesel.)
656-
last_user_agent -> Varchar,
657639
}
658640
}
659641

src/tests/util.rs

+1-4
Original file line numberDiff line numberDiff line change
@@ -276,13 +276,10 @@ impl MockCookieUser {
276276
}
277277

278278
pub fn with_session(&self) -> MockSessionUser {
279-
let ip_addr = "192.168.0.42";
280-
let user_agent = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36";
281-
282279
let token = SecureToken::generate(SecureTokenKind::Session);
283280

284281
let session = self.app.db(|conn| {
285-
PersistentSession::create(self.user.id, &token, ip_addr.parse().unwrap(), user_agent)
282+
PersistentSession::create(self.user.id, &token)
286283
.insert(conn)
287284
.unwrap()
288285
});

src/worker/dump_db/dump-db.toml

-3
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,7 @@ id = "private"
141141
user_id = "private"
142142
hashed_token = "private"
143143
created_at = "private"
144-
last_used_at = "private"
145144
revoked = "private"
146-
last_ip_address = "private"
147-
last_user_agent = "private"
148145

149146
[publish_limit_buckets.columns]
150147
user_id = "private"

0 commit comments

Comments
 (0)