From fa0c0e313dc38bdc00751e3db79c4a760cca2eb7 Mon Sep 17 00:00:00 2001 From: kipavy Date: Sun, 18 Oct 2026 20:56:18 +0000 Subject: [PATCH] feat(teams): several owners per team, and roles only from someone who holds them Fixes a privilege escalation: any member with Manage members could give themselves or anyone the Owner role through the API, since assign_member_role checked only that one permission. The same gap let a Manager create a custom role with every permission and assign it to themselves, or invite an account of their own straight in as owner. One rule now gates every path that confers or revokes a role: the actor must hold every permission the role carries, and only an Owner may grant or remove the Owner role. It covers assigning and removing roles, creating, editing and deleting custom roles, add/invite member, and join links. Invites also reject unknown role names now. Several members can hold Owner (VoltiusApp/voltius#652). teams.owner_id becomes the primary owner: the account billing sits on and the only one who can delete the team. It always keeps the Owner role and cannot be removed or leave, so a team always has an Owner. Other Owners can leave. PUT /v1/teams/:id/billing-owner hands it to another Owner. Self-hosted, any Owner may move it. On the cloud only the current primary owner may, and the target needs their own Teams or Business plan at least as high as the current one, with seats for everyone in their teams plus this one (402 otherwise). Ownership changes lock the team row so a transfer cannot race a demotion. /v1/meta advertises multi_owner. --- src/main.rs | 2 + src/routes/member_names.rs | 16 +- src/routes/meta.rs | 9 + src/routes/mod.rs | 1 + src/routes/team_grants.rs | 7 +- src/routes/team_ownership.rs | 213 +++++++++++++++++++++++ src/routes/teams.rs | 327 +++++++++++++++++++++++++---------- src/team_authority.rs | 207 ++++++++++++++++++++++ src/test_support.rs | 7 + 9 files changed, 690 insertions(+), 99 deletions(-) create mode 100644 src/routes/team_ownership.rs create mode 100644 src/team_authority.rs diff --git a/src/main.rs b/src/main.rs index 920c222..2c26140 100644 --- a/src/main.rs +++ b/src/main.rs @@ -15,6 +15,7 @@ mod routes; mod self_host; mod single_instance; mod session_grants; +mod team_authority; mod team_join_grants; mod team_plan; mod vault_key_exposure; @@ -443,6 +444,7 @@ async fn main() { .layer(middleware::from_fn(features::require_team_invites)), ) .route("/v1/teams/:team_id", delete(routes::teams::delete_team).patch(routes::teams::rename_team)) + .route("/v1/teams/:team_id/billing-owner", put(routes::team_ownership::transfer_billing_owner)) .route( "/v1/teams/:team_id/lock-policy", put(routes::team_lock_policy::set_lock_policy).delete(routes::team_lock_policy::clear_lock_policy), diff --git a/src/routes/member_names.rs b/src/routes/member_names.rs index 8b3ebca..8f49395 100644 --- a/src/routes/member_names.rs +++ b/src/routes/member_names.rs @@ -212,7 +212,7 @@ mod db_tests { use super::*; use crate::permissions::{PERM_INVITE_MEMBERS, PERM_MANAGE_MEMBERS}; use crate::test_pool_or_skip; - use crate::test_support::{add_member, member_with_role, seed_team, seed_user}; + use crate::test_support::{add_member, inviter_with, member_with_role, seed_team, seed_user}; async fn wait_for_audit_rows(pool: &PgPool, team: Uuid, action: &str, expected: i64) { for _ in 0..100 { @@ -284,7 +284,7 @@ mod db_tests { let pool = test_pool_or_skip!(); let owner = seed_user(&pool).await; let team = seed_team(&pool, owner).await; - let inviter = member_with_role(&pool, team, PERM_INVITE_MEMBERS).await; + let inviter = inviter_with(&pool, team, PERM_INVITE_MEMBERS).await; let plain = member_with_role(&pool, team, 0).await; let target = seed_user(&pool).await; add_member(&pool, team, target).await; @@ -368,7 +368,7 @@ mod db_tests { async fn invite_with_name_needs_manage_members() { let pool = test_pool_or_skip!(); let (_, team) = team_for_invites(&pool).await; - let inviter = member_with_role(&pool, team, PERM_INVITE_MEMBERS).await; + let inviter = inviter_with(&pool, team, PERM_INVITE_MEMBERS).await; let email = format!("{}@corp.test", Uuid::new_v4()); assert_eq!(invite_email(&pool, inviter, team, &email, Some("Jan")).await, Err(StatusCode::FORBIDDEN)); @@ -380,7 +380,7 @@ mod db_tests { async fn invite_stores_name_and_reinvite_without_name_keeps_it() { let pool = test_pool_or_skip!(); let (_, team) = team_for_invites(&pool).await; - let admin = member_with_role(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; + let admin = inviter_with(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; let email = format!("{}@corp.test", Uuid::new_v4()); invite_email(&pool, admin, team, &email, Some("Jan Novák")).await.unwrap(); @@ -395,7 +395,7 @@ mod db_tests { async fn add_member_by_id_carries_the_name_into_membership() { let pool = test_pool_or_skip!(); let (_, team) = team_for_invites(&pool).await; - let admin = member_with_role(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; + let admin = inviter_with(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; let invitee = seed_user(&pool).await; let _ = add_member_route( @@ -532,7 +532,7 @@ mod db_tests { async fn reinvite_without_a_name_does_not_revive_the_first_invites_name() { let pool = test_pool_or_skip!(); let (_, team) = team_for_invites(&pool).await; - let admin = member_with_role(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; + let admin = inviter_with(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; let user = seed_user(&pool).await; let email = crate::test_support::test_user_email(user); @@ -584,7 +584,7 @@ mod db_tests { async fn invite_audit_carries_the_name_on_every_path() { let pool = test_pool_or_skip!(); let (_, team) = team_for_invites(&pool).await; - let admin = member_with_role(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; + let admin = inviter_with(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; let existing = seed_user(&pool).await; invite_email(&pool, admin, team, &format!("{}@corp.test", Uuid::new_v4()), Some("ByEmail")).await.unwrap(); invite_email(&pool, admin, team, &crate::test_support::test_user_email(existing), Some("ByKnownEmail")).await.unwrap(); @@ -615,7 +615,7 @@ mod db_tests { async fn pending_list_returns_the_member_name() { let pool = test_pool_or_skip!(); let (_, team) = team_for_invites(&pool).await; - let admin = member_with_role(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; + let admin = inviter_with(&pool, team, PERM_MANAGE_MEMBERS | PERM_INVITE_MEMBERS).await; invite_email(&pool, admin, team, &format!("{}@corp.test", Uuid::new_v4()), Some("Pat")).await.unwrap(); let Json(rows) = crate::routes::teams::list_pending_invitations(State(pool.clone()), Extension(AuthUser(admin)), Path(team)) .await diff --git a/src/routes/meta.rs b/src/routes/meta.rs index d560e02..f1c033c 100644 --- a/src/routes/meta.rs +++ b/src/routes/meta.rs @@ -25,6 +25,8 @@ pub struct MetaResponse { pub passkeys: bool, /// `PUT /v1/auth/wrapped-user-secrets` honours `expected_wrapped_user_secrets`; absent on servers that predate it. pub wrapped_secrets_cas: bool, + /// Only Owners confer the Owner role, and `PUT /v1/teams/:id/billing-owner` exists. + pub multi_owner: bool, } pub async fn get_meta( @@ -44,6 +46,7 @@ pub async fn get_meta( two_factor: cipher.available(), passkeys: passkeys.available(), wrapped_secrets_cas: true, + multi_owner: true, }) } @@ -95,4 +98,10 @@ mod tests { let Json(meta) = get_meta(Extension(Features::open()), Extension(SeedCipher::for_tests()), Extension(Passkeys::for_tests())).await; assert_eq!(serde_json::to_value(&meta).unwrap()["wrapped_secrets_cas"], serde_json::json!(true)); } + + #[tokio::test] + async fn meta_advertises_multi_owner() { + let Json(meta) = get_meta(Extension(Features::open()), Extension(SeedCipher::for_tests()), Extension(Passkeys::for_tests())).await; + assert_eq!(serde_json::to_value(&meta).unwrap()["multi_owner"], serde_json::json!(true)); + } } diff --git a/src/routes/mod.rs b/src/routes/mod.rs index 65dd895..65af04c 100644 --- a/src/routes/mod.rs +++ b/src/routes/mod.rs @@ -18,6 +18,7 @@ pub mod team_lock_policy; pub mod team_sync; pub mod team_objects; pub mod team_object_prefs; +pub mod team_ownership; pub mod team_rule_sets; pub mod teams; pub mod terminal; diff --git a/src/routes/team_grants.rs b/src/routes/team_grants.rs index d16e0ac..1a852ea 100644 --- a/src/routes/team_grants.rs +++ b/src/routes/team_grants.rs @@ -106,6 +106,7 @@ pub async fn create_grant( warn!(team_id = %team_id, user_id = %auth.0, role = %role, "Rejected non-grantable role"); return Err(StatusCode::BAD_REQUEST); } + crate::team_authority::require_may_confer_builtin(&pool, team_id, auth.0, &role).await?; let max_uses = grants::clamp_max_uses(body.max_uses); let ttl = grants::clamp_ttl(body.expires_in_secs); @@ -390,7 +391,7 @@ mod tests { use crate::sync_notifier::SyncEvent; use crate::test_pool_or_skip; use crate::test_support::{ - member_with_role, seed_team_with_roles, seed_user, set_user_seats, + add_member, grant_builtin_role, member_with_role, seed_team_with_roles, seed_user, set_user_seats, }; use crate::permissions::PERM_CONNECT; use std::time::Duration; @@ -408,7 +409,9 @@ mod tests { async fn seed_team_with_manager(pool: &PgPool) -> (Uuid, Uuid, Uuid) { let owner = seed_user(pool).await; let team = seed_team_with_roles(pool, owner).await; - let manager = member_with_role(pool, team, PERM_INVITE_MEMBERS).await; + let manager = seed_user(pool).await; + add_member(pool, team, manager).await; + grant_builtin_role(pool, team, manager, "manager").await; (team, owner, manager) } diff --git a/src/routes/team_ownership.rs b/src/routes/team_ownership.rs new file mode 100644 index 0000000..eb8a0bf --- /dev/null +++ b/src/routes/team_ownership.rs @@ -0,0 +1,213 @@ +use axum::{extract::{Path, State}, http::StatusCode, Extension, Json}; +use serde::Deserialize; +use serde_json::json; +use sqlx::PgPool; +use tracing::{error, info, warn}; +use uuid::Uuid; + +use crate::auth::AuthUser; +use crate::entitlement::effective_tier_for_user; +use crate::routes::audit::write_audit_event; +use crate::routes::teams::{notify_team_members_changed, owner_seat_cap}; +use crate::self_host; +use crate::sync_notifier::SyncNotifier; +use crate::team_authority::{holds_owner_role, lock_billing_owner}; + +#[derive(Deserialize)] +pub struct TransferBillingOwnerRequest { + pub user_id: Uuid, +} + +fn plan_rank(tier: &str) -> u8 { + match tier { + "business" => 2, + "teams" => 1, + _ => 0, + } +} + +/// 402 unless `target`'s own subscription can carry this team at its current plan and size. +async fn ensure_target_can_pay(pool: &PgPool, team_id: Uuid, current: Uuid, target: Uuid) -> Result<(), StatusCode> { + let needed = plan_rank(&effective_tier_for_user(pool, current).await).max(1); + if plan_rank(&effective_tier_for_user(pool, target).await) < needed { + return Err(StatusCode::PAYMENT_REQUIRED); + } + let Some(cap) = owner_seat_cap(pool, target).await? else { + return Ok(()); + }; + let seats_after = sqlx::query_scalar::<_, i64>( + "SELECT COUNT(DISTINCT tm.user_id) FROM team_members tm JOIN teams t ON t.id = tm.team_id \ + WHERE t.owner_id = $1 OR t.id = $2", + ) + .bind(target) + .bind(team_id) + .fetch_one(pool) + .await + .map_err(|e| { + error!(error = %e, "Failed to count seats for billing transfer"); + StatusCode::INTERNAL_SERVER_ERROR + })?; + if seats_after > cap { + return Err(StatusCode::PAYMENT_REQUIRED); + } + Ok(()) +} + +/// Hand the team's billing to another Owner. On a self-hosted server any Owner may move it. +pub async fn transfer_billing_owner( + State(pool): State, + Extension(auth): Extension, + Extension(notifier): Extension, + Path(team_id): Path, + Json(body): Json, +) -> Result { + let self_hosted = self_host::is_self_hosted(); + let db = |e: sqlx::Error| { + error!(error = %e, team_id = %team_id, "Failed to transfer billing owner"); + StatusCode::INTERNAL_SERVER_ERROR + }; + let mut tx = pool.begin().await.map_err(db)?; + let current = lock_billing_owner(&mut tx, team_id).await?; + let may_transfer = auth.0 == current || (self_hosted && holds_owner_role(&mut *tx, team_id, auth.0).await?); + if !may_transfer { + warn!(team_id = %team_id, user_id = %auth.0, "Refused billing transfer by a non-billing owner"); + return Err(StatusCode::FORBIDDEN); + } + if body.user_id == current { + return Ok(StatusCode::NO_CONTENT); + } + if !holds_owner_role(&mut *tx, team_id, body.user_id).await? { + return Err(StatusCode::CONFLICT); + } + if !self_hosted { + ensure_target_can_pay(&pool, team_id, current, body.user_id).await?; + } + sqlx::query("UPDATE teams SET owner_id = $2 WHERE id = $1") + .bind(team_id) + .bind(body.user_id) + .execute(&mut *tx) + .await + .map_err(db)?; + tx.commit().await.map_err(db)?; + + info!(team_id = %team_id, from = %current, to = %body.user_id, "Billing owner transferred"); + tokio::spawn(write_audit_event( + pool.clone(), + team_id, + auth.0, + "team.billing_owner_changed", + Some("user"), + Some(body.user_id.to_string()), + None, + Some(json!({ "from": current, "to": body.user_id })), + )); + crate::team_plan::reconcile_team_plan(&pool, ¬ifier, team_id).await?; + notify_team_members_changed(&pool, ¬ifier, team_id).await; + Ok(StatusCode::NO_CONTENT) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_pool_or_skip; + use crate::test_support::*; + + async fn transfer(pool: &PgPool, actor: Uuid, team: Uuid, to: Uuid) -> Result { + transfer_billing_owner( + State(pool.clone()), + Extension(AuthUser(actor)), + Extension(SyncNotifier::new()), + Path(team), + Json(TransferBillingOwnerRequest { user_id: to }), + ) + .await + } + + async fn billing_owner(pool: &PgPool, team: Uuid) -> Uuid { + sqlx::query_scalar("SELECT owner_id FROM teams WHERE id = $1").bind(team).fetch_one(pool).await.unwrap() + } + + async fn co_owner(pool: &PgPool, team: Uuid) -> Uuid { + let user = seed_user(pool).await; + add_member(pool, team, user).await; + grant_builtin_role(pool, team, user, "owner").await; + user + } + + #[tokio::test] + async fn self_hosted_any_owner_can_take_billing() { + let _mode = BillingMode::self_hosted(); + let pool = test_pool_or_skip!(); + let creator = seed_user(&pool).await; + let team = seed_team_with_roles(&pool, creator).await; + let bob = co_owner(&pool, team).await; + + assert_eq!(transfer(&pool, bob, team, bob).await, Ok(StatusCode::NO_CONTENT)); + assert_eq!(billing_owner(&pool, team).await, bob); + } + + #[tokio::test] + async fn hosted_only_the_billing_owner_can_transfer() { + let _mode = BillingMode::hosted(); + let pool = test_pool_or_skip!(); + let creator = seed_user(&pool).await; + set_user_tier(&pool, creator, "teams").await; + let team = seed_team_with_roles(&pool, creator).await; + let bob = co_owner(&pool, team).await; + set_user_tier(&pool, bob, "teams").await; + + assert_eq!(transfer(&pool, bob, team, bob).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(transfer(&pool, creator, team, bob).await, Ok(StatusCode::NO_CONTENT)); + assert_eq!(billing_owner(&pool, team).await, bob); + } + + #[tokio::test] + async fn the_target_must_already_be_an_owner() { + let _mode = BillingMode::self_hosted(); + let pool = test_pool_or_skip!(); + let creator = seed_user(&pool).await; + let team = seed_team_with_roles(&pool, creator).await; + let manager = seed_user(&pool).await; + add_member(&pool, team, manager).await; + grant_builtin_role(&pool, team, manager, "manager").await; + + assert_eq!(transfer(&pool, creator, team, manager).await, Err(StatusCode::CONFLICT)); + assert_eq!(billing_owner(&pool, team).await, creator); + } + + #[tokio::test] + async fn hosted_target_needs_a_plan_at_least_as_high() { + let _mode = BillingMode::hosted(); + let pool = test_pool_or_skip!(); + let creator = seed_user(&pool).await; + set_user_tier(&pool, creator, "business").await; + let team = seed_team_with_roles(&pool, creator).await; + let bob = co_owner(&pool, team).await; + + for tier in ["free", "pro", "teams"] { + set_user_tier(&pool, bob, tier).await; + assert_eq!(transfer(&pool, creator, team, bob).await, Err(StatusCode::PAYMENT_REQUIRED), "{tier}"); + } + set_user_tier(&pool, bob, "business").await; + assert_eq!(transfer(&pool, creator, team, bob).await, Ok(StatusCode::NO_CONTENT)); + } + + #[tokio::test] + async fn hosted_target_needs_seats_for_their_teams_plus_this_one() { + let _mode = BillingMode::hosted(); + let pool = test_pool_or_skip!(); + let creator = seed_user(&pool).await; + set_user_tier(&pool, creator, "teams").await; + let team = seed_team_with_roles(&pool, creator).await; + let bob = co_owner(&pool, team).await; + set_user_tier(&pool, bob, "teams").await; + let bobs_team = seed_team_with_roles(&pool, bob).await; + let other = seed_user(&pool).await; + add_member(&pool, bobs_team, other).await; + + set_user_seats(&pool, bob, 2).await; + assert_eq!(transfer(&pool, creator, team, bob).await, Err(StatusCode::PAYMENT_REQUIRED)); + set_user_seats(&pool, bob, 3).await; + assert_eq!(transfer(&pool, creator, team, bob).await, Ok(StatusCode::NO_CONTENT)); + } +} diff --git a/src/routes/teams.rs b/src/routes/teams.rs index 6abd304..65595c3 100644 --- a/src/routes/teams.rs +++ b/src/routes/teams.rs @@ -9,6 +9,9 @@ use crate::auth::AuthUser; use crate::models::team::{Team, TeamMember, TeamRole}; use crate::routes::audit::write_audit_event; use crate::sync_notifier::SyncNotifier; +use crate::team_authority::{ + holds_owner_role, lock_billing_owner, require_covers, require_may_confer_builtin, role_authority, Authority, +}; use crate::PresenceMap; pub(crate) async fn notify_team_members_changed(pool: &PgPool, notifier: &SyncNotifier, team_id: Uuid) { @@ -449,10 +452,7 @@ pub async fn add_member( ensure_seat_available(&pool, owner_id).await?; let role_name = body.role.as_deref().unwrap_or("member").to_string(); - const VALID_ROLES: &[&str] = &["owner", "manager", "editor", "member", "connect-only"]; - if !VALID_ROLES.contains(&role_name.as_str()) { - return Err(StatusCode::BAD_REQUEST); - } + require_may_confer_builtin(&pool, team_id, auth.0, &role_name).await?; // Already a member — no-op let already_member = sqlx::query_scalar::<_, bool>( @@ -565,21 +565,9 @@ pub async fn remove_member( warn!(team_id = %team_id, user_id = %auth.0, "Insufficient permission to remove members"); return Err(StatusCode::FORBIDDEN); } - } - - // Cannot remove the team owner - let is_owner = sqlx::query_scalar::<_, bool>( - "SELECT owner_id = $2 FROM teams WHERE id = $1", - ) - .bind(team_id) - .bind(user_id) - .fetch_optional(&pool) - .await - .map_err(|e| { error!(error = %e, "Failed to check team owner"); StatusCode::INTERNAL_SERVER_ERROR })? - .ok_or(StatusCode::NOT_FOUND)?; - - if is_owner { - return Err(StatusCode::FORBIDDEN); + if holds_owner_role(&pool, team_id, user_id).await? { + require_covers(&pool, team_id, auth.0, Authority::OWNER).await?; + } } let mut tx = pool.begin().await.map_err(|e| { @@ -587,6 +575,11 @@ pub async fn remove_member( StatusCode::INTERNAL_SERVER_ERROR })?; + if lock_billing_owner(&mut tx, team_id).await? == user_id { + warn!(team_id = %team_id, "Billing owner cannot leave or be removed"); + return Err(StatusCode::FORBIDDEN); + } + let result = sqlx::query("DELETE FROM team_members WHERE team_id = $1 AND user_id = $2") .bind(team_id) .bind(user_id) @@ -942,6 +935,7 @@ pub async fn create_role( } else { permissions | crate::permissions::PERM_VIEW }; + require_covers(&pool, team_id, auth.0, Authority::role(permissions)).await?; let role = sqlx::query_as::<_, TeamRole>( r#"INSERT INTO team_roles (team_id, name, color, permissions, is_builtin, position) @@ -1033,6 +1027,7 @@ pub async fn update_role( let p = p & crate::permissions::ALL_PERMISSIONS; if legacy { crate::permissions::keep_era_bits(p, role_info.1) } else { p } }); + require_covers(&pool, team_id, auth.0, Authority::role(role_info.1 | permissions.unwrap_or(0))).await?; sqlx::query( r#"UPDATE team_roles @@ -1083,20 +1078,12 @@ pub async fn delete_role( return Err(StatusCode::FORBIDDEN); } - let role_info = sqlx::query_as::<_, (bool,)>( - "SELECT is_builtin FROM team_roles WHERE id = $1 AND team_id = $2", - ) - .bind(role_id) - .bind(team_id) - .fetch_optional(&pool) - .await - .map_err(|e| { error!(error = %e, "Failed to fetch role info"); StatusCode::INTERNAL_SERVER_ERROR })? - .ok_or(StatusCode::NOT_FOUND)?; - - if role_info.0 { + let role = role_authority(&pool, team_id, role_id).await?; + if role.is_builtin { warn!(role_id = %role_id, "Cannot delete builtin role"); return Err(StatusCode::FORBIDDEN); } + require_covers(&pool, team_id, auth.0, role.authority).await?; let mut tx = pool.begin().await.map_err(|e| { error!(error = %e, "Failed to begin delete_role transaction"); @@ -1209,17 +1196,9 @@ pub async fn assign_member_role( return Err(StatusCode::NOT_FOUND); } - let is_builtin = sqlx::query_scalar::<_, bool>( - "SELECT is_builtin FROM team_roles WHERE id = $1 AND team_id = $2", - ) - .bind(body.role_id) - .bind(team_id) - .fetch_optional(&pool) - .await - .map_err(|e| { error!(error = %e, "Failed to verify role"); StatusCode::INTERNAL_SERVER_ERROR })? - .ok_or(StatusCode::NOT_FOUND)?; - - crate::team_plan::require_granular(&pool, team_id, is_builtin).await?; + let role = role_authority(&pool, team_id, body.role_id).await?; + crate::team_plan::require_granular(&pool, team_id, role.is_builtin).await?; + require_covers(&pool, team_id, auth.0, role.authority).await?; let target_display_name = sqlx::query_scalar::<_, String>("SELECT handle FROM users WHERE id = $1") .bind(target_user_id) @@ -1268,32 +1247,17 @@ pub async fn remove_member_role( return Err(StatusCode::FORBIDDEN); } - // Cannot remove the owner role from the team owner - let is_target_team_owner = sqlx::query_scalar::<_, bool>( - "SELECT owner_id = $2 FROM teams WHERE id = $1", - ) - .bind(team_id) - .bind(target_user_id) - .fetch_optional(&pool) - .await - .map_err(|e| { error!(error = %e, "Failed to check team owner"); StatusCode::INTERNAL_SERVER_ERROR })? - .unwrap_or(false); - - if is_target_team_owner { - let is_owner_role = sqlx::query_scalar::<_, bool>( - "SELECT is_builtin AND name = 'owner' FROM team_roles WHERE id = $1 AND team_id = $2", - ) - .bind(role_id) - .bind(team_id) - .fetch_optional(&pool) - .await - .map_err(|e| { error!(error = %e, "Failed to check role type"); StatusCode::INTERNAL_SERVER_ERROR })? - .unwrap_or(false); + let role = role_authority(&pool, team_id, role_id).await?; + require_covers(&pool, team_id, auth.0, role.authority).await?; - if is_owner_role { - warn!(team_id = %team_id, "Cannot remove owner role from team owner"); - return Err(StatusCode::FORBIDDEN); - } + let db = |e: sqlx::Error| { + error!(error = %e, "Failed to remove member role"); + StatusCode::INTERNAL_SERVER_ERROR + }; + let mut tx = pool.begin().await.map_err(db)?; + if role.authority.owner && lock_billing_owner(&mut tx, team_id).await? == target_user_id { + warn!(team_id = %team_id, "Cannot remove owner role from the billing owner"); + return Err(StatusCode::FORBIDDEN); } let result = sqlx::query( @@ -1302,13 +1266,14 @@ pub async fn remove_member_role( .bind(team_id) .bind(target_user_id) .bind(role_id) - .execute(&pool) + .execute(&mut *tx) .await - .map_err(|e| { error!(error = %e, "Failed to remove member role"); StatusCode::INTERNAL_SERVER_ERROR })?; + .map_err(db)?; if result.rows_affected() == 0 { return Err(StatusCode::NOT_FOUND); } + tx.commit().await.map_err(db)?; let target_display_name = sqlx::query_scalar::<_, String>("SELECT handle FROM users WHERE id = $1") .bind(target_user_id) @@ -1410,20 +1375,7 @@ async fn override_guardrails( return Err(StatusCode::FORBIDDEN); } - let target_is_owner = sqlx::query_scalar::<_, bool>( - "SELECT EXISTS( \ - SELECT 1 FROM team_member_roles tmr \ - JOIN team_roles tr ON tr.id = tmr.role_id \ - WHERE tmr.team_id = $1 AND tmr.user_id = $2 \ - AND tr.is_builtin AND tr.name = 'owner')", - ) - .bind(team_id) - .bind(target_user_id) - .fetch_one(pool) - .await - .map_err(|e| { error!(error = %e, "Failed to check target owner role"); StatusCode::INTERNAL_SERVER_ERROR })?; - - if target_is_owner { + if holds_owner_role(pool, team_id, target_user_id).await? { return Err(StatusCode::FORBIDDEN); } @@ -1638,6 +1590,7 @@ pub async fn invite_member( } let role = body.role.as_deref().unwrap_or("member").to_string(); + require_may_confer_builtin(&pool, team_id, auth.0, &role).await?; let owner_id = team_owner(&pool, team_id).await?; @@ -1854,7 +1807,7 @@ mod authz_tests { use crate::terminal_manager::TerminalManager; use crate::test_pool_or_skip; use crate::test_support::{ - add_member as add_team_member, env_lock, member_with_role, seed_role, seed_team, + add_member as add_team_member, env_lock, inviter_with, member_with_role, seed_role, seed_team, seed_team_with_roles, seed_user, set_user_ls_status, set_user_seats, set_user_tier, set_user_trial, unique_handle, BillingMode, EnvLockGuard, }; @@ -2099,7 +2052,7 @@ mod authz_tests { let pool = test_pool_or_skip!(); let owner = seed_user(&pool).await; let team = seed_team(&pool, owner).await; - let caller = member_with_role(&pool, team, PERM_INVITE_MEMBERS).await; + let caller = inviter_with(&pool, team, PERM_INVITE_MEMBERS).await; let invitee = seed_user(&pool).await; let res = add_member( @@ -2778,6 +2731,8 @@ mod authz_tests { assert_eq!(res.unwrap_err(), axum::http::StatusCode::PAYMENT_REQUIRED); } + const ROLE_EDITOR_BITS: i64 = crate::permissions::PERM_CONNECT | crate::permissions::PERM_VIEW | PERM_VIEW_SECRETS; + #[tokio::test] async fn create_role_ok_when_business_and_manage_roles() { let _env = BillingMode::hosted(); @@ -2785,7 +2740,7 @@ mod authz_tests { let owner = seed_user(&pool).await; set_user_tier(&pool, owner, "business").await; let team = seed_team(&pool, owner).await; - let caller = member_with_role(&pool, team, PERM_MANAGE_ROLES).await; + let caller = member_with_role(&pool, team, PERM_MANAGE_ROLES | ROLE_EDITOR_BITS).await; let res = create_role( State(pool.clone()), @@ -2870,7 +2825,7 @@ mod authz_tests { let owner = seed_user(&pool).await; set_user_tier(&pool, owner, "business").await; let team = seed_team(&pool, owner).await; - let caller = member_with_role(&pool, team, PERM_MANAGE_ROLES).await; + let caller = member_with_role(&pool, team, PERM_MANAGE_ROLES | ROLE_EDITOR_BITS).await; let role = seed_role(&pool, team, "editable", PERM_VIEW_SECRETS).await; // non-builtin let res = update_role( @@ -2900,7 +2855,7 @@ mod authz_tests { let team = seed_team(&pool, owner).await; // A caller that CAN invite, so only the seat cap can reject. - let inviter = member_with_role(&pool, team, PERM_INVITE_MEMBERS).await; + let inviter = inviter_with(&pool, team, PERM_INVITE_MEMBERS).await; // `member_with_role` adds `inviter` to `team_members`; `seed_team` does // NOT add the owner, so `inviter` alone already fills the single seat. let invitee = seed_user(&pool).await; @@ -2933,7 +2888,7 @@ mod authz_tests { let owner = seed_user(&pool).await; set_user_seats(&pool, owner, 50).await; let team = seed_team(&pool, owner).await; - let inviter = member_with_role(&pool, team, PERM_INVITE_MEMBERS).await; + let inviter = inviter_with(&pool, team, PERM_INVITE_MEMBERS).await; let invitee = seed_user(&pool).await; let res = add_member( @@ -2957,7 +2912,7 @@ mod authz_tests { /// the inviter. Owner is not a member (seed_team doesn't add them), so the used /// seat count equals `n`. async fn fill_seats(pool: &sqlx::PgPool, team: Uuid, n: usize) -> Uuid { - let inviter = member_with_role(pool, team, PERM_INVITE_MEMBERS).await; + let inviter = inviter_with(pool, team, PERM_INVITE_MEMBERS).await; for _ in 1..n { member_with_role(pool, team, 0).await; } @@ -3857,3 +3812,197 @@ mod rule_set_era_tests { assert_eq!(deny, PERM_COPY_SECRETS | PERM_VIEW); } } + +#[cfg(test)] +mod ownership_tests { + use super::*; + use crate::permissions::{ALL_PERMISSIONS, PERM_ADMINISTRATOR, PERM_INVITE_MEMBERS, PERM_MANAGE_MEMBERS, PERM_MANAGE_ROLES}; + use crate::terminal_manager::TerminalManager; + use crate::test_support::{ + add_member as seed_member, grant_builtin_role, rule_set_client_headers, seed_role, seed_team_with_roles, + seed_user, BillingMode, + }; + use axum::Extension; + + struct Team { + pool: PgPool, + id: Uuid, + creator: Uuid, + owner_role: Uuid, + } + + async fn team() -> Option { + let pool = crate::test_support::test_pool().await?; + let creator = seed_user(&pool).await; + let id = seed_team_with_roles(&pool, creator).await; + let owner_role = sqlx::query_scalar("SELECT id FROM team_roles WHERE team_id = $1 AND name = 'owner' AND is_builtin") + .bind(id) + .fetch_one(&pool) + .await + .unwrap(); + Some(Team { pool, id, creator, owner_role }) + } + + impl Team { + async fn member(&self, builtin: &str) -> Uuid { + let user = seed_user(&self.pool).await; + seed_member(&self.pool, self.id, user).await; + grant_builtin_role(&self.pool, self.id, user, builtin).await; + user + } + + async fn assign(&self, actor: Uuid, target: Uuid, role: Uuid) -> Result { + assign_member_role( + State(self.pool.clone()), + Extension(AuthUser(actor)), + Extension(SyncNotifier::new()), + Path((self.id, target)), + Json(AssignRoleRequest { role_id: role }), + ) + .await + } + + async fn unassign(&self, actor: Uuid, target: Uuid, role: Uuid) -> Result { + remove_member_role( + State(self.pool.clone()), + Extension(AuthUser(actor)), + Extension(SyncNotifier::new()), + Path((self.id, target, role)), + ) + .await + } + + async fn remove(&self, actor: Uuid, target: Uuid) -> Result { + remove_member( + State(self.pool.clone()), + Extension(AuthUser(actor)), + Extension(SyncNotifier::new()), + Extension(TerminalManager::new()), + Path((self.id, target)), + ) + .await + } + + async fn add(&self, actor: Uuid, role: &str) -> Result { + let invitee = seed_user(&self.pool).await; + add_member( + State(self.pool.clone()), + Extension(AuthUser(actor)), + Extension(SyncNotifier::new()), + Path(self.id), + Json(AddMemberRequest { email: None, user_id: Some(invitee), role: Some(role.into()), name: None }), + ) + .await + .map(|(status, _)| status) + } + + async fn create_role(&self, actor: Uuid, permissions: i64) -> Result { + create_role( + State(self.pool.clone()), + Extension(AuthUser(actor)), + Extension(SyncNotifier::new()), + rule_set_client_headers(), + Path(self.id), + Json(CreateRoleRequest { name: format!("r-{}", Uuid::new_v4()), color: None, permissions }), + ) + .await + .map(|(status, _)| status) + } + + async fn is_owner(&self, user: Uuid) -> bool { + holds_owner_role(&self.pool, self.id, user).await.unwrap() + } + } + + #[tokio::test] + async fn a_manager_cannot_make_themselves_or_anyone_an_owner() { + let _mode = BillingMode::self_hosted(); + let Some(t) = team().await else { return }; + let manager = t.member("manager").await; + let member = t.member("member").await; + + assert_eq!(t.assign(manager, manager, t.owner_role).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.assign(manager, member, t.owner_role).await, Err(StatusCode::FORBIDDEN)); + assert!(!t.is_owner(manager).await && !t.is_owner(member).await); + } + + #[tokio::test] + async fn a_manager_cannot_hand_out_a_custom_role_above_their_own_bits() { + let _mode = BillingMode::self_hosted(); + let Some(t) = team().await else { return }; + let manager = t.member("manager").await; + let admin_role = seed_role(&t.pool, t.id, "admins", ALL_PERMISSIONS).await; + + assert_eq!(t.assign(manager, manager, admin_role).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.create_role(manager, PERM_ADMINISTRATOR).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.create_role(manager, PERM_MANAGE_ROLES | PERM_MANAGE_MEMBERS).await, Ok(StatusCode::CREATED)); + } + + #[tokio::test] + async fn an_inviter_cannot_admit_someone_above_their_own_role() { + let _mode = BillingMode::self_hosted(); + let Some(t) = team().await else { return }; + let manager = t.member("manager").await; + let inviter_role = seed_role(&t.pool, t.id, "inviters", PERM_INVITE_MEMBERS).await; + let inviter = seed_user(&t.pool).await; + seed_member(&t.pool, t.id, inviter).await; + crate::test_support::assign_role(&t.pool, t.id, inviter, inviter_role).await; + + assert_eq!(t.add(manager, "owner").await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.add(inviter, "manager").await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.add(manager, "manager").await, Ok(StatusCode::CREATED)); + assert_eq!(t.add(manager, "no-such-role").await, Err(StatusCode::BAD_REQUEST)); + assert_eq!(t.add(t.creator, "owner").await, Ok(StatusCode::CREATED)); + } + + #[tokio::test] + async fn owners_promote_and_demote_each_other_but_never_the_billing_owner() { + let _mode = BillingMode::self_hosted(); + let Some(t) = team().await else { return }; + let bob = t.member("manager").await; + + assert_eq!(t.assign(t.creator, bob, t.owner_role).await, Ok(StatusCode::NO_CONTENT)); + assert!(t.is_owner(bob).await); + assert_eq!(t.unassign(bob, t.creator, t.owner_role).await, Err(StatusCode::FORBIDDEN)); + assert!(t.is_owner(t.creator).await); + assert_eq!(t.unassign(t.creator, bob, t.owner_role).await, Ok(StatusCode::NO_CONTENT)); + assert!(!t.is_owner(bob).await); + } + + #[tokio::test] + async fn a_manager_cannot_demote_or_remove_a_co_owner() { + let _mode = BillingMode::self_hosted(); + let Some(t) = team().await else { return }; + let manager = t.member("manager").await; + let bob = t.member("owner").await; + + assert_eq!(t.unassign(manager, bob, t.owner_role).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.remove(manager, bob).await, Err(StatusCode::FORBIDDEN)); + assert!(t.is_owner(bob).await); + assert_eq!(t.remove(t.creator, bob).await, Ok(StatusCode::NO_CONTENT)); + } + + #[tokio::test] + async fn a_co_owner_may_leave_but_the_billing_owner_must_hand_billing_over_first() { + let _mode = BillingMode::self_hosted(); + let Some(t) = team().await else { return }; + let bob = t.member("owner").await; + let carol = t.member("owner").await; + + assert_eq!(t.remove(t.creator, t.creator).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.remove(bob, t.creator).await, Err(StatusCode::FORBIDDEN)); + assert_eq!(t.remove(carol, carol).await, Ok(StatusCode::NO_CONTENT)); + + crate::routes::team_ownership::transfer_billing_owner( + State(t.pool.clone()), + Extension(AuthUser(bob)), + Extension(SyncNotifier::new()), + Path(t.id), + Json(crate::routes::team_ownership::TransferBillingOwnerRequest { user_id: bob }), + ) + .await + .unwrap(); + assert_eq!(t.remove(t.creator, t.creator).await, Ok(StatusCode::NO_CONTENT)); + assert_eq!(team_owner(&t.pool, t.id).await.unwrap(), bob); + } +} diff --git a/src/team_authority.rs b/src/team_authority.rs new file mode 100644 index 0000000..fd6d369 --- /dev/null +++ b/src/team_authority.rs @@ -0,0 +1,207 @@ +use axum::http::StatusCode; +use sqlx::{PgExecutor, PgPool}; +use tracing::{error, warn}; +use uuid::Uuid; + +use crate::permissions::effective_permissions; + +/// What a role carries, or what a member may hand out. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Authority { + pub permissions: i64, + pub owner: bool, +} + +impl Authority { + pub const OWNER: Self = Self { permissions: 0, owner: true }; + + pub const fn role(permissions: i64) -> Self { + Self { permissions, owner: false } + } + + /// Granting or revoking `other` needs every bit it carries, and the Owner role needs an Owner. + /// Raw bits on purpose: one inert alone can come alive beside another role the target holds. + pub fn covers(self, other: Authority) -> bool { + self.owner || (!other.owner && other.permissions & !self.permissions == 0) + } +} + +pub async fn holds_owner_role<'e>( + executor: impl PgExecutor<'e>, + team_id: Uuid, + user_id: Uuid, +) -> Result { + sqlx::query_scalar::<_, bool>( + "SELECT EXISTS( \ + SELECT 1 FROM team_member_roles tmr \ + JOIN team_roles tr ON tr.id = tmr.role_id \ + WHERE tmr.team_id = $1 AND tmr.user_id = $2 \ + AND tr.is_builtin AND tr.name = 'owner')", + ) + .bind(team_id) + .bind(user_id) + .fetch_one(executor) + .await + .map_err(|e| { + error!(error = %e, team_id = %team_id, user_id = %user_id, "Failed to check owner role"); + StatusCode::INTERNAL_SERVER_ERROR + }) +} + +pub async fn member_authority(pool: &PgPool, team_id: Uuid, user_id: Uuid) -> Result { + Ok(Authority { + permissions: effective_permissions(pool, team_id, user_id).await?, + owner: holds_owner_role(pool, team_id, user_id).await?, + }) +} + +pub struct TeamRoleAuthority { + pub authority: Authority, + pub is_builtin: bool, +} + +pub async fn role_authority(pool: &PgPool, team_id: Uuid, role_id: Uuid) -> Result { + role_lookup(pool, team_id, Some(role_id), None).await +} + +async fn builtin_role_authority(pool: &PgPool, team_id: Uuid, name: &str) -> Result { + role_lookup(pool, team_id, None, Some(name)).await.map_err(|status| match status { + StatusCode::NOT_FOUND => StatusCode::BAD_REQUEST, + other => other, + }) +} + +async fn role_lookup( + pool: &PgPool, + team_id: Uuid, + role_id: Option, + builtin_name: Option<&str>, +) -> Result { + let (permissions, is_builtin, owner) = sqlx::query_as::<_, (i64, bool, bool)>( + "SELECT permissions, is_builtin, is_builtin AND name = 'owner' FROM team_roles \ + WHERE team_id = $1 AND (id = $2 OR ($2 IS NULL AND is_builtin AND name = $3))", + ) + .bind(team_id) + .bind(role_id) + .bind(builtin_name) + .fetch_optional(pool) + .await + .map_err(|e| { + error!(error = %e, team_id = %team_id, "Failed to read role"); + StatusCode::INTERNAL_SERVER_ERROR + })? + .ok_or(StatusCode::NOT_FOUND)?; + Ok(TeamRoleAuthority { authority: Authority { permissions, owner }, is_builtin }) +} + +pub async fn require_covers(pool: &PgPool, team_id: Uuid, actor: Uuid, target: Authority) -> Result<(), StatusCode> { + let held = member_authority(pool, team_id, actor).await?; + if held.covers(target) { + return Ok(()); + } + warn!(team_id = %team_id, actor = %actor, ?held, ?target, "Refused to confer authority the actor does not hold"); + Err(StatusCode::FORBIDDEN) +} + +/// Gate for every path that admits someone under a builtin role by name: 400 for an unknown name. +pub async fn require_may_confer_builtin(pool: &PgPool, team_id: Uuid, actor: Uuid, name: &str) -> Result<(), StatusCode> { + let role = builtin_role_authority(pool, team_id, name).await?; + require_covers(pool, team_id, actor, role.authority).await +} + +/// Locks the team row and returns its billing owner; every ownership change runs under this lock. +pub async fn lock_billing_owner(conn: &mut sqlx::PgConnection, team_id: Uuid) -> Result { + sqlx::query_scalar::<_, Uuid>("SELECT owner_id FROM teams WHERE id = $1 FOR UPDATE") + .bind(team_id) + .fetch_optional(conn) + .await + .map_err(|e| { + error!(error = %e, team_id = %team_id, "Failed to lock team"); + StatusCode::INTERNAL_SERVER_ERROR + })? + .ok_or(StatusCode::NOT_FOUND) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::permissions::{with_dependencies, ALL_PERMISSIONS, BUILTIN_ROLES, PERM_ADMINISTRATOR, PERM_CONNECT, PERM_VIEW_SECRETS}; + + fn builtin(name: &str) -> Authority { + let (_, permissions, _) = BUILTIN_ROLES.iter().find(|(n, _, _)| *n == name).unwrap(); + Authority { permissions: with_dependencies(*permissions), owner: name == "owner" } + } + + #[test] + fn only_an_owner_covers_the_owner_role() { + let owner_role = builtin("owner"); + assert!(builtin("owner").covers(owner_role)); + let all_bits_non_owner = Authority { permissions: ALL_PERMISSIONS, owner: false }; + assert!(!all_bits_non_owner.covers(owner_role)); + for name in ["manager", "editor", "member", "connect-only"] { + assert!(!builtin(name).covers(owner_role), "{name}"); + } + } + + #[test] + fn builtin_roles_cover_exactly_the_roles_at_or_below_them() { + let ranked = ["owner", "manager", "editor", "member", "connect-only"]; + for (i, actor) in ranked.iter().enumerate() { + for (j, target) in ranked.iter().enumerate() { + assert_eq!(builtin(actor).covers(builtin(target)), i <= j, "{actor} over {target}"); + } + } + } + + fn sample_masks() -> impl Iterator { + let mut x: u64 = 0x9E37_79B9_7F4A_7C15; + (0..400).map(move |_| { + x ^= x << 13; + x ^= x >> 7; + x ^= x << 17; + (x as i64) & ALL_PERMISSIONS + }) + } + + #[test] + fn covers_is_reflexive_and_transitive_over_held_authority() { + let held: Vec = sample_masks() + .zip([false, true].into_iter().cycle()) + .map(|(p, owner)| Authority { permissions: with_dependencies(p), owner }) + .collect(); + for a in &held { + assert!(a.covers(*a)); + } + for a in held.iter().take(60) { + for b in held.iter().take(60) { + for c in held.iter().take(60) { + if a.covers(*b) && b.covers(*c) { + assert!(a.covers(*c), "{a:?} {b:?} {c:?}"); + } + } + } + } + } + + #[test] + fn covers_never_lets_a_bit_through_that_the_actor_lacks() { + for actor in sample_masks() { + for bit in 0..=18 { + let p = 1i64 << bit; + let held = Authority { permissions: with_dependencies(actor), owner: false }; + assert_eq!(held.covers(Authority::role(p)), held.permissions & p != 0, "{actor:#x} bit {bit}"); + } + } + } + + #[test] + fn an_administrator_custom_role_cannot_be_handed_out_by_a_manager() { + assert!(!builtin("manager").covers(Authority::role(PERM_ADMINISTRATOR))); + assert!(!builtin("manager").covers(Authority::role(ALL_PERMISSIONS))); + } + + #[test] + fn a_secret_bit_inert_alone_still_needs_cover() { + assert!(!Authority::role(PERM_CONNECT).covers(Authority::role(PERM_VIEW_SECRETS))); + } +} diff --git a/src/test_support.rs b/src/test_support.rs index 0742154..1fad58a 100644 --- a/src/test_support.rs +++ b/src/test_support.rs @@ -307,6 +307,13 @@ pub async fn member_with_role(pool: &PgPool, team: Uuid, perms: i64) -> Uuid { /// Set a user's `subscription_tier` (e.g. "business"). Used by tier-gated /// handler tests; the seed default is 'free'. +/// A member holding the builtin `member` role's bits plus `extra`, so they may admit someone as `member`. +pub async fn inviter_with(pool: &PgPool, team: Uuid, extra: i64) -> Uuid { + seed_builtin_roles(pool, team).await; + let (_, member, _) = crate::permissions::BUILTIN_ROLES.iter().find(|(name, _, _)| *name == "member").expect("member role"); + member_with_role(pool, team, member | extra).await +} + pub async fn set_user_tier(pool: &PgPool, user: Uuid, tier: &str) { sqlx::query("UPDATE users SET subscription_tier = $1 WHERE id = $2") .bind(tier)