From 10037f63507c2032374f74045d28d3a690689f88 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 21:15:36 -0500 Subject: [PATCH] fix: implement revocation for space credentials Signed-off-by: Trezy --- ...00_add_revoked_at_to_space_credentials.sql | 4 + ...00_add_revoked_at_to_space_credentials.sql | 4 + .../docs/experimental/spaces/credentials.md | 14 +- src/spaces/db.rs | 40 ++++++ src/spaces/routes.rs | 22 ++- src/spaces/simplespace.rs | 6 + tests/spaces_credential_revocation.rs | 136 ++++++++++++++++++ tests/spaces_db.rs | 49 +++++++ 8 files changed, 271 insertions(+), 4 deletions(-) create mode 100644 migrations/postgres/20260707000000_add_revoked_at_to_space_credentials.sql create mode 100644 migrations/sqlite/20260707000000_add_revoked_at_to_space_credentials.sql create mode 100644 tests/spaces_credential_revocation.rs diff --git a/migrations/postgres/20260707000000_add_revoked_at_to_space_credentials.sql b/migrations/postgres/20260707000000_add_revoked_at_to_space_credentials.sql new file mode 100644 index 0000000..6911f07 --- /dev/null +++ b/migrations/postgres/20260707000000_add_revoked_at_to_space_credentials.sql @@ -0,0 +1,4 @@ +-- Add revocation support to space credentials (M3). +-- A NULL revoked_at means active; a timestamp means the credential (and any +-- other credential sharing the row) has been revoked and must be rejected. +ALTER TABLE happyview_space_credentials ADD COLUMN revoked_at TEXT; diff --git a/migrations/sqlite/20260707000000_add_revoked_at_to_space_credentials.sql b/migrations/sqlite/20260707000000_add_revoked_at_to_space_credentials.sql new file mode 100644 index 0000000..6911f07 --- /dev/null +++ b/migrations/sqlite/20260707000000_add_revoked_at_to_space_credentials.sql @@ -0,0 +1,4 @@ +-- Add revocation support to space credentials (M3). +-- A NULL revoked_at means active; a timestamp means the credential (and any +-- other credential sharing the row) has been revoked and must be rejected. +ALTER TABLE happyview_space_credentials ADD COLUMN revoked_at TEXT; diff --git a/packages/docs/content/docs/experimental/spaces/credentials.md b/packages/docs/content/docs/experimental/spaces/credentials.md index 81d6621..50876e8 100644 --- a/packages/docs/content/docs/experimental/spaces/credentials.md +++ b/packages/docs/content/docs/experimental/spaces/credentials.md @@ -197,7 +197,19 @@ The JWT payload contains: | `sub` | The full `at://` space URI | | `iat` | Issued at (Unix timestamp) | | `exp` | Expiry (Unix timestamp) | -| `jti` | Random nonce for replay protection | +| `jti` | Random nonce making each issued credential unique | + +The credential is a bearer token: it is reused for the full 2-hour TTL and is not +bound to a specific client or audience, so any service holding it can read the +space's records. Containment relies on the short TTL and on revocation. + +## Revocation + +A credential is invalidated before its TTL expires when its holder is removed +from the space (`com.atproto.simplespace.removeMember`): HappyView records the +revocation and rejects the credential on subsequent reads. Because a credential +is an opaque bearer token, revocation is scoped to a member — removing (and, if +needed, re-adding) a member revokes all of that member's outstanding credentials. ## Using a credential diff --git a/src/spaces/db.rs b/src/spaces/db.rs index adb4473..ace617c 100644 --- a/src/spaces/db.rs +++ b/src/spaces/db.rs @@ -307,6 +307,46 @@ pub async fn remove_member( Ok(result.rows_affected() > 0) } +/// Returns true if a space credential with the given token hash has been revoked. +pub async fn is_space_credential_revoked( + pool: &sqlx::AnyPool, + backend: DatabaseBackend, + token_hash: &str, +) -> Result { + let sql = adapt_sql( + "SELECT revoked_at FROM happyview_space_credentials WHERE token_hash = ? AND revoked_at IS NOT NULL LIMIT 1", + backend, + ); + let row: Option<(String,)> = sqlx::query_as(&sql) + .bind(token_hash) + .fetch_optional(pool) + .await + .map_err(|e| AppError::Internal(format!("failed to check credential revocation: {e}")))?; + Ok(row.is_some()) +} + +/// Revoke all active space credentials issued to `did` within `space_id`. +/// Returns the number of credentials revoked. +pub async fn revoke_space_credentials_for_member( + pool: &sqlx::AnyPool, + backend: DatabaseBackend, + space_id: &str, + did: &str, +) -> Result { + let sql = adapt_sql( + "UPDATE happyview_space_credentials SET revoked_at = ? WHERE space_id = ? AND issued_to = ? AND revoked_at IS NULL", + backend, + ); + let result = sqlx::query(&sql) + .bind(now_rfc3339()) + .bind(space_id) + .bind(did) + .execute(pool) + .await + .map_err(|e| AppError::Internal(format!("failed to revoke space credentials: {e}")))?; + Ok(result.rows_affected()) +} + pub async fn get_member( pool: &sqlx::AnyPool, backend: DatabaseBackend, diff --git a/src/spaces/routes.rs b/src/spaces/routes.rs index 9eef440..ef1be5a 100644 --- a/src/spaces/routes.rs +++ b/src/spaces/routes.rs @@ -357,6 +357,14 @@ fn require_auth(claims: &XrpcClaims) -> Result<&crate::auth::Claims, AppError> { /// Like `require_auth`, but also accepts a verified space credential as an /// identity source. Use this in space endpoints that support `Bearer /// ` in addition to DPoP auth. +/// Whether a verified space credential has been revoked (e.g. its holder was +/// removed from the space). Consulted after signature/exp verification so a +/// leaked or stale credential can be invalidated before its TTL expires (M3). +async fn space_credential_revoked(state: &AppState, token: &str) -> Result { + let token_hash = hex::encode(Sha256::digest(token.as_bytes())); + db::is_space_credential_revoked(&state.db, state.db_backend, &token_hash).await +} + async fn require_auth_or_credential( state: &AppState, claims: &XrpcClaims, @@ -372,6 +380,9 @@ async fn require_auth_or_credential( &state.config.plc_url, ) .await?; + if space_credential_revoked(state, token).await? { + return Err(AppError::Auth("space credential has been revoked".into())); + } return Ok(verified.sub); } @@ -450,13 +461,18 @@ async fn require_membership( .await { Ok(claims) if claims.sub == space_uri => { - // External credential grants read access; write is not supported via space credential - if require_write { + // A revoked credential is treated as invalid — fall through to + // the local membership check rather than granting access. + if space_credential_revoked(state, token).await? { + // fall through + } else if require_write { + // External credential grants read access only. return Err(AppError::Forbidden( "Write access is required for this action".into(), )); + } else { + return Ok(SpaceAccess::Read); } - return Ok(SpaceAccess::Read); } Ok(_) => { // Credential is valid but for a different space — fall through diff --git a/src/spaces/simplespace.rs b/src/spaces/simplespace.rs index d285233..55c5c25 100644 --- a/src/spaces/simplespace.rs +++ b/src/spaces/simplespace.rs @@ -422,6 +422,12 @@ async fn remove_member( return Err(AppError::NotFound("Member not found in this space".into())); } + // Revoke any outstanding space credentials the removed member holds so their + // cross-service access ends immediately rather than lingering for the + // credential's 2h TTL (M3). + db::revoke_space_credentials_for_member(&state.db, state.db_backend, &space.id, &input.did) + .await?; + Ok(Json(serde_json::json!({ "success": true }))) } diff --git a/tests/spaces_credential_revocation.rs b/tests/spaces_credential_revocation.rs new file mode 100644 index 0000000..f6df382 --- /dev/null +++ b/tests/spaces_credential_revocation.rs @@ -0,0 +1,136 @@ +mod common; + +use axum::body::Body; +use axum::http::{Request, StatusCode}; +use happyview::db::now_rfc3339; +use happyview::spaces::db as spaces_db; +use happyview::spaces::types::*; +use serde_json::json; +use serial_test::serial; +use tower::ServiceExt; +use uuid::Uuid; + +use common::app::TestApp; + +const AUTHORITY: &str = "did:plc:cred-authority"; +const MEMBER: &str = "did:plc:cred-holder"; + +fn space_uri() -> String { + format!("at://{AUTHORITY}/space/com.example.cred/main") +} + +async fn enable_spaces(app: &TestApp) { + let (name, value) = app.admin_cookie(); + let req = Request::builder() + .method("PUT") + .uri("/admin/settings/feature.spaces_enabled") + .header(name, value) + .header("content-type", "application/json") + .body(Body::from(json!({ "value": "true" }).to_string())) + .unwrap(); + assert!( + app.router + .clone() + .oneshot(req) + .await + .unwrap() + .status() + .is_success(), + "failed to enable spaces" + ); +} + +/// Removing a member from a space revokes their outstanding space credentials. +/// Before the fix, removeMember left the credential active for its full TTL. +#[tokio::test] +#[serial] +async fn remove_member_revokes_credentials() { + common::require_db!(); + let app = TestApp::new().await; + enable_spaces(&app).await; + + // Create a space and a member. + let space_id = Uuid::new_v4().to_string(); + let now = now_rfc3339(); + let space = Space { + id: space_id.clone(), + did: AUTHORITY.to_string(), + authority_did: AUTHORITY.to_string(), + creator_did: AUTHORITY.to_string(), + type_nsid: "com.example.cred".to_string(), + skey: "main".to_string(), + display_name: None, + description: None, + mint_policy: MintPolicy::MemberList, + app_access: AppAccess::Open, + managing_app_did: None, + config: SpaceConfig::default(), + revision: None, + created_at: now.clone(), + updated_at: now.clone(), + }; + spaces_db::create_space(&app.state.db, app.state.db_backend, &space) + .await + .unwrap(); + spaces_db::add_member( + &app.state.db, + app.state.db_backend, + &SpaceMember { + id: Uuid::new_v4().to_string(), + space_id: space_id.clone(), + did: MEMBER.to_string(), + access: SpaceAccess::Read, + is_delegation: false, + granted_by: Some(AUTHORITY.to_string()), + created_at: now.clone(), + }, + ) + .await + .unwrap(); + + // The member holds an outstanding credential (represented by its hash). + let token_hash = "member-credential-hash"; + let sql = happyview::db::adapt_sql( + "INSERT INTO happyview_space_credentials (id, space_id, issued_to, token_hash, expires_at, created_at) VALUES (?, ?, ?, ?, ?, ?)", + app.state.db_backend, + ); + sqlx::query(&sql) + .bind(Uuid::new_v4().to_string()) + .bind(&space_id) + .bind(MEMBER) + .bind(token_hash) + .bind(&now) + .bind(&now) + .execute(&app.state.db) + .await + .unwrap(); + + assert!( + !spaces_db::is_space_credential_revoked(&app.state.db, app.state.db_backend, token_hash) + .await + .unwrap() + ); + + // The authority removes the member. + let (cookie_name, cookie_val) = + common::auth::admin_cookie_header(AUTHORITY, &app.state.cookie_key); + let req = Request::builder() + .method("POST") + .uri("/xrpc/com.atproto.simplespace.removeMember") + .header(cookie_name, cookie_val) + .header("content-type", "application/json") + .body(Body::from( + json!({ "space": space_uri(), "did": MEMBER }).to_string(), + )) + .unwrap(); + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + // The credential is now revoked. + assert!( + spaces_db::is_space_credential_revoked(&app.state.db, app.state.db_backend, token_hash) + .await + .unwrap(), + "removing a member must revoke their outstanding credentials" + ); +} diff --git a/tests/spaces_db.rs b/tests/spaces_db.rs index 7bdebd3..363532b 100644 --- a/tests/spaces_db.rs +++ b/tests/spaces_db.rs @@ -557,6 +557,55 @@ async fn resolve_members_preserves_read_self() { assert_eq!(access, Some(SpaceAccess::ReadSelf)); } +#[tokio::test] +#[serial] +async fn space_credential_revocation_round_trip() { + common::require_db!(); + let pool = test_db::test_pool().await; + let backend = test_db::test_backend(); + test_db::truncate_all(&pool).await; + + let space_id = new_id(); + let space = make_space(&space_id, "did:plc:cred-owner", "com.example.cred", "cred"); + spaces_db::create_space(&pool, backend, &space) + .await + .expect("create_space failed"); + + let member = "did:plc:cred-member"; + let token_hash = "abc123hash"; + let sql = happyview::db::adapt_sql( + "INSERT INTO happyview_space_credentials (id, space_id, issued_to, token_hash, expires_at, created_at) VALUES (?, ?, ?, ?, ?, ?)", + backend, + ); + sqlx::query(&sql) + .bind(new_id()) + .bind(&space_id) + .bind(member) + .bind(token_hash) + .bind(now_rfc3339()) + .bind(now_rfc3339()) + .execute(&pool) + .await + .expect("insert credential row"); + + assert!( + !spaces_db::is_space_credential_revoked(&pool, backend, token_hash) + .await + .unwrap() + ); + + let revoked = spaces_db::revoke_space_credentials_for_member(&pool, backend, &space_id, member) + .await + .unwrap(); + assert_eq!(revoked, 1); + + assert!( + spaces_db::is_space_credential_revoked(&pool, backend, token_hash) + .await + .unwrap() + ); +} + #[tokio::test] #[serial] async fn remove_member() { -- 2.51.2