Removed persisten_session hashed token index.

rust-lang · Apr 15, 2022 · ae3ada6 · ae3ada6
1 parent 0c1161e
commit ae3ada6
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 43 deletions.
diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql
@@ -13,6 +13,3 @@ CREATE TABLE persistent_sessions
 );
 
 COMMENT ON TABLE persistent_sessions IS 'This table contains the hashed tokens for all of the cookie-based persistent sessions';
-
-CREATE INDEX persistent_sessions_token_index
-    ON persistent_sessions (hashed_token);
diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs
@@ -1,32 +1,19 @@
 use crate::controllers::frontend_prelude::*;
 
 use conduit_cookie::{RequestCookies, RequestSession};
-use cookie::{Cookie, SameSite};
+use cookie::Cookie;
 use oauth2::reqwest::http_client;
 use oauth2::{AuthorizationCode, Scope, TokenResponse};
 
 use crate::email::Emails;
 use crate::github::GithubUser;
+use crate::models::persistent_session::SessionCookie;
 use crate::models::{NewUser, PersistentSession, User};
 use crate::schema::users;
 use crate::util::errors::ReadOnlyMode;
-use crate::util::token::NewSecureToken;
 use crate::util::token::SecureToken;
 use crate::Env;
 
-/// Name of the cookie used for session-based authentication.
-pub const SESSION_COOKIE_NAME: &str = "__Host-auth";
-
-/// Creates a session cookie with the given token.
-pub fn session_cookie(token: &NewSecureToken, secure: bool) -> Cookie<'static> {
-    Cookie::build(SESSION_COOKIE_NAME, token.plaintext().to_string())
-        .http_only(true)
-        .secure(secure)
-        .same_site(SameSite::Strict)
-        .path("/")
-        .finish()
-}
-
 /// Handles the `GET /api/private/session/begin` route.
 ///
 /// This route will return an authorization URL for the GitHub OAuth flow including the crates.io
@@ -123,12 +110,14 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
         .unwrap_or_default();
 
     let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session);
-    PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent)
+    let session = PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent)
         .insert(&*req.db_conn()?)?;
 
+    let cookie = SessionCookie::new(session.id, token.plaintext().to_string());
+
     // Setup persistent session cookie.
     let secure = req.app().config.env() == Env::Production;
-    req.cookies_mut().add(session_cookie(&token, secure));
+    req.cookies_mut().add(cookie.build(secure));
 
     // TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630.
     // Log in by setting a cookie and the middleware authentication.
