Fixes from code overview session #5

Merged
nicole merged 3 commits from code-review-fixes into main 2024-06-09 01:17:55 +00:00
6 changed files with 45 additions and 33 deletions

View file

@ -1,4 +1,10 @@
check:
cargo test
cargo fmt --check
cargo clippy
cargo build --release
run: run:
SECURE_SESSIONS=false RUST_LOG=debug cargo run -- --reload-templates SECURE_SESSIONS=false RUST_LOG=debug cargo run -- --reload-templates
@ -16,9 +22,3 @@ typescript:
typescript-watch: typescript-watch:
npm run build-watch npm run build-watch
migrate:
sea-orm-cli migrate
entities:
sea-orm-cli generate entity -o src/entity --with-serde both

View file

@ -6,4 +6,4 @@ file = "src/schema.rs"
custom_type_derives = ["diesel::query_builder::QueryId", "Clone"] custom_type_derives = ["diesel::query_builder::QueryId", "Clone"]
[migrations_directory] [migrations_directory]
dir = "/home/nicole/Code/pique/migrations" dir = "./migrations"

View file

@ -3,7 +3,7 @@ use diesel::r2d2::{ConnectionManager, Pool};
use diesel::SqliteConnection; use diesel::SqliteConnection;
use diesel_migrations::{embed_migrations, EmbeddedMigrations, MigrationHarness}; 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. /// Establishes a connection to the database using the given URL.
/// ///

View file

@ -7,7 +7,7 @@ use axum_login::AuthSession;
use crate::handler::internal_error; use crate::handler::internal_error;
use crate::models::documents::{self, NewDocument}; use crate::models::documents::{self, NewDocument};
use crate::models::users::User; use crate::models::users::User;
use crate::permissions::q::Permission; use crate::permissions::q::{Decision, Permission};
use crate::permissions::{self}; use crate::permissions::{self};
use crate::prelude::*; 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 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, &mut db,
&user.id, &user.id,
&form.project_id.to_string(), &form.project_id.to_string(),
@ -87,7 +87,7 @@ pub async fn create_document_submit(
) )
.map_err(internal_error)?; .map_err(internal_error)?;
if !project_allowed { if matches!(access_decision, Decision::Denied) {
return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); 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 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) permissions::q::check_user_document(&mut db, &user.id, &id.to_string(), Permission::Write)
.map_err(internal_error)?; .map_err(internal_error)?;
if !document_allowed { if matches!(access_decision, Decision::Denied) {
return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); 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 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) permissions::q::check_user_document(&mut db, &user.id, &id.to_string(), Permission::Write)
.map_err(internal_error)?; .map_err(internal_error)?;
if !document_allowed { if matches!(access_decision, Decision::Denied) {
return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); 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 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, &mut db,
&user.id, &user.id,
&document_id.to_string(), &document_id.to_string(),
@ -203,7 +203,7 @@ pub async fn edit_document_submit(
) )
.map_err(internal_error)?; .map_err(internal_error)?;
if !document_allowed { if matches!(access_decision, Decision::Denied) {
return Err((StatusCode::FORBIDDEN, "permission denied".to_owned())); return Err((StatusCode::FORBIDDEN, "permission denied".to_owned()));
} }

View file

@ -13,31 +13,46 @@ pub mod q {
Admin, Admin,
} }
#[derive(Debug, Clone, Copy)]
pub enum Decision {
Allowed,
Denied,
}
pub fn check_user_project( pub fn check_user_project(
db: &mut SqliteConnection, db: &mut SqliteConnection,
user_id: &str, user_id: &str,
project_id: &str, project_id: &str,
permission: Permission, requested_permission: Permission,
) -> Result<bool, diesel::result::Error> { ) -> Result<Decision, diesel::result::Error> {
use crate::schema::project_memberships::dsl as pm; use crate::schema::project_memberships::dsl as pm;
if permission == Permission::Admin { let row_count = match requested_permission {
erika marked this conversation as resolved
Review

I like the switch to match here.

I like the switch to match here.
let is_admin = pm::project_memberships Permission::Admin => pm::project_memberships
.filter(pm::user_id.eq(user_id)) .filter(pm::user_id.eq(user_id))
.filter(pm::project_id.eq(project_id)) .filter(pm::project_id.eq(project_id))
.filter(pm::role.eq(ProjectRole::Admin.to_string())) .filter(pm::role.eq(ProjectRole::Admin.to_string()))
.count() .count()
.get_result::<i64>(db)?; .get_result::<i64>(db)?,
_ => pm::project_memberships
Ok(is_admin > 0)
} else {
let is_member = pm::project_memberships
.filter(pm::user_id.eq(user_id)) .filter(pm::user_id.eq(user_id))
.filter(pm::project_id.eq(project_id)) .filter(pm::project_id.eq(project_id))
.count() .count()
.get_result::<i64>(db)?; .get_result::<i64>(db)?,
};
Ok(is_member > 0) if row_count > 0 {
erika marked this conversation as resolved
Review

Do we have a scenario where row_count can be greater than 1?

Do we have a scenario where `row_count` can be greater than 1?
Review

I don't think so. There's no constraint in the database to make it so that users are only members of a project once, but I can't imagine why we'd do that on purpose. It could be a good place to put an error log if the row count is over 1, while continuing to work.

I don't think so. There's no constraint in the database to make it so that users are only members of a project once, but I can't imagine why we'd do that on purpose. It could be a good place to put an error log if the row count is over 1, while continuing to work.
Review

I added an error log. Open to changing it, erring on the side of merging.

I added an error log. Open to changing it, erring on the side of merging.
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)
} }
} }
@ -46,7 +61,7 @@ pub mod q {
user_id: &str, user_id: &str,
document_id: &str, document_id: &str,
permission: Permission, permission: Permission,
) -> Result<bool, diesel::result::Error> { ) -> Result<Decision, diesel::result::Error> {
use crate::schema::documents::dsl as d; use crate::schema::documents::dsl as d;
let document = let document =
@ -54,7 +69,7 @@ pub mod q {
match document { match document {
Some(doc) => check_user_project(db, user_id, &doc.project_id, permission), Some(doc) => check_user_project(db, user_id, &doc.project_id, permission),
None => Ok(false), None => Ok(Decision::Denied),
} }
} }

View file

@ -5,7 +5,6 @@ use axum::routing::{get, post};
use axum::Router; use axum::Router;
use axum_login::AuthManagerLayerBuilder; use axum_login::AuthManagerLayerBuilder;
use clap::Parser; use clap::Parser;
use diesel_migrations::{embed_migrations, EmbeddedMigrations};
use tower_http::services::ServeDir; use tower_http::services::ServeDir;
use tower_http::trace::{DefaultOnRequest, DefaultOnResponse, TraceLayer}; use tower_http::trace::{DefaultOnRequest, DefaultOnResponse, TraceLayer};
use tower_sessions::SessionManagerLayer; use tower_sessions::SessionManagerLayer;
@ -27,8 +26,6 @@ use crate::logging::setup_logging;
use crate::provider::Provider; use crate::provider::Provider;
use crate::templates::make_template_loader; use crate::templates::make_template_loader;
pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("./migrations/");
pub async fn run() -> Result<()> { pub async fn run() -> Result<()> {
dotenvy::dotenv()?; dotenvy::dotenv()?;
setup_logging(); setup_logging();