diff --git a/src/controllers.rs b/src/controllers.rs index afa66152e3d..0fe6e2ffb7b 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -19,7 +19,6 @@ mod prelude { pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here pub use crate::middleware::app::RequestApp; - pub use crate::middleware::current_user::RequestUser; use std::collections::HashMap; use std::io; @@ -28,6 +27,10 @@ mod prelude { use serde::Serialize; use url; + pub trait UserAuthenticationExt { + fn authenticate(&self, conn: &PgConnection) -> AppResult; + } + pub trait RequestUtils { fn redirect(&self, url: String) -> Response; @@ -77,6 +80,7 @@ mod prelude { } pub mod helpers; +mod util; pub mod category; pub mod crate_owner_invitation; diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index aa313efff5e..ddf0146756c 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -7,7 +7,7 @@ use crate::views::{EncodableCrateOwnerInvitation, InvitationResponse}; /// Handles the `GET /me/crate_owner_invitations` route. pub fn list(req: &mut dyn Request) -> AppResult { let conn = &*req.db_conn()?; - let user_id = req.user()?.id; + let user_id = req.authenticate(conn)?.user_id(); let crate_owner_invitations = crate_owner_invitations::table .filter(crate_owner_invitations::invited_user_id.eq(user_id)) @@ -41,7 +41,7 @@ pub fn handle_invite(req: &mut dyn Request) -> AppResult { serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?; let crate_invite = crate_invite.crate_owner_invite; - let user_id = req.user()?.id; + let user_id = req.authenticate(conn)?.user_id(); if crate_invite.accepted { accept_invite(req, conn, crate_invite, user_id) @@ -116,8 +116,7 @@ fn decline_invite( ) -> AppResult { use diesel::delete; - let user_id = req.user()?.id; - + let user_id = req.authenticate(conn)?.user_id(); delete(crate_owner_invitations::table.find((user_id, crate_invite.crate_id))).execute(conn)?; #[derive(Serialize)] diff --git a/src/controllers/krate/follow.rs b/src/controllers/krate/follow.rs index 93bd2097aa2..f0be406b840 100644 --- a/src/controllers/krate/follow.rs +++ b/src/controllers/krate/follow.rs @@ -8,15 +8,12 @@ use crate::models::{Crate, Follow}; use crate::schema::*; fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> AppResult { - let user = req.user()?; + let user_id = req.authenticate(conn)?.user_id(); let crate_name = &req.params()["crate_id"]; let crate_id = Crate::by_name(crate_name) .select(crates::id) .first(&**conn)?; - Ok(Follow { - user_id: user.id, - crate_id, - }) + Ok(Follow { user_id, crate_id }) } /// Handles the `PUT /crates/:crate_id/follow` route. diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 1ddf032433d..5a8f7597d3d 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -92,9 +92,10 @@ fn parse_owners_request(req: &mut dyn Request) -> AppResult> { fn modify_owners(req: &mut dyn Request, add: bool) -> AppResult { let logins = parse_owners_request(req)?; let app = req.app(); - let user = req.user()?; let crate_name = &req.params()["crate_id"]; + let conn = req.db_conn()?; + let user = req.authenticate(&conn)?.find_user(&conn)?; conn.transaction(|| { let krate = Crate::by_name(crate_name).first::(&*conn)?; @@ -121,13 +122,13 @@ fn modify_owners(req: &mut dyn Request, add: bool) -> AppResult { if owners.iter().any(login_test) { return Err(cargo_err(&format_args!("`{}` is already an owner", login))); } - let msg = krate.owner_add(app, &conn, user, login)?; + let msg = krate.owner_add(app, &conn, &user, login)?; msgs.push(msg); } msgs.join(",") } else { for login in &logins { - krate.owner_remove(app, &conn, user, login)?; + krate.owner_remove(app, &conn, &user, login)?; } if User::owning(&krate, &conn)?.is_empty() { return Err(cargo_err( diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 2f686cb762f..82fe3599f78 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -8,7 +8,7 @@ use crate::controllers::cargo_prelude::*; use crate::git; use crate::models::dependency; use crate::models::{ - insert_version_owner_action, Badge, Category, Keyword, NewCrate, NewVersion, Rights, User, + insert_version_owner_action, Badge, Category, Keyword, NewCrate, NewVersion, Rights, VersionAction, }; @@ -39,9 +39,11 @@ pub fn publish(req: &mut dyn Request) -> AppResult { // - Then the .crate tarball length is passed to the upload_crate function where the actual // file is read and uploaded. - let (new_crate, user) = parse_new_headers(req)?; + let new_crate = parse_new_headers(req)?; let conn = app.diesel_database.get()?; + let ids = req.authenticate(&conn)?; + let user = ids.find_user(&conn)?; let verified_email_address = user.verified_email(&conn)?; let verified_email_address = verified_email_address.ok_or_else(|| { @@ -150,7 +152,7 @@ pub fn publish(req: &mut dyn Request) -> AppResult { &conn, version.id, user.id, - req.authentication_source()?.api_token_id(), + ids.api_token_id(), VersionAction::Publish, )?; @@ -223,9 +225,8 @@ pub fn publish(req: &mut dyn Request) -> AppResult { /// Used by the `krate::new` function. /// /// This function parses the JSON headers to interpret the data and validates -/// the data during and after the parsing. Returns crate metadata and user -/// information. -fn parse_new_headers(req: &mut dyn Request) -> AppResult<(EncodableCrateUpload, User)> { +/// the data during and after the parsing. Returns crate metadata. +fn parse_new_headers(req: &mut dyn Request) -> AppResult { // Read the json upload request let metadata_length = u64::from(read_le_u32(req.body())?); req.mut_extensions().insert(metadata_length); @@ -264,6 +265,5 @@ fn parse_new_headers(req: &mut dyn Request) -> AppResult<(EncodableCrateUpload, ))); } - let user = req.user()?; - Ok((new, user.clone())) + Ok(new) } diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index ba1624aece0..535ba0e9ce2 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -154,11 +154,12 @@ pub fn search(req: &mut dyn Request) -> AppResult { ), ); } else if params.get("following").is_some() { + let user_id = req.authenticate(&conn)?.user_id(); query = query.filter( crates::id.eq_any( follows::table .select(follows::crate_id) - .filter(follows::user_id.eq(req.user()?.id)), + .filter(follows::user_id.eq(user_id)), ), ); } diff --git a/src/controllers/token.rs b/src/controllers/token.rs index dcda68e0d6a..c426528c250 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -1,6 +1,5 @@ use super::frontend_prelude::*; -use crate::middleware::current_user::AuthenticationSource; use crate::models::ApiToken; use crate::schema::api_tokens; use crate::util::read_fill; @@ -10,10 +9,13 @@ use serde_json as json; /// Handles the `GET /me/tokens` route. pub fn list(req: &mut dyn Request) -> AppResult { - let tokens = ApiToken::belonging_to(req.user()?) + let conn = req.db_conn()?; + let user = req.authenticate(&conn)?.find_user(&conn)?; + + let tokens = ApiToken::belonging_to(&user) .filter(api_tokens::revoked.eq(false)) .order(api_tokens::created_at.desc()) - .load(&*req.db_conn()?)?; + .load(&*conn)?; #[derive(Serialize)] struct R { api_tokens: Vec, @@ -35,12 +37,6 @@ pub fn new(req: &mut dyn Request) -> AppResult { api_token: NewApiToken, } - if req.authentication_source()? != AuthenticationSource::SessionCookie { - return Err(bad_request( - "cannot use an API token to create a new API token", - )); - } - let max_size = 2000; let length = req .content_length() @@ -64,11 +60,18 @@ pub fn new(req: &mut dyn Request) -> AppResult { return Err(bad_request("name must have a value")); } - let user = req.user()?; let conn = req.db_conn()?; + let ids = req.authenticate(&conn)?; + let user = ids.find_user(&conn)?; + + if ids.api_token_id().is_some() { + return Err(bad_request( + "cannot use an API token to create a new API token", + )); + } let max_token_per_user = 500; - let count = ApiToken::belonging_to(user) + let count = ApiToken::belonging_to(&user) .count() .get_result::(&*conn)?; if count >= max_token_per_user { @@ -95,9 +98,11 @@ pub fn revoke(req: &mut dyn Request) -> AppResult { .parse::() .map_err(|e| bad_request(&format!("invalid token id: {:?}", e)))?; - diesel::update(ApiToken::belonging_to(req.user()?).find(id)) + let conn = req.db_conn()?; + let user = req.authenticate(&conn)?.find_user(&conn)?; + diesel::update(ApiToken::belonging_to(&user).find(id)) .set(api_tokens::revoked.eq(true)) - .execute(&*req.db_conn()?)?; + .execute(&*conn)?; #[derive(Serialize)] struct R {} diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 8fbb84086a0..6808fdd754c 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -24,8 +24,8 @@ pub fn me(req: &mut dyn Request) -> AppResult { // perhaps adding `req.mut_extensions().insert(user)` to the // update_user route, however this somehow does not seem to work - let user_id = req.user()?.id; let conn = req.db_conn()?; + let user_id = req.authenticate(&conn)?.user_id(); let (user, verified, email, verification_sent) = users::table .find(user_id) @@ -64,10 +64,10 @@ pub fn me(req: &mut dyn Request) -> AppResult { pub fn updates(req: &mut dyn Request) -> AppResult { use diesel::dsl::any; - let user = req.user()?; let conn = req.db_conn()?; + let user = req.authenticate(&conn)?.find_user(&conn)?; - let followed_crates = Follow::belonging_to(user).select(follows::crate_id); + let followed_crates = Follow::belonging_to(&user).select(follows::crate_id); let data = versions::table .inner_join(crates::table) .left_outer_join(users::table) @@ -118,12 +118,12 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { let mut body = String::new(); req.body().read_to_string(&mut body)?; - let user = req.user()?; - let name = &req.params()["user_id"]; + let param_user_id = &req.params()["user_id"]; let conn = req.db_conn()?; + let user = req.authenticate(&conn)?.find_user(&conn)?; // need to check if current user matches user to be updated - if &user.id.to_string() != name { + if &user.id.to_string() != param_user_id { return Err(bad_request("current user does not match requested user")); } @@ -195,19 +195,19 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { use diesel::dsl::sql; use diesel::update; - let user = req.user()?; - let name = &req.params()["user_id"] + let param_user_id = req.params()["user_id"] .parse::() .chain_error(|| bad_request("invalid user_id"))?; let conn = req.db_conn()?; + let user = req.authenticate(&conn)?.find_user(&conn)?; // need to check if current user matches user to be updated - if &user.id != name { + if user.id != param_user_id { return Err(bad_request("current user does not match requested user")); } conn.transaction(|| { - let email = update(Email::belonging_to(user)) + let email = update(Email::belonging_to(&user)) .set(emails::token.eq(sql("DEFAULT"))) .get_result::(&*conn) .map_err(|_| bad_request("Email could not be found"))?; @@ -238,12 +238,12 @@ pub fn update_email_notifications(req: &mut dyn Request) -> AppResult .map(|c| (c.id, c.email_notifications)) .collect(); - let user = req.user()?; let conn = req.db_conn()?; + let user_id = req.authenticate(&conn)?.user_id(); // Build inserts from existing crates belonging to the current user let to_insert = CrateOwner::by_owner_kind(OwnerKind::User) - .filter(owner_id.eq(user.id)) + .filter(owner_id.eq(user_id)) .select((crate_id, owner_id, owner_kind, email_notifications)) .load(&*conn)? .into_iter() diff --git a/src/controllers/util.rs b/src/controllers/util.rs new file mode 100644 index 00000000000..4642072590f --- /dev/null +++ b/src/controllers/util.rs @@ -0,0 +1,53 @@ +use super::prelude::*; + +use crate::middleware::current_user::TrustedUserId; +use crate::models::{ApiToken, User}; +use crate::util::errors::{internal, AppError, AppResult, ChainError, Unauthorized}; + +#[derive(Debug)] +pub struct AuthenticatedUser { + user_id: i32, + token_id: Option, +} + +impl AuthenticatedUser { + pub fn user_id(&self) -> i32 { + self.user_id + } + + pub fn api_token_id(&self) -> Option { + self.token_id + } + + pub fn find_user(&self, conn: &PgConnection) -> AppResult { + User::find(conn, self.user_id()) + .chain_error(|| internal("user_id from cookie or token not found in database")) + } +} + +impl<'a> UserAuthenticationExt for dyn Request + 'a { + /// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error + fn authenticate(&self, conn: &PgConnection) -> AppResult { + if let Some(id) = self.extensions().find::() { + // A trusted user_id was provided by a signed cookie (or a test `MockCookieUser`) + Ok(AuthenticatedUser { + user_id: id.0, + token_id: None, + }) + } else { + // Otherwise, look for an `Authorization` header on the request + if let Some(headers) = self.headers().find("Authorization") { + ApiToken::find_by_api_token(conn, headers[0]) + .map(|token| AuthenticatedUser { + user_id: token.user_id, + token_id: Some(token.id), + }) + // Convert a NotFound (or other database error) into Unauthorized + .map_err(|_| Box::new(Unauthorized) as Box) + } else { + // Unable to authenticate the user + Err(Box::new(Unauthorized)) + } + } + } +} diff --git a/src/controllers/version.rs b/src/controllers/version.rs index 2970d78ca8a..86a3c790307 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -5,15 +5,17 @@ pub mod yank; use super::prelude::*; +use crate::db::DieselPooledConn; use crate::models::{Crate, CrateVersions, Version}; use crate::schema::versions; -fn version_and_crate(req: &mut dyn Request) -> AppResult<(Version, Crate)> { +fn version_and_crate(req: &dyn Request) -> AppResult<(DieselPooledConn<'_>, Version, Crate)> { let crate_name = &req.params()["crate_id"]; let semver = &req.params()["version"]; if semver::Version::parse(semver).is_err() { return Err(cargo_err(&format_args!("invalid semver: {}", semver))); }; + let conn = req.db_conn()?; let krate = Crate::by_name(crate_name).first::(&*conn)?; let version = krate @@ -26,5 +28,6 @@ fn version_and_crate(req: &mut dyn Request) -> AppResult<(Version, Crate)> { crate_name, semver )) })?; - Ok((version, krate)) + + Ok((conn, version, krate)) } diff --git a/src/controllers/version/downloads.rs b/src/controllers/version/downloads.rs index 1cb3e8f0968..8e174ebfbb8 100644 --- a/src/controllers/version/downloads.rs +++ b/src/controllers/version/downloads.rs @@ -69,8 +69,7 @@ fn increment_download_counts( /// Handles the `GET /crates/:crate_id/:version/downloads` route. pub fn downloads(req: &mut dyn Request) -> AppResult { - let (version, _) = version_and_crate(req)?; - let conn = req.db_conn()?; + let (conn, version, _) = version_and_crate(req)?; let cutoff_end_date = req .query() .get("before_date") diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index b794d6f318c..0f36ab001c6 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -20,8 +20,7 @@ use super::version_and_crate; /// fields for `id`, `version_id`, and `downloads` (which appears to always /// be 0) pub fn dependencies(req: &mut dyn Request) -> AppResult { - let (version, _) = version_and_crate(req)?; - let conn = req.db_conn()?; + let (conn, version, _) = version_and_crate(req)?; let deps = version.dependencies(&*conn)?; let deps = deps .into_iter() @@ -37,8 +36,7 @@ pub fn dependencies(req: &mut dyn Request) -> AppResult { /// Handles the `GET /crates/:crate_id/:version/authors` route. pub fn authors(req: &mut dyn Request) -> AppResult { - let (version, _) = version_and_crate(req)?; - let conn = req.db_conn()?; + let (conn, version, _) = version_and_crate(req)?; let names = version_authors::table .filter(version_authors::version_id.eq(version.id)) .select(version_authors::name) @@ -68,8 +66,7 @@ pub fn authors(req: &mut dyn Request) -> AppResult { /// The frontend doesn't appear to hit this endpoint, but our tests do, and it seems to be a useful /// API route to have. pub fn show(req: &mut dyn Request) -> AppResult { - let (version, krate) = version_and_crate(req)?; - let conn = req.db_conn()?; + let (conn, version, krate) = version_and_crate(req)?; let published_by = version.published_by(&conn); let actions = VersionOwnerAction::by_version(&conn, &version)?; diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index 0d6c8e44721..597d8d53415 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -28,10 +28,11 @@ pub fn unyank(req: &mut dyn Request) -> AppResult { /// Changes `yanked` flag on a crate version record fn modify_yank(req: &mut dyn Request, yanked: bool) -> AppResult { - let (version, krate) = version_and_crate(req)?; - let user = req.user()?; - let conn = req.db_conn()?; + let (conn, version, krate) = version_and_crate(req)?; + let ids = req.authenticate(&conn)?; + let user = ids.find_user(&conn)?; let owners = krate.owners(&conn)?; + if user.rights(req.app(), &owners)? < Rights::Publish { return Err(cargo_err("must already be an owner to yank or unyank")); } @@ -40,9 +41,8 @@ fn modify_yank(req: &mut dyn Request, yanked: bool) -> AppResult { } else { VersionAction::Unyank }; - let api_token_id = req.authentication_source()?.api_token_id(); - insert_version_owner_action(&conn, version.id, user.id, api_token_id, action)?; + insert_version_owner_action(&conn, version.id, user.id, ids.api_token_id(), action)?; git::yank(krate.name, version, yanked) .enqueue(&conn) diff --git a/src/middleware.rs b/src/middleware.rs index d9fb144974d..09b51e626e4 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -10,7 +10,7 @@ mod prelude { pub use prelude::Result; use self::app::AppMiddleware; -use self::current_user::CurrentUser; +use self::current_user::CaptureUserIdFromCookie; use self::debug::*; use self::ember_index_rewrite::EmberIndexRewrite; use self::head::Head; @@ -79,8 +79,8 @@ pub fn build_middleware(app: Arc, endpoints: R404) -> MiddlewareBuilder { } m.add(AppMiddleware::new(app)); - // Sets the current user on each request. - m.add(CurrentUser); + // Parse and save the user_id from the session cookie as part of the authentication logic + m.add(CaptureUserIdFromCookie); // Serve the static files in the *dist* directory, which are the frontend assets. // Not needed for the backend tests. diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index af82852baf3..fc3a6d02ede 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -1,104 +1,37 @@ +//! 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 Request` 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; -use diesel::prelude::*; - -use crate::db::RequestTransaction; -use crate::util::errors::{AppResult, ChainError, Unauthorized}; -use crate::models::ApiToken; -use crate::models::User; -use crate::schema::users; +/// 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); -#[derive(Debug, Clone, Copy)] -pub struct CurrentUser; +/// Middleware that captures the `user_id` from the signed session cookie +pub(super) struct CaptureUserIdFromCookie; -#[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub enum AuthenticationSource { - SessionCookie, - ApiToken { api_token_id: i32 }, -} - -impl Middleware for CurrentUser { +impl Middleware for CaptureUserIdFromCookie { fn before(&self, req: &mut dyn Request) -> Result<()> { - // Check if the request has a session cookie with a `user_id` property inside - let id = { - req.session() - .get("user_id") - .and_then(|s| s.parse::().ok()) - }; - - let conn = req.db_conn().map_err(|e| Box::new(e) as BoxError)?; - - if let Some(id) = id { - // If it did, look for a user in the database with the given `user_id` - let maybe_user = users::table.find(id).first::(&*conn); - drop(conn); - if let Ok(user) = maybe_user { - // Attach the `User` model from the database to the request - req.mut_extensions().insert(user); - req.mut_extensions() - .insert(AuthenticationSource::SessionCookie); - } - } else { - // Otherwise, look for an `Authorization` header on the request - // and try to find a user in the database with a matching API token - let user_auth = if let Some(headers) = req.headers().find("Authorization") { - ApiToken::find_by_api_token(&conn, headers[0]) - .and_then(|api_token| { - User::find(&conn, api_token.user_id).map(|user| { - ( - AuthenticationSource::ApiToken { - api_token_id: api_token.id, - }, - user, - ) - }) - }) - .optional() - .map_err(|e| Box::new(e) as BoxError)? - } else { - None - }; - - drop(conn); - - if let Some((api_token, user)) = user_auth { - // Attach the `User` model from the database and the API token to the request - req.mut_extensions().insert(user); - req.mut_extensions().insert(api_token); - } + if let Some(id) = req.session().get("user_id").and_then(|s| s.parse().ok()) { + req.mut_extensions().insert(TrustedUserId(id)); } Ok(()) } } - -pub trait RequestUser { - fn user(&self) -> AppResult<&User>; - fn authentication_source(&self) -> AppResult; -} - -impl<'a> RequestUser for dyn Request + 'a { - fn user(&self) -> AppResult<&User> { - self.extensions() - .find::() - .chain_error(|| Unauthorized) - } - - fn authentication_source(&self) -> AppResult { - self.extensions() - .find::() - .cloned() - .chain_error(|| Unauthorized) - } -} - -impl AuthenticationSource { - pub fn api_token_id(self) -> Option { - match self { - AuthenticationSource::SessionCookie => None, - AuthenticationSource::ApiToken { api_token_id } => Some(api_token_id), - } - } -} diff --git a/src/tests/all.rs b/src/tests/all.rs index 27f34b09910..bdc95a4c7b9 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -40,6 +40,7 @@ macro_rules! t { }; } +mod authentication; mod badge; mod builders; mod categories; diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs new file mode 100644 index 00000000000..ebf615c1841 --- /dev/null +++ b/src/tests/authentication.rs @@ -0,0 +1,60 @@ +use crate::{util::RequestHelper, TestApp}; + +use cargo_registry::middleware::current_user::TrustedUserId; + +use conduit::{Handler, Method, Request, Response}; +use conduit_test::MockRequest; + +type ResponseResult = Result>; + +static URL: &str = "/api/v1/me/updates"; +static MUST_LOGIN: &[u8] = + b"{\"errors\":[{\"detail\":\"must be logged in to perform that action\"}]}"; +static INTERNAL_ERROR_NO_USER: &str = + "user_id from cookie or token not found in database caused by NotFound"; + +fn call(app: &TestApp, mut request: MockRequest) -> ResponseResult { + app.as_middleware().call(&mut request) +} + +fn into_parts(response: ResponseResult) -> (u32, Vec) { + let mut response = response.unwrap(); + let mut body = Vec::new(); + response.body.write_body(&mut body).unwrap(); + (response.status.0, body) +} + +#[test] +fn anonymous_user_unauthorized() { + let (app, anon) = TestApp::init().empty(); + let request = anon.request_builder(Method::Get, URL); + + let (status, body) = into_parts(call(&app, request)); + assert_eq!(status, 403); + assert_eq!(body, MUST_LOGIN); +} + +#[test] +fn token_auth_cannot_find_token() { + let (app, anon) = TestApp::init().empty(); + let mut request = anon.request_builder(Method::Get, URL); + request.header("Authorization", "fake-token"); + + let (status, body) = into_parts(call(&app, request)); + assert_eq!(status, 403); + assert_eq!(body, MUST_LOGIN); +} + +// Ensure that an unexpected authentication error is available for logging. The user would see +// status 500 instead of 403 as in other authentication tests. Due to foreign-key constraints in +// the database, it is not possible to implement this same test for a token. +#[test] +fn cookie_auth_cannot_find_user() { + let (app, anon) = TestApp::init().empty(); + let mut request = anon.request_builder(Method::Get, URL); + request.mut_extensions().insert(TrustedUserId(-1)); + + let response = call(&app, request); + let log_message = response.map(|_| ()).unwrap_err().to_string(); + assert_eq!(log_message, INTERNAL_ERROR_NO_USER); +} diff --git a/src/tests/util.rs b/src/tests/util.rs index 0e6eb1bc6ea..6a9f4059b36 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -27,7 +27,7 @@ use cargo_registry::{ background_jobs::Environment, db::DieselPool, git::{Credentials, RepositoryConfig}, - middleware::current_user::AuthenticationSource, + middleware::current_user::TrustedUserId, models::{ApiToken, User}, App, Config, }; @@ -195,7 +195,12 @@ impl TestApp { /// Obtain a reference to the inner `App` value pub fn as_inner(&self) -> &App { - &*self.0.app + &self.0.app + } + + /// Obtain a reference to the inner middleware builder + pub fn as_middleware(&self) -> &conduit_middleware::MiddlewareBuilder { + &self.0.middle } } @@ -317,7 +322,7 @@ pub trait RequestHelper { where T: serde::de::DeserializeOwned, { - Response::new(self.app().0.middle.call(&mut request)) + Response::new(self.app().as_middleware().call(&mut request)) } /// Issue a GET request @@ -438,8 +443,8 @@ impl RequestHelper for MockAnonymousUser { /// A type that can generate cookie authenticated requests /// -/// The `User` is directly injected into middleware extensions and thus the cookie logic is not -/// exercised. +/// The `user.id` value is directly injected into a request extension and thus the conduit_cookie +/// session logic is not exercised. pub struct MockCookieUser { app: TestApp, user: User, @@ -448,10 +453,8 @@ pub struct MockCookieUser { impl RequestHelper for MockCookieUser { fn request_builder(&self, method: Method, path: &str) -> MockRequest { let mut request = crate::req(method, path); - request.mut_extensions().insert(self.user.clone()); - request - .mut_extensions() - .insert(AuthenticationSource::SessionCookie); + let id = TrustedUserId(self.user.id); + request.mut_extensions().insert(id); request }