From 72e41b99a1447320d7bae1caffea7170b17f1735 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Thu, 25 Feb 2021 14:42:23 -0500 Subject: [PATCH 1/5] Remove Actix CORS dependency --- Cargo.lock | 15 --------------- Cargo.toml | 1 - 2 files changed, 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0be1048..737636c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -60,20 +60,6 @@ dependencies = [ "trust-dns-resolver", ] -[[package]] -name = "actix-cors" -version = "0.5.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "36b133d8026a9f209a9aeeeacd028e7451bcca975f592881b305d37983f303d7" -dependencies = [ - "actix-web", - "derive_more", - "futures-util", - "log", - "once_cell", - "tinyvec", -] - [[package]] name = "actix-files" version = "0.5.0" @@ -1200,7 +1186,6 @@ name = "image-api" version = "0.1.0" dependencies = [ "actix", - "actix-cors", "actix-files", "actix-multipart", "actix-rt 2.1.0", diff --git a/Cargo.toml b/Cargo.toml index e5d6431..f0ee5a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,6 @@ actix-web = "3" actix-rt = "2" actix-files = "0.5" actix-multipart = "0.3.0" -actix-cors = "0.5" futures = "0.3.5" jsonwebtoken = "7.2.0" serde = "1" From 64bfb58734201176d9ff6c085e426f6d079cbec4 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Thu, 25 Feb 2021 17:42:16 -0500 Subject: [PATCH 2/5] Log when unknown user tries to log in --- src/auth.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/auth.rs b/src/auth.rs index 818da18..b6e7d81 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -2,7 +2,7 @@ use actix_web::web::{HttpResponse, Json}; use actix_web::{post, Responder}; use chrono::{Duration, Utc}; use jsonwebtoken::{encode, EncodingKey, Header}; -use log::debug; +use log::{debug, error}; use crate::data::LoginRequest; use crate::data::{secret_key, Claims, CreateAccountRequest, Token}; @@ -39,6 +39,7 @@ async fn login(creds: Json) -> impl Responder { .unwrap(); HttpResponse::Ok().json(Token { token: &token }) } else { + error!("User not found during login: '{}'", creds.username); HttpResponse::NotFound().finish() } } From e5ad88abd6f3d492c7ca2c6e32829caba7575f49 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Fri, 26 Feb 2021 08:33:02 -0500 Subject: [PATCH 3/5] Create UserDao and unit tests for login --- src/auth.rs | 78 +++++++++++++++--- src/database/mod.rs | 190 +++++++++++++++++++++++++++++++++----------- src/main.rs | 5 +- 3 files changed, 214 insertions(+), 59 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index b6e7d81..4c019b7 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1,19 +1,23 @@ -use actix_web::web::{HttpResponse, Json}; +use actix_web::web::{self, HttpResponse, Json}; use actix_web::{post, Responder}; use chrono::{Duration, Utc}; use jsonwebtoken::{encode, EncodingKey, Header}; use log::{debug, error}; -use crate::data::LoginRequest; -use crate::data::{secret_key, Claims, CreateAccountRequest, Token}; -use crate::database::{create_user, get_user, user_exists}; +use crate::{ + data::{secret_key, Claims, CreateAccountRequest, LoginRequest, Token}, + database::UserDao, +}; #[post("/register")] -async fn register(user: Json) -> impl Responder { +async fn register( + user: Json, + user_dao: web::Data>, +) -> impl Responder { if !user.username.is_empty() && user.password.len() > 5 && user.password == user.confirmation { - if user_exists(&user.username) { + if user_dao.user_exists(&user.username) { HttpResponse::BadRequest() - } else if let Some(_user) = create_user(&user.username, &user.password) { + } else if let Some(_user) = user_dao.create_user(&user.username, &user.password) { HttpResponse::Ok() } else { HttpResponse::InternalServerError() @@ -23,10 +27,12 @@ async fn register(user: Json) -> impl Responder { } } -#[post("/login")] -async fn login(creds: Json) -> impl Responder { +pub async fn login( + creds: Json, + user_dao: web::Data>, +) -> HttpResponse { debug!("Logging in: {}", creds.username); - if let Some(user) = get_user(&creds.username, &creds.password) { + if let Some(user) = user_dao.get_user(&creds.username, &creds.password) { let claims = Claims { sub: user.id.to_string(), exp: (Utc::now() + Duration::days(5)).timestamp(), @@ -43,3 +49,55 @@ async fn login(creds: Json) -> impl Responder { HttpResponse::NotFound().finish() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::database::testhelpers::{BodyReader, TestUserDao}; + + #[actix_rt::test] + async fn test_login_reports_200_when_user_exists() { + let dao = TestUserDao::new(); + dao.create_user("user", "pass"); + + let j = Json(LoginRequest { + username: "user".to_string(), + password: "pass".to_string(), + }); + + let response = login(j, web::Data::new(Box::new(dao))).await; + + assert_eq!(response.status(), 200); + } + + #[actix_rt::test] + async fn test_login_returns_token_on_success() { + let dao = TestUserDao::new(); + dao.create_user("user", "password"); + + let j = Json(LoginRequest { + username: "user".to_string(), + password: "password".to_string(), + }); + + let response = login(j, web::Data::new(Box::new(dao))).await; + + assert_eq!(response.status(), 200); + assert!(response.body().read_to_str().contains("\"token\"")); + } + + #[actix_rt::test] + async fn test_login_reports_400_when_user_does_not_exist() { + let dao = TestUserDao::new(); + dao.create_user("user", "password"); + + let j = Json(LoginRequest { + username: "doesnotexist".to_string(), + password: "password".to_string(), + }); + + let response = login(j, web::Data::new(Box::new(dao))).await; + + assert_eq!(response.status(), 404); + } +} diff --git a/src/database/mod.rs b/src/database/mod.rs index 22a9dc1..96f5915 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -1,72 +1,88 @@ use bcrypt::{hash, verify, DEFAULT_COST}; use diesel::prelude::*; use diesel::sqlite::SqliteConnection; -use dotenv::dotenv; use crate::database::models::{Favorite, InsertFavorite, InsertUser, User}; mod models; mod schema; -fn connect() -> SqliteConnection { - dotenv().ok(); - - let db_url = dotenv::var("DATABASE_URL").expect("DATABASE_URL must be set"); - SqliteConnection::establish(&db_url).expect("Error connecting to DB") +pub trait UserDao { + fn create_user(&self, user: &str, password: &str) -> Option; + fn get_user(&self, user: &str, password: &str) -> Option; + fn user_exists(&self, user: &str) -> bool; } -// TODO: Should probably use Result here -pub fn create_user(user: &str, pass: &str) -> Option { - use schema::users::dsl::*; +pub struct SqliteUserDao { + connection: SqliteConnection, +} - let hashed = hash(pass, DEFAULT_COST); - if let Ok(hash) = hashed { - let connection = connect(); - diesel::insert_into(users) - .values(InsertUser { - username: user, - password: &hash, - }) - .execute(&connection) - .unwrap(); +impl SqliteUserDao { + pub fn new() -> Self { + Self { + connection: connect(), + } + } +} + +impl UserDao for SqliteUserDao { + // TODO: Should probably use Result here + fn create_user(&self, user: &str, pass: &str) -> std::option::Option { + use schema::users::dsl::*; + + let hashed = hash(pass, DEFAULT_COST); + if let Ok(hash) = hashed { + diesel::insert_into(users) + .values(InsertUser { + username: user, + password: &hash, + }) + .execute(&self.connection) + .unwrap(); + + match users + .filter(username.eq(username)) + .load::(&self.connection) + .unwrap() + .first() + { + Some(u) => Some(u.clone()), + None => None, + } + } else { + None + } + } + + fn get_user(&self, user: &str, pass: &str) -> Option { + use schema::users::dsl::*; match users .filter(username.eq(user)) - .load::(&connection) - .unwrap() + .load::(&self.connection) + .unwrap_or_default() .first() { - Some(u) => Some(u.clone()), - None => None, + Some(u) if verify(pass, &u.password).unwrap_or(false) => Some(u.clone()), + _ => None, } - } else { - None + } + + fn user_exists(&self, user: &str) -> bool { + use schema::users::dsl::*; + + users + .filter(username.eq(user)) + .load::(&self.connection) + .unwrap_or_default() + .first() + .is_some() } } -pub fn get_user(user: &str, pass: &str) -> Option { - use schema::users::dsl::*; - - match users - .filter(username.eq(user)) - .load::(&connect()) - .unwrap_or_default() - .first() - { - Some(u) if verify(pass, &u.password).unwrap_or(false) => Some(u.clone()), - _ => None, - } -} - -pub fn user_exists(name: &str) -> bool { - use schema::users::dsl::*; - - users - .filter(username.eq(name)) - .load::(&connect()) - .unwrap_or_default() - .first() - .is_some() +fn connect() -> SqliteConnection { + let db_url = dotenv::var("DATABASE_URL").expect("DATABASE_URL must be set"); + SqliteConnection::establish(&db_url).expect("Error connecting to DB") } pub fn add_favorite(user_id: i32, favorite_path: String) { @@ -90,3 +106,81 @@ pub fn get_favorites(user_id: i32) -> Vec { .load::(&connect()) .unwrap_or_default() } + +#[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() + .filter(|u| u.username == user && u.password == pass) + .collect::>() + .first() + { + Some(u) => { + let copy = (*u).clone(); + Some(copy) + } + None => None, + } + } + + fn user_exists(&self, user: &str) -> bool { + self.user_map + .borrow() + .iter() + .filter(|u| u.username == user) + .collect::>() + .first() + .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) => match body { + Body::Bytes(b) => std::str::from_utf8(&b).unwrap(), + _ => panic!("Unknown response body content"), + }, + _ => panic!("Unknown response body"), + } + } + } +} diff --git a/src/main.rs b/src/main.rs index 3549230..fb7e310 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,6 +3,7 @@ extern crate diesel; extern crate rayon; use crate::auth::login; +use database::{SqliteUserDao, UserDao}; use futures::stream::StreamExt; use std::fs::File; use std::io::prelude::*; @@ -328,8 +329,9 @@ fn main() -> std::io::Result<()> { }); HttpServer::new(move || { + let user_dao = SqliteUserDao::new(); App::new() - .service(login) + .service(web::resource("/login").route(web::post().to(login))) .service(list_photos) .service(get_image) .service(upload_image) @@ -339,6 +341,7 @@ fn main() -> std::io::Result<()> { .service(favorites) .service(post_add_favorite) .app_data(app_data.clone()) + .data::>(Box::new(user_dao)) }) .bind(dotenv::var("BIND_URL").unwrap())? .bind("localhost:8088")? From 1c7e54d3557445876dea9e2e6f89770bbe8e06cb Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sat, 27 Feb 2021 11:53:29 -0500 Subject: [PATCH 4/5] Make playlist generation async This should allow other requests to be answered while we wait for ffmpeg to do its thing. --- Cargo.lock | 1 - Cargo.toml | 1 - src/main.rs | 7 +++++-- src/video.rs | 6 +++--- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 737636c..d444578 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1206,7 +1206,6 @@ dependencies = [ "serde", "serde_json", "sha2", - "tokio 1.2.0", "walkdir", ] diff --git a/Cargo.toml b/Cargo.toml index f0ee5a5..a5c2f64 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,6 @@ image = "0.23.7" walkdir = "2" rayon = "1.3" notify = "4.0" -tokio = "1" path-absolutize = "3.0.6" log="0.4" env_logger="0.8" diff --git a/src/main.rs b/src/main.rs index fb7e310..70cd111 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14,8 +14,11 @@ use std::sync::Arc; use actix::{Actor, Addr}; use actix_files::NamedFile; use actix_multipart as mp; -use actix_web::web::{HttpRequest, HttpResponse, Json}; use actix_web::{get, post, web, App, HttpServer, Responder}; +use actix_web::{ + middleware, + web::{HttpRequest, HttpResponse, Json}, +}; use notify::{watcher, DebouncedEvent, RecursiveMode, Watcher}; use rayon::prelude::*; use serde::Serialize; @@ -157,7 +160,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) { + 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/video.rs b/src/video.rs index 4395e66..c143621 100644 --- a/src/video.rs +++ b/src/video.rs @@ -39,7 +39,7 @@ impl Handler for StreamActor { } } -pub fn create_playlist(video_path: &str, playlist_file: &str) -> Result { +pub async fn create_playlist(video_path: &str, playlist_file: &str) -> Result { if Path::new(playlist_file).exists() { debug!("Playlist already exists: {}", playlist_file); return Err(std::io::Error::from(std::io::ErrorKind::AlreadyExists)); @@ -51,7 +51,7 @@ pub fn create_playlist(video_path: &str, playlist_file: &str) -> Result { .arg("-c:v") .arg("h264") .arg("-crf") - .arg("23") + .arg("21") .arg("-preset") .arg("veryfast") .arg("-hls_time") @@ -67,7 +67,7 @@ pub fn create_playlist(video_path: &str, playlist_file: &str) -> Result { let start_time = std::time::Instant::now(); loop { - std::thread::sleep(std::time::Duration::from_secs(1)); + actix::clock::delay_for(std::time::Duration::from_secs(1)).await; if Path::new(playlist_file).exists() || std::time::Instant::now() - start_time > std::time::Duration::from_secs(5) From e5eb2d9c1f6869eb05081248876b87c16d29a4de Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sat, 27 Feb 2021 11:54:04 -0500 Subject: [PATCH 5/5] Add info level request logging --- src/main.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main.rs b/src/main.rs index 70cd111..1790d6d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -334,6 +334,7 @@ fn main() -> std::io::Result<()> { HttpServer::new(move || { let user_dao = SqliteUserDao::new(); App::new() + .wrap(middleware::Logger::default()) .service(web::resource("/login").route(web::post().to(login))) .service(list_photos) .service(get_image)