Auto merge of #3280 - Turbo87:session-middleware, r=jtgeibel

Remove `CaptureUserIdFromCookie` middleware in favor accessing cookie directly

This PR resolves #2651

Unfortunately for this to work we have to use a few implementation details of `conduit_cookie`, but since we control the crate ourselves I guess this is a reasonable tradeoff. Ideally the `conduit_cookie` crate would export some kind of test helper function though.

r? `@jtgeibel`

/cc `@jsha`

Author: bors

Diff for: 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 for: 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<AuthenticatedUser> {
     let conn = req.db_conn()?;
-    let (user_id, token_id) = if let Some(id) =
-        req.extensions().find::<TrustedUserId>().map(|x| x.0)
-    {
+
+    let session = req.session();
+    let user_id_from_session = session.get("user_id").and_then(|s| s.parse::<i32>().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 for: 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<App>, 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 for: src/middleware/current_user.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::<TrustedUserId>()
-                .map(|x| x.0.to_string());
+            let id = req.session().get("user_id").map(|str| str.to_string());
 
             let user = sentry::User {
                 id,

Diff for: src/middleware/log_request.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 for: src/tests/authentication.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<String>,
@@ -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
     }