Skip to content

Commit

Permalink
Removed persisten_session hashed token index.
Browse files Browse the repository at this point in the history
  • Loading branch information
adsnaider committed Apr 16, 2022
1 parent 3fd685c commit 67fd9d3
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 43 deletions.
3 changes: 0 additions & 3 deletions migrations/2022-02-21-211645_create_sessions/up.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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);
28 changes: 9 additions & 19 deletions src/controllers/user/session.rs
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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?
Expand Down
16 changes: 7 additions & 9 deletions src/controllers/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();

Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
69 changes: 66 additions & 3 deletions src/models/persistent_session.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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()))
}
}
6 changes: 4 additions & 2 deletions src/tests/authentication.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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);
Expand Down
11 changes: 5 additions & 6 deletions src/tests/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 67fd9d3

Please sign in to comment.