diff --git a/domain/Cargo.toml b/domain/Cargo.toml index 1cf507f2..fe98f67d 100644 --- a/domain/Cargo.toml +++ b/domain/Cargo.toml @@ -23,5 +23,8 @@ features = ["debug-print", "runtime-tokio-native-tls", "sqlx-postgres"] mockito = "1.6" serial_test = "3.1" +[features] +mock = ["entity_api/mock"] + diff --git a/domain/src/coaching_relationship.rs b/domain/src/coaching_relationship.rs index 29a4190b..08d8b214 100644 --- a/domain/src/coaching_relationship.rs +++ b/domain/src/coaching_relationship.rs @@ -1,12 +1,13 @@ use crate::coaching_relationships::Model; -use crate::error::Error; +use crate::error::{DomainErrorKind, EntityErrorKind, Error, InternalErrorKind}; use entity_api::query::{IntoQueryFilterMap, QuerySort}; use entity_api::{coaching_relationships, query}; -use sea_orm::DatabaseConnection; +use sea_orm::{DatabaseConnection, TransactionTrait}; pub use entity_api::coaching_relationship::{ create, find_by_id, find_by_organization_with_user_names, find_by_user, - get_relationship_with_user_names, CoachingRelationshipWithUserNames, + find_by_user_and_organization_with_user_names, get_relationship_with_user_names, + CoachingRelationshipWithUserNames, }; pub async fn find_by

