diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ef6c728..8f69075 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -64,7 +64,7 @@ In order of importance the following rules describe what "correct" means for Tra and not something said application relies on for proper functioning. There is bound to be edge cases that these rules don't fully cover. -Here common sense, community sentiment, furthering the goals of atproto itself, and ultimately maintainer opinion take precedence over support for any individual applicaion. +Here common sense, community sentiment, furthering the goals of atproto itself, and ultimately maintainer opinion take precedence over support for any individual application. Even Bluesky. The rules above are meant to capture Tranquils goals of being correct while being community oriented and avoiding as much "Bluesky-defaultism" as possible. diff --git a/Cargo.lock b/Cargo.lock index 9c10a09..e8f6f7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -105,6 +105,21 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "250f629c0161ad8107cf89319e990051fae62832fd343083bea452d93e2205fd" +[[package]] +name = "alloc-no-stdlib" +version = "2.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc7bb162ec39d46ab1ca8c77bf72e890535becd1751bb45f64c597edb4c8c6b3" + +[[package]] +name = "alloc-stdlib" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0e76a019e91224d279006ff972f1e984179a6e9feb050adba6ce8274aef23195" +dependencies = [ + "alloc-no-stdlib", +] + [[package]] name = "allocator-api2" version = "0.2.21" @@ -1250,6 +1265,27 @@ dependencies = [ "cfg_aliases", ] +[[package]] +name = "brotli" +version = "8.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5cc91aac060a7a1e25823bdccbfb6af1875b88f17c6daac97894eed8207166b3" +dependencies = [ + "alloc-no-stdlib", + "alloc-stdlib", + "brotli-decompressor", +] + +[[package]] +name = "brotli-decompressor" +version = "5.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3a32acac15fe1967bc3986b2a6347dffc965602354ea6f450ad07e8bfd253583" +dependencies = [ + "alloc-no-stdlib", + "alloc-stdlib", +] + [[package]] name = "bs58" version = "0.5.1" @@ -7713,6 +7749,7 @@ dependencies = [ "base32", "base64 0.22.1", "bcrypt", + "brotli", "chrono", "hmac", "k256", diff --git a/crates/tranquil-auth/Cargo.toml b/crates/tranquil-auth/Cargo.toml index 6a4c981..863c1d9 100644 --- a/crates/tranquil-auth/Cargo.toml +++ b/crates/tranquil-auth/Cargo.toml @@ -24,3 +24,4 @@ subtle = { workspace = true } totp-rs = { workspace = true } urlencoding = { workspace = true } uuid = { workspace = true } +brotli = "8.0.4" diff --git a/crates/tranquil-auth/src/compress.rs b/crates/tranquil-auth/src/compress.rs new file mode 100644 index 0000000..6b75c8d --- /dev/null +++ b/crates/tranquil-auth/src/compress.rs @@ -0,0 +1,183 @@ +use base64::{Engine as _, engine::general_purpose::URL_SAFE_NO_PAD}; +use brotli::{CompressorWriter, Decompressor}; +use std::fmt; +use std::io::{Read, Write}; + +const COMPRESSED_PREFIX: &str = "$br$"; +const QUALITY: u32 = 9; +const WINDOW_BITS: u32 = 16; +const BUFFER_SIZE: usize = 4096; +const MAX_SCOPE_LEN: u64 = 64 * 1024; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ScopeDecodeError { + Base64DecodeFailed, + DecompressFailed, + TooLarge, +} + +impl fmt::Display for ScopeDecodeError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Base64DecodeFailed => write!(f, "Base64 decode of compressed scope failed"), + Self::DecompressFailed => write!(f, "Brotli decompression of scope failed"), + Self::TooLarge => write!(f, "Decompressed scope exceeds maximum length"), + } + } +} + +impl std::error::Error for ScopeDecodeError {} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ScopeEncodeError { + TooLarge, +} + +impl fmt::Display for ScopeEncodeError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::TooLarge => write!(f, "Scope exceeds maximum length"), + } + } +} + +impl std::error::Error for ScopeEncodeError {} + +fn brotli_compress(input: &str) -> Vec { + let mut writer = CompressorWriter::new(Vec::new(), BUFFER_SIZE, QUALITY, WINDOW_BITS); + + writer + .write_all(input.as_bytes()) + .expect("writing to a Vec cannot fail"); + + writer.into_inner() +} + +fn brotli_decompress(input: &[u8]) -> Result { + let mut output = String::new(); + + Decompressor::new(input, BUFFER_SIZE) + .take(MAX_SCOPE_LEN + 1) + .read_to_string(&mut output) + .map_err(|_| ScopeDecodeError::DecompressFailed)?; + + if output.len() as u64 > MAX_SCOPE_LEN { + return Err(ScopeDecodeError::TooLarge); + } + + Ok(output) +} + +pub fn encode_scope(scope: &str) -> Result { + if scope.len() as u64 > MAX_SCOPE_LEN { + return Err(ScopeEncodeError::TooLarge); + } + + let tagged = format!( + "{COMPRESSED_PREFIX}{}", + URL_SAFE_NO_PAD.encode(brotli_compress(scope)) + ); + + if tagged.len() < scope.len() || scope.starts_with(COMPRESSED_PREFIX) { + Ok(tagged) + } else { + Ok(scope.to_owned()) + } +} + +pub fn decode_scope(scope: &str) -> Result { + let Some(encoded) = scope.strip_prefix(COMPRESSED_PREFIX) else { + return Ok(scope.to_owned()); + }; + + let compressed = URL_SAFE_NO_PAD + .decode(encoded) + .map_err(|_| ScopeDecodeError::Base64DecodeFailed)?; + + brotli_decompress(&compressed) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn long_scope() -> String { + let mut scope = String::from("transition:generic transition:chat.bsky"); + for collection in [ + "social.colibri.message", + "social.colibri.community", + "social.colibri.reaction", + "social.colibri.member", + "social.colibri.channel.read", + ] { + scope.push_str(&format!(" repo:{collection}?action=create&action=delete")); + } + scope + } + + #[test] + fn long_scope_roundtrips_through_compression() { + let scope = long_scope(); + let encoded = encode_scope(&scope).unwrap(); + + assert!(encoded.starts_with(COMPRESSED_PREFIX)); + assert!(encoded.len() < scope.len()); + assert_eq!(decode_scope(&encoded).unwrap(), scope); + } + + #[test] + fn short_scope_stays_plaintext() { + let encoded = encode_scope("com.atproto.access").unwrap(); + + assert_eq!(encoded, "com.atproto.access"); + assert_eq!(decode_scope(&encoded).unwrap(), "com.atproto.access"); + } + + #[test] + fn untagged_scope_passes_through() { + assert_eq!( + decode_scope("com.atproto.refresh").unwrap(), + "com.atproto.refresh" + ); + assert_eq!(decode_scope("").unwrap(), ""); + } + + #[test] + fn malformed_compressed_scope_errors_instead_of_panicking() { + assert_eq!( + decode_scope("$br$not valid base64!"), + Err(ScopeDecodeError::Base64DecodeFailed) + ); + assert_eq!( + decode_scope("$br$AAAAAAAAAAAAAAAA"), + Err(ScopeDecodeError::DecompressFailed) + ); + } + + #[test] + fn compression_bomb_is_rejected() { + let bomb = URL_SAFE_NO_PAD.encode(brotli_compress(&"a".repeat(MAX_SCOPE_LEN as usize * 2))); + + assert_eq!( + decode_scope(&format!("{COMPRESSED_PREFIX}{bomb}")), + Err(ScopeDecodeError::TooLarge) + ); + } + + #[test] + fn plaintext_that_looks_compressed_roundtrips() { + let scope = "$br$repo:*"; + let encoded = encode_scope(scope).unwrap(); + + assert!(encoded.starts_with(COMPRESSED_PREFIX)); + assert_eq!(decode_scope(&encoded).unwrap(), scope); + } + + #[test] + fn encode_rejects_oversized_scope() { + let oversized = "a".repeat(MAX_SCOPE_LEN as usize + 1); + + assert_eq!(encode_scope(&oversized), Err(ScopeEncodeError::TooLarge)); + assert!(encode_scope(&"a".repeat(MAX_SCOPE_LEN as usize)).is_ok()); + } +} diff --git a/crates/tranquil-auth/src/lib.rs b/crates/tranquil-auth/src/lib.rs index 1f9b8c6..aed10ed 100644 --- a/crates/tranquil-auth/src/lib.rs +++ b/crates/tranquil-auth/src/lib.rs @@ -1,3 +1,4 @@ +mod compress; mod token; mod totp; mod types; @@ -12,6 +13,8 @@ pub use token::{ create_service_token_hs256, }; +pub use compress::{ScopeDecodeError, ScopeEncodeError, decode_scope, encode_scope}; + pub use totp::{ TotpError, decrypt_totp_secret, encrypt_totp_secret, generate_backup_codes, generate_qr_png_base64, generate_totp_secret, generate_totp_uri, hash_backup_code, diff --git a/crates/tranquil-auth/src/token.rs b/crates/tranquil-auth/src/token.rs index 9088b67..9fee5fb 100644 --- a/crates/tranquil-auth/src/token.rs +++ b/crates/tranquil-auth/src/token.rs @@ -1,7 +1,9 @@ +use crate::compress::encode_scope; + use super::types::{ ActClaim, Claims, Header, SigningAlgorithm, TokenScope, TokenType, TokenWithMetadata, }; -use anyhow::Result; +use anyhow::{Context, Result}; use base64::Engine as _; use base64::engine::general_purpose::URL_SAFE_NO_PAD; use chrono::{DateTime, Duration, Utc}; @@ -205,7 +207,7 @@ fn create_signed_token_pinned( aud: format!("did:web:{}", aud_hostname), exp: expiration, iat: Utc::now().timestamp(), - scope: Some(scope.to_string()), + scope: Some(encode_scope(scope).context("Scope too large to encode")?), lxm: None, jti: jti.clone(), act, @@ -328,7 +330,7 @@ fn create_hs256_token_with_metadata( ), exp: expiration, iat: Utc::now().timestamp(), - scope: Some(scope.to_string()), + scope: Some(encode_scope(scope).context("Scope too large to encode")?), lxm: None, jti: jti.clone(), act: None, diff --git a/crates/tranquil-auth/src/verify.rs b/crates/tranquil-auth/src/verify.rs index fb59fc3..b0f74bf 100644 --- a/crates/tranquil-auth/src/verify.rs +++ b/crates/tranquil-auth/src/verify.rs @@ -1,3 +1,5 @@ +use crate::compress::decode_scope; + use super::types::{ Claims, Header, SigningAlgorithm, TokenData, TokenDecodeError, TokenScope, TokenType, TokenVerifyError, UnsafeClaims, @@ -164,9 +166,15 @@ pub fn verify_token_es256k( .decode(claims_b64) .map_err(|_| TokenVerifyError::Invalid("Base64 decode of claims failed"))?; - let claims: Claims = serde_json::from_slice(&claims_bytes) + let mut claims: Claims = serde_json::from_slice(&claims_bytes) .map_err(|_| TokenVerifyError::Invalid("JSON decode of claims failed"))?; + if let Some(scope) = &claims.scope { + claims.scope = Some( + decode_scope(scope).map_err(|_| TokenVerifyError::Invalid("Invalid token scope"))?, + ); + } + let now = Utc::now().timestamp(); if claims.exp < now { return Err(TokenVerifyError::Expired); @@ -244,9 +252,13 @@ fn verify_token_hs256_internal( .decode(claims_b64) .context("Base64 decode of claims failed")?; - let claims: Claims = + let mut claims: Claims = serde_json::from_slice(&claims_bytes).context("JSON decode of claims failed")?; + if let Some(scope) = &claims.scope { + claims.scope = Some(decode_scope(scope).context("Invalid scope claim encoding")?); + } + let now = Utc::now().timestamp(); if claims.exp < now { return Err(anyhow!("Token expired")); diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs index e36b95d..3afd89b 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs @@ -11,6 +11,8 @@ pub struct ScopeInfo { pub display_name: String, pub granted: Option, pub restricted: bool, + #[serde(skip_serializing_if = "Option::is_none")] + pub effective_scope: Option, } #[derive(Debug, Serialize)] @@ -214,20 +216,29 @@ pub async fn consent_get( let grant_scope_str: Option<&str> = delegation_grant.as_ref().map(|g| g.granted_scopes.as_str()); - let is_restricted = |scope: &str| -> bool { - grant_scope_str.is_some_and(|g| !tranquil_pds::delegation::grant_covers(g, scope)) + let coverage_of = |scope: &str| -> tranquil_pds::delegation::GrantCoverage { + match grant_scope_str { + Some(g) => tranquil_pds::delegation::grant_coverage(g, scope), + None => tranquil_pds::delegation::GrantCoverage::Full, + } }; let make_scope_info = |scope: &str| -> ScopeInfo { + let (restricted, effective_scope) = match coverage_of(scope) { + tranquil_pds::delegation::GrantCoverage::Full => (false, None), + tranquil_pds::delegation::GrantCoverage::Narrowed(narrowed) => (false, Some(narrowed)), + tranquil_pds::delegation::GrantCoverage::Withheld => (true, None), + }; + let described = effective_scope.as_deref().unwrap_or(scope); let (category, required, description, display_name) = - if let Some(def) = tranquil_pds::oauth::scopes::SCOPE_DEFINITIONS.get(scope) { - let desc = if scope == "atproto" && has_granular_scopes { + if let Some(def) = tranquil_pds::oauth::scopes::SCOPE_DEFINITIONS.get(described) { + let desc = if described == "atproto" && has_granular_scopes { "AT Protocol baseline scope (permissions determined by selected options below)" .to_string() } else { def.description.to_string() }; - let name = if scope == "atproto" && has_granular_scopes { + let name = if described == "atproto" && has_granular_scopes { "AT Protocol Access".to_string() } else { def.display_name.to_string() @@ -238,19 +249,19 @@ pub async fn consent_get( desc, name, ) - } else if scope.starts_with("ref:") { + } else if described.starts_with("ref:") { ( "Reference".to_string(), false, "Referenced scope".to_string(), - scope.to_string(), + described.to_string(), ) } else { ( "Other".to_string(), false, - format!("Access to {}", scope), - scope.to_string(), + format!("Access to {}", described), + described.to_string(), ) }; let granted = pref_map.get(scope).copied(); @@ -261,7 +272,8 @@ pub async fn consent_get( description, display_name, granted, - restricted: is_restricted(scope), + restricted, + effective_scope, } }; diff --git a/crates/tranquil-oauth-server/src/endpoints/token/helpers.rs b/crates/tranquil-oauth-server/src/endpoints/token/helpers.rs index ae0df2d..3a56e08 100644 --- a/crates/tranquil-oauth-server/src/endpoints/token/helpers.rs +++ b/crates/tranquil-oauth-server/src/endpoints/token/helpers.rs @@ -43,7 +43,8 @@ pub fn create_access_token_with_delegation( let issuer = format!("https://{}", pds_hostname); let now = Utc::now().timestamp(); let exp = now + ACCESS_TOKEN_EXPIRY_SECONDS; - let actual_scope = scope.unwrap_or("atproto"); + let actual_scope = tranquil_pds::auth::encode_scope(scope.unwrap_or("atproto")) + .map_err(|_| OAuthError::InvalidScope("Scope too large".to_string()))?; let mut payload = json!({ "iss": issuer, "sub": sub.as_str(), diff --git a/crates/tranquil-pds/src/auth/mod.rs b/crates/tranquil-pds/src/auth/mod.rs index 244c994..a728b7d 100644 --- a/crates/tranquil-pds/src/auth/mod.rs +++ b/crates/tranquil-pds/src/auth/mod.rs @@ -43,14 +43,15 @@ pub use scope_verified::{ pub use service::{ServiceTokenClaims, ServiceTokenError, ServiceTokenVerifier, is_service_token}; pub use tranquil_auth::{ - ActClaim, Claims, Header, SigningAlgorithm, TokenData, TokenDecodeError, TokenScope, TokenType, - TokenVerifyError, TokenWithMetadata, TotpError, UnsafeClaims, create_access_token, - create_access_token_hs256, create_access_token_hs256_with_metadata, - create_access_token_with_delegation, create_access_token_with_jti, - create_access_token_with_metadata, create_access_token_with_scope_metadata, - create_refresh_token, create_refresh_token_hs256, create_refresh_token_hs256_with_metadata, - create_refresh_token_with_jti, create_refresh_token_with_metadata, create_service_token, - create_service_token_hs256, generate_backup_codes, generate_qr_png_base64, + ActClaim, Claims, Header, ScopeDecodeError, ScopeEncodeError, SigningAlgorithm, TokenData, + TokenDecodeError, TokenScope, TokenType, TokenVerifyError, TokenWithMetadata, TotpError, + UnsafeClaims, create_access_token, create_access_token_hs256, + create_access_token_hs256_with_metadata, create_access_token_with_delegation, + create_access_token_with_jti, create_access_token_with_metadata, + create_access_token_with_scope_metadata, create_refresh_token, create_refresh_token_hs256, + create_refresh_token_hs256_with_metadata, create_refresh_token_with_jti, + create_refresh_token_with_metadata, create_service_token, create_service_token_hs256, + decode_scope, encode_scope, generate_backup_codes, generate_qr_png_base64, generate_totp_secret, generate_totp_uri, get_algorithm_from_token, get_did_from_token, get_jti_from_token, hash_backup_code, is_backup_code_format, verify_access_token, verify_access_token_hs256, verify_backup_code, verify_refresh_token, diff --git a/crates/tranquil-pds/src/delegation/mod.rs b/crates/tranquil-pds/src/delegation/mod.rs index 9ae5f3f..e793f8d 100644 --- a/crates/tranquil-pds/src/delegation/mod.rs +++ b/crates/tranquil-pds/src/delegation/mod.rs @@ -5,8 +5,8 @@ pub use roles::{ CanAddControllers, CanControlAccounts, verify_can_add_controllers, verify_can_control_accounts, }; pub use scopes::{ - EDITOR_FULL_SCOPES, InvalidDelegationScopeError, OWNER_FULL_SCOPES, SCOPE_PRESETS, ScopePreset, - ValidatedDelegationScope, grant_covers, intersect_scopes, + EDITOR_FULL_SCOPES, GrantCoverage, InvalidDelegationScopeError, OWNER_FULL_SCOPES, + SCOPE_PRESETS, ScopePreset, ValidatedDelegationScope, grant_coverage, intersect_scopes, }; pub use tranquil_db_traits::DelegationActionType; diff --git a/crates/tranquil-pds/src/delegation/scopes.rs b/crates/tranquil-pds/src/delegation/scopes.rs index 347b3db..382e4ef 100644 --- a/crates/tranquil-pds/src/delegation/scopes.rs +++ b/crates/tranquil-pds/src/delegation/scopes.rs @@ -1,6 +1,6 @@ -use std::collections::HashSet; +use std::collections::BTreeSet; -use tranquil_scopes::{covers, parse_scope}; +use tranquil_scopes::{Coverage, ParsedScope, coverage, parse_scope}; pub use tranquil_db_traits::{ DbScope as ValidatedDelegationScope, InvalidScopeError as InvalidDelegationScopeError, @@ -46,69 +46,86 @@ pub const SCOPE_PRESETS: &[ScopePreset] = &[ }, ]; -pub fn intersect_scopes(requested: &str, granted: &str) -> String { - let requested_set: HashSet<&str> = requested.split_whitespace().collect(); - let granted_parsed: Vec = - granted.split_whitespace().map(parse_scope).collect(); - let has_owner_access = owner_access_is_granted(&granted_parsed); - - let mut scopes: Vec<&str> = requested_set - .iter() - .filter(|requested_scope| { - **requested_scope != "atproto" - && transition_scope_is_covered(requested_scope, &granted_parsed, has_owner_access) - }) - .copied() - .chain(requested_set.contains("atproto").then_some("atproto")) - .collect(); - scopes.sort(); - scopes.join(" ") +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum GrantCoverage { + Full, + Narrowed(String), + Withheld, } -pub fn grant_covers(granted: &str, scope: &str) -> bool { +fn scope_coverage(granted: &[ParsedScope], scope: &str, has_owner_access: bool) -> GrantCoverage { if scope == "atproto" { - return true; - } - let granted_parsed: Vec = - granted.split_whitespace().map(parse_scope).collect(); - transition_scope_is_covered( - scope, - &granted_parsed, - owner_access_is_granted(&granted_parsed), - ) -} + return GrantCoverage::Full; + } -fn transition_scope_is_covered( - requested: &str, - granted: &[tranquil_scopes::ParsedScope], - has_owner_access: bool, -) -> bool { - match parse_scope(requested) { - tranquil_scopes::ParsedScope::TransitionGeneric - | tranquil_scopes::ParsedScope::TransitionChat - if has_owner_access => - { - true + let requested = parse_scope(scope); + let covered = || matches!(coverage(granted, &requested), Coverage::Full); + let transition_covered = match &requested { + ParsedScope::TransitionGeneric | ParsedScope::TransitionChat => { + Some(has_owner_access || covered()) } - tranquil_scopes::ParsedScope::TransitionEmail => { + ParsedScope::TransitionEmail => Some( has_owner_access - || any_granted_covers("account:email?action=read", granted) - || any_granted_covers(requested, granted) + || covered() + || matches!( + coverage(granted, &parse_scope("account:email?action=read")), + Coverage::Full + ), + ), + _ => None, + }; + + if let Some(is_covered) = transition_covered { + return if is_covered { + GrantCoverage::Full + } else { + GrantCoverage::Withheld + }; + } + + match coverage(granted, &requested) { + Coverage::Full => GrantCoverage::Full, + Coverage::Narrowed(ParsedScope::Repo(repo)) => { + GrantCoverage::Narrowed(repo.to_scope_string()) } - _ => any_granted_covers(requested, granted), + Coverage::Narrowed(_) => GrantCoverage::Full, + Coverage::Withheld => GrantCoverage::Withheld, } } -fn owner_access_is_granted(granted: &[tranquil_scopes::ParsedScope]) -> bool { +fn owner_access_is_granted(granted: &[ParsedScope]) -> bool { OWNER_FULL_SCOPES .split_whitespace() .filter(|scope| *scope != "atproto") - .all(|scope| any_granted_covers(scope, granted)) + .all(|scope| matches!(coverage(granted, &parse_scope(scope)), Coverage::Full)) +} + +pub fn grant_coverage(granted: &str, scope: &str) -> GrantCoverage { + let granted_parsed = parse_grant(granted); + let has_owner_access = owner_access_is_granted(&granted_parsed); + scope_coverage(&granted_parsed, scope, has_owner_access) } -fn any_granted_covers(requested: &str, granted: &[tranquil_scopes::ParsedScope]) -> bool { - let requested_parsed = parse_scope(requested); - granted.iter().any(|g| covers(g, &requested_parsed)) +fn parse_grant(granted: &str) -> Vec { + granted.split_whitespace().map(parse_scope).collect() +} + +pub fn intersect_scopes(requested: &str, granted: &str) -> String { + let granted_parsed = parse_grant(granted); + let has_owner_access = owner_access_is_granted(&granted_parsed); + + let scopes: BTreeSet = requested + .split_whitespace() + .filter_map(|requested_scope| { + match scope_coverage(&granted_parsed, requested_scope, has_owner_access) { + GrantCoverage::Full => Some(requested_scope.to_string()), + GrantCoverage::Narrowed(narrowed) => Some(narrowed), + GrantCoverage::Withheld => None, + } + }) + .collect(); + + scopes.into_iter().collect::>().join(" ") } #[cfg(test)] @@ -284,12 +301,33 @@ mod tests { } #[test] - fn test_intersect_partial_action_grant_drops_actionless_request() { + fn test_intersect_partial_action_grant_narrows_actionless_request() { let result = intersect_scopes( "repo:app.bsky.feed.post", "repo:*?action=create&action=delete", ); - assert_eq!(result, ""); + assert_eq!( + result, + "repo:app.bsky.feed.post?action=create&action=delete" + ); + } + + #[test] + fn test_intersect_keeps_collapsed_request_under_split_action_grant() { + assert_eq!( + intersect_scopes( + "repo:io.atcr.manifest?action=create&action=delete", + EDITOR_FULL_SCOPES + ), + "repo:io.atcr.manifest?action=create&action=delete" + ); + assert_eq!( + intersect_scopes( + "repo:io.atcr.manifest?action=create&action=delete", + "repo:*?action=create" + ), + "repo:io.atcr.manifest?action=create" + ); } #[test] @@ -326,33 +364,35 @@ mod tests { } #[test] - fn test_grant_covers_matches_intersection() { + fn test_grant_coverage_full_and_withheld() { let granted = "atproto repo:* blob:*/* account:*?action=manage"; - let intersected = intersect_scopes( - "repo:app.bsky.feed.post?action=create identity:* account:*?action=manage", - granted, + assert_eq!(grant_coverage(granted, "atproto"), GrantCoverage::Full); + assert_eq!( + grant_coverage(granted, "repo:app.bsky.feed.post?action=create"), + GrantCoverage::Full ); - assert!(grant_covers( - granted, - "repo:app.bsky.feed.post?action=create" - )); - assert!(grant_covers(granted, "account:*?action=manage")); - assert!(!grant_covers(granted, "identity:*")); assert_eq!( - grant_covers(granted, "identity:*"), - intersected.contains("identity") + grant_coverage(granted, "identity:*"), + GrantCoverage::Withheld ); + assert_eq!(grant_coverage("", "identity:*"), GrantCoverage::Withheld); } #[test] - fn test_grant_covers_atproto_always_true() { - assert!(grant_covers("", "atproto")); - assert!(grant_covers("repo:*", "atproto")); - } - - #[test] - fn test_grant_covers_empty_grant_covers_nothing_else() { - assert!(!grant_covers("", "repo:app.bsky.feed.post?action=create")); - assert!(!grant_covers("", "identity:*")); + fn test_grant_coverage_narrowed_when_grant_is_a_strict_action_subset() { + assert_eq!( + grant_coverage( + EDITOR_FULL_SCOPES, + "repo:io.atcr.manifest?action=create&action=delete" + ), + GrantCoverage::Full + ); + assert_eq!( + grant_coverage( + "atproto repo:*?action=create blob:*/*", + "repo:io.atcr.manifest?action=create&action=delete" + ), + GrantCoverage::Narrowed("repo:io.atcr.manifest?action=create".to_string()) + ); } } diff --git a/crates/tranquil-pds/src/oauth/verify.rs b/crates/tranquil-pds/src/oauth/verify.rs index da1b630..0d54d83 100644 --- a/crates/tranquil-pds/src/oauth/verify.rs +++ b/crates/tranquil-pds/src/oauth/verify.rs @@ -164,7 +164,9 @@ pub fn extract_oauth_token_info(token: &str) -> Result (DelegatedSession, Value, MockServer) { + create_delegated_session_with_grant( + handle_prefix, + redirect_uri, + scope, + tranquil_pds::delegation::OWNER_FULL_SCOPES, + ) + .await +} + +async fn create_delegated_session_with_grant( + handle_prefix: &str, + redirect_uri: &str, + scope: &str, + controller_scopes: &str, ) -> (DelegatedSession, Value, MockServer) { let url = base_url().await; disable_rate_limiting_once(); @@ -119,7 +141,7 @@ async fn create_delegated_session_with_scope( .bearer_auth(&controller_jwt) .json(&json!({ "handle": delegated_handle, - "controllerScopes": tranquil_pds::delegation::OWNER_FULL_SCOPES + "controllerScopes": controller_scopes })) .send() .await @@ -375,6 +397,131 @@ async fn test_delegated_include_scope_shows_granular_on_consent() { ); } +#[tokio::test] +async fn test_delegated_editor_grant_keeps_collapsed_permission_set() { + seed_permission_set(EDITOR_SET_NSID, PERMISSION_SET_MULTI_ACTION_SCOPE).await; + + let scope = format!("atproto include:{}", EDITOR_SET_NSID); + let (session, consent_body, _mock) = create_delegated_session_with_grant( + "pse", + "https://example.com/permset-editor-callback", + &scope, + tranquil_pds::delegation::EDITOR_FULL_SCOPES, + ) + .await; + + let set_entry = consent_body["permission_sets"] + .as_array() + .expect("consent response should have a permission_sets array") + .iter() + .find(|s| s["nsid"].as_str() == Some(EDITOR_SET_NSID)) + .unwrap_or_else(|| { + panic!( + "permission_sets should contain an entry for nsid '{}'. Got: {:?}", + EDITOR_SET_NSID, consent_body + ) + }); + assert_eq!( + set_entry["restricted"].as_bool(), + Some(false), + "an editor grant spells its actions as separate tokens, but it still permits every \ + action in the collapsed set, so the set must not be marked restricted. Got: {:?}", + set_entry + ); + + let payload = decode_jwt_payload(&session.access_token); + let jwt_scope = tranquil_pds::auth::decode_scope( + payload["scope"] + .as_str() + .expect("access token JWT should have a scope claim"), + ) + .expect("JWT scope claim should decode"); + assert!( + jwt_scope.contains(PERMISSION_SET_MULTI_ACTION_SCOPE), + "delegated intersection must narrow the collapsed repo scope rather than discard it, \ + expected '{}' in decoded scope, got: {}", + PERMISSION_SET_MULTI_ACTION_SCOPE, + jwt_scope + ); +} + +#[tokio::test] +async fn test_delegated_consent_shows_the_scope_the_token_will_carry() { + seed_permission_set(SUBSET_SET_NSID, PERMISSION_SET_CREATE_DELETE_SCOPE).await; + + let scope = format!("atproto include:{}", SUBSET_SET_NSID); + let (session, consent_body, _mock) = create_delegated_session_with_grant( + "pss", + "https://example.com/permset-subset-callback", + &scope, + CREATE_ONLY_GRANT, + ) + .await; + + let set_entry = consent_body["permission_sets"] + .as_array() + .expect("consent response should have a permission_sets array") + .iter() + .find(|s| s["nsid"].as_str() == Some(SUBSET_SET_NSID)) + .unwrap_or_else(|| { + panic!( + "permission_sets should contain an entry for nsid '{}'. Got: {:?}", + SUBSET_SET_NSID, consent_body + ) + }); + assert_eq!( + set_entry["restricted"].as_bool(), + Some(false), + "the create action is still granted, so the set stays approvable. Got: {:?}", + set_entry + ); + + let repo = set_entry["expanded"] + .as_array() + .expect("permission_sets entry should have an expanded array") + .iter() + .find(|s| s["scope"].as_str() == Some(PERMISSION_SET_CREATE_DELETE_SCOPE)) + .unwrap_or_else(|| { + panic!( + "expanded[] should list the requested scope '{}'. Got: {:?}", + PERMISSION_SET_CREATE_DELETE_SCOPE, set_entry + ) + }); + assert_eq!( + repo["restricted"].as_bool(), + Some(false), + "a partially-covered scope is neither fully granted nor withheld. Got: {:?}", + repo + ); + let effective_scope = repo["effective_scope"].as_str().unwrap_or_else(|| { + panic!( + "a scope the grant narrows must report the actions it actually confers. Got: {:?}", + repo + ) + }); + assert_eq!(effective_scope, "repo:io.atcr.manifest?action=create"); + + let payload = decode_jwt_payload(&session.access_token); + let jwt_scope = tranquil_pds::auth::decode_scope( + payload["scope"] + .as_str() + .expect("access token JWT should have a scope claim"), + ) + .expect("JWT scope claim should decode"); + assert!( + jwt_scope.split_whitespace().any(|s| s == effective_scope), + "the consent screen must show the scope the token carries, expected '{}' in decoded \ + scope, got: {}", + effective_scope, + jwt_scope + ); + assert!( + !jwt_scope.contains("action=delete"), + "the grant confers no delete action, so the token must not carry one, got: {}", + jwt_scope + ); +} + #[tokio::test] async fn test_grant_row_keeps_include_jwt_carries_expanded() { seed_permission_set(PERMISSION_SET_NSID, PERMISSION_SET_GRANULAR_SCOPE).await; @@ -505,6 +652,129 @@ async fn test_enforcement_uses_expanded_jwt_scope() { ); } +#[tokio::test] +async fn test_long_expanded_scope_is_compressed_in_jwt() { + const BIG_SET_NSID: &str = "io.atcr.authBigApp"; + let collections = [ + "io.atcr.manifest", + "io.atcr.sailor.star", + "io.atcr.tag", + "io.atcr.blueprint", + "io.atcr.artifact", + "io.atcr.channel.read", + ]; + let granular_scope = collections + .iter() + .map(|coll| format!("repo:{}?action=create&action=delete", coll)) + .collect::>() + .join(" "); + seed_permission_set(BIG_SET_NSID, &granular_scope).await; + + let scope = format!("atproto include:{}", BIG_SET_NSID); + let (session, _consent_body, _mock) = create_delegated_session_with_scope( + "psc", + "https://example.com/permset-compress-callback", + &scope, + ) + .await; + + let payload = decode_jwt_payload(&session.access_token); + let jwt_scope = payload["scope"] + .as_str() + .expect("access token JWT should have a scope claim"); + assert!( + jwt_scope.starts_with("$br$"), + "an expanded scope this long should be compressed in the JWT claim, got: {}", + jwt_scope + ); + + let decoded = tranquil_pds::auth::decode_scope(jwt_scope).expect("scope claim should decode"); + for coll in collections { + assert!( + decoded.contains(&format!("repo:{}?action=create", coll)), + "decoded scope should carry {}, got: {}", + coll, + decoded + ); + } + assert!( + !decoded.contains("include:"), + "decoded scope should not contain the raw include: token, got: {}", + decoded + ); + + let url = base_url().await; + let http_client = client(); + + let introspect_res = http_client + .post(format!("{}/oauth/introspect", url)) + .form(&[("token", session.access_token.as_str())]) + .send() + .await + .expect("introspect request failed"); + assert_eq!(introspect_res.status(), StatusCode::OK); + let introspect_body: Value = introspect_res.json().await.unwrap(); + let introspect_scope = introspect_body["scope"] + .as_str() + .expect("introspect response should have a scope string"); + assert_eq!( + introspect_scope, decoded, + "introspect should report the decoded scope" + ); + + let collection = collections[0]; + let create_res = http_client + .post(format!("{}/xrpc/com.atproto.repo.createRecord", url)) + .bearer_auth(&session.access_token) + .json(&json!({ + "repo": session.delegated_did, + "collection": collection, + "validate": false, + "record": { + "$type": collection, + "note": "compressed scope enforcement test", + "createdAt": Utc::now().to_rfc3339() + } + })) + .send() + .await + .expect("createRecord request failed"); + assert_ne!( + create_res.status(), + StatusCode::FORBIDDEN, + "a compressed scope claim must still authorize the collections it covers. Got body: {:?}", + create_res.text().await + ); + + let refresh_res = http_client + .post(format!("{}/oauth/token", url)) + .form(&[ + ("grant_type", "refresh_token"), + ("refresh_token", session.refresh_token.as_str()), + ("client_id", session.client_id.as_str()), + ]) + .send() + .await + .expect("Refresh request failed"); + assert_eq!(refresh_res.status(), StatusCode::OK); + let refresh_body: Value = refresh_res.json().await.unwrap(); + let refreshed_token = refresh_body["access_token"].as_str().unwrap(); + let refreshed_claim = decode_jwt_payload(refreshed_token)["scope"] + .as_str() + .expect("refreshed JWT should have a scope claim") + .to_string(); + assert!( + refreshed_claim.starts_with("$br$"), + "refreshed claim should also be compressed, got: {}", + refreshed_claim + ); + assert_eq!( + tranquil_pds::auth::decode_scope(&refreshed_claim).expect("refreshed scope should decode"), + decoded, + "refresh must yield a byte-identical decoded scope" + ); +} + #[tokio::test] async fn test_consent_post_errors_when_set_unresolvable() { const UNRESOLVABLE_NSID: &str = "io.atcr.authUnresolvableSet"; diff --git a/crates/tranquil-pds/tests/scope_edge_cases.rs b/crates/tranquil-pds/tests/scope_edge_cases.rs index 4319939..bdca3bb 100644 --- a/crates/tranquil-pds/tests/scope_edge_cases.rs +++ b/crates/tranquil-pds/tests/scope_edge_cases.rs @@ -254,15 +254,18 @@ fn test_scope_with_multiple_params() { } #[test] -fn test_scope_invalid_action_ignored() { - let scope = parse_scope("repo:*?action=invalid"); - if let ParsedScope::Repo(repo) = scope { - assert!(repo.actions.contains(&RepoAction::Create)); - assert!(repo.actions.contains(&RepoAction::Update)); - assert!(repo.actions.contains(&RepoAction::Delete)); - } else { - panic!("Expected Repo scope"); - } +fn test_scope_invalid_action_rejects_whole_scope() { + assert!( + matches!( + parse_scope("repo:*?action=invalid"), + ParsedScope::Unknown(_) + ), + "an unrecognized action must not fall back to granting every action" + ); + assert!(matches!( + parse_scope("repo:*?action=create&action=invalid"), + ParsedScope::Unknown(_) + )); } #[test] diff --git a/crates/tranquil-scopes/src/coverage.rs b/crates/tranquil-scopes/src/coverage.rs index 716da42..ea9e50e 100644 --- a/crates/tranquil-scopes/src/coverage.rs +++ b/crates/tranquil-scopes/src/coverage.rs @@ -1,7 +1,8 @@ use crate::parser::{ AccountAction, AccountAttr, AccountScope, BlobScope, IdentityAttr, IdentityScope, ParsedScope, - RepoScope, RpcScope, + RepoAction, RepoScope, RpcScope, }; +use std::collections::HashSet; pub fn covers(granted: &ParsedScope, requested: &ParsedScope) -> bool { use ParsedScope::*; @@ -21,8 +22,8 @@ pub fn covers(granted: &ParsedScope, requested: &ParsedScope) -> bool { } } -fn repo_covers(g: &RepoScope, r: &RepoScope) -> bool { - let collection_ok = match &g.collection { +fn repo_collection_covers(g: &RepoScope, r: &RepoScope) -> bool { + match &g.collection { None => true, Some(gc) => match &r.collection { None => false, @@ -33,8 +34,54 @@ fn repo_covers(g: &RepoScope, r: &RepoScope) -> bool { None => gc == rc, }, }, - }; - collection_ok && r.actions.is_subset(&g.actions) + } +} + +fn repo_covers(g: &RepoScope, r: &RepoScope) -> bool { + repo_collection_covers(g, r) && r.actions.is_subset(&g.actions) +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Coverage { + Full, + Narrowed(ParsedScope), + Withheld, +} + +pub fn coverage(granted: &[ParsedScope], requested: &ParsedScope) -> Coverage { + if let ParsedScope::Repo(r) = requested { + let actions: HashSet = granted + .iter() + .filter_map(|g| match g { + ParsedScope::Repo(g) if repo_collection_covers(g, r) => Some(&g.actions), + _ => None, + }) + .flat_map(|granted_actions| granted_actions.intersection(&r.actions).copied()) + .collect(); + + return match actions.len() { + 0 => Coverage::Withheld, + _ if actions == r.actions => Coverage::Full, + _ => Coverage::Narrowed(ParsedScope::Repo(RepoScope { + collection: r.collection.clone(), + actions, + })), + }; + } + + if granted.iter().any(|g| covers(g, requested)) { + Coverage::Full + } else { + Coverage::Withheld + } +} + +pub fn narrow(granted: &[ParsedScope], requested: &ParsedScope) -> Option { + match coverage(granted, requested) { + Coverage::Full => Some(requested.clone()), + Coverage::Narrowed(scope) => Some(scope), + Coverage::Withheld => None, + } } fn blob_covers(g: &BlobScope, r: &BlobScope) -> bool { @@ -74,13 +121,33 @@ fn identity_covers(g: &IdentityScope, r: &IdentityScope) -> bool { #[cfg(test)] mod tests { - use super::covers; - use crate::parser::parse_scope; + use super::{Coverage, coverage, covers, narrow}; + use crate::parser::{ParsedScope, parse_scope}; fn c(granted: &str, requested: &str) -> bool { covers(&parse_scope(granted), &parse_scope(requested)) } + fn narrowed(granted: &str, requested: &str) -> Option { + let granted: Vec = granted.split_whitespace().map(parse_scope).collect(); + + match narrow(&granted, &parse_scope(requested)) { + Some(ParsedScope::Repo(repo)) => Some(repo.to_scope_string()), + Some(_) => Some(requested.to_string()), + None => None, + } + } + + fn covered(granted: &str, requested: &str) -> Coverage { + let granted: Vec = granted.split_whitespace().map(parse_scope).collect(); + + coverage(&granted, &parse_scope(requested)) + } + + fn narrowed_to(scope: &str) -> Coverage { + Coverage::Narrowed(parse_scope(scope)) + } + #[test] fn repo_wildcard_covers_specific() { assert!(c("repo:*", "repo:app.bsky.feed.post")); @@ -193,4 +260,66 @@ mod tests { assert!(c("weird:token", "weird:token")); assert!(!c("weird:token", "other:token")); } + + #[test] + fn narrow_intersects_repo_actions() { + assert_eq!( + narrowed( + "repo:*?action=create repo:*?action=update repo:*?action=delete", + "repo:io.atcr.manifest?action=create&action=delete" + ), + Some("repo:io.atcr.manifest?action=create&action=delete".to_string()) + ); + assert_eq!( + narrowed( + "repo:*?action=create", + "repo:io.atcr.manifest?action=create&action=delete" + ), + Some("repo:io.atcr.manifest?action=create".to_string()) + ); + assert_eq!( + narrowed( + "repo:*?action=create", + "repo:io.atcr.manifest?action=delete" + ), + None + ); + assert_eq!(narrowed("repo:app.bsky.*?action=create", "repo:*"), None); + assert_eq!( + narrowed("identity:*", "identity:handle"), + Some("identity:handle".to_string()) + ); + } + + #[test] + fn coverage_distinguishes_full_from_narrowed_repo_actions() { + assert_eq!( + covered( + "repo:*?action=create repo:*?action=update repo:*?action=delete", + "repo:io.atcr.manifest?action=create&action=delete" + ), + Coverage::Full + ); + assert_eq!( + covered( + "repo:*?action=create", + "repo:io.atcr.manifest?action=create&action=delete" + ), + narrowed_to("repo:io.atcr.manifest?action=create") + ); + assert_eq!( + covered( + "repo:*?action=create&action=delete", + "repo:io.atcr.manifest" + ), + narrowed_to("repo:io.atcr.manifest?action=create&action=delete") + ); + assert_eq!( + covered( + "repo:*?action=create", + "repo:io.atcr.manifest?action=delete" + ), + Coverage::Withheld + ); + } } diff --git a/crates/tranquil-scopes/src/lib.rs b/crates/tranquil-scopes/src/lib.rs index 22f3ccb..d9fc1e9 100644 --- a/crates/tranquil-scopes/src/lib.rs +++ b/crates/tranquil-scopes/src/lib.rs @@ -5,7 +5,7 @@ mod parser; mod permission_set; mod permissions; -pub use coverage::covers; +pub use coverage::{Coverage, coverage, covers, narrow}; pub use definitions::{ SCOPE_DEFINITIONS, ScopeCategory, ScopeDefinition, format_scope_for_display, get_required_scopes, get_scope_definition, is_valid_scope, diff --git a/crates/tranquil-scopes/src/parser.rs b/crates/tranquil-scopes/src/parser.rs index bab234a..9095243 100644 --- a/crates/tranquil-scopes/src/parser.rs +++ b/crates/tranquil-scopes/src/parser.rs @@ -29,7 +29,13 @@ pub struct RepoScope { pub actions: HashSet, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +impl RepoScope { + pub fn to_scope_string(&self) -> String { + self.to_string() + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum RepoAction { Create, @@ -38,6 +44,8 @@ pub enum RepoAction { } impl RepoAction { + pub const ALL: [RepoAction; 3] = [Self::Create, Self::Update, Self::Delete]; + pub fn parse_str(s: &str) -> Option { match s { "create" => Some(Self::Create), @@ -47,7 +55,7 @@ impl RepoAction { } } - fn as_str(self) -> &'static str { + pub fn as_str(&self) -> &'static str { match self { Self::Create => "create", Self::Update => "update", @@ -280,6 +288,14 @@ fn parse_query_params(query: &str) -> HashMap> { }) } +fn parse_repo_actions(params: &HashMap>) -> Option> { + match params.get("action") { + None => Some(RepoAction::ALL.into_iter().collect()), + Some(values) if values.is_empty() => None, + Some(values) => values.iter().map(|s| RepoAction::parse_str(s)).collect(), + } +} + pub fn parse_scope(scope: &str) -> ParsedScope { match scope { "atproto" => return ParsedScope::Atproto, @@ -299,20 +315,9 @@ pub fn parse_scope(scope: &str) -> ParsedScope { Some(rest.to_string()) }; - let actions: HashSet = params - .get("action") - .map(|action_values| { - action_values - .iter() - .filter_map(|s| RepoAction::parse_str(s)) - .collect() - }) - .filter(|set: &HashSet| !set.is_empty()) - .unwrap_or_else(|| { - [RepoAction::Create, RepoAction::Update, RepoAction::Delete] - .into_iter() - .collect() - }); + let Some(actions) = parse_repo_actions(¶ms) else { + return ParsedScope::Unknown(scope.to_string()); + }; return ParsedScope::Repo(RepoScope { collection, @@ -321,20 +326,10 @@ pub fn parse_scope(scope: &str) -> ParsedScope { } if base == "repo" { - let actions: HashSet = params - .get("action") - .map(|action_values| { - action_values - .iter() - .filter_map(|s| RepoAction::parse_str(s)) - .collect() - }) - .filter(|set: &HashSet| !set.is_empty()) - .unwrap_or_else(|| { - [RepoAction::Create, RepoAction::Update, RepoAction::Delete] - .into_iter() - .collect() - }); + let Some(actions) = parse_repo_actions(¶ms) else { + return ParsedScope::Unknown(scope.to_string()); + }; + return ParsedScope::Repo(RepoScope { collection: None, actions, @@ -478,6 +473,22 @@ mod tests { } } + #[test] + fn test_parse_repo_unrecognized_action_is_not_a_repo_scope() { + assert!(matches!( + parse_scope("repo:app.bsky.feed.post?action=read"), + ParsedScope::Unknown(_) + )); + assert!(matches!( + parse_scope("repo:app.bsky.feed.post?action="), + ParsedScope::Unknown(_) + )); + assert!(matches!( + parse_scope("repo?action=read"), + ParsedScope::Unknown(_) + )); + } + #[test] fn test_parse_blob_wildcard() { let scope = parse_scope("blob:*/*"); diff --git a/crates/tranquil-scopes/src/permission_set.rs b/crates/tranquil-scopes/src/permission_set.rs index 9f64b73..ec2c401 100644 --- a/crates/tranquil-scopes/src/permission_set.rs +++ b/crates/tranquil-scopes/src/permission_set.rs @@ -3,8 +3,8 @@ use hickory_resolver::TokioAsyncResolver; use hickory_resolver::config::{ResolverConfig, ResolverOpts}; use reqwest::Client; use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; -use tracing::debug; +use std::collections::{BTreeMap, BTreeSet, HashMap}; +use tracing::{debug, warn}; use tranquil_types::{Did, Nsid}; #[derive(Debug, thiserror::Error)] @@ -333,45 +333,49 @@ fn is_under_authority(target_nsid: &str, authority: &str) -> bool { .is_some_and(|c| c == '.') } -const DEFAULT_ACTIONS: &[RepoAction] = - &[RepoAction::Create, RepoAction::Update, RepoAction::Delete]; +fn parse_permission_actions(actions: Option<&Vec>) -> Option> { + match actions { + None => Some(RepoAction::ALL.into_iter().collect()), + Some(values) => values + .iter() + .map(|value| { + let parsed = RepoAction::parse_str(value); + if parsed.is_none() { + warn!( + action = %value, + "skipping permission entry with unrecognized repo action" + ); + } + parsed + }) + .collect(), + } +} fn build_expanded_scopes( permissions: &[PermissionEntry], default_aud: Option<&str>, namespace_authority: &str, ) -> String { - let mut scopes: Vec = Vec::new(); + let mut ungrouped_repo_scopes: BTreeMap> = BTreeMap::new(); + let mut rpc_scopes: Vec = Vec::new(); permissions .iter() .for_each(|perm| match perm.resource.as_str() { "repo" => { - if let Some(collections) = &perm.collection { - let actions: Vec = perm - .action - .as_ref() - .map(|actions| { - actions - .iter() - .filter_map(|action| RepoAction::parse_str(action)) - .collect() - }) - .unwrap_or_else(|| DEFAULT_ACTIONS.to_vec()); - + if let Some(collections) = &perm.collection + && let Some(actions) = parse_permission_actions(perm.action.as_ref()) + && !actions.is_empty() + { collections .iter() .filter(|coll| is_under_authority(coll, namespace_authority)) .for_each(|coll| { - actions.iter().copied().for_each(|action| { - scopes.push( - ParsedScope::Repo(RepoScope { - collection: Some(coll.clone()), - actions: HashSet::from([action]), - }) - .to_string(), - ); - }); + ungrouped_repo_scopes + .entry(coll.to_string()) + .or_default() + .extend(actions.iter().copied()); }); } } @@ -382,20 +386,38 @@ fn build_expanded_scopes( lxms.iter() .filter(|lxm| is_under_authority(lxm, namespace_authority)) .for_each(|lxm| { - scopes.push( - ParsedScope::Rpc(RpcScope { - lxm: Some(lxm.clone()), - aud: perm_aud.map(str::to_string), - }) - .to_string(), - ); + let scope = ParsedScope::Rpc(RpcScope { + lxm: Some(lxm.clone()), + aud: perm_aud.map(str::to_string), + }) + .to_string(); + + if !rpc_scopes.contains(&scope) { + rpc_scopes.push(scope); + } }); } } _ => {} }); - scopes.join(" ") + let grouped_repo_scopes: Vec = ungrouped_repo_scopes + .iter() + .map(|(repo, actions)| { + ParsedScope::Repo(RepoScope { + collection: Some(repo.clone()), + actions: actions.iter().copied().collect(), + }) + .to_string() + }) + .collect(); + + let combined_repo_scopes = grouped_repo_scopes.join(" "); + let combined_rpc_scopes = rpc_scopes.join(" "); + + format!("{} {}", combined_repo_scopes, combined_rpc_scopes) + .trim() + .to_string() } #[cfg(test)] @@ -474,10 +496,11 @@ mod tests { }]; let expanded = build_expanded_scopes(&permissions, None, "io.atcr"); - assert!(expanded.contains("repo:io.atcr.manifest?action=create")); - assert!(expanded.contains("repo:io.atcr.manifest?action=delete")); - assert!(expanded.contains("repo:io.atcr.sailor.star?action=create")); - assert!(!expanded.contains("app.bsky.feed.post")); + assert_eq!( + expanded, + "repo:io.atcr.manifest?action=create&action=delete \ + repo:io.atcr.sailor.star?action=create&action=delete" + ); } #[test] @@ -491,9 +514,138 @@ mod tests { }]; let expanded = build_expanded_scopes(&permissions, None, "io.atcr"); - assert!(expanded.contains("repo:io.atcr.manifest?action=create")); - assert!(expanded.contains("repo:io.atcr.manifest?action=update")); - assert!(expanded.contains("repo:io.atcr.manifest?action=delete")); + assert_eq!( + expanded, + "repo:io.atcr.manifest?action=create&action=update&action=delete" + ); + } + + #[test] + fn test_build_expanded_scopes_repo_omitted_action_grants_all() { + let permissions = vec![PermissionEntry { + resource: "repo".to_string(), + action: None, + collection: Some(vec!["io.atcr.manifest".to_string()]), + lxm: None, + aud: None, + }]; + + let expanded = build_expanded_scopes(&permissions, None, "io.atcr"); + assert_eq!( + expanded, "repo:io.atcr.manifest?action=create&action=update&action=delete", + "an omitted action list means all actions" + ); + } + + #[test] + fn test_build_expanded_scopes_repo_empty_action_list_skips_entry() { + let permissions = vec![PermissionEntry { + resource: "repo".to_string(), + action: Some(vec![]), + collection: Some(vec!["io.atcr.manifest".to_string()]), + lxm: None, + aud: None, + }]; + + let expanded = build_expanded_scopes(&permissions, None, "io.atcr"); + assert!( + expanded.is_empty(), + "an explicitly empty action list is invalid, so the entry is skipped rather \ + than expanded to all actions or emitted as a bare `?action=`, got: {expanded}" + ); + } + + #[test] + fn test_build_expanded_scopes_is_deterministic() { + let permissions = vec![ + PermissionEntry { + resource: "repo".to_string(), + action: Some(vec!["create".to_string()]), + collection: Some(vec![ + "io.atcr.sailor.star".to_string(), + "io.atcr.manifest".to_string(), + "io.atcr.blob".to_string(), + ]), + lxm: None, + aud: None, + }, + PermissionEntry { + resource: "rpc".to_string(), + action: None, + collection: None, + lxm: Some(vec![ + "io.atcr.getManifest".to_string(), + "io.atcr.listTags".to_string(), + ]), + aud: Some("*".to_string()), + }, + ]; + + let first = build_expanded_scopes(&permissions, None, "io.atcr"); + assert_eq!( + first, + "repo:io.atcr.blob?action=create repo:io.atcr.manifest?action=create \ + repo:io.atcr.sailor.star?action=create \ + rpc:io.atcr.getManifest?aud=%2A rpc:io.atcr.listTags?aud=%2A" + ); + + for _ in 0..16 { + assert_eq!(build_expanded_scopes(&permissions, None, "io.atcr"), first); + } + } + + #[test] + fn test_build_expanded_scopes_dedupes_and_canonicalizes_actions() { + let permissions = vec![ + PermissionEntry { + resource: "repo".to_string(), + action: Some(vec!["delete".to_string(), "create".to_string()]), + collection: Some(vec!["io.atcr.manifest".to_string()]), + lxm: None, + aud: None, + }, + PermissionEntry { + resource: "repo".to_string(), + action: Some(vec!["create".to_string(), "update".to_string()]), + collection: Some(vec!["io.atcr.manifest".to_string()]), + lxm: None, + aud: None, + }, + PermissionEntry { + resource: "rpc".to_string(), + action: None, + collection: None, + lxm: Some(vec![ + "io.atcr.getManifest".to_string(), + "io.atcr.getManifest".to_string(), + ]), + aud: Some("*".to_string()), + }, + ]; + + let expanded = build_expanded_scopes(&permissions, None, "io.atcr"); + assert_eq!( + expanded, + "repo:io.atcr.manifest?action=create&action=update&action=delete \ + rpc:io.atcr.getManifest?aud=%2A" + ); + } + + #[test] + fn test_build_expanded_scopes_repo_unrecognized_action_skips_entry() { + let permissions = vec![PermissionEntry { + resource: "repo".to_string(), + action: Some(vec!["read".to_string()]), + collection: Some(vec!["io.atcr.manifest".to_string()]), + lxm: None, + aud: None, + }]; + + let expanded = build_expanded_scopes(&permissions, None, "io.atcr"); + assert!( + expanded.is_empty(), + "an unrecognized repo action must not expand to all actions, got: {expanded}" + ); } #[test] diff --git a/deploy/quadlets/tranquil-pds-app.container b/deploy/quadlets/tranquil-pds-app.container index 0b13b6a..3bf5c3b 100644 --- a/deploy/quadlets/tranquil-pds-app.container +++ b/deploy/quadlets/tranquil-pds-app.container @@ -10,7 +10,7 @@ Environment=SERVER_PORT=3000 Volume=/srv/tranquil-pds/config/config.toml:/etc/tranquil-pds/config.toml:ro,Z Volume=/srv/tranquil-pds/blobs:/var/lib/tranquil-pds/blobs:Z Volume=/srv/tranquil-pds/store:/var/lib/tranquil-pds/store:Z -HealthCmd=wget -q --spider http://localhost:3000/xrpc/_health +HealthCmd=["/usr/local/bin/tranquil-pds", "healthcheck"] HealthInterval=30s HealthTimeout=10s HealthRetries=3 diff --git a/frontend/src/routes/OAuthConsent.svelte b/frontend/src/routes/OAuthConsent.svelte index b18b350..02f619b 100644 --- a/frontend/src/routes/OAuthConsent.svelte +++ b/frontend/src/routes/OAuthConsent.svelte @@ -10,6 +10,7 @@ display_name: string granted: boolean | null restricted?: boolean + effective_scope?: string } const SCOPE_LOCALE_MAP: Record = { @@ -326,7 +327,7 @@ ) function getLocalizedScopeName(scope: ScopeInfo): string { - const localeKey = SCOPE_LOCALE_MAP[scope.scope] + const localeKey = SCOPE_LOCALE_MAP[scope.effective_scope ?? scope.scope] if (!localeKey) return scope.display_name if (scope.scope === 'atproto' && hasGranularScopes) { @@ -339,7 +340,7 @@ } function getLocalizedScopeDescription(scope: ScopeInfo): string { - const localeKey = SCOPE_LOCALE_MAP[scope.scope] + const localeKey = SCOPE_LOCALE_MAP[scope.effective_scope ?? scope.scope] if (!localeKey) return scope.description if (scope.scope === 'atproto' && hasGranularScopes) { @@ -361,7 +362,7 @@ const rpc: string[] = [] const other: ScopeInfo[] = [] for (const s of expanded) { - const [base, query = ''] = s.scope.split('?') + const [base, query = ''] = (s.effective_scope ?? s.scope).split('?') const params = new URLSearchParams(query) if (base.startsWith('repo:')) { const collection = base.slice('repo:'.length) || '*'