Merge pull request 'feature/improve-files-endpoint' (#15) from feature/improve-files-endpoint into master

Reviewed-on: #15

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",

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"

src/auth.rs

@@ -32,6 +32,7 @@ pub async fn login(
     user_dao: web::Data<Box<dyn UserDao>>,
 ) -> 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() {

src/data/mod.rs

@@ -1,5 +1,7 @@
 use std::{fs, str::FromStr};
 
+use anyhow::{anyhow, Context};
+
 use chrono::{DateTime, Utc};
 use log::error;
 
@@ -35,7 +37,7 @@ impl FromStr for Claims {
         let token = *(s.split("Bearer ").collect::<Vec<_>>().last().unwrap_or(&""));
 
         match decode::<Claims>(
-            &token,
+            token,
             &DecodingKey::from_secret(secret_key().as_bytes()),
             &Validation::new(Algorithm::HS256),
         ) {
@@ -54,25 +56,34 @@ 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) {
+        req.headers()
-            Some(header) => Claims::from_str(header.to_str().unwrap_or("")),
+            .get(header::AUTHORIZATION)
-            None => Err(jsonwebtoken::errors::Error::from(
+            .map_or_else(
-                jsonwebtoken::errors::ErrorKind::InvalidToken,
+                || Err(anyhow!("No authorization header")),
-            )),
+                |header| {
-        };
+                    header
-
+                        .to_str()
-        if let Ok(claims) = claims {
+                        .context("Unable to read Authorization header to string")
-            ok(claims)
+                },
-        } else {
+            )
-            err(ErrorUnauthorized("Bad token"))
+            .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,
+            )
     }
 }
 
-#[derive(Serialize)]
+#[derive(Serialize, Deserialize, Debug)]
-pub struct PhotosResponse<'a> {
+pub struct PhotosResponse {
-    pub photos: &'a [String],
+    pub photos: Vec<String>,
-    pub dirs: &'a [String],
+    pub dirs: Vec<String>,
 }
 
 #[derive(Deserialize)]
@@ -156,4 +167,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)
+            }
+        }
+    }
 }

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<Vec<User>>,
-    }
-
-    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<User> {
-            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<User> {
-            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<Body> {
-        fn read_to_str(&self) -> &str {
-            match self {
-                ResponseBody::Body(Body::Bytes(ref b)) => std::str::from_utf8(b).unwrap(),
-                _ => panic!("Unknown response body"),
-            }
-        }
-    }
-}

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<ThumbnailRequest>) -> 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::<Vec<String>>();
+
+        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::<Vec<String>>();
+
+        HttpResponse::Ok().json(PhotosResponse { photos, dirs })
+    } else {
+        error!("Bad photos request: {}", req.path);
+        HttpResponse::BadRequest().finish()
+    }
+}
+
 pub fn list_files(dir: &Path) -> io::Result<Vec<PathBuf>> {
     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::<Vec<PathBuf>>();
 
     Ok(files)
@@ -42,29 +89,42 @@ pub fn is_valid_path(path: &str) -> Option<PathBuf> {
 }
 
 fn is_valid_full_path(base: &Path, path: &str) -> Option<PathBuf> {
-    let mut path = PathBuf::from(path);
+    debug!("Base: {:?}. Path: {}", base, path);
-    if path.is_relative() {
+
+    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()
+        full_path
-    } else if let Ok(path) = is_path_above_base_dir(base, &mut path) {
-        Some(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<PathBuf, Error> {
+fn is_path_above_base_dir(base: &Path, full_path: &mut PathBuf) -> anyhow::Result<PathBuf> {
-    full_path.absolutize().and_then(|p| {
+    full_path
-        if p.starts_with(base) {
+        .absolutize()
-            Ok(p.into_owned())
+        .with_context(|| format!("Unable to resolve absolute path: {:?}", full_path))
-        } else {
+        .map_or_else(
-            Err(io::Error::new(
+            |e| Err(anyhow!(e)),
-                io::ErrorKind::Other,
+            |p| {
-                "Path below base directory",
+                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<ThumbnailRequest> = 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::<Vec<&String>>()
+                .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<ThumbnailRequest> = 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!(
+        assert_eq!(None, is_valid_full_path(&base, path));
-            Some(PathBuf::from("/tmp/relative/path/test.png")),
-            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")),
+            Some(PathBuf::from("/tmp/test.png")),
-            is_valid_full_path(&base, path)
+            is_valid_full_path(&base, "/tmp/test.png")
         );
     }
 
-    #[test]
+    macro_rules! extension_test {
-    fn png_valid_extension_test() {
+        ($name:ident, $filename:literal) => {
-        assert!(is_image_or_video(Path::new("image.png")));
+            #[test]
-        assert!(is_image_or_video(Path::new("image.PNG")));
+            fn $name() {
-        assert!(is_image_or_video(Path::new("image.pNg")));
+                assert!(is_image_or_video(Path::new($filename)));
+            }
+        };
     }
 
-    #[test]
+    extension_test!(valid_png, "image.png");
-    fn jpg_valid_extension_test() {
+    extension_test!(valid_png_mixed_case, "image.pNg");
-        assert!(is_image_or_video(Path::new("image.jpeg")));
+    extension_test!(valid_png_upper_case, "image.PNG");
-        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")));
-    }
 
-    #[test]
+    extension_test!(valid_jpeg, "image.jpeg");
-    fn mp4_valid_extension_test() {
+    extension_test!(valid_jpeg_upper_case, "image.JPEG");
-        assert!(is_image_or_video(Path::new("image.mp4")));
+    extension_test!(valid_jpg, "image.jpg");
-        assert!(is_image_or_video(Path::new("image.mP4")));
+    extension_test!(valid_jpg_upper_case, "image.JPG");
-        assert!(is_image_or_video(Path::new("image.MP4")));
-    }
 
-    #[test]
+    extension_test!(valid_mp4, "image.mp4");
-    fn mov_valid_extension_test() {
+    extension_test!(valid_mp4_mixed_case, "image.mP4");
-        assert!(is_image_or_video(Path::new("image.mov")));
+    extension_test!(valid_mp4_upper_case, "image.MP4");
-        assert!(is_image_or_video(Path::new("image.MOV")));
-        assert!(is_image_or_video(Path::new("image.MoV")));
-    }
 
-    #[test]
+    extension_test!(valid_mov, "image.mov");
-    fn nef_valid_extension_test() {
+    extension_test!(valid_mov_mixed_case, "image.mOV");
-        assert!(is_image_or_video(Path::new("image.nef")));
+    extension_test!(valid_mov_upper_case, "image.MOV");
-        assert!(is_image_or_video(Path::new("image.NEF")));
+
-        assert!(is_image_or_video(Path::new("image.NeF")));
+    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() {

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;
@@ -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",
@@ -55,33 +58,6 @@ lazy_static! {
     .unwrap();
 }
 
-#[get("/photos")]
-async fn list_photos(_claims: Claims, req: Query<ThumbnailRequest>) -> 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::<Vec<String>>();
-
-        let dirs = &files
-            .iter()
-            .filter(|&f| f.metadata().map_or(false, |md| md.is_dir()))
-            .map(|f| f.to_str().unwrap().to_string())
-            .collect::<Vec<String>>();
-
-        HttpResponse::Ok().json(PhotosResponse { photos, dirs })
-    } else {
-        error!("Bad photos request: {}", req.path);
-        HttpResponse::BadRequest().finish()
-    }
-}
-
 #[get("/image")]
 async fn get_image(
     _claims: Claims,
@@ -190,7 +166,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));
             }
@@ -255,8 +231,8 @@ async fn favorites(
             .collect::<Vec<String>>();
 
     HttpResponse::Ok().json(PhotosResponse {
-        photos: &favorites,
+        photos: favorites,
-        dirs: &Vec::new(),
+        dirs: Vec::new(),
     })
 }
 
@@ -488,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)

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<Vec<User>>,
+}
+
+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<User> {
+        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<User> {
+        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<Body> {
+    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<Body> {
+    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"),
+        }
+    }
+}