From 30473344afdc8eb0baef2f914f90f52d69afcf8f Mon Sep 17 00:00:00 2001 From: Nicole Tietz-Sokolskaya Date: Wed, 5 Jun 2024 20:54:06 -0400 Subject: [PATCH 1/3] Fix slop in DB code. Removed extraneous embedded migrations, added explicit relative path to migrations folder, removed hardcoded folder in diesel.toml --- diesel.toml | 2 +- src/db.rs | 2 +- src/server.rs | 3 --- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/diesel.toml b/diesel.toml index be78c73..83d15a9 100644 --- a/diesel.toml +++ b/diesel.toml @@ -6,4 +6,4 @@ file = "src/schema.rs" custom_type_derives = ["diesel::query_builder::QueryId", "Clone"] [migrations_directory] -dir = "/home/nicole/Code/pique/migrations" +dir = "./migrations" diff --git a/src/db.rs b/src/db.rs index cc8dce6..4b4305b 100644 --- a/src/db.rs +++ b/src/db.rs @@ -3,7 +3,7 @@ use diesel::r2d2::{ConnectionManager, Pool}; use diesel::SqliteConnection; use diesel_migrations::{embed_migrations, EmbeddedMigrations, MigrationHarness}; -pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!(); +pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("./migrations/"); /// Establishes a connection to the database using the given URL. /// diff --git a/src/server.rs b/src/server.rs index 490c518..fec9cd5 100644 --- a/src/server.rs +++ b/src/server.rs @@ -5,7 +5,6 @@ use axum::routing::{get, post}; use axum::Router; use axum_login::AuthManagerLayerBuilder; use clap::Parser; -use diesel_migrations::{embed_migrations, EmbeddedMigrations}; use tower_http::services::ServeDir; use tower_http::trace::{DefaultOnRequest, DefaultOnResponse, TraceLayer}; use tower_sessions::SessionManagerLayer; @@ -27,8 +26,6 @@ use crate::logging::setup_logging; use crate::provider::Provider; use crate::templates::make_template_loader; -pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("./migrations/"); - pub async fn run() -> Result<()> { dotenvy::dotenv()?; setup_logging(); -- 2.34.1 From 45b58ed9ab9fc10c9acef555fa3f5d8b38cdbd29 Mon Sep 17 00:00:00 2001 From: Nicole Tietz-Sokolskaya Date: Wed, 5 Jun 2024 22:13:02 -0400 Subject: [PATCH 2/3] Remove unused Makefile commands, and switch from boolean permission checks to an enum --- Makefile | 6 ------ src/handler/documents.rs | 18 +++++++++--------- src/permissions.rs | 33 ++++++++++++++++++++------------- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/Makefile b/Makefile index 658e8d7..8b3ee64 100644 --- a/Makefile +++ b/Makefile @@ -16,9 +16,3 @@ typescript: typescript-watch: npm run build-watch - -migrate: - sea-orm-cli migrate - -entities: - sea-orm-cli generate entity -o src/entity --with-serde both diff --git a/src/handler/documents.rs b/src/handler/documents.rs index 5170155..1281761 100644 --- a/src/handler/documents.rs +++ b/src/handler/documents.rs @@ -7,7 +7,7 @@ use axum_login::AuthSession; use crate::handler::internal_error; use crate::models::documents::{self, NewDocument}; use crate::models::users::User; -use crate::permissions::q::Permission; +use crate::permissions::q::{Decision, Permission}; use crate::permissions::{self}; use crate::prelude::*; @@ -79,7 +79,7 @@ pub async fn create_document_submit( }; let mut db = provider.db_pool.get().map_err(internal_error)?; - let project_allowed = permissions::q::check_user_project( + let access_decision = permissions::q::check_user_project( &mut db, &user.id, &form.project_id.to_string(), @@ -87,7 +87,7 @@ pub async fn create_document_submit( ) .map_err(internal_error)?; - if !project_allowed { + if matches!(access_decision, Decision::Denied) { return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); } @@ -116,11 +116,11 @@ pub async fn view_document_page( let mut db = provider.db_pool.get().map_err(internal_error)?; - let document_allowed = + let access_decision = permissions::q::check_user_document(&mut db, &user.id, &id.to_string(), Permission::Write) .map_err(internal_error)?; - if !document_allowed { + if matches!(access_decision, Decision::Denied) { return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); } @@ -155,11 +155,11 @@ pub async fn edit_document_page( let mut db = provider.db_pool.get().map_err(internal_error)?; - let document_allowed = + let access_decision = permissions::q::check_user_document(&mut db, &user.id, &id.to_string(), Permission::Write) .map_err(internal_error)?; - if !document_allowed { + if matches!(access_decision, Decision::Denied) { return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); } @@ -195,7 +195,7 @@ pub async fn edit_document_submit( let mut db = provider.db_pool.get().map_err(internal_error)?; - let document_allowed = permissions::q::check_user_document( + let access_decision = permissions::q::check_user_document( &mut db, &user.id, &document_id.to_string(), @@ -203,7 +203,7 @@ pub async fn edit_document_submit( ) .map_err(internal_error)?; - if !document_allowed { + if matches!(access_decision, Decision::Denied) { return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); } diff --git a/src/permissions.rs b/src/permissions.rs index a7c64d2..5590545 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -13,31 +13,38 @@ pub mod q { Admin, } + #[derive(Debug, Clone, Copy)] + pub enum Decision { + Allowed, + Denied, + } + pub fn check_user_project( db: &mut SqliteConnection, user_id: &str, project_id: &str, - permission: Permission, - ) -> Result { + requested_permission: Permission, + ) -> Result { use crate::schema::project_memberships::dsl as pm; - if permission == Permission::Admin { - let is_admin = pm::project_memberships + let row_count = match requested_permission { + Permission::Admin => pm::project_memberships .filter(pm::user_id.eq(user_id)) .filter(pm::project_id.eq(project_id)) .filter(pm::role.eq(ProjectRole::Admin.to_string())) .count() - .get_result::(db)?; - - Ok(is_admin > 0) - } else { - let is_member = pm::project_memberships + .get_result::(db)?, + _ => pm::project_memberships .filter(pm::user_id.eq(user_id)) .filter(pm::project_id.eq(project_id)) .count() - .get_result::(db)?; + .get_result::(db)?, + }; - Ok(is_member > 0) + if row_count > 0 { + Ok(Decision::Allowed) + } else { + Ok(Decision::Denied) } } @@ -46,7 +53,7 @@ pub mod q { user_id: &str, document_id: &str, permission: Permission, - ) -> Result { + ) -> Result { use crate::schema::documents::dsl as d; let document = @@ -54,7 +61,7 @@ pub mod q { match document { Some(doc) => check_user_project(db, user_id, &doc.project_id, permission), - None => Ok(false), + None => Ok(Decision::Denied), } } -- 2.34.1 From adb3b3717e80f6e58772e40422afd543d0acf91f Mon Sep 17 00:00:00 2001 From: Nicole Tietz-Sokolskaya Date: Sat, 8 Jun 2024 21:12:12 -0400 Subject: [PATCH 3/3] Add error log if there is more than one project membership for a given user while checking permissions --- Makefile | 6 ++++++ src/permissions.rs | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/Makefile b/Makefile index 8b3ee64..130caa3 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,10 @@ +check: + cargo test + cargo fmt --check + cargo clippy + cargo build --release + run: SECURE_SESSIONS=false RUST_LOG=debug cargo run -- --reload-templates diff --git a/src/permissions.rs b/src/permissions.rs index 5590545..1a0062b 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -42,6 +42,14 @@ pub mod q { }; if row_count > 0 { + if row_count > 1 { + tracing::error!( + row_count = row_count, + user_id = user_id, + project_id = project_id, + "unexpected row count: more than one project membership for this user" + ) + } Ok(Decision::Allowed) } else { Ok(Decision::Denied) -- 2.34.1