Fixes from code overview session #5
3 changed files with 29 additions and 28 deletions
6
Makefile
6
Makefile
|
@ -16,9 +16,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
|
|
||||||
|
|
|
@ -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()));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -13,31 +13,38 @@ 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
|
|||||||
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
erika
commented
Do we have a scenario where Do we have a scenario where `row_count` can be greater than 1?
nicole
commented
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.
nicole
commented
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.
|
|||||||
|
Ok(Decision::Allowed)
|
||||||
|
} else {
|
||||||
|
Ok(Decision::Denied)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -46,7 +53,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 +61,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),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue
I like the switch to match here.