Removed last_used_* fields from persistent session table.

adsnaider · adsnaider · commit 694ce86872fc · 2022-04-16T16:54:40.000-07:00
diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql
@@ -6,10 +6,7 @@ CREATE TABLE persistent_sessions
     user_id INTEGER NOT NULL,
     hashed_token bytea NOT NULL,
     created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
-    last_used_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL,
-    revoked BOOLEAN DEFAULT FALSE NOT NULL,
-    last_ip_address inet NOT NULL,
-    last_user_agent VARCHAR NOT NULL
+    revoked BOOLEAN DEFAULT FALSE NOT NULL
 );
 
 COMMENT ON TABLE persistent_sessions IS 'This table contains the hashed tokens for all of the cookie-based persistent sessions';
diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs
@@ -4,9 +4,11 @@ use conduit_cookie::{RequestCookies, RequestSession};
 use cookie::Cookie;
 use oauth2::reqwest::http_client;
 use oauth2::{AuthorizationCode, Scope, TokenResponse};
+use thiserror::Error;
 
 use crate::email::Emails;
 use crate::github::GithubUser;
+use crate::models::persistent_session::ParseSessionCookieError;
 use crate::models::persistent_session::SessionCookie;
 use crate::models::{NewUser, PersistentSession, User};
 use crate::schema::users;
@@ -102,16 +104,8 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
     let user = save_user_to_database(&ghuser, token.secret(), &req.app().emails, &*req.db_conn()?)?;
 
     // Setup a persistent session for the newly logged in user.
-    let user_agent = req
-        .headers()
-        .get(header::USER_AGENT)
-        .and_then(|value| value.to_str().ok())
-        .map(|value| value.to_string())
-        .unwrap_or_default();
-
     let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session);
-    let session = PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent)
-        .insert(&*req.db_conn()?)?;
+    let session = PersistentSession::create(user.id, &token).insert(&*req.db_conn()?)?;
 
     let cookie = SessionCookie::new(session.id, token.plaintext().to_string());
 
@@ -157,25 +151,36 @@ fn save_user_to_database(
     })
 }
 
