diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 6095509073c..36106dd9da8 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -5,7 +5,6 @@ use oauth2::reqwest::http_client; use oauth2::{AuthorizationCode, Scope, TokenResponse}; use crate::github::GithubUser; -use crate::middleware::current_user::TrustedUserId; use crate::models::{NewUser, User}; use crate::schema::users; use crate::util::errors::ReadOnlyMode; @@ -108,7 +107,6 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { // Log in by setting a cookie and the middleware authentication req.session_mut() .insert("user_id".to_string(), user.id.to_string()); - req.mut_extensions().insert(TrustedUserId(user.id)); super::me::me(req) } diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 4c7f4f84f29..e0f943216be 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -1,8 +1,8 @@ use chrono::Utc; +use conduit_cookie::RequestSession; use super::prelude::*; -use crate::middleware::current_user::TrustedUserId; use crate::middleware::log_request; use crate::models::{ApiToken, User}; use crate::util::errors::{ @@ -62,9 +62,11 @@ fn verify_origin(req: &dyn RequestExt) -> AppResult<()> { fn authenticate_user(req: &dyn RequestExt) -> AppResult { let conn = req.db_conn()?; - let (user_id, token_id) = if let Some(id) = - req.extensions().find::().map(|x| x.0) - { + + let session = req.session(); + let user_id_from_session = session.get("user_id").and_then(|s| s.parse::().ok()); + + let (user_id, token_id) = if let Some(id) = user_id_from_session { (id, None) } else { // Otherwise, look for an `Authorization` header on the request diff --git a/src/middleware.rs b/src/middleware.rs index 4fd209f942b..4272b0eaa22 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -4,7 +4,6 @@ mod prelude { } use self::app::AppMiddleware; -use self::current_user::CaptureUserIdFromCookie; use self::debug::*; use self::ember_html::EmberHtml; use self::head::Head; @@ -14,7 +13,6 @@ use self::static_or_continue::StaticOrContinue; pub mod app; mod balance_capacity; mod block_traffic; -pub mod current_user; mod debug; mod ember_html; mod ensure_well_formed_500; @@ -70,9 +68,6 @@ pub fn build_middleware(app: Arc, endpoints: R404) -> MiddlewareBuilder { m.add(AppMiddleware::new(app)); - // Parse and save the user_id from the session cookie as part of the authentication logic - m.add(CaptureUserIdFromCookie); - // Note: The following `m.around()` middleware is run from bottom to top // This is currently the final middleware to run. If a middleware layer requires a database diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs deleted file mode 100644 index 2c95c83b155..00000000000 --- a/src/middleware/current_user.rs +++ /dev/null @@ -1,37 +0,0 @@ -//! Middleware that captures the `user_id` from the signed session cookie -//! -//! Due to lifetimes it is not possible to call `mut_extensions()` while a reference obtained from -//! `extensions()` is still live. The call to `conduit_cookie::RequestSession::session` needs -//! mutable access to the request and its extensions, and there is no read-only alternative in -//! `conduit_cookie` to access the session cookie. This means that it is not possible to access -//! the session cookie while holding onto a database connection (which is obtained from the -//! `AppMiddleware` via `extensions()`). -//! -//! This is particularly problematic for the user authentication code. When an API token is used -//! for authentication, the datbase must be queried to obtain the `user_id`, so endpoint code must -//! obtain and pass in a database connection. Because of that connection, it is no longer possible -//! to use or pass around the `&mut dyn RequestExt` that it was derived from and it is not possible -//! to access the session cookie. In order to support authentication via session cookies and API -//! tokens via the same code path, the `user_id` is extracted from the session cookie and stored in -//! a `TrustedUserId` that can be read from while a connection reference is live. - -use super::prelude::*; - -use conduit_cookie::RequestSession; - -/// A trusted user_id extracted from a signed cookie or added to the request by the test harness -#[derive(Clone, Copy, Debug)] -pub struct TrustedUserId(pub i32); - -/// Middleware that captures the `user_id` from the signed session cookie -pub(super) struct CaptureUserIdFromCookie; - -impl Middleware for CaptureUserIdFromCookie { - fn before(&self, req: &mut dyn RequestExt) -> BeforeResult { - if let Some(id) = req.session().get("user_id").and_then(|s| s.parse().ok()) { - req.mut_extensions().insert(TrustedUserId(id)); - } - - Ok(()) - } -} diff --git a/src/middleware/log_request.rs b/src/middleware/log_request.rs index f54fc788c27..0d48a970692 100644 --- a/src/middleware/log_request.rs +++ b/src/middleware/log_request.rs @@ -2,10 +2,10 @@ //! information that we care about like User-Agent use super::prelude::*; -use crate::middleware::current_user::TrustedUserId; use crate::util::request_header; use conduit::{header, Host, RequestExt, Scheme, StatusCode}; +use conduit_cookie::RequestSession; use sentry::Level; use std::fmt::{self, Display, Formatter}; @@ -86,10 +86,7 @@ fn report_to_sentry(req: &dyn RequestExt, res: &AfterResult, response_time: u64) let url = format!("{}://{}{}", scheme, host, path).parse().ok(); { - let id = req - .extensions() - .find::() - .map(|x| x.0.to_string()); + let id = req.session().get("user_id").map(|str| str.to_string()); let user = sentry::User { id, diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index d909ff814b3..23a0340bff0 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -1,8 +1,7 @@ use crate::{util::RequestHelper, TestApp}; -use cargo_registry::middleware::current_user::TrustedUserId; - -use conduit::{header, Handler, HandlerResult, Method, RequestExt, StatusCode}; +use crate::util::encode_session_header; +use conduit::{header, Handler, HandlerResult, Method, StatusCode}; use conduit_test::MockRequest; static URL: &str = "/api/v1/me/updates"; @@ -49,8 +48,12 @@ fn token_auth_cannot_find_token() { #[test] fn cookie_auth_cannot_find_user() { let (app, anon) = TestApp::init().empty(); + + let session_key = &app.as_inner().session_key; + let cookie = encode_session_header(session_key, -1); + let mut request = anon.request_builder(Method::GET, URL); - request.mut_extensions().insert(TrustedUserId(-1)); + request.header(header::COOKIE, &cookie); let response = call(&app, request); let log_message = response.map(|_| ()).unwrap_err().to_string(); diff --git a/src/tests/util.rs b/src/tests/util.rs index ce39cf8af81..33c875397b3 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -27,7 +27,6 @@ use cargo_registry::{ background_jobs::Environment, db::DieselPool, git::{Credentials, RepositoryConfig}, - middleware::current_user::TrustedUserId, models::{ApiToken, CreatedApiToken, User}, util::AppResponse, App, Config, @@ -37,7 +36,8 @@ use serde_json::Value; use std::{marker::PhantomData, rc::Rc, sync::Arc, time::Duration}; use swirl::Runner; -use conduit::{Handler, HandlerResult, Method, RequestExt}; +use conduit::{Handler, HandlerResult, Method}; +use conduit_cookie::SessionMiddleware; use conduit_test::MockRequest; use cargo_registry::git::Repository as WorkerRepository; @@ -46,6 +46,8 @@ use git2::Repository as UpstreamRepository; use url::Url; pub use conduit::{header, StatusCode}; +use cookie::Cookie; +use std::collections::HashMap; pub fn init_logger() { let _ = tracing_subscriber::fmt() @@ -209,6 +211,37 @@ impl TestApp { } } +/// This function can be used to create a `Cookie` header for mock requests that +/// include cookie-based authentication. +/// +/// ``` +/// let cookie = encode_session_header(session_key, user_id); +/// request.header(header::COOKIE, &cookie); +/// ``` +/// +/// The implementation matches roughly what is happening inside of the +/// `SessionMiddleware` from `conduit_cookie`. +pub fn encode_session_header(session_key: &str, user_id: i32) -> String { + let cookie_name = "cargo_session"; + let cookie_key = cookie::Key::derive_from(session_key.as_bytes()); + + // build session data map + let mut map = HashMap::new(); + map.insert("user_id".into(), user_id.to_string()); + + // encode the map into a cookie value string + let session_middleware = SessionMiddleware::new(cookie_name, cookie_key.clone(), false); + let encoded = session_middleware.encode(&map); + + // put the cookie into a signed cookie jar + let cookie = Cookie::build(cookie_name, encoded).finish(); + let mut jar = cookie::CookieJar::new(); + jar.signed(&cookie_key).add(cookie); + + // read the raw cookie from the cookie jar + jar.get(&cookie_name).unwrap().to_string() +} + pub struct TestAppBuilder { config: Config, proxy: Option, @@ -463,9 +496,11 @@ pub struct MockCookieUser { impl RequestHelper for MockCookieUser { fn request_builder(&self, method: Method, path: &str) -> MockRequest { + let session_key = &self.app.as_inner().session_key; + let cookie = encode_session_header(session_key, self.user.id); + let mut request = req(method, path); - let id = TrustedUserId(self.user.id); - request.mut_extensions().insert(id); + request.header(header::COOKIE, &cookie); request }