(db: &DatabaseConnection, params: P) -> Result, Error> @@ -20,3 +21,54 @@ where .await?; Ok(coaching_relationships) } + +/// Finds coaching relationships for a user within an organization, respecting role-based access. +/// +/// - SuperAdmins (global role with organization_id = NULL) see all relationships in the organization +/// - Admins (organization-specific role) see all relationships in their organization +/// - Regular users see only relationships where they are the coach or coachee +/// +/// This function uses a database transaction to prevent Time-of-Check to Time-of-Use (TOCTOU) +/// vulnerabilities by ensuring the role check and data retrieval happen atomically within +/// a consistent database snapshot. +pub async fn find_by_organization_for_user_with_user_names( + db: &DatabaseConnection, + user_id: crate::Id, + organization_id: crate::Id, +) -> Result, Error> { + // Begin transaction to ensure atomicity and prevent TOCTOU vulnerabilities + let txn = db.begin().await.map_err(|e| Error { + source: Some(Box::new(e)), + error_kind: DomainErrorKind::Internal(InternalErrorKind::Entity( + EntityErrorKind::DbTransaction, + )), + })?; + + // Check if user has admin access using entity_api layer + let is_admin = entity_api::user::has_admin_access(&txn, user_id, organization_id) + .await + .map_err(|e| Error { + source: Some(Box::new(e)), + error_kind: DomainErrorKind::Internal(InternalErrorKind::Entity( + EntityErrorKind::DbTransaction, + )), + })?; + + let coaching_relationships = if is_admin { + // Admin users see all relationships in the organization + find_by_organization_with_user_names(&txn, organization_id).await? + } else { + // Regular users see only relationships they're associated with (as coach or coachee) + find_by_user_and_organization_with_user_names(&txn, user_id, organization_id).await? + }; + + // Commit transaction + txn.commit().await.map_err(|e| Error { + source: Some(Box::new(e)), + error_kind: DomainErrorKind::Internal(InternalErrorKind::Entity( + EntityErrorKind::DbTransaction, + )), + })?; + + Ok(coaching_relationships) +} diff --git a/entity_api/src/coaching_relationship.rs b/entity_api/src/coaching_relationship.rs index c603e852..91c36f9f 100644 --- a/entity_api/src/coaching_relationship.rs +++ b/entity_api/src/coaching_relationship.rs @@ -125,7 +125,7 @@ pub async fn find_by_organization( } pub async fn find_by_organization_with_user_names( - db: &DatabaseConnection, + db: &impl ConnectionTrait, organization_id: Id, ) -> Result, Error> { let coaches = Alias::new("coaches"); @@ -159,6 +159,47 @@ pub async fn find_by_organization_with_user_names( Ok(query.all(db).await?) } +pub async fn find_by_user_and_organization_with_user_names( + db: &impl ConnectionTrait, + user_id: Id, + organization_id: Id, +) -> Result, Error> { + let coaches = Alias::new("coaches"); + let coachees = Alias::new("coachees"); + + let query = by_organization(coaching_relationships::Entity::find(), organization_id) + .await + .filter( + Condition::any() + .add(coaching_relationships::Column::CoachId.eq(user_id)) + .add(coaching_relationships::Column::CoacheeId.eq(user_id)), + ) + .join_as( + JoinType::Join, + coaches::Relation::CoachingRelationships.def().rev(), + coaches.clone(), + ) + .join_as( + JoinType::Join, + coachees::Relation::CoachingRelationships.def().rev(), + coachees.clone(), + ) + .select_only() + .column(coaching_relationships::Column::Id) + .column(coaching_relationships::Column::OrganizationId) + .column(coaching_relationships::Column::CoachId) + .column(coaching_relationships::Column::CoacheeId) + .column(coaching_relationships::Column::CreatedAt) + .column(coaching_relationships::Column::UpdatedAt) + .column_as(Expr::cust("coaches.first_name"), "coach_first_name") + .column_as(Expr::cust("coaches.last_name"), "coach_last_name") + .column_as(Expr::cust("coachees.first_name"), "coachee_first_name") + .column_as(Expr::cust("coachees.last_name"), "coachee_last_name") + .into_model::(); + + Ok(query.all(db).await?) +} + pub async fn get_relationship_with_user_names( db: &DatabaseConnection, relationship_id: Id, @@ -237,7 +278,7 @@ pub async fn delete_by_user_id(db: &impl ConnectionTrait, user_id: Id) -> Result // A convenient combined struct that holds the results of looking up the Users associated // with the coach/coachee ids. This should be used as an implementation detail only. -#[derive(FromQueryResult, Debug, PartialEq)] +#[derive(FromQueryResult, Debug, PartialEq, Clone)] pub struct CoachingRelationshipWithUserNames { pub id: Id, pub coach_id: Id, diff --git a/entity_api/src/user.rs b/entity_api/src/user.rs index 31fc1237..d77e4adf 100644 --- a/entity_api/src/user.rs +++ b/entity_api/src/user.rs @@ -7,7 +7,9 @@ use entity::users::{ActiveModel, Column, Entity, Model}; use entity::{roles, user_roles, Id}; use log::*; use password_auth; -use sea_orm::{entity::prelude::*, ConnectionTrait, DatabaseConnection, Set, TransactionTrait}; +use sea_orm::{ + entity::prelude::*, Condition, ConnectionTrait, DatabaseConnection, Set, TransactionTrait, +}; use serde::Deserialize; use std::sync::Arc; use utoipa::{IntoParams, ToSchema}; @@ -127,6 +129,42 @@ pub async fn find_by_organization( .collect()) } +/// Checks if a user has admin privileges for an organization. +/// +/// Returns `true` if the user is: +/// - A SuperAdmin (has `SuperAdmin` role with `organization_id = NULL`), OR +/// - An Admin for the specific organization (has `Admin` role for the given organization) +/// +/// This function encapsulates the SeaORM query logic for role checking, +/// keeping database-specific implementation details out of the domain layer. +pub async fn has_admin_access( + db: &impl ConnectionTrait, + user_id: Id, + organization_id: Id, +) -> Result { + let admin_role = user_roles::Entity::find() + .filter(user_roles::Column::UserId.eq(user_id)) + .filter( + Condition::any() + // SuperAdmin with organization_id = NULL + .add( + Condition::all() + .add(user_roles::Column::Role.eq(Role::SuperAdmin)) + .add(user_roles::Column::OrganizationId.is_null()), + ) + // Admin for this specific organization + .add( + Condition::all() + .add(user_roles::Column::Role.eq(Role::Admin)) + .add(user_roles::Column::OrganizationId.eq(organization_id)), + ), + ) + .one(db) + .await?; + + Ok(admin_role.is_some()) +} + pub async fn delete(db: &impl ConnectionTrait, user_id: Id) -> Result<(), Error> { Entity::delete_by_id(user_id).exec(db).await?; Ok(()) @@ -382,4 +420,160 @@ mod test { Ok(()) } + + #[tokio::test] + async fn has_admin_access_returns_true_for_super_admin() -> Result<(), Error> { + let user_id = Id::new_v4(); + let organization_id = Id::new_v4(); + let role_id = Id::new_v4(); + let now = chrono::Utc::now(); + + // Create a SuperAdmin role with NULL organization_id + let super_admin_role = user_roles::Model { + id: role_id, + user_id, + role: Role::SuperAdmin, + organization_id: None, + created_at: now.into(), + updated_at: now.into(), + }; + + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![vec![super_admin_role]]) + .into_connection(); + + let result = has_admin_access(&db, user_id, organization_id).await?; + + assert!( + result, + "SuperAdmin should have admin access to any organization" + ); + + Ok(()) + } + + #[tokio::test] + async fn has_admin_access_returns_true_for_organization_admin() -> Result<(), Error> { + let user_id = Id::new_v4(); + let organization_id = Id::new_v4(); + let role_id = Id::new_v4(); + let now = chrono::Utc::now(); + + // Create an Admin role for the specific organization + let org_admin_role = user_roles::Model { + id: role_id, + user_id, + role: Role::Admin, + organization_id: Some(organization_id), + created_at: now.into(), + updated_at: now.into(), + }; + + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![vec![org_admin_role]]) + .into_connection(); + + let result = has_admin_access(&db, user_id, organization_id).await?; + + assert!( + result, + "Organization Admin should have admin access to their organization" + ); + + Ok(()) + } + + #[tokio::test] + async fn has_admin_access_returns_false_for_regular_user() -> Result<(), Error> { + let user_id = Id::new_v4(); + let organization_id = Id::new_v4(); + + // Mock returns empty result (no admin roles found) + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![Vec::::new()]) + .into_connection(); + + let result = has_admin_access(&db, user_id, organization_id).await?; + + assert!(!result, "Regular users should not have admin access"); + + Ok(()) + } + + #[tokio::test] + async fn has_admin_access_returns_false_for_admin_of_different_organization( + ) -> Result<(), Error> { + let user_id = Id::new_v4(); + let organization_id_a = Id::new_v4(); // Organization being queried + let _organization_id_b = Id::new_v4(); // Organization where user is admin + + // Mock returns empty result (no matching admin role for org A) + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![Vec::::new()]) + .into_connection(); + + let result = has_admin_access(&db, user_id, organization_id_a).await?; + + assert!( + !result, + "Admin of different organization should not have access" + ); + + Ok(()) + } + + #[tokio::test] + async fn has_admin_access_returns_false_for_nonexistent_user() -> Result<(), Error> { + let nonexistent_user_id = Id::new_v4(); + let organization_id = Id::new_v4(); + + // Mock returns empty result (user doesn't exist) + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![Vec::::new()]) + .into_connection(); + + let result = has_admin_access(&db, nonexistent_user_id, organization_id).await?; + + assert!(!result, "Nonexistent user should not have admin access"); + + Ok(()) + } + + #[tokio::test] + async fn has_admin_access_with_admin_role_for_multiple_organizations() -> Result<(), Error> { + let user_id = Id::new_v4(); + let organization_id_a = Id::new_v4(); + let organization_id_b = Id::new_v4(); + let role_id = Id::new_v4(); + let now = chrono::Utc::now(); + + // Create an Admin role for organization A + let org_admin_role = user_roles::Model { + id: role_id, + user_id, + role: Role::Admin, + organization_id: Some(organization_id_a), + created_at: now.into(), + updated_at: now.into(), + }; + + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![vec![org_admin_role]]) + .into_connection(); + + // Should have access to organization A + let result_a = has_admin_access(&db, user_id, organization_id_a).await?; + assert!(result_a, "Should have admin access to organization A"); + + // Create new mock for organization B query (no matching role) + let db_b = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results(vec![Vec::::new()]) + .into_connection(); + + // Should NOT have access to organization B + let result_b = has_admin_access(&db_b, user_id, organization_id_b).await?; + assert!(!result_b, "Should not have admin access to organization B"); + + Ok(()) + } } diff --git a/web/src/controller/organization/coaching_relationship_controller.rs b/web/src/controller/organization/coaching_relationship_controller.rs index bd5a2846..d827e539 100644 --- a/web/src/controller/organization/coaching_relationship_controller.rs +++ b/web/src/controller/organization/coaching_relationship_controller.rs @@ -115,16 +115,22 @@ pub async fn read( )] pub async fn index( CompareApiVersion(_v): CompareApiVersion, - AuthenticatedUser(_user): AuthenticatedUser, + AuthenticatedUser(user): AuthenticatedUser, State(app_state): State, Path(organization_id): Path, ) -> Result { - debug!("GET all CoachingRelationships"); - let coaching_relationships = CoachingRelationshipApi::find_by_organization_with_user_names( - app_state.db_conn_ref(), - organization_id, - ) - .await?; + debug!( + "GET all CoachingRelationships for user {} in organization {}", + user.id, organization_id + ); + + let coaching_relationships = + CoachingRelationshipApi::find_by_organization_for_user_with_user_names( + app_state.db_conn_ref(), + user.id, + organization_id, + ) + .await?; debug!("Found CoachingRelationships: {coaching_relationships:?}"); diff --git a/web/src/protect/coaching_relationships.rs b/web/src/protect/coaching_relationships.rs deleted file mode 100644 index 1536ac0e..00000000 --- a/web/src/protect/coaching_relationships.rs +++ /dev/null @@ -1,33 +0,0 @@ -use crate::{extractors::authenticated_user::AuthenticatedUser, AppState}; -use axum::{ - extract::{Path, Request, State}, - http::StatusCode, - middleware::Next, - response::IntoResponse, -}; - -use domain::{organization, Id}; -use std::collections::HashSet; - -/// Checks that the organization record referenced by `organization_id` -/// exists and that the authenticated user is associated with i.t -/// Intended to be given to axum::middleware::from_fn_with_state in the router -pub(crate) async fn index( - State(app_state): State, - AuthenticatedUser(user): AuthenticatedUser, - Path(organization_id): Path, - request: Request, - next: Next, -) -> impl IntoResponse { - let user_organization_ids = organization::find_by_user(app_state.db_conn_ref(), user.id) - .await - .unwrap_or(vec![]) - .into_iter() - .map(|org| org.id) - .collect::>(); - if user_organization_ids.contains(&organization_id) { - next.run(request).await - } else { - (StatusCode::UNAUTHORIZED, "UNAUTHORIZED").into_response() - } -} diff --git a/web/src/protect/mod.rs b/web/src/protect/mod.rs index 9b115d56..b67bf3cc 100644 --- a/web/src/protect/mod.rs +++ b/web/src/protect/mod.rs @@ -10,7 +10,6 @@ pub(crate) mod actions; pub(crate) mod agreements; -pub(crate) mod coaching_relationships; pub(crate) mod coaching_sessions; pub(crate) mod jwt; pub(crate) mod notes; diff --git a/web/src/protect/organizations/coaching_relationships.rs b/web/src/protect/organizations/coaching_relationships.rs index bf54e447..182afab1 100644 --- a/web/src/protect/organizations/coaching_relationships.rs +++ b/web/src/protect/organizations/coaching_relationships.rs @@ -8,6 +8,20 @@ use axum::{ use domain::Id; +/// Checks that the authenticated user is associated with the organization specified by `organization_id` +/// Intended to be given to axum::middleware::from_fn_with_state in the router +pub(crate) async fn index( + State(app_state): State, + AuthenticatedUser(authenticated_user): AuthenticatedUser, + Path(organization_id): Path, + request: Request, + next: Next, +) -> impl IntoResponse { + let checks: Vec = vec![Predicate::new(UserInOrganization, vec![organization_id])]; + + crate::protect::authorize(&app_state, authenticated_user, request, next, checks).await +} + /// Checks that the authenticated user is associated with the organization specified by `organization_id` /// and that the authenticated user is an admin /// Intended to be given to axum::middleware::from_fn_with_state in the router diff --git a/web/src/router.rs b/web/src/router.rs index 5550c30e..002f1e26 100644 --- a/web/src/router.rs +++ b/web/src/router.rs @@ -276,7 +276,7 @@ fn organization_coaching_relationship_routes(app_state: AppState) -> Router { ) .route_layer(from_fn_with_state( app_state.clone(), - protect::coaching_relationships::index, + protect::organizations::coaching_relationships::index, )), ) .route(