Fixes from code overview session (#5)

A collection of small changes:

- Switch from using a boolean for permission check results to using an enum (allowed or denied)
- Remove some unused Makefile commands left over from a previous migration, add a check command
- Change some hardcoded paths to be relative paths
- Remove unused embed_migrations (duplicated)
- Log error when checking permissions if a user belongs to a project multiple times

Reviewed-on: #5

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
 
@@ -16,9 +22,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

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"

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.
 ///

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()));
     }
 

src/permissions.rs

@@ -13,31 +13,46 @@ 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<bool, diesel::result::Error> {
+        requested_permission: Permission,
+    ) -> Result<Decision, diesel::result::Error> {
         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::<i64>(db)?;
-
-            Ok(is_admin > 0)
-        } else {
-            let is_member = pm::project_memberships
+                .get_result::<i64>(db)?,
+            _ => pm::project_memberships
                 .filter(pm::user_id.eq(user_id))
                 .filter(pm::project_id.eq(project_id))
                 .count()
-                .get_result::<i64>(db)?;
+                .get_result::<i64>(db)?,
+        };
 
-            Ok(is_member > 0)
+        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)
         }
     }
 
@@ -46,7 +61,7 @@ pub mod q {
         user_id: &str,
         document_id: &str,
         permission: Permission,
-    ) -> Result<bool, diesel::result::Error> {
+    ) -> Result<Decision, diesel::result::Error> {
         use crate::schema::documents::dsl as d;
 
         let document =
@@ -54,7 +69,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),
         }
     }
 

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();