From 6cd15e380a57e0d20e99d4ccc7b6dc3bc6dd5440 Mon Sep 17 00:00:00 2001 From: Brent Schroeter Date: Wed, 10 Dec 2025 22:01:47 +0000 Subject: [PATCH] apply auth checks when accessing workspace records --- phono-models/src/accessors/mod.rs | 37 +---- phono-models/src/accessors/portal.rs | 23 +-- phono-models/src/accessors/workspace.rs | 138 ++++++++++++++++++ phono-models/src/errors.rs | 53 +++++-- phono-models/src/workspace_user_perm.rs | 86 +++++++---- phono-server/src/errors.rs | 4 +- .../routes/workspaces_multi/add_handlers.rs | 3 +- .../routes/workspaces_multi/list_handlers.rs | 9 ++ .../add_service_credential_handler.rs | 19 ++- .../workspaces_single/add_table_handler.rs | 27 +++- .../routes/workspaces_single/nav_handler.rs | 19 ++- .../service_credentials_handler.rs | 27 ++-- .../workspaces_single/settings_handler.rs | 21 ++- .../workspaces_single/update_name_handler.rs | 25 +++- ...update_service_cred_permissions_handler.rs | 18 ++- phono-server/src/workspace_pooler.rs | 7 +- 16 files changed, 389 insertions(+), 127 deletions(-) create mode 100644 phono-models/src/accessors/workspace.rs diff --git a/phono-models/src/accessors/mod.rs b/phono-models/src/accessors/mod.rs index 202719c..e7dcdaf 100644 --- a/phono-models/src/accessors/mod.rs +++ b/phono-models/src/accessors/mod.rs @@ -1,44 +1,19 @@ -use std::sync::Arc; - -use derive_builder::UninitializedFieldError; use uuid::Uuid; -pub mod portal; +use crate::errors::AccessError; -#[derive(Clone, Copy, Debug, PartialEq)] +pub mod portal; +pub mod workspace; + +#[derive(Clone, Copy, Debug, PartialEq, strum::Display)] pub enum Actor { /// Bypass explicit auth checks. Bypass, + #[strum(to_string = "User({0})")] User(Uuid), } -/// Encompasses all possible (non-fatal) errors encountered while executing an -/// [`Accessor`]'s `fetch_one()` or `fetch_optional()` methods. -#[derive(Clone, Debug, thiserror::Error)] -pub enum AccessError { - #[error("database error: {0}")] - Database(Arc), - - #[error("not found or access denied")] - NotFound, - - #[error("incomplete access spec: {0}")] - Incomplete(UninitializedFieldError), -} - -impl From for AccessError { - fn from(value: sqlx::Error) -> Self { - Self::Database(Arc::new(value)) - } -} - -impl From for AccessError { - fn from(value: UninitializedFieldError) -> Self { - Self::Incomplete(value) - } -} - /// Provides methods for fetching database entities of type `T`, typically with /// authorization checks. pub trait Accessor: Default + Sized { diff --git a/phono-models/src/accessors/portal.rs b/phono-models/src/accessors/portal.rs index dbd1c4a..0e87dad 100644 --- a/phono-models/src/accessors/portal.rs +++ b/phono-models/src/accessors/portal.rs @@ -6,7 +6,7 @@ use phono_backends::{ pg_role::RoleTree, rolnames::ROLE_PREFIX_USER, }; use sqlx::postgres::types::Oid; -use tracing::{Instrument, info, info_span}; +use tracing::{Instrument, debug, info_span}; use uuid::Uuid; use crate::{client::AppDbClient, portal::Portal}; @@ -84,21 +84,25 @@ impl<'a> Accessor for PortalAccessor<'a> { async fn fetch_one(self) -> Result { let spec = self.build()?; async { + debug!("accessing portal"); let portal = Portal::with_id(spec.id) .fetch_optional(spec.using_app_db) .await? - .ok_or(AccessError::NotFound)?; + .ok_or_else(|| { + debug!("portal not found"); + AccessError::NotFound + })?; spec.verify_workspace_id .is_none_or(|value| portal.workspace_id == value) .ok_or_else(|| { - info!("workspace_id check failed for portal"); + debug!("workspace_id check failed for portal"); AccessError::NotFound })?; spec.verify_rel_oid .is_none_or(|value| portal.class_oid == value) .ok_or_else(|| { - info!("rel_oid check failed for portal"); + debug!("rel_oid check failed for portal"); AccessError::NotFound })?; @@ -109,7 +113,7 @@ impl<'a> Accessor for PortalAccessor<'a> { .fetch_optional(spec.using_workspace_client) .await? .ok_or_else(|| { - info!("unable to fetch PgClass for portal"); + debug!("unable to fetch PgClass for portal"); AccessError::NotFound })? }; @@ -137,7 +141,7 @@ impl<'a> Accessor for PortalAccessor<'a> { .iter() .any(|privilege| privilege.privilege == PgPrivilegeType::Connect) }) { - info!("actor lacks postgres connect privileges"); + debug!("actor lacks postgres connect privileges"); return Err(AccessError::NotFound); } @@ -182,7 +186,7 @@ impl<'a> Accessor for PortalAccessor<'a> { } } if !actor_permissions.is_superset(&spec.verify_rel_permissions) { - info!("actor lacks postgres privileges"); + debug!("actor lacks postgres privileges"); return Err(AccessError::NotFound); } @@ -200,7 +204,7 @@ impl<'a> Accessor for PortalAccessor<'a> { .iter() .any(|value| value.rolname == actor_rolname)) { - info!("actor is not relation owner"); + debug!("actor is not relation owner"); return Err(AccessError::NotFound); } } @@ -208,8 +212,9 @@ impl<'a> Accessor for PortalAccessor<'a> { Ok(portal) } .instrument(info_span!( - "PortalAccessor::fetch_optional()", + "PortalAccessor::fetch_one()", portal_id = spec.id.to_string(), + actor = spec.as_actor.to_string(), )) .await } diff --git a/phono-models/src/accessors/workspace.rs b/phono-models/src/accessors/workspace.rs new file mode 100644 index 0000000..f23b20f --- /dev/null +++ b/phono-models/src/accessors/workspace.rs @@ -0,0 +1,138 @@ +use derive_builder::Builder; +use phono_backends::{ + client::WorkspaceClient, pg_acl::PgPrivilegeType, pg_database::PgDatabase, + rolnames::ROLE_PREFIX_USER, +}; +use tracing::{Instrument as _, debug, info_span}; +use uuid::Uuid; + +use crate::{ + client::AppDbClient, + errors::{AccessError, QueryResult}, + workspace::Workspace, + workspace_user_perm::WorkspaceMembership, +}; + +use super::{Accessor, Actor}; + +/// Utility for fetching a [`Workspace`], with authorization. +#[derive(Builder, Debug)] +#[builder( + // Build fn should only be called internally, via `fetch_optional()`. + build_fn(private, error = "AccessError"), + // Callers interact primarily with the generated builder struct. + name = "WorkspaceAccessor", + vis = "pub", + // "Owned" pattern circumvents the `Clone` trait bound on fields. + pattern = "owned", +)] +struct GeneratedWorkspaceAccessor<'a> { + /// Required. ID of the workspace to be accessed. + id: Uuid, + + /// Required. Identity against which to evaluate authorization checks. + as_actor: Actor, + + /// Required. Client for the backing database. Providing a client authorized + /// as the root Phonograph user will not compromise the integrity of the + /// authorization checks. + using_workspace_client: &'a mut WorkspaceClient, + + /// Required. Client for the application database. + using_app_db: &'a mut AppDbClient, +} + +impl<'a> Accessor for WorkspaceAccessor<'a> { + async fn fetch_one(self) -> Result { + let spec = self.build()?; + async { + debug!("accessing workspace"); + let workspace = if let Some(value) = Workspace::with_id(spec.id) + .fetch_optional(spec.using_app_db) + .await? + { + value + } else { + debug!("workspace access denied: workspace not found"); + clear_workspace_membership(spec.id, spec.as_actor, spec.using_app_db).await?; + return Err(AccessError::NotFound); + }; + debug!("workspace found"); + + let actor_rolname = match spec.as_actor { + Actor::Bypass => None, + Actor::User(user_id) => Some(format!( + "{ROLE_PREFIX_USER}{user_id}", + user_id = user_id.simple() + )), + }; + + if let Some(actor_rolname) = actor_rolname { + // Verify database CONNECT permissions. + let pg_db = PgDatabase::current() + .fetch_one(spec.using_workspace_client) + .await?; + if !pg_db.datacl.unwrap_or_default().iter().any(|acl| { + // Currently database connect permissions are always granted + // directly, though this may change in the future. + // TODO: Generalize to inherited roles + acl.grantee == actor_rolname + && acl + .privileges + .iter() + .any(|privilege| privilege.privilege == PgPrivilegeType::Connect) + }) { + debug!("workspace access denied: actor lacks postgres connect privilege"); + clear_workspace_membership(spec.id, spec.as_actor, spec.using_app_db).await?; + return Err(AccessError::NotFound); + } + } + + debug!("workspace access approved"); + cache_workspace_membership(spec.id, spec.as_actor, spec.using_app_db).await?; + Ok(workspace) + } + .instrument(info_span!( + "WorkspaceAccessor::fetch_one()", + workspace_id = spec.id.to_string(), + actor = spec.as_actor.to_string(), + )) + .await + } +} + +async fn clear_workspace_membership( + workspace_id: Uuid, + actor: Actor, + app_db: &mut AppDbClient, +) -> QueryResult<()> { + if let Actor::User(user_id) = actor { + WorkspaceMembership::delete() + .user_id(user_id) + .workspace_id(workspace_id) + .execute(app_db) + .await?; + debug!("cleared workspace membership cache entry if present"); + } else { + debug!("no action for workspace membership cache: actor is \"{actor}\""); + } + Ok(()) +} + +async fn cache_workspace_membership( + workspace_id: Uuid, + actor: Actor, + app_db: &mut AppDbClient, +) -> QueryResult<()> { + if let Actor::User(user_id) = actor { + WorkspaceMembership::upsert() + .user_id(user_id) + .workspace_id(workspace_id) + .execute(app_db) + .await?; + debug!("cached workspace membership"); + } else { + debug!("no action for workspace membership cache: actor is \"{actor}\""); + } + Ok(()) +} diff --git a/phono-models/src/errors.rs b/phono-models/src/errors.rs index eeb533a..c22ff2a 100644 --- a/phono-models/src/errors.rs +++ b/phono-models/src/errors.rs @@ -1,21 +1,54 @@ +use std::sync::Arc; + use thiserror::Error; -#[derive(Debug, Error)] +/// Any error encountered while building, validating, or executing a query +/// struct. +#[derive(Clone, Debug, Error)] pub enum QueryError { - #[error("query validation failed: {0}")] - ValidationErrors(validator::ValidationErrors), #[error("sqlx error: {0}")] - SqlxError(sqlx::Error), -} + Database(Arc), -impl From for QueryError { - fn from(value: validator::ValidationErrors) -> Self { - Self::ValidationErrors(value) - } + #[error("query validation failed: {0}")] + Validation(validator::ValidationErrors), + + #[error("uninitialized field: {0}")] + Incomplete(derive_builder::UninitializedFieldError), } impl From for QueryError { fn from(value: sqlx::Error) -> Self { - Self::SqlxError(value) + Self::Database(Arc::new(value)) + } +} + +impl From for QueryError { + fn from(value: validator::ValidationErrors) -> Self { + Self::Validation(value) + } +} + +impl From for QueryError { + fn from(value: derive_builder::UninitializedFieldError) -> Self { + Self::Incomplete(value) + } +} + +pub type QueryResult = Result; + +/// Encompasses all possible (non-fatal) errors encountered while executing an +/// [`Accessor`]'s `fetch_one()` or `fetch_optional()` methods. +#[derive(Clone, Debug, thiserror::Error)] +pub enum AccessError { + #[error("not found or access denied")] + NotFound, + + #[error(transparent)] + Query(QueryError), +} + +impl> From for AccessError { + fn from(value: T) -> Self { + Self::Query(value.into()) } } diff --git a/phono-models/src/workspace_user_perm.rs b/phono-models/src/workspace_user_perm.rs index 253f995..820bb74 100644 --- a/phono-models/src/workspace_user_perm.rs +++ b/phono-models/src/workspace_user_perm.rs @@ -1,9 +1,12 @@ use derive_builder::Builder; use serde::{Deserialize, Serialize}; -use sqlx::query_as; +use sqlx::{query, query_as}; use uuid::Uuid; -use crate::client::AppDbClient; +use crate::{ + client::AppDbClient, + errors::{QueryError, QueryResult}, +}; /// Assigns an access control permission on a workspace to a user. These are /// derived from the permission grants of the workspace's backing database. @@ -29,9 +32,15 @@ impl WorkspaceMembership { BelongingToUserQuery { id } } - /// Build an insert statement to create a new object. - pub fn insert() -> InsertBuilder { - InsertBuilder::default() + /// Build an upsert statement to create a new object. If the new object is a + /// duplicate, no update is performed. + pub fn upsert() -> UpsertBuilder { + UpsertBuilder::default() + } + + /// Build a delete statement to remove the matching record if it exists. + pub fn delete() -> DeleteBuilder { + DeleteBuilder::default() } } @@ -66,38 +75,53 @@ where p.user_id = $1 } #[derive(Builder, Clone, Debug)] -pub struct Insert { +#[builder(build_fn(error = "QueryError"), vis = "pub", pattern = "owned")] +struct Upsert { workspace_id: Uuid, user_id: Uuid, } -impl Insert { - pub async fn execute( - self, - app_db: &mut AppDbClient, - ) -> Result { - query_as!( - WorkspaceMembership, +impl UpsertBuilder { + pub async fn execute(self, app_db: &mut AppDbClient) -> QueryResult<()> { + // To circumvent performance and complexity concerns, this method does not + // return the upserted record. Refer to: + // https://stackoverflow.com/a/42217872 + + let spec = self.build()?; + query!( r#" -with p as ( - insert into workspace_memberships (workspace_id, user_id) values ($1, $2) - returning - id, - workspace_id, - user_id -) -select - p.id as id, - p.workspace_id as workspace_id, - p.user_id as user_id, - w.display_name as workspace_display_name -from p inner join workspaces as w - on w.id = p.workspace_id +insert into workspace_memberships (workspace_id, user_id) values ($1, $2) +on conflict (workspace_id, user_id) do nothing "#, - self.workspace_id, - self.user_id, + spec.workspace_id, + spec.user_id, ) - .fetch_one(app_db.get_conn()) - .await + .execute(app_db.get_conn()) + .await?; + Ok(()) + } +} + +#[derive(Builder, Clone, Debug)] +#[builder(build_fn(error = "QueryError"), vis = "pub", pattern = "owned")] +pub struct Delete { + workspace_id: Uuid, + user_id: Uuid, +} + +impl DeleteBuilder { + pub async fn execute(self, app_db: &mut AppDbClient) -> QueryResult<()> { + let spec = self.build()?; + query!( + r#" +delete from workspace_memberships +where workspace_id = $1 and user_id = $2 +"#, + spec.workspace_id, + spec.user_id, + ) + .execute(app_db.get_conn()) + .await?; + Ok(()) } } diff --git a/phono-server/src/errors.rs b/phono-server/src/errors.rs index 35e29be..0b6756b 100644 --- a/phono-server/src/errors.rs +++ b/phono-server/src/errors.rs @@ -64,7 +64,7 @@ impl IntoResponse for AppError { } Self::TooManyRequests(client_message) => { // Debug level so that if this is from a runaway loop, it won't - // overwhelm server logs + // overwhelm production logs. tracing::debug!("Too many requests: {}", client_message); (StatusCode::TOO_MANY_REQUESTS, client_message).into_response() } @@ -76,7 +76,7 @@ impl IntoResponse for AppError { } } -// Easily convert semi-arbitrary errors to InternalServerError +// Easily convert semi-arbitrary errors to InternalServerError. impl From for AppError where E: Into, diff --git a/phono-server/src/routes/workspaces_multi/add_handlers.rs b/phono-server/src/routes/workspaces_multi/add_handlers.rs index 1b0a4a3..957f2f3 100644 --- a/phono-server/src/routes/workspaces_multi/add_handlers.rs +++ b/phono-server/src/routes/workspaces_multi/add_handlers.rs @@ -126,10 +126,9 @@ async fn grant_workspace_membership( .execute(workspace_root_client.get_conn()) .await?; - WorkspaceMembership::insert() + WorkspaceMembership::upsert() .workspace_id(workspace.id) .user_id(user.id) - .build()? .execute(app_db_client) .await?; diff --git a/phono-server/src/routes/workspaces_multi/list_handlers.rs b/phono-server/src/routes/workspaces_multi/list_handlers.rs index 2dde87e..c850937 100644 --- a/phono-server/src/routes/workspaces_multi/list_handlers.rs +++ b/phono-server/src/routes/workspaces_multi/list_handlers.rs @@ -19,6 +19,15 @@ pub(super) async fn get( navigator: Navigator, AppDbConn(mut app_db): AppDbConn, ) -> Result { + // Workspace memberships may be pulled directly from the cache table without + // additional auth checks in this case, because at worst it should only + // inaccurately give the user the impression that they still have access to + // a previously visible workspace. In a specific failure mode, this may + // allow the user to continue seeing updates to the workspace name after + // access has been revoked, until the cache record is cleared, e.g. by the + // user attempting to actually access the data of the workspace. This isn't + // ideal, but it should only happen under exceedingly rare circumstances and + // in those cases is not expected to be catastrophic. let workspace_perms = WorkspaceMembership::belonging_to_user(user.id) .fetch_all(&mut app_db) .await?; diff --git a/phono-server/src/routes/workspaces_single/add_service_credential_handler.rs b/phono-server/src/routes/workspaces_single/add_service_credential_handler.rs index 1354b42..6cb5ff4 100644 --- a/phono-server/src/routes/workspaces_single/add_service_credential_handler.rs +++ b/phono-server/src/routes/workspaces_single/add_service_credential_handler.rs @@ -7,7 +7,10 @@ use phono_backends::{ escape_identifier, rolnames::{ROLE_PREFIX_SERVICE_CRED, SERVICE_CRED_CONN_LIMIT, SERVICE_CRED_SUFFIX_LEN}, }; -use phono_models::{service_cred::ServiceCred, workspace::Workspace}; +use phono_models::{ + accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}, + service_cred::ServiceCred, +}; use rand::distributions::{Alphanumeric, DistString}; use redact::Secret; use serde::Deserialize; @@ -38,16 +41,20 @@ pub(super) async fn post( navigator: Navigator, Path(PathParams { workspace_id }): Path, ) -> Result { - // FIXME: auth and csrf - - let workspace = Workspace::with_id(workspace_id) - .fetch_one(&mut app_db) - .await?; + // FIXME: CSRF let mut workspace_client = pooler .acquire_for(workspace_id, RoleAssignment::User(user.id)) .await?; + let workspace = WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_app_db(&mut app_db) + .using_workspace_client(&mut workspace_client) + .fetch_one() + .await?; + let rolname = format!( "{ROLE_PREFIX_SERVICE_CRED}{uid}_{suffix}", uid = user.id.simple(), diff --git a/phono-server/src/routes/workspaces_single/add_table_handler.rs b/phono-server/src/routes/workspaces_single/add_table_handler.rs index 813f0e3..ebfab40 100644 --- a/phono-server/src/routes/workspaces_single/add_table_handler.rs +++ b/phono-server/src/routes/workspaces_single/add_table_handler.rs @@ -9,11 +9,13 @@ use phono_backends::{ ROLE_PREFIX_USER, }, }; +use phono_models::accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}; use serde::Deserialize; use sqlx::{Acquire as _, query}; use uuid::Uuid; use crate::{ + app::AppDbConn, errors::AppError, navigator::{Navigator, NavigatorPage as _}, user::CurrentUser, @@ -35,23 +37,34 @@ pub(super) struct PathParams { /// deserialize to a UUID. pub(super) async fn post( State(mut pooler): State, + AppDbConn(mut app_db): AppDbConn, CurrentUser(user): CurrentUser, navigator: Navigator, Path(PathParams { workspace_id }): Path, ) -> Result { - // FIXME: CSRF, Check workspace authorization. + // FIXME: CSRF + + // TODO: Condition table creation on schema "CREATE" privileges, which will + // allow this client to be configured with user-level permissions rather + // than root-level. + let mut root_client = pooler + .acquire_for(workspace_id, RoleAssignment::Root) + .await?; + + // For authorization only. + WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_workspace_client(&mut root_client) + .using_app_db(&mut app_db) + .fetch_one() + .await?; const NAME_LEN_WORDS: usize = 3; let table_name = phono_namegen::default_generator() .with_separator('_') .generate_name(NAME_LEN_WORDS); - let mut root_client = pooler - // FIXME: Should this be scoped down to the unprivileged role after - // setting up the table owner? - .acquire_for(workspace_id, RoleAssignment::Root) - .await?; - let user_rolname = format!("{ROLE_PREFIX_USER}{uid}", uid = user.id.simple()); let rolname_uuid = Uuid::new_v4().simple(); let rolname_table_owner = format!("{ROLE_PREFIX_TABLE_OWNER}{rolname_uuid}"); diff --git a/phono-server/src/routes/workspaces_single/nav_handler.rs b/phono-server/src/routes/workspaces_single/nav_handler.rs index dfb94c9..a5d9659 100644 --- a/phono-server/src/routes/workspaces_single/nav_handler.rs +++ b/phono-server/src/routes/workspaces_single/nav_handler.rs @@ -4,7 +4,10 @@ use axum::{ extract::{Path, State}, response::{Html, IntoResponse}, }; -use phono_models::workspace::Workspace; +use phono_models::{ + accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}, + workspace::Workspace, +}; use serde::Deserialize; use uuid::Uuid; @@ -35,16 +38,18 @@ pub(super) async fn get( navigator: Navigator, State(mut pooler): State, ) -> Result { - // FIXME: Check workspace authorization. - - let workspace = Workspace::with_id(workspace_id) - .fetch_one(&mut app_db) - .await?; - let mut workspace_client = pooler .acquire_for(workspace_id, RoleAssignment::User(user.id)) .await?; + let workspace = WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_app_db(&mut app_db) + .using_workspace_client(&mut workspace_client) + .fetch_one() + .await?; + #[derive(Template)] #[template(path = "workspaces_single/nav.html")] struct ResponseTemplate { diff --git a/phono-server/src/routes/workspaces_single/service_credentials_handler.rs b/phono-server/src/routes/workspaces_single/service_credentials_handler.rs index 455e4fc..503c455 100644 --- a/phono-server/src/routes/workspaces_single/service_credentials_handler.rs +++ b/phono-server/src/routes/workspaces_single/service_credentials_handler.rs @@ -7,7 +7,10 @@ use axum::{ }; use futures::{lock::Mutex, prelude::*, stream}; use phono_backends::{pg_class::PgClass, pg_role::RoleTree}; -use phono_models::{service_cred::ServiceCred, workspace::Workspace}; +use phono_models::{ + accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}, + service_cred::ServiceCred, +}; use redact::Secret; use serde::Deserialize; use url::Url; @@ -35,19 +38,12 @@ pub(super) struct PathParams { #[debug_handler(state = App)] pub(super) async fn get( State(settings): State, + State(mut pooler): State, CurrentUser(user): CurrentUser, AppDbConn(mut app_db): AppDbConn, Path(PathParams { workspace_id }): Path, navigator: Navigator, - State(mut pooler): State, ) -> Result { - // FIXME: auth - - let workspace = Workspace::with_id(workspace_id) - .fetch_one(&mut app_db) - .await?; - let cluster = workspace.fetch_cluster(&mut app_db).await?; - // Mutex is required to use client in async closures. let workspace_client = Mutex::new( pooler @@ -55,6 +51,19 @@ pub(super) async fn get( .await?, ); + let workspace = { + let mut locked_client = workspace_client.lock().await; + WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_app_db(&mut app_db) + .using_workspace_client(&mut locked_client) + .fetch_one() + .await? + }; + + let cluster = workspace.fetch_cluster(&mut app_db).await?; + struct ServiceCredInfo { service_cred: ServiceCred, member_of: Vec, diff --git a/phono-server/src/routes/workspaces_single/settings_handler.rs b/phono-server/src/routes/workspaces_single/settings_handler.rs index 1470aec..3aee8de 100644 --- a/phono-server/src/routes/workspaces_single/settings_handler.rs +++ b/phono-server/src/routes/workspaces_single/settings_handler.rs @@ -4,7 +4,10 @@ use axum::{ extract::{Path, State}, response::{Html, IntoResponse}, }; -use phono_models::workspace::Workspace; +use phono_models::{ + accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}, + workspace::Workspace, +}; use serde::Deserialize; use uuid::Uuid; @@ -34,15 +37,17 @@ pub(super) async fn get( navigator: Navigator, State(mut pooler): State, ) -> Result { - // FIXME: Check workspace authorization. - // permission to access/alter both as needed. - - let workspace = Workspace::with_id(workspace_id) - .fetch_one(&mut app_db) + let mut workspace_client = pooler + .acquire_for(workspace_id, RoleAssignment::User(user.id)) .await?; - let mut workspace_client = pooler - .acquire_for(workspace.id, RoleAssignment::User(user.id)) + // TODO: Limit access to workspace "owners" or equivalent. + let workspace = WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_app_db(&mut app_db) + .using_workspace_client(&mut workspace_client) + .fetch_one() .await?; #[derive(Debug, Template)] diff --git a/phono-server/src/routes/workspaces_single/update_name_handler.rs b/phono-server/src/routes/workspaces_single/update_name_handler.rs index 8236f34..6815800 100644 --- a/phono-server/src/routes/workspaces_single/update_name_handler.rs +++ b/phono-server/src/routes/workspaces_single/update_name_handler.rs @@ -1,4 +1,9 @@ -use axum::{debug_handler, extract::Path, response::Response}; +use axum::{ + debug_handler, + extract::{Path, State}, + response::Response, +}; +use phono_models::accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}; use serde::Deserialize; use sqlx::query; use uuid::Uuid; @@ -10,6 +15,7 @@ use crate::{ extractors::ValidatedForm, navigator::{Navigator, NavigatorPage as _}, user::CurrentUser, + workspace_pooler::{RoleAssignment, WorkspacePooler}, }; #[derive(Debug, Deserialize)] @@ -25,13 +31,26 @@ pub(super) struct FormBody { /// HTTP POST handler for updating a workspace's name. #[debug_handler(state = App)] pub(super) async fn post( + State(mut pooler): State, AppDbConn(mut app_db): AppDbConn, - CurrentUser(_user): CurrentUser, + CurrentUser(user): CurrentUser, navigator: Navigator, Path(PathParams { workspace_id }): Path, ValidatedForm(FormBody { name }): ValidatedForm, ) -> Result { - // FIXME: Check workspace authorization. + let mut workspace_client = pooler + .acquire_for(workspace_id, RoleAssignment::User(user.id)) + .await?; + + // For auth only. + // TODO: Limit access to workspace "owners" or equivalent. + WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_app_db(&mut app_db) + .using_workspace_client(&mut workspace_client) + .fetch_one() + .await?; query!( "update workspaces set display_name = $1 where id = $2", diff --git a/phono-server/src/routes/workspaces_single/update_service_cred_permissions_handler.rs b/phono-server/src/routes/workspaces_single/update_service_cred_permissions_handler.rs index dabdd8b..a1c20c7 100644 --- a/phono-server/src/routes/workspaces_single/update_service_cred_permissions_handler.rs +++ b/phono-server/src/routes/workspaces_single/update_service_cred_permissions_handler.rs @@ -8,7 +8,10 @@ use axum::{ use axum_extra::extract::Form; use futures::{lock::Mutex, prelude::*, stream}; use phono_backends::{escape_identifier, pg_class::PgClass}; -use phono_models::service_cred::ServiceCred; +use phono_models::{ + accessors::{Accessor as _, Actor, workspace::WorkspaceAccessor}, + service_cred::ServiceCred, +}; use serde::Deserialize; use sqlx::query; use uuid::Uuid; @@ -50,6 +53,19 @@ pub(super) async fn post( .await?, ); + { + // Ensure lock is dropped as soon as we're finished with it, or else + // `workspace_client` mutex will deadlock later on. + let mut locked_client = workspace_client.lock().await; + WorkspaceAccessor::new() + .id(workspace_id) + .as_actor(Actor::User(user.id)) + .using_app_db(&mut app_db) + .using_workspace_client(&mut locked_client) + .fetch_one() + .await?; + } + let all_rels = { let mut locked_client = workspace_client.lock().await; PgClass::belonging_to_namespace(PHONO_TABLE_NAMESPACE) diff --git a/phono-server/src/workspace_pooler.rs b/phono-server/src/workspace_pooler.rs index 65aebb3..24559f4 100644 --- a/phono-server/src/workspace_pooler.rs +++ b/phono-server/src/workspace_pooler.rs @@ -84,7 +84,7 @@ discard sequences; .clone()) } - /// Note that while using `set role` simulates impersonation for most data + /// Note that while using `SET ROLE` simulates impersonation for most data /// access and RLS purposes, it is both incomplete and easily reversible: /// some commands and system tables will still behave according to the /// privileges of the session user, and clients relying on this abstraction @@ -98,6 +98,11 @@ discard sequences; let mut client = WorkspaceClient::from_pool_conn(pool.acquire().await?); match set_role { RoleAssignment::User(uid) => { + // TODO: Return error if user does not have "CONNECT" privileges + // on backing database. Note that this change will entail a + // fairly broad refactor of [`phono-server`] code for + // initializing user roles and for performing workspace auth + // checks. client .init_role(&format!("{ROLE_PREFIX_USER}{uid}", uid = uid.simple())) .await?;