From 2e923105183340ddea1e58c3747b7c38839f806f Mon Sep 17 00:00:00 2001 From: Trezy Date: Fri, 28 Aug 2026 21:14:06 -0500 Subject: [PATCH] fix: allow `transition:generic` to be used with granular scopes Signed-off-by: Trezy --- .../src/endpoints/authorize/consent.rs | 13 + .../src/endpoints/par.rs | 26 -- crates/tranquil-scopes/src/definitions.rs | 2 +- crates/tranquil-scopes/src/lib.rs | 2 +- crates/tranquil-scopes/src/permissions.rs | 237 +++++++++++++++--- frontend/src/locales/en.json | 8 +- frontend/src/routes/OAuthConsent.svelte | 60 ++++- frontend/src/styles/pages.css | 7 +- .../src/tests/OAuthConsentSupersede.test.ts | 196 +++++++++++++++ 9 files changed, 485 insertions(+), 66 deletions(-) create mode 100644 frontend/src/tests/OAuthConsentSupersede.test.ts diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs index d6c1e40..df8508a 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs @@ -1,4 +1,5 @@ use super::*; +use tranquil_scopes::{ParsedScope, parse_scope}; use tranquil_types::Nsid; #[derive(Debug, Serialize)] @@ -10,6 +11,7 @@ pub struct ScopeInfo { pub display_name: String, pub granted: Option, pub restricted: bool, + pub superseded: bool, #[serde(skip_serializing_if = "Option::is_none")] pub effective_scope: Option, } @@ -27,6 +29,7 @@ pub struct PermissionSetInfo { pub expanded: Vec, pub granted: Option, pub restricted: bool, + pub superseded: bool, } #[derive(Debug, Serialize)] @@ -49,6 +52,7 @@ pub struct ConsentResponse { pub logo_uri: Option, pub scopes: Vec, pub permission_sets: Vec, + pub transition_supersedes: bool, pub failed_sets: Vec, pub show_consent: bool, pub did: Did, @@ -185,6 +189,9 @@ pub async fn consent_get( .await .unwrap_or(true); let has_granular_scopes = requested_scopes.iter().any(|s| is_granular_scope(s)); + let has_transition_generic = requested_scopes + .iter() + .any(|s| matches!(parse_scope(s), ParsedScope::TransitionGeneric)); let grant_scope_str: Option<&str> = delegation_grant.as_ref().map(|g| g.granted_scopes.as_str()); @@ -237,6 +244,8 @@ pub async fn consent_get( ) }; let granted = pref_map.get(scope).copied(); + let superseded = has_transition_generic + && tranquil_scopes::superseded_by_transition_generic(&parse_scope(scope)); ScopeInfo { scope: scope.to_string(), category, @@ -245,6 +254,7 @@ pub async fn consent_get( display_name, granted, restricted, + superseded, effective_scope, } }; @@ -267,6 +277,7 @@ pub async fn consent_get( }; let expanded: Vec = g.expanded.iter().map(|s| make_scope_info(s)).collect(); let restricted = !expanded.is_empty() && expanded.iter().all(|s| s.restricted); + let superseded = !expanded.is_empty() && expanded.iter().all(|s| s.superseded); PermissionSetInfo { nsid: g.nsid.clone(), aud: g.aud.clone(), @@ -276,6 +287,7 @@ pub async fn consent_get( include_scope, expanded, restricted, + superseded, } }) .collect(); @@ -340,6 +352,7 @@ pub async fn consent_get( logo_uri: client_metadata.as_ref().and_then(|m| m.logo_uri.clone()), scopes, permission_sets, + transition_supersedes: has_transition_generic && has_granular_scopes, failed_sets, show_consent, did: did.clone(), diff --git a/crates/tranquil-oauth-server/src/endpoints/par.rs b/crates/tranquil-oauth-server/src/endpoints/par.rs index c122161..4ebb162 100644 --- a/crates/tranquil-oauth-server/src/endpoints/par.rs +++ b/crates/tranquil-oauth-server/src/endpoints/par.rs @@ -187,32 +187,6 @@ fn validate_scope( ))); } - let has_transition = requested_scopes.iter().any(|s| { - matches!( - parse_scope(s), - ParsedScope::TransitionGeneric - | ParsedScope::TransitionChat - | ParsedScope::TransitionEmail - ) - }); - let has_granular = requested_scopes.iter().any(|s| { - matches!( - parse_scope(s), - ParsedScope::Repo(_) - | ParsedScope::Blob(_) - | ParsedScope::Rpc(_) - | ParsedScope::Account(_) - | ParsedScope::Identity(_) - | ParsedScope::Include(_) - ) - }); - - if has_transition && has_granular { - return Err(OAuthError::InvalidScope( - "Cannot mix transition scopes with granular scopes. Use either transition:* scopes OR granular scopes (repo:*, blob:*, rpc:*, account:*, include:*), not both.".to_string() - )); - } - if let Some(client_scope) = &client_metadata.scope { let client_scopes: Vec<&str> = client_scope.split_whitespace().collect(); if let Some(unregistered) = requested_scopes diff --git a/crates/tranquil-scopes/src/definitions.rs b/crates/tranquil-scopes/src/definitions.rs index 3265d74..ac7c4e8 100644 --- a/crates/tranquil-scopes/src/definitions.rs +++ b/crates/tranquil-scopes/src/definitions.rs @@ -48,7 +48,7 @@ pub static SCOPE_DEFINITIONS: LazyLock> = category: ScopeCategory::Transition, required: false, description: "Generic transition scope for compatibility", - display_name: "Transition Access", + display_name: "Generic Access", }, ScopeDefinition { scope: "transition:chat.bsky", diff --git a/crates/tranquil-scopes/src/lib.rs b/crates/tranquil-scopes/src/lib.rs index 1a124e5..807fb19 100644 --- a/crates/tranquil-scopes/src/lib.rs +++ b/crates/tranquil-scopes/src/lib.rs @@ -19,4 +19,4 @@ pub use permission_set::{ ExpansionOutcome, FailedSet, FetchedSet, ResolveFailure, ResolvedSetGroup, ScopeExpansionError, fetch_and_expand, parse_include_scope, }; -pub use permissions::ScopePermissions; +pub use permissions::{ScopePermissions, superseded_by_transition_generic}; diff --git a/crates/tranquil-scopes/src/permissions.rs b/crates/tranquil-scopes/src/permissions.rs index 7fb4a74..47b1189 100644 --- a/crates/tranquil-scopes/src/permissions.rs +++ b/crates/tranquil-scopes/src/permissions.rs @@ -43,7 +43,26 @@ impl ScopePermissions { has_transition_email, } } +} +/// Whether holding `transition:generic` makes `scope` redundant. +pub fn superseded_by_transition_generic(scope: &ParsedScope) -> bool { + match scope { + ParsedScope::Repo(_) | ParsedScope::Blob(_) => true, + ParsedScope::Rpc(rpc) => !rpc + .lxm + .as_deref() + .is_some_and(|lxm| lxm == "*" || lxm.starts_with("chat.bsky.")), + ParsedScope::Account(_) + | ParsedScope::Identity(_) + | ParsedScope::TransitionEmail + | ParsedScope::TransitionChat => false, + ParsedScope::Include(_) => false, + ParsedScope::TransitionGeneric | ParsedScope::Atproto | ParsedScope::Unknown(_) => false, + } +} + +impl ScopePermissions { pub fn has_scope(&self, scope: &str) -> bool { self.scopes.contains(scope) } @@ -158,22 +177,17 @@ impl ScopePermissions { } pub fn assert_rpc(&self, aud: &str, lxm: &Nsid) -> Result<(), ScopeError> { - if lxm.starts_with("chat.bsky.") { - if self.has_transition_chat { - return Ok(()); - } - if self.has_transition_generic && !self.has_transition_chat { - return Err(ScopeError::InsufficientScope { - required: "transition:chat.bsky".to_string(), - message: format!( - "Chat access requires transition:chat.bsky scope to call {}", - lxm - ), - }); - } + let is_chat = lxm.starts_with("chat.bsky."); + + if is_chat && self.has_transition_chat { + return Ok(()); } - if self.has_transition_generic { + // `transition:generic` covers every lexicon except chat. Note it does not *block* chat: + // holding it must never remove access a granular `rpc:chat.bsky.*` scope would grant on + // its own, so chat requests fall through to the granular check below rather than + // failing here. + if self.has_transition_generic && !is_chat { return Ok(()); } @@ -198,13 +212,24 @@ impl ScopePermissions { }); if has_permission { - Ok(()) - } else { - Err(ScopeError::InsufficientScope { + return Ok(()); + } + + // Point a caller holding only `transition:generic` at the scope it actually needs, + // rather than at a granular rpc scope it probably did not mean to request. + Err(match is_chat && self.has_transition_generic { + true => ScopeError::InsufficientScope { + required: "transition:chat.bsky".to_string(), + message: format!( + "Chat access requires transition:chat.bsky scope to call {}", + lxm + ), + }, + false => ScopeError::InsufficientScope { required: format!("rpc:{}?aud={}", lxm, aud), message: format!("Insufficient scope to call {} on {}", lxm, aud), - }) - } + }, + }) } pub fn assert_account( @@ -212,10 +237,6 @@ impl ScopePermissions { attr: AccountAttr, action: AccountAction, ) -> Result<(), ScopeError> { - if self.has_transition_generic { - return Ok(()); - } - if attr == AccountAttr::Email && action == AccountAction::Read && self.has_transition_email { return Ok(()); @@ -245,8 +266,7 @@ impl ScopePermissions { } pub fn allows_email_read(&self) -> bool { - self.has_transition_generic - || self.has_transition_email + self.has_transition_email || self .find_account_scopes() .any(|a| a.attr == AccountAttr::Email || a.attr == AccountAttr::Wildcard) @@ -269,10 +289,6 @@ impl ScopePermissions { } pub fn assert_identity(&self, attr: IdentityAttr) -> Result<(), ScopeError> { - if self.has_transition_generic { - return Ok(()); - } - let has_permission = self.find_identity_scopes().any(|identity_scope| { identity_scope.attr == IdentityAttr::Wildcard || identity_scope.attr == attr }); @@ -336,6 +352,7 @@ impl Default for ScopePermissions { #[cfg(test)] mod tests { use super::*; + use crate::parser::parse_scope; fn c(s: &str) -> Nsid { s.parse().unwrap() @@ -512,10 +529,10 @@ mod tests { } #[test] - fn test_transition_generic_grants_identity() { + fn test_transition_generic_does_not_grant_identity() { let perms = ScopePermissions::from_scope_string(Some("transition:generic")); - assert!(perms.allows_identity(IdentityAttr::Handle)); - assert!(perms.allows_identity(IdentityAttr::Wildcard)); + assert!(!perms.allows_identity(IdentityAttr::Handle)); + assert!(!perms.allows_identity(IdentityAttr::Wildcard)); } #[test] @@ -597,4 +614,160 @@ mod tests { &c("app.bsky.feed.getAuthorFeed") )); } + + #[test] + fn transition_generic_supersedes_granular_scopes() { + for scope in [ + "repo:app.bsky.feed.post?action=create", + "blob:image/png", + "rpc:app.bsky.actor.getProfile?aud=*", + ] { + assert!( + superseded_by_transition_generic(&parse_scope(scope)), + "{scope} should be superseded by transition:generic" + ); + } + } + + #[test] + fn transition_generic_does_not_supersede_chat() { + // assert_rpc rejects chat.bsky.* when transition:generic is held without + // transition:chat.bsky, so neither the transition scope nor an rpc scope that + // could reach a chat lexicon is covered by it. + for scope in [ + "transition:chat.bsky", + "rpc:chat.bsky.convo.sendMessage?aud=*", + "rpc:*?aud=did:web:api.bsky.app", + "account:email?action=manage", + "account:email?action=read", + "account:status?action=read", + "identity:handle", + "identity:*", + "transition:email", + ] { + assert!( + !superseded_by_transition_generic(&parse_scope(scope)), + "{scope} must not be treated as superseded" + ); + } + } + + #[test] + fn transition_generic_does_not_supersede_itself_or_baseline() { + assert!(!superseded_by_transition_generic(&parse_scope( + "transition:generic" + ))); + assert!(!superseded_by_transition_generic(&parse_scope("atproto"))); + } + + #[test] + fn superseded_matches_enforcement_for_chat_and_feed() { + // Cross-check against ScopePermissions so the two cannot drift apart. + let perms = ScopePermissions::from_scope_string(Some("atproto transition:generic")); + let feed = Nsid::new("app.bsky.feed.getTimeline").unwrap(); + let chat = Nsid::new("chat.bsky.convo.sendMessage").unwrap(); + assert!(perms.allows_rpc("did:web:api.bsky.app", &feed)); + assert!(!perms.allows_rpc("did:web:api.bsky.app", &chat)); + } + + #[test] + fn granular_chat_rpc_works_without_transition_generic() { + // Baseline for the test below: on its own, a granular chat rpc scope grants chat. + let perms = ScopePermissions::from_scope_string(Some( + "atproto rpc:chat.bsky.convo.sendMessage?aud=*", + )); + assert!(perms.allows_rpc("did:web:api.bsky.chat", &c("chat.bsky.convo.sendMessage"))); + } + + #[test] + fn transition_generic_does_not_revoke_granular_chat_rpc() { + // Adding a broader scope must never remove access. transition:generic does not cover + // chat lexicons, but it must not stop a granular chat rpc scope from doing so either. + let perms = ScopePermissions::from_scope_string(Some( + "atproto transition:generic rpc:chat.bsky.convo.sendMessage?aud=*", + )); + assert!(perms.allows_rpc("did:web:api.bsky.chat", &c("chat.bsky.convo.sendMessage"))); + // ...and still grants everything else it covers. + assert!(perms.allows_rpc("did:web:api.bsky.app", &c("app.bsky.feed.getTimeline"))); + } + + #[test] + fn transition_generic_does_not_widen_granular_chat_rpc() { + // The granular scope grants exactly one chat lexicon; transition:generic must not be + // read as covering the rest of chat. + let perms = ScopePermissions::from_scope_string(Some( + "atproto transition:generic rpc:chat.bsky.convo.sendMessage?aud=*", + )); + assert!(!perms.allows_rpc("did:web:api.bsky.chat", &c("chat.bsky.convo.deleteMessage"))); + } + + #[test] + fn chat_denial_still_names_the_scope_the_caller_needs() { + // transition:generic alone: the useful advice is "ask for transition:chat.bsky", + // not "ask for rpc:chat.bsky.convo.listConvos". + let generic = ScopePermissions::from_scope_string(Some("atproto transition:generic")); + let err = generic + .assert_rpc("did:web:api.bsky.chat", &c("chat.bsky.convo.listConvos")) + .expect_err("chat must be denied without transition:chat.bsky"); + match err { + ScopeError::InsufficientScope { required, .. } => { + assert_eq!(required, "transition:chat.bsky"); + } + other => panic!("unexpected error: {other:?}"), + } + + // Without transition:generic the granular scope is the right thing to name. + let bare = ScopePermissions::from_scope_string(Some("atproto")); + let err = bare + .assert_rpc("did:web:api.bsky.chat", &c("chat.bsky.convo.listConvos")) + .expect_err("chat must be denied with no rpc scope at all"); + match err { + ScopeError::InsufficientScope { required, .. } => { + assert!(required.starts_with("rpc:chat.bsky.convo.listConvos")); + } + other => panic!("unexpected error: {other:?}"), + } + } + + #[test] + fn transition_generic_does_not_grant_account_management() { + // "no account management actions: change handle, change email, delete or deactivate + // account, migrate account" -- atproto OAuth spec. + let perms = ScopePermissions::from_scope_string(Some("atproto transition:generic")); + assert!(!perms.allows_account(AccountAttr::Email, AccountAction::Manage)); + assert!(!perms.allows_account(AccountAttr::Repo, AccountAction::Manage)); + assert!(!perms.allows_account(AccountAttr::Status, AccountAction::Manage)); + } + + #[test] + fn transition_generic_does_not_grant_email_read() { + // Reading the account email is what transition:email is for. + let perms = ScopePermissions::from_scope_string(Some("atproto transition:generic")); + assert!(!perms.allows_email_read()); + assert!(!perms.allows_account(AccountAttr::Email, AccountAction::Read)); + } + + #[test] + fn granular_scopes_still_grant_alongside_transition_generic() { + // Removing the short-circuit must not stop an explicitly granted scope from working. + let perms = ScopePermissions::from_scope_string(Some( + "atproto transition:generic account:email?action=manage identity:handle", + )); + assert!(perms.allows_account(AccountAttr::Email, AccountAction::Manage)); + assert!(perms.allows_identity(IdentityAttr::Handle)); + + let with_email = ScopePermissions::from_scope_string(Some( + "atproto transition:generic transition:email", + )); + assert!(with_email.allows_email_read()); + } + + #[test] + fn transition_generic_still_grants_what_the_spec_says_it_does() { + let perms = ScopePermissions::from_scope_string(Some("atproto transition:generic")); + assert!(perms.allows_repo(RepoAction::Create, &c("app.bsky.feed.post"))); + assert!(perms.allows_repo(RepoAction::Delete, &c("app.bsky.feed.post"))); + assert!(perms.allows_blob("image/png")); + assert!(perms.allows_rpc("did:web:api.bsky.app", &c("app.bsky.feed.getTimeline"))); + } } diff --git a/frontend/src/locales/en.json b/frontend/src/locales/en.json index fe26323..82fcd8a 100644 --- a/frontend/src/locales/en.json +++ b/frontend/src/locales/en.json @@ -634,7 +634,13 @@ "title": "Unexpected State", "description": "The consent page is in an unexpected state. Please check the browser console for errors.", "reload": "Reload Page" - } + }, + "supersedeWarningTitle": "This app asked for broad access", + "supersedeWarningBody": "This application has requested full read and write access to your account.", + "supersededNote": "Already covered by transition:generic", + "deselectedWarningTitle": "Some permissions are disabled", + "deselectedWarningBody": "You have disabled some of the permissions requested by this app. This may cause some parts of the app to be broken or unavailable.", + "supersedeWarningBodyMixed": "This application has requested full read and write access to your account, alongside more specific permissions. The specific permissions are meaningless if you grant the application complete and total control by leaving transition:generic selected." }, "accounts": { "title": "Choose account", diff --git a/frontend/src/routes/OAuthConsent.svelte b/frontend/src/routes/OAuthConsent.svelte index 0860894..0a225fd 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 + superseded?: boolean effective_scope?: string } @@ -43,6 +44,7 @@ expanded: ScopeInfo[] granted: boolean | null restricted?: boolean + superseded?: boolean } type SetFailureReason = @@ -81,6 +83,7 @@ logo_uri: string | null scopes: ScopeInfo[] permission_sets: PermissionSetInfo[] + transition_supersedes?: boolean failed_sets: FailedSetInfo[] show_consent: boolean did: string @@ -264,10 +267,32 @@ } } + const TRANSITION_GENERIC = 'transition:generic' + + let transitionGenericSelected = $derived(scopeSelections[TRANSITION_GENERIC] === true) + + let anyDeselected = $derived( + Object.values(scopeSelections).some((selected) => selected === false) + ) + + function isSupersededNow(item: { superseded?: boolean }): boolean { + return Boolean(consentData?.transition_supersedes && item.superseded && transitionGenericSelected) + } + function handleScopeToggle(scope: string) { const scopeInfo = consentData?.scopes.find(s => s.scope === scope) if (scopeInfo?.required) return - scopeSelections[scope] = !scopeSelections[scope] + if (scopeInfo && isSupersededNow(scopeInfo)) return + const next = !scopeSelections[scope] + scopeSelections[scope] = next + if (scope === TRANSITION_GENERIC && next) { + for (const s of consentData?.scopes ?? []) { + if (s.superseded && !s.restricted) scopeSelections[s.scope] = true + } + for (const set of consentData?.permission_sets ?? []) { + if (set.superseded && !set.restricted) scopeSelections[set.include_scope] = true + } + } } const CATEGORY_ORDER = [ @@ -460,6 +485,30 @@ {/if} + + {#if transitionGenericSelected} +
+
+ + {$_('oauth.consent.supersedeWarningTitle')} +
+

+ {consentData.transition_supersedes + ? $_('oauth.consent.supersedeWarningBodyMixed') + : $_('oauth.consent.supersedeWarningBody')} +

+
+ {/if} + + {#if anyDeselected} +
+
+ + {$_('oauth.consent.deselectedWarningTitle')} +
+

{$_('oauth.consent.deselectedWarningBody')}

+
+ {/if}
@@ -482,8 +531,8 @@ {/each} @@ -509,7 +561,7 @@