From d6e4a01c884947ce3293d14c4d024f8266a930f4 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Thu, 7 Oct 2021 20:28:34 -0400 Subject: [PATCH] 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)