From 2c50b4ae2f7ae90ee3734b06cfe29407d93e662b Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Thu, 7 Oct 2021 20:09:05 -0400 Subject: [PATCH 1/2] Add anyhow, Improve Auth token code Moved test helper code to its own module. --- Cargo.lock | 7 ++++ Cargo.toml | 1 + src/auth.rs | 4 ++- src/data/mod.rs | 48 ++++++++++++++++++------- src/database/mod.rs | 75 ++------------------------------------- src/main.rs | 5 ++- src/testhelpers.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 138 insertions(+), 88 deletions(-) create mode 100644 src/testhelpers.rs diff --git a/Cargo.lock b/Cargo.lock index 3052e80..7878924 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -372,6 +372,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "anyhow" +version = "1.0.44" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61604a8f862e1d5c3229fdd78f8b02c68dcf73a4c4b05fd636d12240aaa242c1" + [[package]] name = "async-trait" version = "0.1.50" @@ -1156,6 +1162,7 @@ dependencies = [ "actix-rt", "actix-web", "actix-web-prom", + "anyhow", "bcrypt", "chrono", "diesel", diff --git a/Cargo.toml b/Cargo.toml index 6e95091..7ff382e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,3 +35,4 @@ env_logger="0.8" actix-web-prom = "0.5.1" prometheus = "0.11" lazy_static = "1.1" +anyhow = "1.0" diff --git a/src/auth.rs b/src/auth.rs index d5fac85..5be9209 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -32,6 +32,7 @@ pub async fn login( user_dao: web::Data>, ) -> HttpResponse { debug!("Logging in: {}", creds.username); + if let Some(user) = user_dao.get_user(&creds.username, &creds.password) { let claims = Claims { sub: user.id.to_string(), @@ -43,6 +44,7 @@ pub async fn login( &EncodingKey::from_secret(secret_key().as_bytes()), ) .unwrap(); + HttpResponse::Ok().json(Token { token: &token }) } else { error!( @@ -56,7 +58,7 @@ pub async fn login( #[cfg(test)] mod tests { use super::*; - use crate::database::testhelpers::{BodyReader, TestUserDao}; + use crate::testhelpers::{BodyReader, TestUserDao}; #[actix_rt::test] async fn test_login_reports_200_when_user_exists() { diff --git a/src/data/mod.rs b/src/data/mod.rs index 98e40ac..cdeb4ae 100644 --- a/src/data/mod.rs +++ b/src/data/mod.rs @@ -35,7 +35,7 @@ impl FromStr for Claims { let token = *(s.split("Bearer ").collect::>().last().unwrap_or(&"")); match decode::( - &token, + token, &DecodingKey::from_secret(secret_key().as_bytes()), &Validation::new(Algorithm::HS256), ) { @@ -54,18 +54,27 @@ impl FromRequest for Claims { type Config = (); fn from_request(req: &HttpRequest, _payload: &mut dev::Payload) -> Self::Future { - let claims = match req.headers().get(header::AUTHORIZATION) { - Some(header) => Claims::from_str(header.to_str().unwrap_or("")), - None => Err(jsonwebtoken::errors::Error::from( - jsonwebtoken::errors::ErrorKind::InvalidToken, - )), - }; - - if let Ok(claims) = claims { - ok(claims) - } else { - err(ErrorUnauthorized("Bad token")) - } + req.headers() + .get(header::AUTHORIZATION) + .map_or_else( + || Err(anyhow!("No authorization header")), + |header| { + header + .to_str() + .context("Unable to read Authorization header to string") + }, + ) + .and_then(|header| { + Claims::from_str(header) + .with_context(|| format!("Unable to decode token from: {}", header)) + }) + .map_or_else( + |e| { + error!("{}", e); + err(ErrorUnauthorized("Bad token")) + }, + ok, + ) } } @@ -156,4 +165,17 @@ mod tests { } } } + + #[test] + fn test_junk_token_is_invalid() { + let err = Claims::from_str("uni-֍ՓՓՓՓՓՓՓՓՓՓՓՓՓՓՓ"); + + match err.unwrap_err().into_kind() { + ErrorKind::InvalidToken => assert!(true), + kind => { + println!("Unexpected error: {:?}", kind); + assert!(false) + } + } + } } diff --git a/src/database/mod.rs b/src/database/mod.rs index e29ba6b..aac67ce 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -8,7 +8,7 @@ use std::{ use crate::database::models::{Favorite, InsertFavorite, InsertUser, User}; -mod models; +pub mod models; mod schema; pub trait UserDao { @@ -141,7 +141,7 @@ impl FavoriteDao for SqliteFavoriteDao { diesel::insert_into(favorites) .values(InsertFavorite { userid: &user_id, - path: &favorite_path, + path: favorite_path, }) .execute(connection) .map_err(|_| DbError::new(DbErrorKind::InsertError)) @@ -168,74 +168,3 @@ impl FavoriteDao for SqliteFavoriteDao { .map_err(|_| DbError::new(DbErrorKind::QueryError)) } } - -#[cfg(test)] -pub mod testhelpers { - use actix_web::dev::{Body, ResponseBody}; - - use super::{models::User, UserDao}; - use std::cell::RefCell; - use std::option::Option; - - pub struct TestUserDao { - pub user_map: RefCell>, - } - - impl TestUserDao { - pub fn new() -> Self { - Self { - user_map: RefCell::new(Vec::new()), - } - } - } - - impl UserDao for TestUserDao { - fn create_user(&self, username: &str, password: &str) -> Option { - let u = User { - id: (self.user_map.borrow().len() + 1) as i32, - username: username.to_string(), - password: password.to_string(), - }; - - self.user_map.borrow_mut().push(u.clone()); - - Some(u) - } - - fn get_user(&self, user: &str, pass: &str) -> Option { - match self - .user_map - .borrow() - .iter() - .find(|&u| u.username == user && u.password == pass) - { - Some(u) => { - let copy = (*u).clone(); - Some(copy) - } - None => None, - } - } - - fn user_exists(&self, user: &str) -> bool { - self.user_map - .borrow() - .iter() - .find(|&u| u.username == user) - .is_some() - } - } - - pub trait BodyReader { - fn read_to_str(&self) -> &str; - } - - impl BodyReader for ResponseBody { - fn read_to_str(&self) -> &str { - match self { - ResponseBody::Body(Body::Bytes(ref b)) => std::str::from_utf8(b).unwrap(), - _ => panic!("Unknown response body"), - } - } - } -} diff --git a/src/main.rs b/src/main.rs index d41bf71..0746183 100644 --- a/src/main.rs +++ b/src/main.rs @@ -42,6 +42,9 @@ mod database; mod files; mod video; +#[cfg(test)] +mod testhelpers; + lazy_static! { static ref IMAGE_GAUGE: IntGauge = IntGauge::new( "imageserver_image_total", @@ -190,7 +193,7 @@ async fn generate_video( let filename = name.to_str().expect("Filename should convert to string"); let playlist = format!("tmp/{}.m3u8", filename); if let Some(path) = is_valid_path(&body.path) { - if let Ok(child) = create_playlist(&path.to_str().unwrap(), &playlist).await { + if let Ok(child) = create_playlist(path.to_str().unwrap(), &playlist).await { data.stream_manager .do_send(ProcessMessage(playlist.clone(), child)); } diff --git a/src/testhelpers.rs b/src/testhelpers.rs new file mode 100644 index 0000000..3ba990f --- /dev/null +++ b/src/testhelpers.rs @@ -0,0 +1,86 @@ +use actix_web::dev::{Body, ResponseBody}; +use serde::Deserialize; + +use crate::database::{models::User, UserDao}; +use std::cell::RefCell; +use std::option::Option; + +pub struct TestUserDao { + pub user_map: RefCell>, +} + +impl TestUserDao { + pub fn new() -> Self { + Self { + user_map: RefCell::new(Vec::new()), + } + } +} + +impl UserDao for TestUserDao { + fn create_user(&self, username: &str, password: &str) -> Option { + let u = User { + id: (self.user_map.borrow().len() + 1) as i32, + username: username.to_string(), + password: password.to_string(), + }; + + self.user_map.borrow_mut().push(u.clone()); + + Some(u) + } + + fn get_user(&self, user: &str, pass: &str) -> Option { + match self + .user_map + .borrow() + .iter() + .find(|&u| u.username == user && u.password == pass) + { + Some(u) => { + let copy = (*u).clone(); + Some(copy) + } + None => None, + } + } + + fn user_exists(&self, user: &str) -> bool { + self.user_map + .borrow() + .iter() + .find(|&u| u.username == user) + .is_some() + } +} + +pub trait BodyReader { + fn read_to_str(&self) -> &str; +} + +impl BodyReader for ResponseBody { + fn read_to_str(&self) -> &str { + match self { + ResponseBody::Body(Body::Bytes(ref b)) => std::str::from_utf8(b).unwrap(), + _ => panic!("Unknown response body"), + } + } +} + +pub trait TypedBodyReader<'a, T> +where + T: Deserialize<'a>, +{ + fn read_body(&'a self) -> T; +} + +impl<'a, T: Deserialize<'a>> TypedBodyReader<'a, T> for ResponseBody { + fn read_body(&'a self) -> T { + match self { + ResponseBody::Body(Body::Bytes(ref b)) => { + serde_json::from_str(std::str::from_utf8(b).unwrap()).unwrap() + } + _ => panic!("Unknown response body"), + } + } +} -- 2.49.1 From d6e4a01c884947ce3293d14c4d024f8266a930f4 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Thu, 7 Oct 2021 20:28:34 -0400 Subject: [PATCH 2/2] Move list_photos to files module Added some tests, refactored the error handling/logging, and refactored the extension tests. --- src/data/mod.rs | 10 +- src/files.rs | 255 +++++++++++++++++++++++++++++++++++------------- src/main.rs | 37 +------ 3 files changed, 200 insertions(+), 102 deletions(-) diff --git a/src/data/mod.rs b/src/data/mod.rs index cdeb4ae..1dabfa8 100644 --- a/src/data/mod.rs +++ b/src/data/mod.rs @@ -1,5 +1,7 @@ use std::{fs, str::FromStr}; +use anyhow::{anyhow, Context}; + use chrono::{DateTime, Utc}; use log::error; @@ -78,10 +80,10 @@ impl FromRequest for Claims { } } -#[derive(Serialize)] -pub struct PhotosResponse<'a> { - pub photos: &'a [String], - pub dirs: &'a [String], +#[derive(Serialize, Deserialize, Debug)] +pub struct PhotosResponse { + pub photos: Vec, + pub dirs: Vec, } #[derive(Deserialize)] diff --git a/src/files.rs b/src/files.rs index 8c6acea..022a41b 100644 --- a/src/files.rs +++ b/src/files.rs @@ -1,21 +1,68 @@ use std::fs::read_dir; use std::io; -use std::io::Error; use std::path::{Path, PathBuf}; +use ::anyhow; +use anyhow::{anyhow, Context}; + +use actix_web::web::{HttpResponse, Query}; + +use log::{debug, error}; + +use crate::data::{Claims, PhotosResponse, ThumbnailRequest}; + use path_absolutize::*; +pub async fn list_photos(_: Claims, req: Query) -> HttpResponse { + let path = &req.path; + if let Some(path) = is_valid_path(path) { + debug!("Valid path: {:?}", path); + let files = list_files(&path).unwrap_or_default(); + + let photos = files + .iter() + .filter(|&f| { + f.metadata().map_or_else( + |e| { + error!("Failed getting file metadata: {:?}", e); + false + }, + |md| md.is_file(), + ) + }) + .map(|path: &PathBuf| { + let relative = path + .strip_prefix(dotenv::var("BASE_PATH").unwrap()) + .unwrap(); + relative.to_path_buf() + }) + .map(|f| f.to_str().unwrap().to_string()) + .collect::>(); + + let dirs = files + .iter() + .filter(|&f| f.metadata().map_or(false, |md| md.is_dir())) + .map(|path: &PathBuf| { + let relative = path + .strip_prefix(dotenv::var("BASE_PATH").unwrap()) + .unwrap(); + relative.to_path_buf() + }) + .map(|f| f.to_str().unwrap().to_string()) + .collect::>(); + + HttpResponse::Ok().json(PhotosResponse { photos, dirs }) + } else { + error!("Bad photos request: {}", req.path); + HttpResponse::BadRequest().finish() + } +} + pub fn list_files(dir: &Path) -> io::Result> { let files = read_dir(dir)? - .map(|res| res.unwrap()) + .filter_map(|res| res.ok()) .filter(|entry| is_image_or_video(&entry.path()) || entry.file_type().unwrap().is_dir()) .map(|entry| entry.path()) - .map(|path: PathBuf| { - let relative = path - .strip_prefix(dotenv::var("BASE_PATH").unwrap()) - .unwrap(); - relative.to_path_buf() - }) .collect::>(); Ok(files) @@ -42,29 +89,42 @@ pub fn is_valid_path(path: &str) -> Option { } fn is_valid_full_path(base: &Path, path: &str) -> Option { - let mut path = PathBuf::from(path); - if path.is_relative() { + debug!("Base: {:?}. Path: {}", base, path); + + let path = PathBuf::from(path); + let mut path = if path.is_relative() { let mut full_path = PathBuf::from(base); full_path.push(&path); - is_path_above_base_dir(base, &mut full_path).ok() - } else if let Ok(path) = is_path_above_base_dir(base, &mut path) { - Some(path) + full_path } else { - None + path + }; + + match is_path_above_base_dir(base, &mut path) { + Ok(path) => Some(path), + Err(e) => { + error!("{}", e); + None + } } } -fn is_path_above_base_dir(base: &Path, full_path: &mut PathBuf) -> Result { - full_path.absolutize().and_then(|p| { - if p.starts_with(base) { - Ok(p.into_owned()) - } else { - Err(io::Error::new( - io::ErrorKind::Other, - "Path below base directory", - )) - } - }) +fn is_path_above_base_dir(base: &Path, full_path: &mut PathBuf) -> anyhow::Result { + full_path + .absolutize() + .with_context(|| format!("Unable to resolve absolute path: {:?}", full_path)) + .map_or_else( + |e| Err(anyhow!(e)), + |p| { + if p.starts_with(base) && p.exists() { + Ok(p.into_owned()) + } else if !p.exists() { + Err(anyhow!("Path does not exist: {:?}", p)) + } else { + Err(anyhow!("Path above base directory")) + } + }, + ) } #[cfg(test)] @@ -74,6 +134,77 @@ mod tests { use super::*; + mod api { + use actix_web::{web::Query, HttpResponse}; + + use super::list_photos; + use crate::{ + data::{Claims, PhotosResponse, ThumbnailRequest}, + testhelpers::TypedBodyReader, + }; + + use std::fs; + + fn setup() { + let _ = env_logger::builder().is_test(true).try_init(); + } + + #[actix_rt::test] + async fn test_list_photos() { + setup(); + + let claims = Claims { + sub: String::from("1"), + exp: 12345, + }; + + let request: Query = Query::from_query("path=").unwrap(); + + std::env::set_var("BASE_PATH", "/tmp"); + let mut temp_photo = std::env::temp_dir(); + let mut tmp = temp_photo.clone(); + + tmp.push("test-dir"); + fs::create_dir_all(tmp).unwrap(); + + temp_photo.push("photo.jpg"); + + fs::File::create(temp_photo).unwrap(); + + let response: HttpResponse = list_photos(claims, request).await; + + let body: PhotosResponse = response.body().read_body(); + + assert_eq!(response.status(), 200); + assert!(body.photos.contains(&String::from("photo.jpg"))); + assert!(body.dirs.contains(&String::from("test-dir"))); + assert!(body + .photos + .iter() + .filter(|filename| !filename.ends_with(".png") + && !filename.ends_with(".jpg") + && !filename.ends_with(".jpeg")) + .collect::>() + .is_empty()); + } + + #[actix_rt::test] + async fn test_list_below_base_fails_400() { + setup(); + + let claims = Claims { + sub: String::from("1"), + exp: 12345, + }; + + let request: Query = Query::from_query("path=..").unwrap(); + + let response = list_photos(claims, request).await; + + assert_eq!(response.status(), 400); + } + } + #[test] fn directory_traversal_test() { assert_eq!(None, is_valid_path("../")); @@ -85,22 +216,24 @@ mod tests { } #[test] - fn build_from_relative_path_test() { + fn build_from_path_relative_to_base_test() { let base = env::temp_dir(); let mut test_file = PathBuf::from(&base); test_file.push("test.png"); File::create(test_file).unwrap(); assert!(is_valid_full_path(&base, "test.png").is_some()); + } + + #[test] + fn build_from_relative_returns_none_if_directory_does_not_exist_test() { + let base = env::temp_dir(); let path = "relative/path/test.png"; let mut test_file = PathBuf::from(&base); test_file.push(path); - assert_eq!( - Some(PathBuf::from("/tmp/relative/path/test.png")), - is_valid_full_path(&base, path) - ); + assert_eq!(None, is_valid_full_path(&base, path)); } #[test] @@ -112,51 +245,41 @@ mod tests { assert!(is_valid_full_path(&base, test_file.to_str().unwrap()).is_some()); - let path = "relative/path/test.png"; - let mut test_file = PathBuf::from(&base); - test_file.push(path); - assert_eq!( - Some(PathBuf::from("/tmp/relative/path/test.png")), - is_valid_full_path(&base, path) + Some(PathBuf::from("/tmp/test.png")), + is_valid_full_path(&base, "/tmp/test.png") ); } - #[test] - fn png_valid_extension_test() { - assert!(is_image_or_video(Path::new("image.png"))); - assert!(is_image_or_video(Path::new("image.PNG"))); - assert!(is_image_or_video(Path::new("image.pNg"))); + macro_rules! extension_test { + ($name:ident, $filename:literal) => { + #[test] + fn $name() { + assert!(is_image_or_video(Path::new($filename))); + } + }; } - #[test] - fn jpg_valid_extension_test() { - assert!(is_image_or_video(Path::new("image.jpeg"))); - assert!(is_image_or_video(Path::new("image.JPEG"))); - assert!(is_image_or_video(Path::new("image.jpg"))); - assert!(is_image_or_video(Path::new("image.JPG"))); - } + extension_test!(valid_png, "image.png"); + extension_test!(valid_png_mixed_case, "image.pNg"); + extension_test!(valid_png_upper_case, "image.PNG"); - #[test] - fn mp4_valid_extension_test() { - assert!(is_image_or_video(Path::new("image.mp4"))); - assert!(is_image_or_video(Path::new("image.mP4"))); - assert!(is_image_or_video(Path::new("image.MP4"))); - } + extension_test!(valid_jpeg, "image.jpeg"); + extension_test!(valid_jpeg_upper_case, "image.JPEG"); + extension_test!(valid_jpg, "image.jpg"); + extension_test!(valid_jpg_upper_case, "image.JPG"); - #[test] - fn mov_valid_extension_test() { - assert!(is_image_or_video(Path::new("image.mov"))); - assert!(is_image_or_video(Path::new("image.MOV"))); - assert!(is_image_or_video(Path::new("image.MoV"))); - } + extension_test!(valid_mp4, "image.mp4"); + extension_test!(valid_mp4_mixed_case, "image.mP4"); + extension_test!(valid_mp4_upper_case, "image.MP4"); - #[test] - fn nef_valid_extension_test() { - assert!(is_image_or_video(Path::new("image.nef"))); - assert!(is_image_or_video(Path::new("image.NEF"))); - assert!(is_image_or_video(Path::new("image.NeF"))); - } + extension_test!(valid_mov, "image.mov"); + extension_test!(valid_mov_mixed_case, "image.mOV"); + extension_test!(valid_mov_upper_case, "image.MOV"); + + extension_test!(valid_nef, "image.nef"); + extension_test!(valid_nef_mixed_case, "image.nEF"); + extension_test!(valid_nef_upper_case, "image.NEF"); #[test] fn hidden_file_not_valid_test() { diff --git a/src/main.rs b/src/main.rs index 0746183..545e0b5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,7 +22,7 @@ use actix_web::{ delete, error::BlockingError, get, middleware, post, put, - web::{self, BufMut, BytesMut, HttpRequest, HttpResponse, Query}, + web::{self, BufMut, BytesMut, HttpRequest, HttpResponse}, App, HttpServer, Responder, }; use notify::{watcher, DebouncedEvent, RecursiveMode, Watcher}; @@ -33,7 +33,7 @@ use log::{debug, error, info}; use crate::auth::login; use crate::data::*; use crate::database::*; -use crate::files::{is_image_or_video, is_valid_path, list_files}; +use crate::files::{is_image_or_video, is_valid_path}; use crate::video::*; mod auth; @@ -58,33 +58,6 @@ lazy_static! { .unwrap(); } -#[get("/photos")] -async fn list_photos(_claims: Claims, req: Query) -> impl Responder { - info!("{}", req.path); - - let path = &req.path; - if let Some(path) = is_valid_path(path) { - let files = list_files(&path).unwrap_or_default(); - - let photos = &files - .iter() - .filter(|&f| f.metadata().map_or(false, |md| md.is_file())) - .map(|f| f.to_str().unwrap().to_string()) - .collect::>(); - - let dirs = &files - .iter() - .filter(|&f| f.metadata().map_or(false, |md| md.is_dir())) - .map(|f| f.to_str().unwrap().to_string()) - .collect::>(); - - HttpResponse::Ok().json(PhotosResponse { photos, dirs }) - } else { - error!("Bad photos request: {}", req.path); - HttpResponse::BadRequest().finish() - } -} - #[get("/image")] async fn get_image( _claims: Claims, @@ -258,8 +231,8 @@ async fn favorites( .collect::>(); HttpResponse::Ok().json(PhotosResponse { - photos: &favorites, - dirs: &Vec::new(), + photos: favorites, + dirs: Vec::new(), }) } @@ -491,7 +464,7 @@ fn main() -> std::io::Result<()> { App::new() .wrap(middleware::Logger::default()) .service(web::resource("/login").route(web::post().to(login))) - .service(list_photos) + .service(web::resource("/photos").route(web::get().to(files::list_photos))) .service(get_image) .service(upload_image) .service(generate_video) -- 2.49.1