diff --git a/crates/tranquil-api/src/delegation.rs b/crates/tranquil-api/src/delegation.rs index f6bfcb0..43f7d71 100644 --- a/crates/tranquil-api/src/delegation.rs +++ b/crates/tranquil-api/src/delegation.rs @@ -174,9 +174,7 @@ pub async fn remove_controller( .session .delete_app_passwords_by_controller(&auth.did, &input.controller_did) .await - .unwrap_or(0) - .try_into() - .unwrap_or(0usize); + .unwrap_or(0); let revoked_oauth_tokens = state .repos @@ -232,6 +230,20 @@ pub async fn update_controller_scopes( .await { Ok(true) => { + let revoked_app_passwords = state + .repos + .session + .delete_app_passwords_by_controller(&auth.did, &input.controller_did) + .await + .unwrap_or(0); + + let revoked_oauth_tokens = state + .repos + .oauth + .revoke_tokens_for_controller(&auth.did, &input.controller_did) + .await + .unwrap_or(0); + let _ = state .repos .delegation @@ -241,7 +253,9 @@ pub async fn update_controller_scopes( Some(&input.controller_did), DelegationActionType::ScopesModified, Some(json!({ - "new_scopes": input.granted_scopes.as_str() + "new_scopes": input.granted_scopes.as_str(), + "revoked_app_passwords": revoked_app_passwords, + "revoked_oauth_tokens": revoked_oauth_tokens })), None, None, diff --git a/crates/tranquil-api/src/server/app_password.rs b/crates/tranquil-api/src/server/app_password.rs index 9f187a4..dacee00 100644 --- a/crates/tranquil-api/src/server/app_password.rs +++ b/crates/tranquil-api/src/server/app_password.rs @@ -116,7 +116,10 @@ pub async fn create_app_password( .await .ok() .flatten(); - let granted_scopes = grant.map(|g| g.granted_scopes).unwrap_or_default(); + let granted_scopes = match grant { + Some(g) => g.granted_scopes, + None => return Err(ApiError::InsufficientScope(None)), + }; let requested = input.scopes.as_deref().unwrap_or("atproto"); let intersected = intersect_scopes(requested, granted_scopes.as_str()); diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs index bb84be9..85d3d97 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs @@ -351,34 +351,16 @@ pub async fn consent_post( } else { original_scope_str.to_string() }; - let requested_scopes: Vec<&str> = effective_scope_str.split_whitespace().collect(); - let has_granular_scopes = requested_scopes.iter().any(|s| is_granular_scope(s)); - let user_denied_some_granular = has_granular_scopes - && requested_scopes - .iter() - .filter(|s| is_granular_scope(s)) - .any(|s| !form.approved_scopes.contains(&s.to_string())); let atproto_was_requested = requested_scopes.contains(&"atproto"); - if atproto_was_requested - && !has_granular_scopes - && !form.approved_scopes.contains(&"atproto".to_string()) - { + if atproto_was_requested && !form.approved_scopes.contains(&"atproto".to_string()) { return json_error( StatusCode::BAD_REQUEST, "invalid_request", "The atproto scope was requested and must be approved", ); } - let final_approved: Vec = if user_denied_some_granular { - form.approved_scopes - .iter() - .filter(|s| *s != "atproto") - .cloned() - .collect() - } else { - form.approved_scopes.clone() - }; + let final_approved: Vec = form.approved_scopes.clone(); if final_approved.is_empty() { return json_error( StatusCode::BAD_REQUEST, diff --git a/crates/tranquil-oauth-server/src/endpoints/token/grants.rs b/crates/tranquil-oauth-server/src/endpoints/token/grants.rs index 183fbff..698eb40 100644 --- a/crates/tranquil-oauth-server/src/endpoints/token/grants.rs +++ b/crates/tranquil-oauth-server/src/endpoints/token/grants.rs @@ -148,7 +148,14 @@ pub async fn handle_authorization_code_grant( .await .ok() .flatten(); - let granted_scopes = grant.map(|g| g.granted_scopes).unwrap_or_default(); + let granted_scopes = match grant { + Some(g) => g.granted_scopes, + None => { + return Err(OAuthError::InvalidGrant( + "Delegation grant not found or revoked".to_string(), + )); + } + }; let requested = authorized.parameters.scope.as_deref().unwrap_or("atproto"); let intersected = intersect_scopes(requested, granted_scopes.as_str()); (Some(intersected), Some(controller.clone())) diff --git a/crates/tranquil-pds/src/delegation/mod.rs b/crates/tranquil-pds/src/delegation/mod.rs index 2a29b5b..598c56e 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::{ - InvalidDelegationScopeError, SCOPE_PRESETS, ScopePreset, ValidatedDelegationScope, - intersect_scopes, + EDITOR_FULL_SCOPES, InvalidDelegationScopeError, OWNER_FULL_SCOPES, SCOPE_PRESETS, ScopePreset, + ValidatedDelegationScope, 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 e9efd8c..f8bf636 100644 --- a/crates/tranquil-pds/src/delegation/scopes.rs +++ b/crates/tranquil-pds/src/delegation/scopes.rs @@ -12,12 +12,18 @@ pub struct ScopePreset { pub scopes: &'static str, } +pub const OWNER_FULL_SCOPES: &str = + "atproto repo:* blob:*/* identity:* account:*?action=manage"; + +pub const EDITOR_FULL_SCOPES: &str = + "atproto repo:*?action=create repo:*?action=update repo:*?action=delete blob:*/*"; + pub const SCOPE_PRESETS: &[ScopePreset] = &[ ScopePreset { name: "owner", label: "Owner", description: "Full control including delegation management", - scopes: "atproto", + scopes: OWNER_FULL_SCOPES, }, ScopePreset { name: "admin", @@ -29,7 +35,7 @@ pub const SCOPE_PRESETS: &[ScopePreset] = &[ name: "editor", label: "Editor", description: "Post content and upload media", - scopes: "repo:*?action=create repo:*?action=update repo:*?action=delete blob:*/*", + scopes: EDITOR_FULL_SCOPES, }, ScopePreset { name: "viewer", @@ -40,36 +46,19 @@ pub const SCOPE_PRESETS: &[ScopePreset] = &[ ]; pub fn intersect_scopes(requested: &str, granted: &str) -> String { - if granted.is_empty() { - return String::new(); - } - let requested_set: HashSet<&str> = requested.split_whitespace().collect(); let granted_set: HashSet<&str> = granted.split_whitespace().collect(); - let granted_has_atproto = granted_set.contains("atproto"); - let requested_has_atproto = requested_set.contains("atproto"); - - if granted_has_atproto { - let mut scopes: Vec<&str> = requested_set.into_iter().collect(); - scopes.sort(); - return scopes.join(" "); - } - - if requested_has_atproto { - let mut scopes: Vec<&str> = granted_set.into_iter().collect(); - scopes.sort(); - return scopes.join(" "); - } - - let mut result: Vec<&str> = requested_set + let mut scopes: Vec<&str> = requested_set .iter() - .filter(|requested_scope| any_granted_covers(requested_scope, &granted_set)) + .filter(|requested_scope| { + **requested_scope != "atproto" && any_granted_covers(requested_scope, &granted_set) + }) .copied() + .chain(requested_set.contains("atproto").then_some("atproto")) .collect(); - - result.sort(); - result.join(" ") + scopes.sort(); + scopes.join(" ") } fn any_granted_covers(requested: &str, granted: &HashSet<&str>) -> bool { @@ -159,17 +148,91 @@ mod tests { } #[test] - fn test_intersect_granted_atproto() { - let result = intersect_scopes("repo:* blob:*/*", "atproto"); + fn test_intersect_owner_grant_covers_requested() { + let result = intersect_scopes("repo:* blob:*/*", OWNER_FULL_SCOPES); assert!(result.contains("repo:*")); assert!(result.contains("blob:*/*")); } #[test] - fn test_intersect_requested_atproto() { - let result = intersect_scopes("atproto", "repo:* blob:*/*"); - assert!(result.contains("repo:*")); + fn test_intersect_bare_atproto_grant_is_auth_only() { + let requested = "atproto repo:*?action=create blob:*/*"; + assert_eq!(intersect_scopes(requested, "atproto"), "atproto"); + } + + #[test] + fn test_intersect_bare_atproto_request_is_auth_only() { + assert_eq!(intersect_scopes("atproto", "repo:* blob:*/*"), "atproto"); + } + + #[test] + fn test_intersect_downscoped_request_keeps_atproto() { + let approved = "atproto repo:*?action=create blob:*/* account:*?action=manage"; + let result = intersect_scopes(approved, OWNER_FULL_SCOPES); + assert!(result.split_whitespace().any(|s| s == "atproto")); + assert!(result.contains("account:*?action=manage")); + assert!(result.contains("repo:*?action=create")); assert!(result.contains("blob:*/*")); + assert!(!result.contains("identity")); + } + + #[test] + fn test_intersect_owner_passes_through_identity() { + let requested = "atproto repo:*?action=create identity:* account:*?action=manage"; + let result = intersect_scopes(requested, OWNER_FULL_SCOPES); + assert!(result.contains("identity:*")); + assert!(result.contains("account:*?action=manage")); + } + + #[test] + fn test_intersect_admin_excludes_identity() { + let requested = "atproto repo:*?action=create identity:* account:*?action=manage"; + let granted = "atproto repo:* blob:*/* account:*?action=manage"; + let result = intersect_scopes(requested, granted); + assert!(!result.contains("identity")); + assert!(result.contains("account:*?action=manage")); + } + + #[test] + fn test_intersect_admin_excludes_identity_coverage_path() { + let requested = "repo:*?action=create identity:* account:*?action=manage"; + let granted = "atproto repo:* blob:*/* account:*?action=manage"; + let result = intersect_scopes(requested, granted); + assert!(!result.contains("identity")); + assert!(result.contains("account:*?action=manage")); + assert!(result.contains("repo:*?action=create")); + } + + #[test] + fn test_intersect_editor_grant_keeps_atproto() { + let editor = SCOPE_PRESETS + .iter() + .find(|p| p.name == "editor") + .expect("editor preset") + .scopes; + let requested = + "atproto repo:*?action=create identity:* account:*?action=manage blob:*/*"; + let result = intersect_scopes(requested, editor); + assert!(result.split_whitespace().any(|s| s == "atproto")); + assert!(result.contains("repo:*?action=create")); + assert!(result.contains("blob:*/*")); + assert!(!result.contains("identity")); + assert!(!result.contains("account")); + } + + #[test] + fn test_intersect_guarantees_atproto_for_custom_grant() { + let result = intersect_scopes( + "atproto repo:*?action=create blob:*/*", + "repo:*?action=create blob:*/*", + ); + assert!(result.split_whitespace().any(|s| s == "atproto")); + assert!(result.contains("blob:*/*")); + } + + #[test] + fn test_intersect_no_atproto_request_stays_empty_when_uncovered() { + assert_eq!(intersect_scopes("identity:*", "repo:* blob:*/*"), ""); } #[test] @@ -181,8 +244,14 @@ mod tests { } #[test] - fn test_intersect_empty_granted() { - assert_eq!(intersect_scopes("atproto", ""), ""); + fn test_intersect_viewer_grant_keeps_atproto() { + let requested = "atproto repo:*?action=create blob:*/* identity:*"; + assert_eq!(intersect_scopes(requested, ""), "atproto"); + } + + #[test] + fn test_intersect_empty_grant_without_atproto_request_is_empty() { + assert_eq!(intersect_scopes("repo:*?action=create", ""), ""); } #[test] diff --git a/crates/tranquil-pds/src/state.rs b/crates/tranquil-pds/src/state.rs index 1927d5c..5d6c3f3 100644 --- a/crates/tranquil-pds/src/state.rs +++ b/crates/tranquil-pds/src/state.rs @@ -482,6 +482,46 @@ struct TranquilStoreWiring { segments_dir: PathBuf, } +fn migrate_delegation_preset_scopes(metastore: &tranquil_store::metastore::Metastore) { + const MARKER_KEY: &str = "migration:delegation_preset_scopes_v1"; + const LEGACY_EDITOR_SCOPES: &str = + "repo:*?action=create repo:*?action=update repo:*?action=delete blob:*/*"; + + let infra = metastore.infra_ops(); + if infra + .get_server_config(MARKER_KEY) + .ok() + .flatten() + .is_some() + { + return; + } + + let ops = metastore.delegation_ops(); + let owners = match ops.remap_grant_scopes("atproto", crate::delegation::OWNER_FULL_SCOPES) { + Ok(n) => n, + Err(e) => { + tracing::error!(error = ?e, "delegation owner-scope migration failed, will retry on next start"); + return; + } + }; + let editors = + match ops.remap_grant_scopes(LEGACY_EDITOR_SCOPES, crate::delegation::EDITOR_FULL_SCOPES) { + Ok(n) => n, + Err(e) => { + tracing::error!(error = ?e, "delegation editor-scope migration failed, will retry on next start"); + return; + } + }; + if owners + editors > 0 { + tracing::info!(owners, editors, "upgraded legacy delegation grants to preset scopes"); + } + + if let Err(e) = infra.upsert_server_config(MARKER_KEY, "done") { + tracing::error!(error = ?e, "failed to record delegation scope migration marker, will retry"); + } +} + fn wire_tranquil_store( store_cfg: &tranquil_config::TranquilStoreConfig, shutdown: CancellationToken, @@ -619,6 +659,8 @@ fn wire_tranquil_store( } } + migrate_delegation_preset_scopes(&metastore); + let notifier = bridge.notifier(); let signal_db = metastore.database().clone(); let signal_ks = metastore.signal_keyspace(); diff --git a/crates/tranquil-store/src/metastore/delegation_ops.rs b/crates/tranquil-store/src/metastore/delegation_ops.rs index 7740249..19b4c81 100644 --- a/crates/tranquil-store/src/metastore/delegation_ops.rs +++ b/crates/tranquil-store/src/metastore/delegation_ops.rs @@ -187,6 +187,40 @@ impl DelegationOps { } } + pub fn remap_grant_scopes(&self, from: &str, to: &str) -> Result { + let prefix = super::encoding::KeyBuilder::new() + .tag(super::keys::KeyTag::DELEG_GRANT) + .build(); + + let mut batch = self.db.batch(); + let migrated = self.indexes.prefix(prefix.as_slice()).try_fold( + 0usize, + |count, guard| -> Result { + let (key_bytes, val_bytes) = guard.into_inner().map_err(MetastoreError::Fjall)?; + match DelegationGrantValue::deserialize(&val_bytes) { + Some(mut val) if val.granted_scopes == from => { + val.granted_scopes = to.to_owned(); + batch.insert(&self.indexes, key_bytes, val.serialize()); + Ok(count + 1) + } + Some(_) => Ok(count), + None => { + tracing::warn!("skipping corrupt delegation grant during scope remap"); + Ok(count) + } + } + }, + )?; + + match migrated { + 0 => Ok(0), + _ => { + batch.commit().map_err(MetastoreError::Fjall)?; + Ok(migrated) + } + } + } + pub fn get_delegation( &self, delegated_did: &Did, @@ -410,3 +444,87 @@ impl DelegationOps { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::metastore::{Metastore, MetastoreConfig}; + + const OWNER_FULL: &str = "atproto repo:* blob:*/* identity:* account:*?action=manage"; + + fn fresh() -> (tempfile::TempDir, Metastore) { + let dir = tempfile::tempdir().expect("tempdir"); + let ms = Metastore::open(dir.path(), MetastoreConfig::default()).expect("open metastore"); + (dir, ms) + } + + fn did(s: &str) -> Did { + Did::new(s.to_owned()).expect("valid did") + } + + #[test] + fn remap_upgrades_only_matching_grants() { + let (_dir, ms) = fresh(); + let ops = ms.delegation_ops(); + + let owner = did("did:plc:nel"); + let ctrl_owner = did("did:plc:olaren"); + let ctrl_editor = did("did:plc:teq"); + + ops.create_delegation(&owner, &ctrl_owner, &DbScope::new("atproto").unwrap(), &owner) + .unwrap(); + ops.create_delegation( + &owner, + &ctrl_editor, + &DbScope::new("repo:* blob:*/*").unwrap(), + &owner, + ) + .unwrap(); + + assert_eq!(ops.remap_grant_scopes("atproto", OWNER_FULL).unwrap(), 1); + + let upgraded = ops.get_delegation(&owner, &ctrl_owner).unwrap().unwrap(); + assert_eq!(upgraded.granted_scopes.as_str(), OWNER_FULL); + + let untouched = ops.get_delegation(&owner, &ctrl_editor).unwrap().unwrap(); + assert_eq!(untouched.granted_scopes.as_str(), "repo:* blob:*/*"); + } + + #[test] + fn remap_is_idempotent() { + let (_dir, ms) = fresh(); + let ops = ms.delegation_ops(); + let owner = did("did:plc:limpet"); + let ctrl = did("did:plc:whelk"); + + ops.create_delegation(&owner, &ctrl, &DbScope::new("atproto").unwrap(), &owner) + .unwrap(); + + assert_eq!(ops.remap_grant_scopes("atproto", OWNER_FULL).unwrap(), 1); + assert_eq!(ops.remap_grant_scopes("atproto", OWNER_FULL).unwrap(), 0); + } + + #[test] + fn remap_skips_corrupt_grant() { + let (_dir, ms) = fresh(); + let ops = ms.delegation_ops(); + + let owner = did("did:plc:nautilus"); + let ctrl = did("did:plc:periwinkle"); + ops.create_delegation(&owner, &ctrl, &DbScope::new("atproto").unwrap(), &owner) + .unwrap(); + + let corrupt_key = grant_key( + UserHash::from_did("did:plc:conch"), + UserHash::from_did("did:plc:scallop"), + ); + let mut batch = ops.db.batch(); + batch.insert(&ops.indexes, corrupt_key.as_slice(), b"not a grant".as_slice()); + batch.commit().unwrap(); + + assert_eq!(ops.remap_grant_scopes("atproto", OWNER_FULL).unwrap(), 1); + + let upgraded = ops.get_delegation(&owner, &ctrl).unwrap().unwrap(); + assert_eq!(upgraded.granted_scopes.as_str(), OWNER_FULL); + } +} diff --git a/frontend/src/components/dashboard/ControllersContent.svelte b/frontend/src/components/dashboard/ControllersContent.svelte index 26a6799..b1e1c26 100644 --- a/frontend/src/components/dashboard/ControllersContent.svelte +++ b/frontend/src/components/dashboard/ControllersContent.svelte @@ -42,6 +42,7 @@ let controllers = $state([]) let controlledAccounts = $state([]) let scopePresets = $state([]) + let defaultScopes = $state('') let hasControllers = $derived(controllers.length > 0) let controlsAccounts = $derived(controlledAccounts.length > 0) @@ -50,7 +51,7 @@ let showAddController = $state(false) let addControllerIdentifier = $state('') - let addControllerScopes = $state('atproto') + let addControllerScopes = $state('') let addingController = $state(false) let addControllerConfirmed = $state(false) let resolvedController = $state<{ did: string; handle?: string; pdsUrl?: string; isLocal: boolean } | null>(null) @@ -119,7 +120,7 @@ let showCreateDelegated = $state(false) let newDelegatedHandle = $state('') let newDelegatedEmail = $state('') - let newDelegatedScopes = $state('atproto') + let newDelegatedScopes = $state('') let creatingDelegated = $state(false) onMount(async () => { @@ -168,6 +169,9 @@ description: p.description, scopes: unsafeAsScopeSet(p.scopes) })) + defaultScopes = scopePresets.find(p => p.name === 'owner')?.scopes ?? scopePresets[0]?.scopes ?? '' + addControllerScopes = defaultScopes + newDelegatedScopes = defaultScopes } } @@ -181,7 +185,7 @@ if (result.ok) { toast.success($_('delegation.controllerAdded')) addControllerIdentifier = '' - addControllerScopes = 'atproto' + addControllerScopes = defaultScopes addControllerConfirmed = false resolvedController = null showAddController = false @@ -214,7 +218,7 @@ toast.success($_('delegation.accountCreated', { values: { handle: result.value.handle } })) newDelegatedHandle = '' newDelegatedEmail = '' - newDelegatedScopes = 'atproto' + newDelegatedScopes = defaultScopes showCreateDelegated = false await loadControlledAccounts() } @@ -224,7 +228,6 @@ function getScopeLabel(scopes: ScopeSet): string { const preset = scopePresets.find(p => p.scopes === scopes) if (preset) return preset.label - if ((scopes as string) === 'atproto') return $_('delegation.scopeOwner') if ((scopes as string) === '') return $_('delegation.scopeViewer') return $_('delegation.scopeCustom') } diff --git a/frontend/src/lib/oauth.ts b/frontend/src/lib/oauth.ts index d343412..e797a91 100644 --- a/frontend/src/lib/oauth.ts +++ b/frontend/src/lib/oauth.ts @@ -3,7 +3,7 @@ const OAUTH_VERIFIER_KEY = "tranquil_pds_oauth_verifier"; const DPOP_KEY_STORE = "tranquil_pds_dpop_keys"; const DPOP_NONCE_KEY = "tranquil_pds_dpop_nonce"; -const SCOPES = [ +export const SCOPES = [ "atproto", "repo:*?action=create", "repo:*?action=update", @@ -16,7 +16,7 @@ const SCOPES = [ const CLIENT_ID = !(import.meta.env.DEV) || globalThis.location?.hostname !== 'localhost' ? `${globalThis.location.origin}/oauth-client-metadata.json` - : `http://localhost/?scope=${SCOPES}`; + : `http://localhost/?scope=${encodeURIComponent(SCOPES)}`; const REDIRECT_URI = `${globalThis.location.origin}/app/`; diff --git a/frontend/src/routes/ActAs.svelte b/frontend/src/routes/ActAs.svelte index 6b2293e..5663498 100644 --- a/frontend/src/routes/ActAs.svelte +++ b/frontend/src/routes/ActAs.svelte @@ -1,7 +1,7 @@