From 7244551ae136be55fcb93d2112220f4cafaf25ff Mon Sep 17 00:00:00 2001 From: Trezy Date: Fri, 24 Jul 2026 09:41:24 -0500 Subject: [PATCH] refactor: clean up property names and document stringy values Signed-off-by: Trezy --- .../src/endpoints/authorize/consent.rs | 48 ++++++------ .../endpoints/authorize/scope_resolution.rs | 74 ++++++++++++++----- .../src/endpoints/token/grants.rs | 70 +++++++++--------- crates/tranquil-pds/src/delegation/scopes.rs | 5 +- .../src/oauth/permission_set_resolver.rs | 30 +++++--- .../tests/oauth_permission_sets.rs | 15 ++-- crates/tranquil-scopes/src/coverage.rs | 34 +++++++-- crates/tranquil-scopes/src/lib.rs | 4 +- crates/tranquil-scopes/src/permission_set.rs | 36 ++++----- 9 files changed, 195 insertions(+), 121 deletions(-) diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs index a8a85aa..d459421 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs @@ -29,9 +29,12 @@ pub struct PermissionSetInfo { #[derive(Debug, Serialize)] pub struct FailedSetInfo { - pub nsid: String, - #[serde(skip_serializing_if = "Option::is_none")] - pub aud: Option, + // `given_*` is the value as requested by the client. + // Left as strings because the value from the client could be malformed. + #[serde(rename = "nsid")] + pub given_nsid: String, + #[serde(rename = "aud", skip_serializing_if = "Option::is_none")] + pub given_aud: Option, pub reason: tranquil_scopes::ResolveFailure, } @@ -147,13 +150,10 @@ pub async fn consent_get( Some(grant) => scope_resolution::Authority::Delegated(&grant.granted_scopes), None => scope_resolution::Authority::FullSelf, }; - let effective = scope_resolution::resolve_effective_scopes( - &*state.cache, - requested_scope_str, - authority, - ) - .await; - let requested_scopes: Vec<&str> = effective.resolved.split_whitespace().collect(); + let effective = + scope_resolution::resolve_effective_scopes(&*state.cache, requested_scope_str, authority) + .await; + let requested_scopes: Vec<&str> = effective.permitted.split_whitespace().collect(); let preferences = state .repos .oauth @@ -184,9 +184,8 @@ pub async fn consent_get( .unwrap_or(true); let has_granular_scopes = requested_scopes.iter().any(|s| is_granular_scope(s)); - let grant_scope_str: Option<&str> = delegation_grant - .as_ref() - .map(|g| g.granted_scopes.as_str()); + 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)) }; @@ -254,8 +253,7 @@ pub async fn consent_get( Some(a) => format!("include:{}?aud={}", g.nsid, a), None => format!("include:{}", g.nsid), }; - let expanded: Vec = - g.expanded.iter().map(|s| make_scope_info(s)).collect(); + let expanded: Vec = g.expanded.iter().map(|s| make_scope_info(s)).collect(); let restricted = !expanded.is_empty() && expanded.iter().all(|s| s.restricted); PermissionSetInfo { nsid: g.nsid.clone(), @@ -275,8 +273,8 @@ pub async fn consent_get( .failures .iter() .map(|f| FailedSetInfo { - nsid: f.nsid.clone(), - aud: f.aud.clone(), + given_nsid: f.given_nsid.clone(), + given_aud: f.given_aud.clone(), reason: f.reason.clone(), }) .collect(); @@ -422,12 +420,9 @@ pub async fn consent_post( Some(grant) => scope_resolution::Authority::Delegated(&grant.granted_scopes), None => scope_resolution::Authority::FullSelf, }; - let effective = scope_resolution::resolve_effective_scopes( - &*state.cache, - original_scope_str, - authority, - ) - .await; + let effective = + scope_resolution::resolve_effective_scopes(&*state.cache, original_scope_str, authority) + .await; let include_token = |nsid: &str, aud: &Option| -> String { match aud { Some(a) => format!("include:{}?aud={}", nsid, a), @@ -438,8 +433,11 @@ pub async fn consent_post( .outcome .failures .iter() - .filter(|f| form.approved_scopes.contains(&include_token(&f.nsid, &f.aud))) - .map(|f| f.nsid.clone()) + .filter(|f| { + form.approved_scopes + .contains(&include_token(&f.given_nsid, &f.given_aud)) + }) + .map(|f| f.given_nsid.clone()) .collect(); if !approved_failed_sets.is_empty() { return json_error( diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize/scope_resolution.rs b/crates/tranquil-oauth-server/src/endpoints/authorize/scope_resolution.rs index ab08789..ac12090 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize/scope_resolution.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize/scope_resolution.rs @@ -10,7 +10,9 @@ pub enum Authority<'a> { } pub struct EffectiveScopes { - pub resolved: String, + // The expanded set of scopes, minus any denied by delegation + pub permitted: String, + // The expanded set of scopes, before delegation processing pub outcome: ExpansionOutcome, } @@ -21,11 +23,11 @@ pub async fn resolve_effective_scopes( ) -> EffectiveScopes { let outcome = expand_scopes(cache, requested).await; let expanded = outcome.to_scope_string(); - let resolved = match authority { + let permitted = match authority { Authority::FullSelf => expanded, Authority::Delegated(granted) => intersect_scopes(&expanded, granted.as_str()), }; - EffectiveScopes { resolved, outcome } + EffectiveScopes { permitted, outcome } } #[cfg(test)] @@ -40,18 +42,31 @@ mod tests { struct MapCache(Mutex>); #[async_trait::async_trait] impl Cache for MapCache { - async fn get(&self, k: &str) -> Option { self.0.lock().unwrap().get(k).cloned() } + async fn get(&self, k: &str) -> Option { + self.0.lock().unwrap().get(k).cloned() + } async fn set(&self, k: &str, v: &str, _t: Duration) -> Result<(), CacheError> { - self.0.lock().unwrap().insert(k.into(), v.into()); Ok(()) + self.0.lock().unwrap().insert(k.into(), v.into()); + Ok(()) + } + async fn delete(&self, k: &str) -> Result<(), CacheError> { + self.0.lock().unwrap().remove(k); + Ok(()) + } + async fn get_bytes(&self, _k: &str) -> Option> { + None + } + async fn set_bytes(&self, _k: &str, _v: &[u8], _t: Duration) -> Result<(), CacheError> { + Ok(()) } - async fn delete(&self, k: &str) -> Result<(), CacheError> { self.0.lock().unwrap().remove(k); Ok(()) } - async fn get_bytes(&self, _k: &str) -> Option> { None } - async fn set_bytes(&self, _k: &str, _v: &[u8], _t: Duration) -> Result<(), CacheError> { Ok(()) } } fn cache_with(nsid: &str, scopes: &str) -> MapCache { let c = MapCache::default(); - let key = tranquil_pds::cache_keys::permission_set_key(&tranquil_types::Nsid::new(nsid).unwrap(), None); + let key = tranquil_pds::cache_keys::permission_set_key( + &tranquil_types::Nsid::new(nsid).unwrap(), + None, + ); let json = serde_json::json!({ "scope": scopes, "title": null, @@ -65,24 +80,43 @@ mod tests { #[tokio::test] async fn full_self_keeps_all_expanded() { - let c = cache_with("io.atcr.authFullApp", "repo:io.atcr.manifest?action=create identity:*"); - let eff = resolve_effective_scopes(&c, "atproto include:io.atcr.authFullApp", Authority::FullSelf).await; - assert!(eff.resolved.contains("atproto")); - assert!(eff.resolved.contains("repo:io.atcr.manifest?action=create")); - assert!(eff.resolved.contains("identity:*")); + let c = cache_with( + "io.atcr.authFullApp", + "repo:io.atcr.manifest?action=create identity:*", + ); + let eff = resolve_effective_scopes( + &c, + "atproto include:io.atcr.authFullApp", + Authority::FullSelf, + ) + .await; + assert!(eff.permitted.contains("atproto")); + assert!( + eff.permitted + .contains("repo:io.atcr.manifest?action=create") + ); + assert!(eff.permitted.contains("identity:*")); assert!(eff.outcome.failures.is_empty()); } #[tokio::test] async fn delegated_intersects_expanded_against_grant() { - let c = cache_with("io.atcr.authFullApp", "repo:io.atcr.manifest?action=create identity:*"); + let c = cache_with( + "io.atcr.authFullApp", + "repo:io.atcr.manifest?action=create identity:*", + ); let granted = DbScope::new("atproto repo:* blob:*/* account:*?action=manage").unwrap(); let eff = resolve_effective_scopes( - &c, "atproto include:io.atcr.authFullApp", + &c, + "atproto include:io.atcr.authFullApp", Authority::Delegated(&granted), - ).await; - assert!(eff.resolved.contains("atproto")); - assert!(eff.resolved.contains("repo:io.atcr.manifest?action=create")); - assert!(!eff.resolved.contains("identity")); + ) + .await; + assert!(eff.permitted.contains("atproto")); + assert!( + eff.permitted + .contains("repo:io.atcr.manifest?action=create") + ); + assert!(!eff.permitted.contains("identity")); } } diff --git a/crates/tranquil-oauth-server/src/endpoints/token/grants.rs b/crates/tranquil-oauth-server/src/endpoints/token/grants.rs index 8bc7c5b..b9698d5 100644 --- a/crates/tranquil-oauth-server/src/endpoints/token/grants.rs +++ b/crates/tranquil-oauth-server/src/endpoints/token/grants.rs @@ -133,21 +133,22 @@ pub async fn handle_authorization_code_grant( let controller_did = authorized.controller_did.clone(); let requested_scope = authorized.parameters.scope.clone(); - let granted_scopes: Option = if let Some(ref controller) = controller_did { - let grant = state - .repos - .delegation - .get_delegation(&did, controller) - .await - .ok() - .flatten() - .ok_or_else(|| { - OAuthError::InvalidGrant("Delegation grant not found or revoked".to_string()) - })?; - Some(grant.granted_scopes.clone()) - } else { - None - }; + let granted_scopes: Option = + if let Some(ref controller) = controller_did { + let grant = state + .repos + .delegation + .get_delegation(&did, controller) + .await + .ok() + .flatten() + .ok_or_else(|| { + OAuthError::InvalidGrant("Delegation grant not found or revoked".to_string()) + })?; + Some(grant.granted_scopes.clone()) + } else { + None + }; let authority = match granted_scopes.as_ref() { Some(g) => crate::endpoints::authorize::scope_resolution::Authority::Delegated(g), None => crate::endpoints::authorize::scope_resolution::Authority::FullSelf, @@ -164,14 +165,14 @@ pub async fn handle_authorization_code_grant( .outcome .failures .iter() - .map(|f| f.nsid.clone()) + .map(|f| f.given_nsid.clone()) .collect(); return Err(OAuthError::InvalidScope(format!( "Could not resolve permission set(s): {}", names.join(", ") ))); } - let resolved_scope = effective.resolved; + let resolved_scope = effective.permitted; let access_token = create_access_token_with_delegation( &token_id, @@ -254,21 +255,22 @@ async fn recompute_resolved_scope( token_data: &TokenData, ) -> Result { let requested = token_data.scope.as_deref().unwrap_or("atproto"); - let granted_scopes: Option = if let Some(ref controller) = token_data.controller_did { - let grant = state - .repos - .delegation - .get_delegation(&token_data.did, controller) - .await - .ok() - .flatten() - .ok_or_else(|| { - OAuthError::InvalidGrant("Delegation grant not found or revoked".to_string()) - })?; - Some(grant.granted_scopes.clone()) - } else { - None - }; + let granted_scopes: Option = + if let Some(ref controller) = token_data.controller_did { + let grant = state + .repos + .delegation + .get_delegation(&token_data.did, controller) + .await + .ok() + .flatten() + .ok_or_else(|| { + OAuthError::InvalidGrant("Delegation grant not found or revoked".to_string()) + })?; + Some(grant.granted_scopes.clone()) + } else { + None + }; let authority = match granted_scopes.as_ref() { Some(g) => crate::endpoints::authorize::scope_resolution::Authority::Delegated(g), None => crate::endpoints::authorize::scope_resolution::Authority::FullSelf, @@ -284,14 +286,14 @@ async fn recompute_resolved_scope( .outcome .failures .iter() - .map(|f| f.nsid.clone()) + .map(|f| f.given_nsid.clone()) .collect(); return Err(OAuthError::InvalidScope(format!( "Permission set(s) expired and unresolvable: {}", names.join(", ") ))); } - Ok(effective.resolved) + Ok(effective.permitted) } pub async fn handle_refresh_token_grant( diff --git a/crates/tranquil-pds/src/delegation/scopes.rs b/crates/tranquil-pds/src/delegation/scopes.rs index 6c0eba0..9ccaaaf 100644 --- a/crates/tranquil-pds/src/delegation/scopes.rs +++ b/crates/tranquil-pds/src/delegation/scopes.rs @@ -268,7 +268,10 @@ mod tests { "repo:app.bsky.feed.post?action=create identity:* account:*?action=manage", granted, ); - assert!(grant_covers(granted, "repo:app.bsky.feed.post?action=create")); + 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!( diff --git a/crates/tranquil-pds/src/oauth/permission_set_resolver.rs b/crates/tranquil-pds/src/oauth/permission_set_resolver.rs index 2d40184..a9420bc 100644 --- a/crates/tranquil-pds/src/oauth/permission_set_resolver.rs +++ b/crates/tranquil-pds/src/oauth/permission_set_resolver.rs @@ -3,8 +3,8 @@ use crate::cache_keys::permission_set_key; use serde::{Deserialize, Serialize}; use std::time::Duration; use tranquil_scopes::{ - fetch_and_expand, parse_include_scope, ExpansionOutcome, FailedSet, ResolveFailure, - ResolvedSetGroup, ScopeExpansionError, + ExpansionOutcome, FailedSet, ResolveFailure, ResolvedSetGroup, ScopeExpansionError, + fetch_and_expand, parse_include_scope, }; use tranquil_types::Nsid; @@ -38,8 +38,8 @@ pub async fn expand_scopes(cache: &dyn Cache, scope_string: &str) -> ExpansionOu match resolve_one(cache, nsid, aud).await { Ok(group) => outcome.sets.push(group), Err(reason) => outcome.failures.push(FailedSet { - nsid: nsid.to_string(), - aud: aud.map(str::to_string), + given_nsid: nsid.to_string(), + given_aud: aud.map(str::to_string), reason, }), } @@ -84,10 +84,20 @@ async fn resolve_one( }; if let Ok(json) = serde_json::to_string(&stored) { let _ = cache - .set(&key, &json, Duration::from_secs(PERMISSION_SET_CACHE_TTL_SECS)) + .set( + &key, + &json, + Duration::from_secs(PERMISSION_SET_CACHE_TTL_SECS), + ) .await; } - Ok(group_from(parsed, aud, fetched.expanded, fetched.title, fetched.detail)) + Ok(group_from( + parsed, + aud, + fetched.expanded, + fetched.title, + fetched.detail, + )) } Err(e) => match cached { Some(v) => Ok(group_from(parsed, aud, v.scope, v.title, v.detail)), @@ -160,7 +170,8 @@ mod tests { } fn seed_at(cache: &MapCache, nsid: &str, scope: &str, refreshed_at: i64) { - let key = crate::cache_keys::permission_set_key(&tranquil_types::Nsid::new(nsid).unwrap(), None); + let key = + crate::cache_keys::permission_set_key(&tranquil_types::Nsid::new(nsid).unwrap(), None); let val = serde_json::to_string(&CachedPermissionSet { scope: scope.to_string(), title: Some("Basic".into()), @@ -225,7 +236,8 @@ mod tests { None, ); // Shape written before `refreshed_at` existed. - let legacy = r#"{"scope":"repo:nonexistent.fake.record?action=create","title":null,"detail":null}"#; + let legacy = + r#"{"scope":"repo:nonexistent.fake.record?action=create","title":null,"detail":null}"#; cache.0.lock().unwrap().insert(key, legacy.to_string()); let out = expand_scopes(&cache, "include:nonexistent.fake.permissionSet").await; assert!(out.failures.is_empty()); @@ -247,6 +259,6 @@ mod tests { let out = expand_scopes(&cache, "include:nonexistent.fake.permissionSet").await; assert_eq!(out.sets.len(), 0); assert_eq!(out.failures.len(), 1); - assert_eq!(out.failures[0].nsid, "nonexistent.fake.permissionSet"); + assert_eq!(out.failures[0].given_nsid, "nonexistent.fake.permissionSet"); } } diff --git a/crates/tranquil-pds/tests/oauth_permission_sets.rs b/crates/tranquil-pds/tests/oauth_permission_sets.rs index eff8453..09003a2 100644 --- a/crates/tranquil-pds/tests/oauth_permission_sets.rs +++ b/crates/tranquil-pds/tests/oauth_permission_sets.rs @@ -53,7 +53,10 @@ async fn setup_mock_client_metadata(redirect_uri: &str) -> MockServer { async fn seed_permission_set(nsid: &str, granular_scope: &str) { let state = common::get_test_app_state().await; - let key = tranquil_pds::cache_keys::permission_set_key(&tranquil_types::Nsid::new(nsid).unwrap(), None); + let key = tranquil_pds::cache_keys::permission_set_key( + &tranquil_types::Nsid::new(nsid).unwrap(), + None, + ); let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap() @@ -597,7 +600,10 @@ async fn test_consent_post_errors_when_set_unresolvable() { ); let state = common::get_test_app_state().await; - let key = tranquil_pds::cache_keys::permission_set_key(&tranquil_types::Nsid::new(UNRESOLVABLE_NSID).unwrap(), None); + let key = tranquil_pds::cache_keys::permission_set_key( + &tranquil_types::Nsid::new(UNRESOLVABLE_NSID).unwrap(), + None, + ); state.cache.delete(&key).await.unwrap(); let approved_scopes: Vec<&str> = scope.split_whitespace().collect(); @@ -663,10 +669,7 @@ async fn test_consent_post_succeeds_when_unapproved_set_fails() { let client_id = mock_client.uri(); let (_code_verifier, code_challenge) = generate_pkce(); - let scope = format!( - "atproto include:{} include:{}", - GOOD_SET_NSID, BAD_SET_NSID - ); + let scope = format!("atproto include:{} include:{}", GOOD_SET_NSID, BAD_SET_NSID); let par_res = http_client .post(format!("{}/oauth/par", url)) .form(&[ diff --git a/crates/tranquil-scopes/src/coverage.rs b/crates/tranquil-scopes/src/coverage.rs index 3dcb013..716da42 100644 --- a/crates/tranquil-scopes/src/coverage.rs +++ b/crates/tranquil-scopes/src/coverage.rs @@ -27,7 +27,9 @@ fn repo_covers(g: &RepoScope, r: &RepoScope) -> bool { Some(gc) => match &r.collection { None => false, Some(rc) => match gc.strip_suffix(".*") { - Some(prefix) => rc.starts_with(prefix) && rc.as_bytes().get(prefix.len()) == Some(&b'.'), + Some(prefix) => { + rc.starts_with(prefix) && rc.as_bytes().get(prefix.len()) == Some(&b'.') + } None => gc == rc, }, }, @@ -94,13 +96,22 @@ mod tests { fn repo_action_subset_required() { assert!(c("repo:*", "repo:*?action=create")); assert!(!c("repo:*?action=create", "repo:*?action=delete")); - assert!(c("repo:*?action=create&action=delete", "repo:*?action=create")); - assert!(!c("repo:*?action=create", "repo:*?action=create&action=delete")); + assert!(c( + "repo:*?action=create&action=delete", + "repo:*?action=create" + )); + assert!(!c( + "repo:*?action=create", + "repo:*?action=create&action=delete" + )); } #[test] fn repo_partial_action_grant_does_not_cover_actionless_request() { - assert!(!c("repo:*?action=create&action=delete", "repo:app.bsky.feed.post")); + assert!(!c( + "repo:*?action=create&action=delete", + "repo:app.bsky.feed.post" + )); } #[test] @@ -125,15 +136,24 @@ mod tests { #[test] fn rpc_wildcards() { assert!(c("rpc:*?aud=did:web:x", "rpc:app.bsky.getX?aud=did:web:x")); - assert!(c("rpc:app.bsky.getX?aud=*", "rpc:app.bsky.getX?aud=did:web:x")); - assert!(!c("rpc:app.bsky.getX?aud=did:web:x", "rpc:app.bsky.getY?aud=did:web:x")); + assert!(c( + "rpc:app.bsky.getX?aud=*", + "rpc:app.bsky.getX?aud=did:web:x" + )); + assert!(!c( + "rpc:app.bsky.getX?aud=did:web:x", + "rpc:app.bsky.getY?aud=did:web:x" + )); } #[test] fn account_manage_covers_read_and_wildcard_attr() { assert!(c("account:*?action=manage", "account:email?action=read")); assert!(c("account:*?action=manage", "account:email?action=manage")); - assert!(!c("account:email?action=read", "account:email?action=manage")); + assert!(!c( + "account:email?action=read", + "account:email?action=manage" + )); } #[test] diff --git a/crates/tranquil-scopes/src/lib.rs b/crates/tranquil-scopes/src/lib.rs index 50439a3..1160e56 100644 --- a/crates/tranquil-scopes/src/lib.rs +++ b/crates/tranquil-scopes/src/lib.rs @@ -16,7 +16,7 @@ pub use parser::{ ParsedScope, RepoAction, RepoScope, RpcScope, parse_scope, parse_scope_string, }; pub use permission_set::{ - ExpansionOutcome, FailedSet, FetchedSet, ResolveFailure, ResolvedSetGroup, - ScopeExpansionError, fetch_and_expand, parse_include_scope, + ExpansionOutcome, FailedSet, FetchedSet, ResolveFailure, ResolvedSetGroup, ScopeExpansionError, + fetch_and_expand, parse_include_scope, }; pub use permissions::ScopePermissions; diff --git a/crates/tranquil-scopes/src/permission_set.rs b/crates/tranquil-scopes/src/permission_set.rs index b160c8d..8897ef0 100644 --- a/crates/tranquil-scopes/src/permission_set.rs +++ b/crates/tranquil-scopes/src/permission_set.rs @@ -45,8 +45,9 @@ pub enum ResolveFailure { #[derive(Debug, Clone)] pub struct FailedSet { - pub nsid: String, - pub aud: Option, + // NSID and aud are left as strings to avoid issues from malformed requests. + pub given_nsid: String, + pub given_aud: Option, pub reason: ResolveFailure, } @@ -70,11 +71,11 @@ impl ExpansionOutcome { pub fn flat_scopes(&self) -> Vec { let mut out = Vec::new(); let mut seen = std::collections::HashSet::new(); - for s in self.passthrough.iter().chain( - self.sets - .iter() - .flat_map(|group| group.expanded.iter()), - ) { + for s in self + .passthrough + .iter() + .chain(self.sets.iter().flat_map(|group| group.expanded.iter())) + { if seen.insert(s.as_str()) { out.push(s.clone()); } @@ -163,12 +164,13 @@ pub async fn fetch_and_expand( main_def.def_type.clone(), )); } - let permissions = main_def - .permissions - .as_ref() - .ok_or(ScopeExpansionError::MissingDefinition( - "permissions".to_string(), - ))?; + let permissions = + main_def + .permissions + .as_ref() + .ok_or(ScopeExpansionError::MissingDefinition( + "permissions".to_string(), + ))?; let namespace_authority = extract_namespace_authority(nsid); let expanded = build_expanded_scopes(permissions, aud, &namespace_authority); if expanded.is_empty() { @@ -235,8 +237,8 @@ async fn fetch_lexicon_via_atproto(nsid: &Nsid) -> Result