From e322fd9e54286020b5efc708dcf212b05981b7a9 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 21 Feb 2022 14:13:51 -0800 Subject: [PATCH 01/19] Created persistent sessions table. Unfortunately, due to https://github.com/diesel-rs/diesel/issues/2411, the table can't be named `sessions` as it causes patch conflicts with recent_crate_downloads. --- .../down.sql | 1 + .../2022-02-21-211645_create_sessions/up.sql | 21 ++++ src/schema.rs | 97 +++++++++++++++---- src/worker/dump_db/dump-db.toml | 10 ++ 4 files changed, 111 insertions(+), 18 deletions(-) create mode 100644 migrations/2022-02-21-211645_create_sessions/down.sql create mode 100644 migrations/2022-02-21-211645_create_sessions/up.sql diff --git a/migrations/2022-02-21-211645_create_sessions/down.sql b/migrations/2022-02-21-211645_create_sessions/down.sql new file mode 100644 index 00000000000..2690500f010 --- /dev/null +++ b/migrations/2022-02-21-211645_create_sessions/down.sql @@ -0,0 +1 @@ +DROP TABLE persistent_sessions; diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql new file mode 100644 index 00000000000..fe7a7cc0ba1 --- /dev/null +++ b/migrations/2022-02-21-211645_create_sessions/up.sql @@ -0,0 +1,21 @@ +CREATE TABLE persistent_sessions +( + id SERIAL + CONSTRAINT persistent_sessions_pk + PRIMARY KEY, + user_id INTEGER NOT NULL + CONSTRAINT persistent_sessions_users_id_fk + REFERENCES users + ON UPDATE CASCADE ON DELETE CASCADE, + 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 +); + +COMMENT ON TABLE persistent_sessions IS 'This table contains the hashed tokens for all of the cookie-based persistent sessions'; + +CREATE INDEX persistent_sessions_user_id_index + ON persistent_sessions (user_id); diff --git a/src/schema.rs b/src/schema.rs index 1cb6ffada2d..41c5df57ffc 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -598,6 +598,65 @@ table! { } } +table! { + use diesel::sql_types::*; + use diesel_full_text_search::{TsVector as Tsvector}; + + /// Representation of the `persistent_sessions` table. + /// + /// (Automatically generated by Diesel.) + persistent_sessions (id) { + /// The `id` column of the `persistent_sessions` table. + /// + /// Its SQL type is `Int4`. + /// + /// (Automatically generated by Diesel.) + id -> Int4, + /// The `user_id` column of the `persistent_sessions` table. + /// + /// Its SQL type is `Int4`. + /// + /// (Automatically generated by Diesel.) + user_id -> Int4, + /// The `hashed_token` column of the `persistent_sessions` table. + /// + /// Its SQL type is `Bytea`. + /// + /// (Automatically generated by Diesel.) + hashed_token -> Bytea, + /// The `created_at` column of the `persistent_sessions` table. + /// + /// Its SQL type is `Timestamp`. + /// + /// (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, + } +} + table! { use diesel::sql_types::*; use diesel_full_text_search::{TsVector as Tsvector}; @@ -627,6 +686,24 @@ table! { } } +table! { + /// Representation of the `recent_crate_downloads` view. + /// + /// This data represents the downloads in the last 90 days. + /// This view does not contain realtime data. + /// It is refreshed by the `update-downloads` script. + recent_crate_downloads (crate_id) { + /// The `crate_id` column of the `recent_crate_downloads` view. + /// + /// Its SQL type is `Integer`. + crate_id -> Integer, + /// The `downloads` column of the `recent_crate_downloads` table. + /// + /// Its SQL type is `BigInt`. + downloads -> BigInt, + } +} + table! { use diesel::sql_types::*; use diesel_full_text_search::{TsVector as Tsvector}; @@ -679,24 +756,6 @@ table! { } } -table! { - /// Representation of the `recent_crate_downloads` view. - /// - /// This data represents the downloads in the last 90 days. - /// This view does not contain realtime data. - /// It is refreshed by the `update-downloads` script. - recent_crate_downloads (crate_id) { - /// The `crate_id` column of the `recent_crate_downloads` view. - /// - /// Its SQL type is `Integer`. - crate_id -> Integer, - /// The `downloads` column of the `recent_crate_downloads` table. - /// - /// Its SQL type is `BigInt`. - downloads -> BigInt, - } -} - table! { use diesel::sql_types::*; use diesel_full_text_search::{TsVector as Tsvector}; @@ -1023,6 +1082,7 @@ joinable!(dependencies -> versions (version_id)); joinable!(emails -> users (user_id)); joinable!(follows -> crates (crate_id)); joinable!(follows -> users (user_id)); +joinable!(persistent_sessions -> users (user_id)); joinable!(publish_limit_buckets -> users (user_id)); joinable!(publish_rate_overrides -> users (user_id)); joinable!(readme_renderings -> versions (version_id)); @@ -1050,6 +1110,7 @@ allow_tables_to_appear_in_same_query!( follows, keywords, metadata, + persistent_sessions, publish_limit_buckets, publish_rate_overrides, readme_renderings, diff --git a/src/worker/dump_db/dump-db.toml b/src/worker/dump_db/dump-db.toml index 8d8b2dd9c23..7c09aebaac1 100644 --- a/src/worker/dump_db/dump-db.toml +++ b/src/worker/dump_db/dump-db.toml @@ -136,6 +136,16 @@ created_at = "public" [metadata.columns] total_downloads = "public" +[persistent_sessions.columns] +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" tokens = "private" From 81ec8c6c22cdba663466e327d510ee054c1b9265 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 21 Feb 2022 21:01:24 -0800 Subject: [PATCH 02/19] models: Created persistent session model. --- Cargo.lock | 11 +++++++++++ Cargo.toml | 2 +- src/models.rs | 2 ++ src/models/persistent_session.rs | 16 ++++++++++++++++ 4 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 src/models/persistent_session.rs diff --git a/Cargo.lock b/Cargo.lock index 23caec3941d..fbca48d4f99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -771,6 +771,8 @@ dependencies = [ "byteorder", "chrono", "diesel_derives", + "ipnetwork", + "libc", "pq-sys", "r2d2", "serde_json", @@ -1357,6 +1359,15 @@ dependencies = [ "serde", ] +[[package]] +name = "ipnetwork" +version = "0.18.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4088d739b183546b239688ddbc79891831df421773df95e236daf7867866d355" +dependencies = [ + "serde", +] + [[package]] name = "itoa" version = "1.0.1" diff --git a/Cargo.toml b/Cargo.toml index 3a419492762..b2bf44db133 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,7 +50,7 @@ cookie = { version = "=0.16.0", features = ["secure"] } dashmap = { version = "=5.2.0", features = ["raw-api"] } derive_deref = "=1.1.1" dialoguer = "=0.10.1" -diesel = { version = "=1.4.8", features = ["postgres", "serde_json", "chrono", "r2d2"] } +diesel = { version = "=1.4.8", features = ["postgres", "serde_json", "chrono", "r2d2", "network-address"] } diesel_full_text_search = "=1.0.1" diesel_migrations = { version = "=1.4.0", features = ["postgres"] } dotenv = "=0.15.0" diff --git a/src/models.rs b/src/models.rs index 261427a1465..68b45cd3c91 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,6 +1,7 @@ pub use self::action::{insert_version_owner_action, VersionAction, VersionOwnerAction}; pub use self::badge::{Badge, CrateBadge, MaintenanceStatus}; pub use self::category::{Category, CrateCategory, NewCategory}; +pub use self::persistent_session::PersistentSession; pub use self::crate_owner_invitation::{CrateOwnerInvitation, NewCrateOwnerInvitationOutcome}; pub use self::dependency::{Dependency, DependencyKind, ReverseDependency}; pub use self::download::VersionDownload; @@ -20,6 +21,7 @@ pub mod helpers; mod action; mod badge; pub mod category; +mod persistent_session; mod crate_owner_invitation; pub mod dependency; mod download; diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs new file mode 100644 index 00000000000..9a3042e43d9 --- /dev/null +++ b/src/models/persistent_session.rs @@ -0,0 +1,16 @@ +use crate::schema::persistent_sessions; +use chrono::NaiveDateTime; +use ipnetwork::IpNetwork; + +#[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable)] +#[table_name = "persistent_sessions"] +pub struct PersistentSession { + pub id: i32, + pub user_id: i32, + pub hashed_token: Vec, + pub created_at: NaiveDateTime, + pub last_used_at: NaiveDateTime, + pub revoked: bool, + pub last_ip_address: IpNetwork, + pub last_user_agent: String, +} From 5c209f4a4784cdbce8af79b974b38cdda76d289b Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 14 Mar 2022 12:01:42 -0700 Subject: [PATCH 03/19] Setup new persistent session and cookie on auth. --- src/controllers/user/session.rs | 36 ++++++++++++++++++++++++++-- src/models.rs | 4 ++-- src/models/persistent_session.rs | 41 ++++++++++++++++++++++++++++++-- src/util/token.rs | 4 +++- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 7dbd2a25330..b0a29682d2d 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -1,14 +1,29 @@ use crate::controllers::frontend_prelude::*; -use conduit_cookie::RequestSession; +use conduit_cookie::{RequestCookies, RequestSession}; +use cookie::{Cookie, SameSite}; use oauth2::reqwest::http_client; use oauth2::{AuthorizationCode, Scope, TokenResponse}; use crate::email::Emails; use crate::github::GithubUser; -use crate::models::{NewUser, User}; +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; + +pub const SESSION_COOKIE_NAME: &str = "crates_auth"; + +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. /// @@ -102,7 +117,24 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { &*req.db_write()?, )?; + let user_agent = req + .headers() + .get(header::USER_AGENT) + .and_then(|value| value.to_str().ok()) + .map(|value| value.to_string()) + .unwrap_or_default(); + + // Setup a session for the newly logged in user. + let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session); + let session = PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent); + session.insert(&*req.db_conn()?)?; + + // Setup session cookie. + let secure = req.app().config.env() == Env::Production; + req.cookies_mut().add(session_cookie(&token, secure)); + // Log in by setting a cookie and the middleware authentication + // TODO(adsnaider): Remove. req.session_mut() .insert("user_id".to_string(), user.id.to_string()); diff --git a/src/models.rs b/src/models.rs index 68b45cd3c91..173ded3a051 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,7 +1,6 @@ pub use self::action::{insert_version_owner_action, VersionAction, VersionOwnerAction}; pub use self::badge::{Badge, CrateBadge, MaintenanceStatus}; pub use self::category::{Category, CrateCategory, NewCategory}; -pub use self::persistent_session::PersistentSession; pub use self::crate_owner_invitation::{CrateOwnerInvitation, NewCrateOwnerInvitationOutcome}; pub use self::dependency::{Dependency, DependencyKind, ReverseDependency}; pub use self::download::VersionDownload; @@ -10,6 +9,7 @@ pub use self::follow::Follow; pub use self::keyword::{CrateKeyword, Keyword}; pub use self::krate::{Crate, CrateVersions, NewCrate, RecentCrateDownloads}; pub use self::owner::{CrateOwner, Owner, OwnerKind}; +pub use self::persistent_session::{NewPersistentSession, PersistentSession}; pub use self::rights::Rights; pub use self::team::{NewTeam, Team}; pub use self::token::{ApiToken, CreatedApiToken}; @@ -21,7 +21,6 @@ pub mod helpers; mod action; mod badge; pub mod category; -mod persistent_session; mod crate_owner_invitation; pub mod dependency; mod download; @@ -30,6 +29,7 @@ mod follow; mod keyword; pub mod krate; mod owner; +mod persistent_session; mod rights; mod team; mod token; diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index 9a3042e43d9..ee6b4cb8572 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -1,16 +1,53 @@ -use crate::schema::persistent_sessions; use chrono::NaiveDateTime; +use diesel::prelude::*; use ipnetwork::IpNetwork; +use std::net::IpAddr; + +use crate::schema::persistent_sessions; +use crate::util::token::SecureToken; #[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable)] #[table_name = "persistent_sessions"] pub struct PersistentSession { pub id: i32, pub user_id: i32, - pub hashed_token: Vec, + pub hashed_token: SecureToken, pub created_at: NaiveDateTime, pub last_used_at: NaiveDateTime, pub revoked: bool, pub last_ip_address: IpNetwork, pub last_user_agent: String, } + +impl PersistentSession { + pub fn create<'a, 'b>( + user_id: i32, + token: &'a SecureToken, + last_ip_address: IpAddr, + last_user_agent: &'b str, + ) -> NewPersistentSession<'a, 'b> { + NewPersistentSession { + user_id, + hashed_token: token, + last_ip_address: last_ip_address.into(), + last_user_agent, + } + } +} + +#[derive(Clone, Debug, PartialEq, Eq, Insertable)] +#[table_name = "persistent_sessions"] +pub struct NewPersistentSession<'a, 'b> { + user_id: i32, + hashed_token: &'a SecureToken, + last_ip_address: IpNetwork, + last_user_agent: &'b str, +} + +impl NewPersistentSession<'_, '_> { + pub fn insert(self, conn: &PgConnection) -> Result { + diesel::insert_into(persistent_sessions::table) + .values(self) + .get_result(conn) + } +} diff --git a/src/util/token.rs b/src/util/token.rs index 38869c2db15..5a1e967d2aa 100644 --- a/src/util/token.rs +++ b/src/util/token.rs @@ -84,7 +84,7 @@ impl std::ops::Deref for NewSecureToken { } } -fn generate_secure_alphanumeric_string(len: usize) -> String { +pub(crate) fn generate_secure_alphanumeric_string(len: usize) -> String { const CHARS: &[u8] = b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; OsRng @@ -122,6 +122,7 @@ secure_token_kind! { #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] pub(crate) enum SecureTokenKind { Api => "cio", // Crates.IO + Session => "ses", // Session tokens. } } @@ -172,6 +173,7 @@ mod tests { }; ensure(SecureTokenKind::Api, "cio"); + ensure(SecureTokenKind::Session, "ses"); assert!( remaining.is_empty(), From 5ef54f0cd4089d8afbae75209759d55b62f312d3 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 14 Mar 2022 16:46:50 -0700 Subject: [PATCH 04/19] Revoke session tokens on logout. --- .../2022-02-21-211645_create_sessions/up.sql | 9 +++----- src/controllers/user/session.rs | 17 ++++++++++++++ src/models/persistent_session.rs | 23 +++++++++++++++++++ src/schema.rs | 1 - 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql index fe7a7cc0ba1..08038fbe8a4 100644 --- a/migrations/2022-02-21-211645_create_sessions/up.sql +++ b/migrations/2022-02-21-211645_create_sessions/up.sql @@ -3,10 +3,7 @@ CREATE TABLE persistent_sessions id SERIAL CONSTRAINT persistent_sessions_pk PRIMARY KEY, - user_id INTEGER NOT NULL - CONSTRAINT persistent_sessions_users_id_fk - REFERENCES users - ON UPDATE CASCADE ON DELETE CASCADE, + 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, @@ -17,5 +14,5 @@ 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_user_id_index - ON persistent_sessions (user_id); +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 index b0a29682d2d..aba9062e6ef 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -173,7 +173,24 @@ fn save_user_to_database( /// Handles the `DELETE /api/private/session` route. pub fn logout(req: &mut dyn RequestExt) -> EndpointResult { + // TODO(adsnaider): Remove this. req.session_mut().remove(&"user_id".to_string()); + if let Some(session_token) = req + .cookies() + .get(SESSION_COOKIE_NAME) + .map(|cookie| cookie.value().to_string()) + { + req.cookies_mut().remove(Cookie::named(SESSION_COOKIE_NAME)); + + if let Ok(conn) = req.db_conn() { + match PersistentSession::revoke_from_token(&conn, &session_token) { + Ok(0) => {} + Ok(1) => {} + Ok(_count) => {} + Err(_e) => {} + } + } + } Ok(req.json(&true)) } diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index ee6b4cb8572..80b1baf0ded 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -2,9 +2,11 @@ use chrono::NaiveDateTime; use diesel::prelude::*; use ipnetwork::IpNetwork; use std::net::IpAddr; +use thiserror::Error; use crate::schema::persistent_sessions; use crate::util::token::SecureToken; +use crate::util::token::SecureTokenKind; #[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable)] #[table_name = "persistent_sessions"] @@ -19,6 +21,14 @@ pub struct PersistentSession { pub last_user_agent: String, } +#[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 { pub fn create<'a, 'b>( user_id: i32, @@ -33,6 +43,19 @@ impl PersistentSession { last_user_agent, } } + + pub fn revoke_from_token(conn: &PgConnection, token: &str) -> Result { + 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) + } } #[derive(Clone, Debug, PartialEq, Eq, Insertable)] diff --git a/src/schema.rs b/src/schema.rs index 41c5df57ffc..a4d0211362a 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -1082,7 +1082,6 @@ joinable!(dependencies -> versions (version_id)); joinable!(emails -> users (user_id)); joinable!(follows -> crates (crate_id)); joinable!(follows -> users (user_id)); -joinable!(persistent_sessions -> users (user_id)); joinable!(publish_limit_buckets -> users (user_id)); joinable!(publish_rate_overrides -> users (user_id)); joinable!(readme_renderings -> versions (version_id)); From c51ab6531adcc42cfdc3cd2f3047c2df806495cf Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 14 Mar 2022 19:05:17 -0700 Subject: [PATCH 05/19] Use persistent sessions for authentication. --- src/controllers/util.rs | 36 +++++++++++++++++++++++++++++--- src/models/persistent_session.rs | 26 +++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index ca0074c0bb1..417ecaa5fc1 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -1,13 +1,14 @@ use chrono::Utc; -use conduit_cookie::RequestSession; +use conduit_cookie::RequestCookies; use super::prelude::*; - +use crate::controllers::user::session::SESSION_COOKIE_NAME; use crate::middleware::log_request; -use crate::models::{ApiToken, User}; +use crate::models::{ApiToken, PersistentSession, User}; use crate::util::errors::{ account_locked, forbidden, internal, AppError, AppResult, InsecurelyGeneratedTokenRevoked, }; +use conduit_cookie::RequestSession; #[derive(Debug)] pub struct AuthenticatedUser { @@ -67,6 +68,7 @@ fn verify_origin(req: &dyn RequestExt) -> AppResult<()> { fn authenticate_user(req: &dyn RequestExt) -> AppResult { let conn = req.db_write()?; + // TODO(adsnaider): Remove this. let session = req.session(); let user_id_from_session = session.get("user_id").and_then(|s| s.parse::().ok()); @@ -80,6 +82,34 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { }); } + if let Some(session_token) = req + .cookies() + .get(SESSION_COOKIE_NAME) + .map(|cookie| cookie.value()) + { + 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_from_token_and_update( + &conn, + session_token, + 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, + }); + } + } + // Otherwise, look for an `Authorization` header on the request let maybe_authorization = req .headers() diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index 80b1baf0ded..b68ba4db520 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -44,6 +44,32 @@ impl PersistentSession { } } + pub fn find_from_token_and_update( + conn: &PgConnection, + token: &str, + ip_address: IpAddr, + user_agent: &str, + ) -> Result, SessionError> { + let hashed_token = SecureToken::parse(SecureTokenKind::Session, token) + .ok_or(SessionError::InvalidSessionToken)?; + let sessions = persistent_sessions::table + .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) + } + pub fn revoke_from_token(conn: &PgConnection, token: &str) -> Result { let hashed_token = SecureToken::parse(SecureTokenKind::Session, token) .ok_or(SessionError::InvalidSessionToken)?; From 04fe1d28b7b7b4dc0fa938554b56285ffc1a9f3a Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 4 Apr 2022 15:54:20 -0700 Subject: [PATCH 06/19] Added session tests. --- src/controllers/user/session.rs | 2 +- src/tests/authentication.rs | 38 +++++++++++++++++++++++++++++ src/tests/util.rs | 42 +++++++++++++++++++++++++++++++++ src/util.rs | 2 +- src/util/token.rs | 12 +++++----- 5 files changed, 88 insertions(+), 8 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index aba9062e6ef..36210335bdd 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -16,7 +16,7 @@ use crate::Env; pub const SESSION_COOKIE_NAME: &str = "crates_auth"; -fn session_cookie(token: &NewSecureToken, secure: bool) -> Cookie<'static> { +pub fn session_cookie(token: &NewSecureToken, secure: bool) -> Cookie<'static> { Cookie::build(SESSION_COOKIE_NAME, token.plaintext().to_string()) .http_only(true) .secure(secure) diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index 2af5a689588..b95c1a48786 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -2,6 +2,9 @@ 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::util::token::SecureToken; +use cargo_registry::util::token::SecureTokenKind; use conduit::{header, Body, Method, StatusCode}; static URL: &str = "/api/v1/me/updates"; @@ -9,6 +12,41 @@ static MUST_LOGIN: &[u8] = br#"{"errors":[{"detail":"must be logged in to perfor static INTERNAL_ERROR_NO_USER: &str = "user_id from cookie not found in database caused by NotFound"; +#[test] +fn persistent_session_user() { + let (app, _) = TestApp::init().empty(); + let user = app.db_new_user("user1").with_session(); + let request = user.request_builder(Method::GET, URL); + let response: Response = user.run(request); + assert_eq!(response.status(), StatusCode::OK); +} + +#[test] +fn incorrect_session_is_forbidden() { + let (_, anon) = TestApp::init().empty(); + + let token = SecureToken::generate(SecureTokenKind::Session); + // Create a cookie that isn't in the database. + let cookie = session_cookie(&token, false).to_string(); + let mut request = anon.request_builder(Method::GET, URL); + request.header(header::COOKIE, &cookie); + let response: Response = anon.run(request); + assert_eq!(response.status(), StatusCode::FORBIDDEN); + assert_eq!( + response.into_json(), + json!({"errors": [{"detail": "must be logged in to perform that action"}]}) + ); +} + +#[test] +fn cookie_user() { + let (_, _, cookie_user) = TestApp::init().with_user(); + let request = cookie_user.request_builder(Method::GET, URL); + + let response: Response = cookie_user.run(request); + assert_eq!(response.status(), StatusCode::OK); +} + #[test] fn anonymous_user_unauthorized() { let (_, anon) = TestApp::init().empty(); diff --git a/src/tests/util.rs b/src/tests/util.rs index 20f1e15891c..edc4012c0a7 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -23,7 +23,12 @@ use crate::{ builders::PublishBuilder, CategoryListResponse, CategoryResponse, CrateList, CrateResponse, GoodCrate, OkBool, OwnersResponse, VersionResponse, }; +use cargo_registry::controllers::user::session::session_cookie; +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; use conduit::{BoxError, Handler, Method}; use conduit_cookie::SessionMiddleware; @@ -271,6 +276,43 @@ impl MockCookieUser { token, } } + + 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); + + 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, + } + } +} + +pub struct MockSessionUser { + app: TestApp, + token: NewSecureToken, +} + +impl RequestHelper for MockSessionUser { + fn request_builder(&self, method: Method, path: &str) -> MockRequest { + let cookie = session_cookie(&self.token, false).to_string(); + + let mut request = req(method, path); + request.header(header::COOKIE, &cookie); + request + } + + fn app(&self) -> &TestApp { + &self.app + } } /// A type that can generate token authenticated requests diff --git a/src/util.rs b/src/util.rs index f6e9a3d2e89..577d261e7ae 100644 --- a/src/util.rs +++ b/src/util.rs @@ -12,7 +12,7 @@ mod io_util; mod request_helpers; mod request_proxy; pub mod rfc3339; -pub(crate) mod token; +pub mod token; pub type AppResponse = Response; pub type EndpointResult = Result>; diff --git a/src/util/token.rs b/src/util/token.rs index 5a1e967d2aa..5474d5524d7 100644 --- a/src/util/token.rs +++ b/src/util/token.rs @@ -12,7 +12,7 @@ pub struct SecureToken { } impl SecureToken { - pub(crate) fn generate(kind: SecureTokenKind) -> NewSecureToken { + pub fn generate(kind: SecureTokenKind) -> NewSecureToken { let plaintext = format!( "{}{}", kind.prefix(), @@ -26,7 +26,7 @@ impl SecureToken { } } - pub(crate) fn parse(kind: SecureTokenKind, plaintext: &str) -> Option { + pub fn parse(kind: SecureTokenKind, plaintext: &str) -> Option { // This will both reject tokens without a prefix and tokens of the wrong kind. if SecureTokenKind::from_token(plaintext) != Some(kind) { return None; @@ -60,18 +60,18 @@ impl FromSql for SecureToken { } } -pub(crate) struct NewSecureToken { +pub struct NewSecureToken { plaintext: String, inner: SecureToken, } impl NewSecureToken { - pub(crate) fn plaintext(&self) -> &str { + pub fn plaintext(&self) -> &str { &self.plaintext } #[cfg(test)] - pub(crate) fn into_inner(self) -> SecureToken { + pub fn into_inner(self) -> SecureToken { self.inner } } @@ -120,7 +120,7 @@ secure_token_kind! { /// NEVER CHANGE THE PREFIX OF EXISTING TOKEN TYPES!!! Doing so will implicitly revoke all the /// tokens of that kind, distrupting production users. #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] - pub(crate) enum SecureTokenKind { + pub enum SecureTokenKind { Api => "cio", // Crates.IO Session => "ses", // Session tokens. } From 21d5d71962167ad4b3c3de22dfac05c9677489c3 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 4 Apr 2022 18:49:42 -0700 Subject: [PATCH 07/19] Added documentation and comments. --- src/controllers/user/session.rs | 26 ++++++++++++------------- src/controllers/util.rs | 4 +++- src/models/persistent_session.rs | 33 ++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 36210335bdd..9b96b15bef9 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -14,8 +14,10 @@ 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 = "crates_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) @@ -117,6 +119,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { &*req.db_write()?, )?; + // Setup a persistent session for the newly logged in user. let user_agent = req .headers() .get(header::USER_AGENT) @@ -124,17 +127,16 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { .map(|value| value.to_string()) .unwrap_or_default(); - // Setup a session for the newly logged in user. let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session); - let session = PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent); - session.insert(&*req.db_conn()?)?; + PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent) + .insert(&*req.db_conn()?)?; - // Setup session cookie. + // Setup persistent session cookie. let secure = req.app().config.env() == Env::Production; req.cookies_mut().add(session_cookie(&token, secure)); - // Log in by setting a cookie and the middleware authentication - // TODO(adsnaider): Remove. + // 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. req.session_mut() .insert("user_id".to_string(), user.id.to_string()); @@ -173,8 +175,10 @@ fn save_user_to_database( /// Handles the `DELETE /api/private/session` route. pub fn logout(req: &mut dyn RequestExt) -> EndpointResult { - // TODO(adsnaider): Remove this. + // 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 .cookies() .get(SESSION_COOKIE_NAME) @@ -183,12 +187,8 @@ pub fn logout(req: &mut dyn RequestExt) -> EndpointResult { req.cookies_mut().remove(Cookie::named(SESSION_COOKIE_NAME)); if let Ok(conn) = req.db_conn() { - match PersistentSession::revoke_from_token(&conn, &session_token) { - Ok(0) => {} - Ok(1) => {} - Ok(_count) => {} - Err(_e) => {} - } + // TODO(adsnaider): Maybe log errors somehow? + let _ = PersistentSession::revoke_from_token(&conn, &session_token); } } Ok(req.json(&true)) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 417ecaa5fc1..979b96a59fc 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -68,7 +68,8 @@ fn verify_origin(req: &dyn RequestExt) -> AppResult<()> { fn authenticate_user(req: &dyn RequestExt) -> AppResult { let conn = req.db_write()?; - // TODO(adsnaider): Remove this. + // TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630. + // Log in with the session id. let session = req.session(); let user_id_from_session = session.get("user_id").and_then(|s| s.parse::().ok()); @@ -82,6 +83,7 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { }); } + // Log in with persistent session token. if let Some(session_token) = req .cookies() .get(SESSION_COOKIE_NAME) diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index b68ba4db520..0bedf13155b 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -8,19 +8,34 @@ use crate::schema::persistent_sessions; use crate::util::token::SecureToken; use crate::util::token::SecureTokenKind; +/// A persistent session model (as is stored in the database). +/// +/// The sessions table works by maintaining a `hashed_token`. In order for a user to securely +/// demonstrate authenticity, the user provides us with the token (stored as part of a cookie). We +/// hash the token and search in the database for matches. If we find one and the token hasn't +/// been revoked, then we update the session with the latest values and authorize the user. #[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable)] #[table_name = "persistent_sessions"] pub struct PersistentSession { + /// The id of this session. pub id: i32, + /// The user id associated with this session. pub user_id: i32, + /// The token (hashed) that identifies the session. 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")] @@ -30,6 +45,7 @@ pub enum SessionError { } impl PersistentSession { + /// Creates a `NewPersistentSession` that can be inserted into the database. pub fn create<'a, 'b>( user_id: i32, token: &'a SecureToken, @@ -44,6 +60,13 @@ impl PersistentSession { } } + /// Finds an unrevoked session that matches `token` from the database and returns it. + /// + /// # 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_from_token_and_update( conn: &PgConnection, token: &str, @@ -56,6 +79,8 @@ impl PersistentSession { .filter(persistent_sessions::revoked.eq(false)) .filter(persistent_sessions::hashed_token.eq(hashed_token)); + // TODO: Do we want to check if the user agent or IP address don't match? What about the + // created_at/last_user_at times, do we want to expire the tokens? conn.transaction(|| { diesel::update(sessions.clone()) .set(( @@ -70,6 +95,12 @@ impl PersistentSession { .map_err(SessionError::DieselError) } + /// 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 { let hashed_token = SecureToken::parse(SecureTokenKind::Session, token) .ok_or(SessionError::InvalidSessionToken)?; @@ -84,6 +115,7 @@ impl PersistentSession { } } +/// A new, insertable persistent session. #[derive(Clone, Debug, PartialEq, Eq, Insertable)] #[table_name = "persistent_sessions"] pub struct NewPersistentSession<'a, 'b> { @@ -94,6 +126,7 @@ pub struct NewPersistentSession<'a, 'b> { } impl NewPersistentSession<'_, '_> { + /// Inserts the session into the database. pub fn insert(self, conn: &PgConnection) -> Result { diesel::insert_into(persistent_sessions::table) .values(self) From b9af39c4e0b8bc4f2496eef25c34fe78ed2e01d4 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 4 Apr 2022 19:24:50 -0700 Subject: [PATCH 08/19] Fixed visiblity of token alphanumeric generator. --- src/util/token.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/token.rs b/src/util/token.rs index 5474d5524d7..4b30ee60ae7 100644 --- a/src/util/token.rs +++ b/src/util/token.rs @@ -84,7 +84,7 @@ impl std::ops::Deref for NewSecureToken { } } -pub(crate) fn generate_secure_alphanumeric_string(len: usize) -> String { +fn generate_secure_alphanumeric_string(len: usize) -> String { const CHARS: &[u8] = b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; OsRng From 2af70a2eb573a8b830268c5e5a151f0e5db3ac9c Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Mon, 4 Apr 2022 19:42:17 -0700 Subject: [PATCH 09/19] Fix lint error and make clippy happier. --- src/tests/util.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/util.rs b/src/tests/util.rs index edc4012c0a7..047d144aa43 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -285,7 +285,7 @@ impl MockCookieUser { self.app.db(|conn| { PersistentSession::create(self.user.id, &token, ip_addr.parse().unwrap(), user_agent) - .insert(&conn) + .insert(conn) .unwrap() }); From c774eee1b0aedd32412454f07defa09f0bd118b6 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Fri, 15 Apr 2022 14:38:46 -0700 Subject: [PATCH 10/19] Changed persistent sessions id to BIGSERIAL. --- migrations/2022-02-21-211645_create_sessions/up.sql | 6 +++--- src/models/persistent_session.rs | 2 +- src/schema.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql index 08038fbe8a4..9029c524fb3 100644 --- a/migrations/2022-02-21-211645_create_sessions/up.sql +++ b/migrations/2022-02-21-211645_create_sessions/up.sql @@ -1,8 +1,8 @@ CREATE TABLE persistent_sessions ( - id SERIAL - CONSTRAINT persistent_sessions_pk - PRIMARY KEY, + id BIGSERIAL + CONSTRAINT persistent_sessions_pk + PRIMARY KEY, user_id INTEGER NOT NULL, hashed_token bytea NOT NULL, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL, diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index 0bedf13155b..a5459e9f38a 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -18,7 +18,7 @@ use crate::util::token::SecureTokenKind; #[table_name = "persistent_sessions"] pub struct PersistentSession { /// The id of this session. - pub id: i32, + pub id: i64, /// The user id associated with this session. pub user_id: i32, /// The token (hashed) that identifies the session. diff --git a/src/schema.rs b/src/schema.rs index a4d0211362a..0b7ca1082c0 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -608,10 +608,10 @@ table! { persistent_sessions (id) { /// The `id` column of the `persistent_sessions` table. /// - /// Its SQL type is `Int4`. + /// Its SQL type is `Int8`. /// /// (Automatically generated by Diesel.) - id -> Int4, + id -> Int8, /// The `user_id` column of the `persistent_sessions` table. /// /// Its SQL type is `Int4`. From 81831e595c61f0cdcf0120fda090b9645da090b4 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Fri, 15 Apr 2022 14:40:37 -0700 Subject: [PATCH 11/19] Changed persistent session token prefix to `scio`. --- src/util/token.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/token.rs b/src/util/token.rs index 4b30ee60ae7..beb9b977c14 100644 --- a/src/util/token.rs +++ b/src/util/token.rs @@ -122,7 +122,7 @@ secure_token_kind! { #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] pub enum SecureTokenKind { Api => "cio", // Crates.IO - Session => "ses", // Session tokens. + Session => "scio", // Session tokens. } } @@ -173,7 +173,7 @@ mod tests { }; ensure(SecureTokenKind::Api, "cio"); - ensure(SecureTokenKind::Session, "ses"); + ensure(SecureTokenKind::Session, "scio"); assert!( remaining.is_empty(), From b0fb29d828eaa77a6c9c8eb69dbfe2edfefd3f52 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Fri, 15 Apr 2022 14:42:46 -0700 Subject: [PATCH 12/19] Changed persistent session cookie name to `Host-auth`. --- src/controllers/user/session.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 9b96b15bef9..b75662a00b2 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -15,7 +15,7 @@ use crate::util::token::SecureToken; use crate::Env; /// Name of the cookie used for session-based authentication. -pub const SESSION_COOKIE_NAME: &str = "crates_auth"; +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> { From 12802325ba5abc3837d78f6ed053ce2c849a8e1d Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Fri, 15 Apr 2022 14:45:37 -0700 Subject: [PATCH 13/19] Clean up TODO. --- src/models/persistent_session.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index a5459e9f38a..02ea5e63fa0 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -79,8 +79,6 @@ impl PersistentSession { .filter(persistent_sessions::revoked.eq(false)) .filter(persistent_sessions::hashed_token.eq(hashed_token)); - // TODO: Do we want to check if the user agent or IP address don't match? What about the - // created_at/last_user_at times, do we want to expire the tokens? conn.transaction(|| { diesel::update(sessions.clone()) .set(( From 14ad3cc22c0757ca30dcfff5f45eb1f6b3365ac9 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Fri, 15 Apr 2022 15:40:32 -0700 Subject: [PATCH 14/19] Removed persisten_session hashed token index. --- .../2022-02-21-211645_create_sessions/up.sql | 3 - src/controllers/user/session.rs | 28 +++----- src/controllers/util.rs | 16 ++--- src/models.rs | 2 +- src/models/persistent_session.rs | 69 ++++++++++++++++++- src/tests/authentication.rs | 6 +- src/tests/util.rs | 11 ++- 7 files changed, 92 insertions(+), 43 deletions(-) diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql index 9029c524fb3..372d542c245 100644 --- 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 index b75662a00b2..e5f81d28a97 100644 --- 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 @@ -128,12 +115,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. @@ -181,10 +170,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 index 979b96a59fc..546a4725be2 100644 --- 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 { } // 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::()) { let ip_addr = req.remote_addr().ip(); @@ -97,12 +98,9 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { .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 index 173ded3a051..3d19ae9cddf 100644 --- 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 index 02ea5e63fa0..a607f5147c7 100644 --- 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, 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 { + 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 index b95c1a48786..fdc094d4fa7 100644 --- 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 = anon.run(request); diff --git a/src/tests/util.rs b/src/tests/util.rs index 047d144aa43..e7d58d68329 100644 --- 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; @@ -283,7 +282,7 @@ 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() @@ -291,19 +290,19 @@ impl MockCookieUser { 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); From c215d23e9f6ae263cba1e7d3d58b092188d2597a Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Sat, 16 Apr 2022 16:54:40 -0700 Subject: [PATCH 15/19] Removed last_used_* fields from persistent session table. --- .../2022-02-21-211645_create_sessions/up.sql | 5 +- src/controllers/user/session.rs | 43 ++++--- src/controllers/util.rs | 27 ++-- src/models/persistent_session.rs | 119 +++++++----------- src/schema.rs | 18 --- src/tests/util.rs | 5 +- src/worker/dump_db/dump-db.toml | 3 - 7 files changed, 80 insertions(+), 140 deletions(-) diff --git a/migrations/2022-02-21-211645_create_sessions/up.sql b/migrations/2022-02-21-211645_create_sessions/up.sql index 372d542c245..9a427c90205 100644 --- 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 index e5f81d28a97..8ee41e8c7ab 100644 --- 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; @@ -107,16 +109,8 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { )?; // 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()); @@ -162,25 +156,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::()?; - 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 index 546a4725be2..c0262d6cbbd 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -90,23 +90,16 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { .map(|cookie| cookie.value()) .map(|cookie| cookie.parse::()) { - 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 index a607f5147c7..7f55515beca 100644 --- 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, 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, 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 { - 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::(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 { 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 index 0b7ca1082c0..d9ce8db9c99 100644 --- 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 index e7d58d68329..adf35352aab 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -277,13 +277,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 index 7c09aebaac1..24bd9f86eec 100644 --- 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" From 7466b6352b800c2c0a7e6a235b8be7375647bbac Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Sat, 16 Apr 2022 16:58:40 -0700 Subject: [PATCH 16/19] Added test for persistent session after logout. --- src/tests/authentication.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index fdc094d4fa7..5459acc4cd9 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -21,6 +21,25 @@ fn persistent_session_user() { assert_eq!(response.status(), StatusCode::OK); } +#[test] +fn persistent_session_revoked_after_logout() { + let (app, _) = TestApp::init().empty(); + let user = app.db_new_user("user1").with_session(); + let request = user.request_builder(Method::GET, URL); + let response: Response = user.run(request); + assert_eq!(response.status(), StatusCode::OK); + + // Logout + let request = user.request_builder(Method::DELETE, "/api/private/session"); + let response: Response = user.run(request); + assert_eq!(response.status(), StatusCode::OK); + + // Now this request should fail since we logged out. + let request = user.request_builder(Method::GET, URL); + let response: Response = user.run(request); + assert_eq!(response.status(), StatusCode::FORBIDDEN); +} + #[test] fn incorrect_session_is_forbidden() { let (_, anon) = TestApp::init().empty(); From 2c51d08a4998fe7cd1e25cb7ccb8b15ad92d2dbd Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Sat, 16 Apr 2022 17:30:04 -0700 Subject: [PATCH 17/19] Improved persistent session API. --- src/controllers/user/session.rs | 6 +---- src/models/persistent_session.rs | 42 ++++++++++++++++++++------------ src/tests/authentication.rs | 8 ++---- src/tests/util.rs | 10 +++----- src/util.rs | 2 +- src/util/token.rs | 12 ++++----- 6 files changed, 40 insertions(+), 40 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 8ee41e8c7ab..5928cf418ac 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -13,7 +13,6 @@ 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::SecureToken; use crate::Env; /// Handles the `GET /api/private/session/begin` route. @@ -109,10 +108,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { )?; // Setup a persistent session for the newly logged in user. - let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session); - let session = PersistentSession::create(user.id, &token).insert(&*req.db_conn()?)?; - - let cookie = SessionCookie::new(session.id, token.plaintext().to_string()); + let (_session, cookie) = PersistentSession::create(user.id).insert(&*req.db_conn()?)?; // Setup persistent session cookie. let secure = req.app().config.env() == Env::Production; diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index 7f55515beca..c9db3eea493 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -6,6 +6,7 @@ use std::str::FromStr; use thiserror::Error; use crate::schema::persistent_sessions; +use crate::util::token::NewSecureToken; use crate::util::token::SecureToken; use crate::util::token::SecureTokenKind; @@ -31,12 +32,10 @@ pub struct PersistentSession { } impl PersistentSession { - /// Creates a `NewPersistentSession` that can be inserted into the database. - pub fn create<'a>(user_id: i32, token: &'a SecureToken) -> NewPersistentSession<'a> { - NewPersistentSession { - user_id, - hashed_token: token, - } + /// Creates a `NewPersistentSession` for the `user_id` and the token associated with it. + pub fn create<'a>(user_id: i32) -> NewPersistentSession { + let token = SecureToken::generate(SecureTokenKind::Session); + NewPersistentSession { user_id, token } } /// Finds the session with the ID. @@ -81,19 +80,32 @@ impl PersistentSession { } /// A new, insertable persistent session. -#[derive(Clone, Debug, PartialEq, Eq, Insertable)] -#[table_name = "persistent_sessions"] -pub struct NewPersistentSession<'a> { +pub struct NewPersistentSession { user_id: i32, - hashed_token: &'a SecureToken, + token: NewSecureToken, } -impl NewPersistentSession<'_> { +impl NewPersistentSession { /// Inserts the session into the database. - pub fn insert(self, conn: &PgConnection) -> Result { - diesel::insert_into(persistent_sessions::table) - .values(self) - .get_result(conn) + /// + /// # Returns + /// + /// The + pub fn insert( + self, + conn: &PgConnection, + ) -> Result<(PersistentSession, SessionCookie), diesel::result::Error> { + let session: PersistentSession = diesel::insert_into(persistent_sessions::table) + .values(( + persistent_sessions::user_id.eq(&self.user_id), + persistent_sessions::hashed_token.eq(&*self.token), + )) + .get_result(conn)?; + let id = session.id; + Ok(( + session, + SessionCookie::new(id, self.token.plaintext().to_string()), + )) } } diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index 5459acc4cd9..ba45ba501b1 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -3,8 +3,6 @@ use crate::TestApp; use crate::util::encode_session_header; 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}; static URL: &str = "/api/v1/me/updates"; @@ -44,11 +42,9 @@ fn persistent_session_revoked_after_logout() { fn incorrect_session_is_forbidden() { let (_, anon) = TestApp::init().empty(); - let token = SecureToken::generate(SecureTokenKind::Session); + let token = "scio:1234:abcdfdfdsafdsfd".to_string(); // Create a cookie that isn't in the database. - let cookie = SessionCookie::new(123, token.plaintext().to_string()) - .build(false) - .to_string(); + let cookie = SessionCookie::new(123, token).build(false).to_string(); let mut request = anon.request_builder(Method::GET, URL); request.header(header::COOKIE, &cookie); let response: Response = anon.run(request); diff --git a/src/tests/util.rs b/src/tests/util.rs index adf35352aab..d8e5ac79d0a 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -26,8 +26,6 @@ use crate::{ use cargo_registry::models::persistent_session::SessionCookie; use cargo_registry::models::PersistentSession; use cargo_registry::models::{ApiToken, CreatedApiToken, User}; -use cargo_registry::util::token::SecureToken; -use cargo_registry::util::token::SecureTokenKind; use conduit::{BoxError, Handler, Method}; use conduit_cookie::SessionMiddleware; @@ -277,17 +275,15 @@ impl MockCookieUser { } pub fn with_session(&self) -> MockSessionUser { - let token = SecureToken::generate(SecureTokenKind::Session); - - let session = self.app.db(|conn| { - PersistentSession::create(self.user.id, &token) + let (_session, session_cookie) = self.app.db(|conn| { + PersistentSession::create(self.user.id) .insert(conn) .unwrap() }); MockSessionUser { app: self.app.clone(), - session_cookie: SessionCookie::new(session.id, token.plaintext().to_string()), + session_cookie, } } } diff --git a/src/util.rs b/src/util.rs index 577d261e7ae..f6e9a3d2e89 100644 --- a/src/util.rs +++ b/src/util.rs @@ -12,7 +12,7 @@ mod io_util; mod request_helpers; mod request_proxy; pub mod rfc3339; -pub mod token; +pub(crate) mod token; pub type AppResponse = Response; pub type EndpointResult = Result>; diff --git a/src/util/token.rs b/src/util/token.rs index beb9b977c14..ae56539e3f7 100644 --- a/src/util/token.rs +++ b/src/util/token.rs @@ -12,7 +12,7 @@ pub struct SecureToken { } impl SecureToken { - pub fn generate(kind: SecureTokenKind) -> NewSecureToken { + pub(crate) fn generate(kind: SecureTokenKind) -> NewSecureToken { let plaintext = format!( "{}{}", kind.prefix(), @@ -26,7 +26,7 @@ impl SecureToken { } } - pub fn parse(kind: SecureTokenKind, plaintext: &str) -> Option { + pub(crate) fn parse(kind: SecureTokenKind, plaintext: &str) -> Option { // This will both reject tokens without a prefix and tokens of the wrong kind. if SecureTokenKind::from_token(plaintext) != Some(kind) { return None; @@ -60,18 +60,18 @@ impl FromSql for SecureToken { } } -pub struct NewSecureToken { +pub(crate) struct NewSecureToken { plaintext: String, inner: SecureToken, } impl NewSecureToken { - pub fn plaintext(&self) -> &str { + pub(crate) fn plaintext(&self) -> &str { &self.plaintext } #[cfg(test)] - pub fn into_inner(self) -> SecureToken { + pub(crate) fn into_inner(self) -> SecureToken { self.inner } } @@ -120,7 +120,7 @@ secure_token_kind! { /// NEVER CHANGE THE PREFIX OF EXISTING TOKEN TYPES!!! Doing so will implicitly revoke all the /// tokens of that kind, distrupting production users. #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] - pub enum SecureTokenKind { + pub(crate) enum SecureTokenKind { Api => "cio", // Crates.IO Session => "scio", // Session tokens. } From 26aeb88d6dbef3160acfcf948ae2bc82336ef9cf Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Sat, 16 Apr 2022 18:29:08 -0700 Subject: [PATCH 18/19] Make clippy happy again. --- src/models/persistent_session.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/persistent_session.rs b/src/models/persistent_session.rs index c9db3eea493..2f6122b53ff 100644 --- a/src/models/persistent_session.rs +++ b/src/models/persistent_session.rs @@ -33,7 +33,7 @@ pub struct PersistentSession { impl PersistentSession { /// Creates a `NewPersistentSession` for the `user_id` and the token associated with it. - pub fn create<'a>(user_id: i32) -> NewPersistentSession { + pub fn create(user_id: i32) -> NewPersistentSession { let token = SecureToken::generate(SecureTokenKind::Session); NewPersistentSession { user_id, token } } From 731e2fd9477e925a485af70a1aa1e48d7e2c8996 Mon Sep 17 00:00:00 2001 From: Adam Snaider Date: Fri, 20 May 2022 13:02:09 -0700 Subject: [PATCH 19/19] Fixed merge conflicts. --- Cargo.lock | 12 ++++++------ src/controllers/user/session.rs | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fbca48d4f99..642859cf75c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -267,7 +267,7 @@ dependencies = [ "hyper", "hyper-tls", "indexmap", - "ipnetwork", + "ipnetwork 0.19.0", "lazy_static", "lettre", "minijinja", @@ -771,7 +771,7 @@ dependencies = [ "byteorder", "chrono", "diesel_derives", - "ipnetwork", + "ipnetwork 0.18.0", "libc", "pq-sys", "r2d2", @@ -1352,18 +1352,18 @@ checksum = "879d54834c8c76457ef4293a689b2a8c59b076067ad77b15efafbb05f92a592b" [[package]] name = "ipnetwork" -version = "0.19.0" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1f84f1612606f3753f205a4e9a2efd6fe5b4c573a6269b2cc6c3003d44a0d127" +checksum = "4088d739b183546b239688ddbc79891831df421773df95e236daf7867866d355" dependencies = [ "serde", ] [[package]] name = "ipnetwork" -version = "0.18.0" +version = "0.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4088d739b183546b239688ddbc79891831df421773df95e236daf7867866d355" +checksum = "1f84f1612606f3753f205a4e9a2efd6fe5b4c573a6269b2cc6c3003d44a0d127" dependencies = [ "serde", ] diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 5928cf418ac..a4acd9d0fa7 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -108,7 +108,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { )?; // Setup a persistent session for the newly logged in user. - let (_session, cookie) = PersistentSession::create(user.id).insert(&*req.db_conn()?)?; + let (_session, cookie) = PersistentSession::create(user.id).insert(&*req.db_write()?)?; // Setup persistent session cookie. let secure = req.app().config.env() == Env::Production; @@ -178,7 +178,7 @@ pub fn logout(req: &mut dyn RequestExt) -> EndpointResult { req.cookies_mut() .remove(Cookie::named(SessionCookie::SESSION_COOKIE_NAME)); - let conn = req.db_conn()?; + let conn = req.db_write()?; let mut session = PersistentSession::find(session_cookie.session_id(), &conn)? .ok_or(LogoutError::SessionNotInDB)?; session.revoke().update(&conn)?;