+#[derive(Error, Debug, PartialEq)]
+pub enum LogoutError {
+    #[error("No session cookie found.")]
+    MissingSessionCookie,
+    #[error("Session cookie had an unexpected format.")]
+    SessionCookieMalformatted(#[from] ParseSessionCookieError),
+    #[error("Session is not in the database.")]
+    SessionNotInDB,
+}
+
 /// Handles the `DELETE /api/private/session` route.
 pub fn logout(req: &mut dyn RequestExt) -> EndpointResult {
     // TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630.
     req.session_mut().remove(&"user_id".to_string());
 
     // Remove persistent session from database.
-    if let Some(session_token) = req
+    let session_cookie = req
         .cookies()
         .get(SessionCookie::SESSION_COOKIE_NAME)
-        .map(|cookie| cookie.value().to_string())
-    {
-        req.cookies_mut()
-            .remove(Cookie::named(SessionCookie::SESSION_COOKIE_NAME));
+        .ok_or(LogoutError::MissingSessionCookie)?
+        .value()
+        .parse::<SessionCookie>()?;
 
-        if let Ok(conn) = req.db_conn() {
-            // TODO(adsnaider): Maybe log errors somehow?
-            let _ = PersistentSession::revoke_from_token(&conn, &session_token);
-        }
-    }
+    req.cookies_mut()
+        .remove(Cookie::named(SessionCookie::SESSION_COOKIE_NAME));
+
+    let conn = req.db_conn()?;
+    let mut session = PersistentSession::find(session_cookie.session_id(), &conn)?
+        .ok_or(LogoutError::SessionNotInDB)?;
+    session.revoke().update(&conn)?;
     Ok(req.json(&true))
 }
 
diff --git a/src/controllers/util.rs b/src/controllers/util.rs
@@ -90,23 +90,16 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
         .map(|cookie| cookie.value())
         .map(|cookie| cookie.parse::<SessionCookie>())
     {
-        let ip_addr = req.remote_addr().ip();
-
-        let user_agent = req
-            .headers()
-            .get(header::USER_AGENT)
-            .and_then(|value| value.to_str().ok())
-            .unwrap_or_default();
-
-        if let Some(session) =
-            PersistentSession::find_and_update(&conn, &session_cookie, ip_addr, user_agent)?
-        {
-            let user = User::find(&conn, session.user_id)
-                .map_err(|e| e.chain(internal("user_id from session not found in the database")))?;
-            return Ok(AuthenticatedUser {
-                user,
-                token_id: None,
-            });
+        if let Some(session) = PersistentSession::find(session_cookie.session_id(), &conn)? {
+            if session.is_authorized(session_cookie.token()) {
+                let user = User::find(&conn, session.user_id).map_err(|e| {
+                    e.chain(internal("user_id from session not found in the database"))
+                })?;
+                return Ok(AuthenticatedUser {
+                    user,
+                    token_id: None,
+                });
+            }
         }
     }
 
diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs
@@ -1,8 +1,6 @@
 use chrono::NaiveDateTime;
 use cookie::{Cookie, SameSite};
 use diesel::prelude::*;
-use ipnetwork::IpNetwork;
-use std::net::IpAddr;
 use std::num::ParseIntError;
 use std::str::FromStr;
 use thiserror::Error;
@@ -28,106 +26,69 @@ pub struct PersistentSession {
     pub hashed_token: SecureToken,
     /// Datetime the session was created.
     pub created_at: NaiveDateTime,
-    /// Datetime the session was last used.
-    pub last_used_at: NaiveDateTime,
     /// Whether the session is revoked.
     pub revoked: bool,
-    /// Last IP address that used the session.
-    pub last_ip_address: IpNetwork,
-    /// Last user agent that used the session.
-    pub last_user_agent: String,
-}
-
-/// Session-related errors.
-#[derive(Error, Debug, PartialEq)]
-pub enum SessionError {
-    #[error("token prefix doesn't match session tokens")]
-    InvalidSessionToken,
-    #[error("database manipulation error")]
-    DieselError(#[from] diesel::result::Error),
 }
 
 impl PersistentSession {
     /// Creates a `NewPersistentSession` that can be inserted into the database.
-    pub fn create<'a, 'b>(
-        user_id: i32,
-        token: &'a SecureToken,
-        last_ip_address: IpAddr,
-        last_user_agent: &'b str,
-    ) -> NewPersistentSession<'a, 'b> {
+    pub fn create<'a>(user_id: i32, token: &'a SecureToken) -> NewPersistentSession<'a> {
         NewPersistentSession {
             user_id,
             hashed_token: token,
-            last_ip_address: last_ip_address.into(),
-            last_user_agent,
         }
     }
 
-    /// Finds an unrevoked session that matches `token` from the database and returns it.
+    /// Finds the session with the ID.
     ///
     /// # Returns
     ///
-    /// * `Ok(Some(...))` if a session matches the `token`.
-    /// * `Ok(None)` if no session matches the `token`.
-    /// * `Err(...)` for other errors such as invalid tokens or diesel errors.
-    pub fn find_and_update(
-        conn: &PgConnection,
-        session_cookie: &SessionCookie,
-        ip_address: IpAddr,
-        user_agent: &str,
-    ) -> Result<Option<Self>, SessionError> {
-        let hashed_token = SecureToken::parse(SecureTokenKind::Session, &session_cookie.token)
-            .ok_or(SessionError::InvalidSessionToken)?;
-        let sessions = persistent_sessions::table
-            .filter(persistent_sessions::id.eq(session_cookie.id))
-            .filter(persistent_sessions::revoked.eq(false))
-            .filter(persistent_sessions::hashed_token.eq(hashed_token));
-
-        conn.transaction(|| {
-            diesel::update(sessions.clone())
-                .set((
-                    persistent_sessions::last_used_at.eq(diesel::dsl::now),
-                    persistent_sessions::last_ip_address.eq(IpNetwork::from(ip_address)),
-                    persistent_sessions::last_user_agent.eq(user_agent),
-                ))
-                .get_result(conn)
-                .optional()
-        })
-        .or_else(|_| sessions.first(conn).optional())
-        .map_err(SessionError::DieselError)
+    /// * `Ok(Some(...))` if a session matches the id.
+    /// * `Ok(None)` if no session matches the id.
+    /// * `Err(...)` for other errors..
+    pub fn find(id: i64, conn: &PgConnection) -> Result<Option<Self>, diesel::result::Error> {
+        persistent_sessions::table
+            .find(id)
+            .get_result(conn)
+            .optional()
     }
 
-    /// Revokes the `token` in the database.
-    ///
-    /// # Returns
-    ///
-    /// The number of sessions that were revoked or an error if the `token` isn't valid or there
-    /// was an issue with the database connection.
-    pub fn revoke_from_token(conn: &PgConnection, token: &str) -> Result<usize, SessionError> {
-        let hashed_token = SecureToken::parse(SecureTokenKind::Session, token)
-            .ok_or(SessionError::InvalidSessionToken)?;
-        let sessions = persistent_sessions::table
-            .filter(persistent_sessions::hashed_token.eq(hashed_token))
-            .filter(persistent_sessions::revoked.eq(false));
-
-        diesel::update(sessions)
-            .set(persistent_sessions::revoked.eq(true))
-            .execute(conn)
-            .map_err(SessionError::DieselError)
+    /// Updates the session in the database.
+    pub fn update(&self, conn: &PgConnection) -> Result<(), diesel::result::Error> {
+        diesel::update(persistent_sessions::table.find(self.id))
+            .set((
+                persistent_sessions::user_id.eq(&self.user_id),
+                persistent_sessions::hashed_token.eq(&self.hashed_token),
+                persistent_sessions::revoked.eq(&self.revoked),
+            ))
+            .get_result::<Self>(conn)
+            .map(|_| ())
+    }
+
+    pub fn is_authorized(&self, token: &str) -> bool {
+        if let Some(hashed_token) = SecureToken::parse(SecureTokenKind::Session, token) {
+            !self.revoked && self.hashed_token == hashed_token
+        } else {
+            false
+        }
+    }
+
+    /// Revokes the session (needs update).
+    pub fn revoke(&mut self) -> &mut Self {
+        self.revoked = true;
+        self
     }
 }
 
 /// A new, insertable persistent session.
 #[derive(Clone, Debug, PartialEq, Eq, Insertable)]
 #[table_name = "persistent_sessions"]
-pub struct NewPersistentSession<'a, 'b> {
+pub struct NewPersistentSession<'a> {
     user_id: i32,
     hashed_token: &'a SecureToken,
-    last_ip_address: IpNetwork,
-    last_user_agent: &'b str,
 }
 
-impl NewPersistentSession<'_, '_> {
+impl NewPersistentSession<'_> {
     /// Inserts the session into the database.
     pub fn insert(self, conn: &PgConnection) -> Result<PersistentSession, diesel::result::Error> {
         diesel::insert_into(persistent_sessions::table)
@@ -166,6 +127,14 @@ impl SessionCookie {
         .path("/")
         .finish()
     }
+
+    pub fn session_id(&self) -> i64 {
+        self.id
+    }
+
+    pub fn token(&self) -> &str {
+        &self.token
+    }
 }
 
 /// Error returned when the session cookie couldn't be parsed.
diff --git a/src/schema.rs b/src/schema.rs
@@ -630,30 +630,12 @@ table! {
         ///
         /// (Automatically generated by Diesel.)
         created_at -> Timestamp,
-        /// The `last_used_at` column of the `persistent_sessions` table.
-        ///
-        /// Its SQL type is `Timestamp`.
-        ///
-        /// (Automatically generated by Diesel.)
-        last_used_at -> Timestamp,
         /// The `revoked` column of the `persistent_sessions` table.
         ///
         /// Its SQL type is `Bool`.
         ///
         /// (Automatically generated by Diesel.)
         revoked -> Bool,
-        /// The `last_ip_address` column of the `persistent_sessions` table.
-        ///
-        /// Its SQL type is `Inet`.
-        ///
-        /// (Automatically generated by Diesel.)
-        last_ip_address -> Inet,
-        /// The `last_user_agent` column of the `persistent_sessions` table.
-        ///
-        /// Its SQL type is `Varchar`.
-        ///
-        /// (Automatically generated by Diesel.)
-        last_user_agent -> Varchar,
     }
 }
 
diff --git a/src/tests/util.rs b/src/tests/util.rs
@@ -276,13 +276,10 @@ impl MockCookieUser {
     }
 
     pub fn with_session(&self) -> MockSessionUser {
-        let ip_addr = "192.168.0.42";
-        let user_agent = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36";
-
         let token = SecureToken::generate(SecureTokenKind::Session);
 
         let session = self.app.db(|conn| {
-            PersistentSession::create(self.user.id, &token, ip_addr.parse().unwrap(), user_agent)
+            PersistentSession::create(self.user.id, &token)
                 .insert(conn)
                 .unwrap()
         });
diff --git a/src/worker/dump_db/dump-db.toml b/src/worker/dump_db/dump-db.toml
@@ -141,10 +141,7 @@ id = "private"
 user_id = "private"
 hashed_token = "private"
 created_at = "private"
-last_used_at = "private"
 revoked = "private"
-last_ip_address = "private"
-last_user_agent = "private"
 
 [publish_limit_buckets.columns]
 user_id = "private"