@@ -176,10 +165,11 @@ pub fn logout(req: &mut dyn RequestExt) -> EndpointResult {
     // Remove persistent session from database.
     if let Some(session_token) = req
         .cookies()
-        .get(SESSION_COOKIE_NAME)
+        .get(SessionCookie::SESSION_COOKIE_NAME)
         .map(|cookie| cookie.value().to_string())
     {
-        req.cookies_mut().remove(Cookie::named(SESSION_COOKIE_NAME));
+        req.cookies_mut()
+            .remove(Cookie::named(SessionCookie::SESSION_COOKIE_NAME));
 
         if let Ok(conn) = req.db_conn() {
             // TODO(adsnaider): Maybe log errors somehow?

diff --git a/src/controllers/util.rs b/src/controllers/util.rs
@@ -2,8 +2,8 @@ use chrono::Utc;
 use conduit_cookie::RequestCookies;
 
 use super::prelude::*;
-use crate::controllers::user::session::SESSION_COOKIE_NAME;
 use crate::middleware::log_request;
+use crate::models::persistent_session::SessionCookie;
 use crate::models::{ApiToken, PersistentSession, User};
 use crate::util::errors::{
     account_locked, forbidden, internal, AppError, AppResult, InsecurelyGeneratedTokenRevoked,
@@ -84,10 +84,11 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
     }
 
     // Log in with persistent session token.
-    if let Some(session_token) = req
+    if let Some(Ok(session_cookie)) = req
         .cookies()
-        .get(SESSION_COOKIE_NAME)
+        .get(SessionCookie::SESSION_COOKIE_NAME)
         .map(|cookie| cookie.value())
+        .map(|cookie| cookie.parse::<SessionCookie>())
     {
         let ip_addr = req.remote_addr().ip();
 
@@ -97,12 +98,9 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
             .and_then(|value| value.to_str().ok())
             .unwrap_or_default();
 
-        if let Some(session) = PersistentSession::find_from_token_and_update(
-            &conn,
-            session_token,
-            ip_addr,
-            user_agent,
-        )? {
+        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 {

diff --git a/src/models.rs b/src/models.rs
@@ -29,7 +29,7 @@ mod follow;
 mod keyword;
 pub mod krate;
 mod owner;
-mod persistent_session;
+pub mod persistent_session;
 mod rights;
 mod team;
 mod token;

diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs
@@ -1,7 +1,10 @@
 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;
 
 use crate::schema::persistent_sessions;
@@ -67,15 +70,16 @@ impl PersistentSession {
     /// * `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_from_token_and_update(
+    pub fn find_and_update(
         conn: &PgConnection,
-        token: &str,
+        session_cookie: &SessionCookie,
         ip_address: IpAddr,
         user_agent: &str,
     ) -> Result<Option<Self>, SessionError> {
-        let hashed_token = SecureToken::parse(SecureTokenKind::Session, token)
+        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));
 
@@ -131,3 +135,62 @@ impl NewPersistentSession<'_, '_> {
             .get_result(conn)
     }
 }
+
+/// Holds the information needed for the session cookie.
+#[derive(Debug, PartialEq, Eq)]
+pub struct SessionCookie {
+    /// The session ID in the database.
+    id: i64,
+    /// The token
+    token: String,
+}
+
+impl SessionCookie {
+    /// Name of the cookie used for session-based authentication.
+    pub const SESSION_COOKIE_NAME: &'static str = "__Host-auth";
+
+    /// Creates a new `SessionCookie`.
+    pub fn new(id: i64, token: String) -> Self {
+        Self { id, token }
+    }
+
+    /// Returns the `[Cookie]`.
+    pub fn build(&self, secure: bool) -> Cookie<'static> {
+        Cookie::build(
+            Self::SESSION_COOKIE_NAME,
+            format!("{}:{}", self.id, &self.token),
+        )
+        .http_only(true)
+        .secure(secure)
+        .same_site(SameSite::Strict)
+        .path("/")
+        .finish()
+    }
+}
+
+/// Error returned when the session cookie couldn't be parsed.
+#[derive(Error, Debug, PartialEq)]
+pub enum ParseSessionCookieError {
+    #[error("The session id wasn't in the cookie.")]
+    MissingSessionId,
+    #[error("The session id couldn't be parsed from the cookie.")]
+    IdParseError(#[from] ParseIntError),
+    #[error("The session token wasn't in the cookie.")]
+    MissingToken,
+}
+
+impl FromStr for SessionCookie {
+    type Err = ParseSessionCookieError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let mut id_and_token = s.split(':');
+        let id: i64 = id_and_token
+            .next()
+            .ok_or(ParseSessionCookieError::MissingSessionId)?
+            .parse()?;
+        let token = id_and_token
+            .next()
+            .ok_or(ParseSessionCookieError::MissingToken)?;
+
+        Ok(Self::new(id, token.to_string()))
+    }
+}
diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs
@@ -2,7 +2,7 @@ use crate::util::{RequestHelper, Response};
 use crate::TestApp;
 
 use crate::util::encode_session_header;
-use cargo_registry::controllers::user::session::session_cookie;
+use cargo_registry::models::persistent_session::SessionCookie;
 use cargo_registry::util::token::SecureToken;
 use cargo_registry::util::token::SecureTokenKind;
 use conduit::{header, Body, Method, StatusCode};
@@ -27,7 +27,9 @@ fn incorrect_session_is_forbidden() {
 
     let token = SecureToken::generate(SecureTokenKind::Session);
     // Create a cookie that isn't in the database.
-    let cookie = session_cookie(&token, false).to_string();
+    let cookie = SessionCookie::new(123, token.plaintext().to_string())
+        .build(false)
+        .to_string();
     let mut request = anon.request_builder(Method::GET, URL);
     request.header(header::COOKIE, &cookie);
     let response: Response<Body> = anon.run(request);

diff --git a/src/tests/util.rs b/src/tests/util.rs
@@ -23,10 +23,9 @@ use crate::{
     builders::PublishBuilder, CategoryListResponse, CategoryResponse, CrateList, CrateResponse,
     GoodCrate, OkBool, OwnersResponse, VersionResponse,
 };
-use cargo_registry::controllers::user::session::session_cookie;
+use cargo_registry::models::persistent_session::SessionCookie;
 use cargo_registry::models::PersistentSession;
 use cargo_registry::models::{ApiToken, CreatedApiToken, User};
-use cargo_registry::util::token::NewSecureToken;
 use cargo_registry::util::token::SecureToken;
 use cargo_registry::util::token::SecureTokenKind;
 
@@ -282,27 +281,27 @@ impl MockCookieUser {
 
         let token = SecureToken::generate(SecureTokenKind::Session);
 
-        self.app.db(|conn| {
+        let session = self.app.db(|conn| {
             PersistentSession::create(self.user.id, &token, ip_addr.parse().unwrap(), user_agent)
                 .insert(conn)
                 .unwrap()
         });
 
         MockSessionUser {
             app: self.app.clone(),
-            token,
+            session_cookie: SessionCookie::new(session.id, token.plaintext().to_string()),
         }
     }
 }
 
 pub struct MockSessionUser {
     app: TestApp,
-    token: NewSecureToken,
+    session_cookie: SessionCookie,
 }
 
 impl RequestHelper for MockSessionUser {
     fn request_builder(&self, method: Method, path: &str) -> MockRequest {
-        let cookie = session_cookie(&self.token, false).to_string();
+        let cookie = self.session_cookie.build(false).to_string();
 
         let mut request = req(method, path);
         request.header(header::COOKIE, &cookie);