From ed57c934dafee35f13005f90b3afd37ef651894c Mon Sep 17 00:00:00 2001 From: Trezy Date: Thu, 30 Apr 2026 10:20:57 -0500 Subject: [PATCH] feat: put permissioned spaces behind an experimental flag --- src/admin/feature_flags.rs | 18 ++ src/admin/mod.rs | 2 + src/admin/settings.rs | 1 + src/delegation/mod.rs | 1 + src/delegation/unlink_account.rs | 9 +- src/error.rs | 23 +++ src/feature_flags.rs | 45 +++++ src/feature_middleware.rs | 35 ++++ src/lib.rs | 2 + src/lua/atproto_api.rs | 36 ++++ src/lua/context.rs | 1 + src/server.rs | 17 +- src/xrpc/procedure.rs | 1 + tests/e2e_feature_flags.rs | 302 +++++++++++++++++++++++++++++++ 14 files changed, 487 insertions(+), 6 deletions(-) create mode 100644 src/admin/feature_flags.rs create mode 100644 src/feature_flags.rs create mode 100644 src/feature_middleware.rs create mode 100644 tests/e2e_feature_flags.rs diff --git a/src/admin/feature_flags.rs b/src/admin/feature_flags.rs new file mode 100644 index 0000000..5e6a972 --- /dev/null +++ b/src/admin/feature_flags.rs @@ -0,0 +1,18 @@ +use axum::Json; +use axum::extract::State; + +use crate::AppState; +use crate::error::AppError; +use crate::feature_flags; + +use super::auth::UserAuth; +use super::permissions::Permission; + +pub(super) async fn list( + State(state): State, + auth: UserAuth, +) -> Result>, AppError> { + auth.require(Permission::SettingsManage).await?; + let flags = feature_flags::list_flags(&state.db, state.db_backend).await; + Ok(Json(flags)) +} diff --git a/src/admin/mod.rs b/src/admin/mod.rs index 5dec953..6a65c66 100644 --- a/src/admin/mod.rs +++ b/src/admin/mod.rs @@ -5,6 +5,7 @@ mod backfill; mod dead_letters; mod domains; mod events; +mod feature_flags; mod labelers; mod lexicons; mod network_lexicons; @@ -72,6 +73,7 @@ pub fn admin_routes(_state: AppState) -> Router { "/labelers/{did}", patch(labelers::update).delete(labelers::delete), ) + .route("/feature-flags", get(feature_flags::list)) .route("/settings", get(settings::list)) .route( "/settings/logo", diff --git a/src/admin/settings.rs b/src/admin/settings.rs index 77b059d..f86c065 100644 --- a/src/admin/settings.rs +++ b/src/admin/settings.rs @@ -17,6 +17,7 @@ use super::types::{SettingEntry, UpsertSettingBody}; const ENV_FALLBACKS: &[(&str, &str)] = &[ ("app_name", "APP_NAME"), ("client_uri", "CLIENT_URI"), + ("feature.spaces_enabled", "FEATURE_SPACES_ENABLED"), ("logo_uri", "LOGO_URI"), ("tos_uri", "TOS_URI"), ("policy_uri", "POLICY_URI"), diff --git a/src/delegation/mod.rs b/src/delegation/mod.rs index f366ff3..9af9c0c 100644 --- a/src/delegation/mod.rs +++ b/src/delegation/mod.rs @@ -42,6 +42,7 @@ impl DelegateRole { } } + #[allow(clippy::should_implement_trait)] pub fn from_str(s: &str) -> Option { match s { "owner" => Some(DelegateRole::Owner), diff --git a/src/delegation/unlink_account.rs b/src/delegation/unlink_account.rs index 2d19477..3251af1 100644 --- a/src/delegation/unlink_account.rs +++ b/src/delegation/unlink_account.rs @@ -55,17 +55,16 @@ pub async fn unlink_account( db::delete_delegated_account(&state.db, state.db_backend, account_did).await?; // Delete the DPoP session for the target account using the stored api_client_id - if let Some(api_client_id) = stored_api_client_id { - if let Err(e) = crate::oauth::sessions::delete_dpop_session( + if let Some(api_client_id) = stored_api_client_id + && let Err(e) = crate::oauth::sessions::delete_dpop_session( &state.db, state.db_backend, &api_client_id, account_did, ) .await - { - tracing::warn!(account_did, %e, "failed to clean up DPoP session on unlink"); - } + { + tracing::warn!(account_did, %e, "failed to clean up DPoP session on unlink"); } log_event( diff --git a/src/error.rs b/src/error.rs index 8cd3a2e..5a814fb 100644 --- a/src/error.rs +++ b/src/error.rs @@ -52,6 +52,7 @@ pub enum AppError { BadGateway(String), BadRequest(String), Conflict(String), + FeatureDisabled(String), Forbidden(String), InsufficientPermissions(String), Internal(String), @@ -78,6 +79,7 @@ impl std::fmt::Display for AppError { AppError::BadGateway(msg) => write!(f, "bad gateway: {msg}"), AppError::BadRequest(msg) => write!(f, "bad request: {msg}"), AppError::Conflict(msg) => write!(f, "conflict: {msg}"), + AppError::FeatureDisabled(msg) => write!(f, "feature disabled: {msg}"), AppError::Forbidden(msg) => write!(f, "forbidden: {msg}"), AppError::InsufficientPermissions(perm) => write!(f, "Missing permission: {perm}"), AppError::Internal(msg) => write!(f, "internal error: {msg}"), @@ -142,6 +144,13 @@ impl IntoResponse for AppError { }); (status, axum::Json(body)).into_response() } + AppError::FeatureDisabled(msg) => { + let body = serde_json::json!({ + "error": "FeatureDisabled", + "message": msg, + }); + (StatusCode::NOT_FOUND, axum::Json(body)).into_response() + } AppError::InsufficientPermissions(perm) => { let body = serde_json::json!({ "error": "InsufficientPermissions", @@ -181,6 +190,7 @@ impl IntoResponse for AppError { AppError::NotFound(msg) => (StatusCode::NOT_FOUND, msg.clone()), AppError::PdsError(..) | AppError::AuthDpopNonce(..) + | AppError::FeatureDisabled(..) | AppError::InsufficientPermissions(..) | AppError::RateLimited { .. } | AppError::ScriptError { .. } => unreachable!(), @@ -275,6 +285,15 @@ mod tests { assert_eq!(body["line"], 5); } + #[tokio::test] + async fn feature_disabled_returns_404() { + let (status, body) = + response_parts(AppError::FeatureDisabled("spaces not enabled".into())).await; + assert_eq!(status, StatusCode::NOT_FOUND); + assert_eq!(body["error"], "FeatureDisabled"); + assert_eq!(body["message"], "spaces not enabled"); + } + #[tokio::test] async fn not_found_returns_404() { let (status, body) = response_parts(AppError::NotFound("no such thing".into())).await; @@ -343,6 +362,10 @@ mod tests { AppError::Internal("z".into()).to_string(), "internal error: z" ); + assert_eq!( + AppError::FeatureDisabled("x".into()).to_string(), + "feature disabled: x" + ); assert_eq!(AppError::NotFound("w".into()).to_string(), "not found: w"); assert_eq!( AppError::PdsError(StatusCode::BAD_GATEWAY, Bytes::new()).to_string(), diff --git a/src/feature_flags.rs b/src/feature_flags.rs new file mode 100644 index 0000000..28cb2ff --- /dev/null +++ b/src/feature_flags.rs @@ -0,0 +1,45 @@ +use sqlx::AnyPool; + +use crate::admin::settings::get_setting; +use crate::db::DatabaseBackend; + +pub struct FeatureFlag; + +impl FeatureFlag { + pub const SPACES_ENABLED: &str = "feature.spaces_enabled"; +} + +pub async fn is_enabled(pool: &AnyPool, key: &str, backend: DatabaseBackend) -> bool { + get_setting(pool, key, backend) + .await + .map(|v| v.eq_ignore_ascii_case("true")) + .unwrap_or(false) +} + +#[derive(serde::Serialize)] +pub struct FeatureFlagStatus { + pub key: String, + pub name: String, + pub description: String, + pub enabled: bool, +} + +pub async fn list_flags(pool: &AnyPool, backend: DatabaseBackend) -> Vec { + let all_flags = [( + FeatureFlag::SPACES_ENABLED, + "Permissioned Spaces", + "Collaborative data spaces with granular permissions, membership, and invites.", + )]; + + let mut result = Vec::new(); + for (key, name, description) in all_flags { + let enabled = is_enabled(pool, key, backend).await; + result.push(FeatureFlagStatus { + key: key.to_string(), + name: name.to_string(), + description: description.to_string(), + enabled, + }); + } + result +} diff --git a/src/feature_middleware.rs b/src/feature_middleware.rs new file mode 100644 index 0000000..58a9c92 --- /dev/null +++ b/src/feature_middleware.rs @@ -0,0 +1,35 @@ +use axum::extract::{Request, State}; +use axum::middleware::Next; +use axum::response::Response; + +use crate::AppState; +use crate::error::AppError; + +async fn require_feature( + flag_key: &'static str, + State(state): State, + req: Request, + next: Next, +) -> Result { + if !crate::feature_flags::is_enabled(&state.db, flag_key, state.db_backend).await { + return Err(AppError::FeatureDisabled(format!( + "The feature '{}' is not currently enabled on this instance", + flag_key + ))); + } + Ok(next.run(req).await) +} + +pub async fn require_spaces( + state: State, + req: Request, + next: Next, +) -> Result { + require_feature( + crate::feature_flags::FeatureFlag::SPACES_ENABLED, + state, + req, + next, + ) + .await +} diff --git a/src/lib.rs b/src/lib.rs index daf60e9..d467b65 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,6 +10,8 @@ pub mod domain_middleware; pub mod error; pub mod event_log; pub mod external_auth; +pub mod feature_flags; +pub mod feature_middleware; pub mod jetstream; pub mod labeler; pub mod lexicon; diff --git a/src/lua/atproto_api.rs b/src/lua/atproto_api.rs index 58e64d6..bc1c614 100644 --- a/src/lua/atproto_api.rs +++ b/src/lua/atproto_api.rs @@ -266,6 +266,15 @@ pub fn register_atproto_api( lua.create_async_function(move |_lua, (space_uri, did): (String, String)| { let state = state_clone.clone(); async move { + if !crate::feature_flags::is_enabled( + &state.db, + crate::feature_flags::FeatureFlag::SPACES_ENABLED, + state.db_backend, + ) + .await + { + return Err(mlua::Error::runtime("spaces feature is not enabled")); + } let uri = crate::spaces::SpaceUri::parse(&space_uri) .map_err(|e| mlua::Error::runtime(format!("invalid space URI: {e}")))?; let space = crate::spaces::db::get_space_by_address( @@ -298,6 +307,15 @@ pub fn register_atproto_api( lua.create_async_function(move |_lua, (space_uri, did): (String, String)| { let state = state_clone.clone(); async move { + if !crate::feature_flags::is_enabled( + &state.db, + crate::feature_flags::FeatureFlag::SPACES_ENABLED, + state.db_backend, + ) + .await + { + return Err(mlua::Error::runtime("spaces feature is not enabled")); + } let uri = crate::spaces::SpaceUri::parse(&space_uri) .map_err(|e| mlua::Error::runtime(format!("invalid space URI: {e}")))?; let space = crate::spaces::db::get_space_by_address( @@ -329,6 +347,15 @@ pub fn register_atproto_api( let list_members_fn = lua.create_async_function(move |lua, space_uri: String| { let state = state_clone.clone(); async move { + if !crate::feature_flags::is_enabled( + &state.db, + crate::feature_flags::FeatureFlag::SPACES_ENABLED, + state.db_backend, + ) + .await + { + return Err(mlua::Error::runtime("spaces feature is not enabled")); + } let uri = crate::spaces::SpaceUri::parse(&space_uri) .map_err(|e| mlua::Error::runtime(format!("invalid space URI: {e}")))?; let space = crate::spaces::db::get_space_by_address( @@ -368,6 +395,15 @@ pub fn register_atproto_api( let query_fn = lua.create_async_function(move |lua, opts: mlua::Table| { let state = state_clone.clone(); async move { + if !crate::feature_flags::is_enabled( + &state.db, + crate::feature_flags::FeatureFlag::SPACES_ENABLED, + state.db_backend, + ) + .await + { + return Err(mlua::Error::runtime("spaces feature is not enabled")); + } let space_uri: String = opts .get("space_uri") .map_err(|_| mlua::Error::runtime("space_uri is required"))?; diff --git a/src/lua/context.rs b/src/lua/context.rs index b62a882..eab578e 100644 --- a/src/lua/context.rs +++ b/src/lua/context.rs @@ -32,6 +32,7 @@ fn set_space_context(lua: &Lua, space: Option<&SpaceContext>) -> LuaResult<()> { } /// Set global context variables for a procedure script. +#[allow(clippy::too_many_arguments)] pub fn set_procedure_context( lua: &Lua, method: &str, diff --git a/src/server.rs b/src/server.rs index 942d1b1..4a0a261 100644 --- a/src/server.rs +++ b/src/server.rs @@ -62,7 +62,12 @@ pub fn router(state: AppState) -> Router { let serve_dir = ServeDir::new(&static_dir).not_found_service(spa_fallback); let domain_routes = Router::new() - .merge(crate::spaces::routes::space_routes()) + .merge( + crate::spaces::routes::space_routes().layer(axum::middleware::from_fn_with_state( + state.clone(), + crate::feature_middleware::require_spaces, + )), + ) .nest("/auth", crate::auth::routes::routes()) .nest("/external-auth", crate::external_auth::routes()) .nest("/oauth", crate::oauth::routes::routes()) @@ -188,6 +193,13 @@ async fn config_endpoint( _ => env!("CARGO_PKG_VERSION"), }; + let spaces_enabled = crate::feature_flags::is_enabled( + pool, + crate::feature_flags::FeatureFlag::SPACES_ENABLED, + backend, + ) + .await; + Json(serde_json::json!({ "public_url": domain_url, "version": version, @@ -199,6 +211,9 @@ async fn config_endpoint( "default_rate_limit_refill_rate": state.config.default_rate_limit_refill_rate, "app_name": app_name, "logo_url": logo_url, + "features": { + "spaces": spaces_enabled, + }, })) } diff --git a/src/xrpc/procedure.rs b/src/xrpc/procedure.rs index 13850d0..ab1d618 100644 --- a/src/xrpc/procedure.rs +++ b/src/xrpc/procedure.rs @@ -346,6 +346,7 @@ async fn handle_delete_record( } } +#[allow(clippy::too_many_arguments)] async fn handle_dpop_procedure( state: &AppState, claims: &Claims, diff --git a/tests/e2e_feature_flags.rs b/tests/e2e_feature_flags.rs new file mode 100644 index 0000000..617d7c9 --- /dev/null +++ b/tests/e2e_feature_flags.rs @@ -0,0 +1,302 @@ +mod common; + +use axum::body::Body; +use axum::http::{Method, Request, StatusCode}; +use http_body_util::BodyExt; +use serde_json::{Value, json}; +use serial_test::serial; +use tower::ServiceExt; + +use common::app::TestApp; + +async fn json_body(resp: axum::response::Response) -> Value { + let body = resp.into_body().collect().await.unwrap().to_bytes(); + serde_json::from_slice(&body).unwrap() +} + +fn admin_get( + uri: &str, + cookie: (axum::http::HeaderName, axum::http::HeaderValue), +) -> Request { + Request::builder() + .uri(uri) + .header(cookie.0, cookie.1) + .body(Body::empty()) + .unwrap() +} + +fn admin_put( + uri: &str, + cookie: (axum::http::HeaderName, axum::http::HeaderValue), + body: &Value, +) -> Request { + Request::builder() + .method(Method::PUT) + .uri(uri) + .header(cookie.0, cookie.1) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_vec(body).unwrap())) + .unwrap() +} + +fn admin_delete( + uri: &str, + cookie: (axum::http::HeaderName, axum::http::HeaderValue), +) -> Request { + Request::builder() + .method(Method::DELETE) + .uri(uri) + .header(cookie.0, cookie.1) + .body(Body::empty()) + .unwrap() +} + +#[tokio::test] +#[serial] +#[ignore] +async fn space_routes_blocked_when_flag_disabled() { + let app = TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/xrpc/dev.happyview.space.list") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let body = json_body(resp).await; + assert_eq!(body["error"], "FeatureDisabled"); +} + +#[tokio::test] +#[serial] +#[ignore] +async fn space_routes_allowed_after_enabling_flag() { + let app = TestApp::new().await; + + // Enable the feature flag + let resp = app + .router + .clone() + .oneshot(admin_put( + "/admin/settings/feature.spaces_enabled", + app.admin_cookie(), + &json!({ "value": "true" }), + )) + .await + .unwrap(); + assert!(resp.status().is_success()); + + // Space routes should now pass through (will get auth error, not FeatureDisabled) + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/xrpc/dev.happyview.space.list") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + let body = json_body(resp).await; + assert_ne!( + body["error"].as_str().unwrap_or(""), + "FeatureDisabled", + "expected request to pass through feature gate" + ); +} + +#[tokio::test] +#[serial] +#[ignore] +async fn space_routes_blocked_again_after_disabling_flag() { + let app = TestApp::new().await; + + // Enable + let resp = app + .router + .clone() + .oneshot(admin_put( + "/admin/settings/feature.spaces_enabled", + app.admin_cookie(), + &json!({ "value": "true" }), + )) + .await + .unwrap(); + assert!(resp.status().is_success()); + + // Disable + let resp = app + .router + .clone() + .oneshot(admin_delete( + "/admin/settings/feature.spaces_enabled", + app.admin_cookie(), + )) + .await + .unwrap(); + assert!(resp.status().is_success()); + + // Space routes should be blocked again + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/xrpc/dev.happyview.space.list") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let body = json_body(resp).await; + assert_eq!(body["error"], "FeatureDisabled"); +} + +#[tokio::test] +#[serial] +#[ignore] +async fn admin_feature_flags_lists_flags() { + let app = TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot(admin_get("/admin/feature-flags", app.admin_cookie())) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + let body = json_body(resp).await; + let flags = body.as_array().expect("expected array"); + assert!(!flags.is_empty()); + + let spaces_flag = flags + .iter() + .find(|f| f["key"] == "feature.spaces_enabled") + .expect("spaces flag not found"); + assert_eq!(spaces_flag["enabled"], false); + assert!(spaces_flag["name"].as_str().is_some()); + assert!(spaces_flag["description"].as_str().is_some()); +} + +#[tokio::test] +#[serial] +#[ignore] +async fn admin_feature_flags_reflects_enabled_state() { + let app = TestApp::new().await; + + // Enable the flag + let resp = app + .router + .clone() + .oneshot(admin_put( + "/admin/settings/feature.spaces_enabled", + app.admin_cookie(), + &json!({ "value": "true" }), + )) + .await + .unwrap(); + assert!(resp.status().is_success()); + + let resp = app + .router + .clone() + .oneshot(admin_get("/admin/feature-flags", app.admin_cookie())) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + let body = json_body(resp).await; + let flags = body.as_array().unwrap(); + let spaces_flag = flags + .iter() + .find(|f| f["key"] == "feature.spaces_enabled") + .unwrap(); + assert_eq!(spaces_flag["enabled"], true); +} + +#[tokio::test] +#[serial] +#[ignore] +async fn config_endpoint_includes_features() { + let app = TestApp::new().await; + + // Default: spaces disabled + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/config") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + let body = json_body(resp).await; + assert_eq!(body["features"]["spaces"], false); + + // Enable the flag + let resp = app + .router + .clone() + .oneshot(admin_put( + "/admin/settings/feature.spaces_enabled", + app.admin_cookie(), + &json!({ "value": "true" }), + )) + .await + .unwrap(); + assert!(resp.status().is_success()); + + // Now config should reflect enabled + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/config") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); + let body = json_body(resp).await; + assert_eq!(body["features"]["spaces"], true); +} + +#[tokio::test] +#[serial] +#[ignore] +async fn admin_feature_flags_requires_auth() { + let app = TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/admin/feature-flags") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); +} -- 2.51.2