diff --git a/crates/didbot-scope/src/account.rs b/crates/didbot-scope/src/account.rs new file mode 100644 index 00000000..a4b92933 --- /dev/null +++ b/crates/didbot-scope/src/account.rs @@ -0,0 +1,87 @@ +//! `account:` scopes, as the permission spec defines them: one attribute of +//! the account's hosting, and how much control over it. + +use std::fmt; + +use percent_encoding::percent_decode_str; + +use crate::parse::split_query; +use crate::{Scope, ScopeParseError}; + +/// The part of an account's hosting an `account:` scope names. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum AccountAttr { + /// `email`: the account's email address, and whether it is confirmed. + Email, + /// `repo`: the whole repository, imported as a CAR file. + Repo, +} + +/// How much control an `account:` scope grants over its attribute. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum AccountAction { + /// `read`, which a scope naming no action grants. + Read, + /// `manage`, which includes `read`. + Manage, +} + +impl AccountAttr { + fn parse(value: &str) -> Result { + match value { + "email" => Ok(Self::Email), + "repo" => Ok(Self::Repo), + other => Err(ScopeParseError::UnknownAttribute(other.to_owned())), + } + } +} + +impl fmt::Display for AccountAttr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + Self::Email => "email", + Self::Repo => "repo", + }) + } +} + +impl AccountAction { + fn parse(value: &str) -> Result { + match value { + "read" => Ok(Self::Read), + "manage" => Ok(Self::Manage), + other => Err(ScopeParseError::UnknownAction(other.to_owned())), + } + } + + /// Whether this action allows everything `other` allows. + pub(crate) fn includes(self, other: Self) -> bool { + self == other || self == Self::Manage + } +} + +/// Reads what follows `account`: the attribute, written positionally or as +/// `attr`, and at most one `action`, `read` when none is given. +pub(crate) fn parse(rest: &str, atom: &str) -> Result { + let (value, pairs) = split_query(rest)?; + let decode = |value: &str| percent_decode_str(value).decode_utf8_lossy().into_owned(); + let mut attr = (!value.is_empty()).then(|| decode(value)); + let mut action = None; + for (key, value) in pairs { + match key { + "attr" if attr.is_none() => attr = Some(decode(value)), + "action" if action.is_none() => action = Some(AccountAction::parse(&decode(value))?), + _ => { + return Err(ScopeParseError::UnexpectedParameter( + key.to_owned(), + atom.to_owned(), + )) + } + } + } + let attr = attr.ok_or_else(|| ScopeParseError::MissingValue(atom.to_owned()))?; + Ok(Scope::Account { + attr: AccountAttr::parse(&attr)?, + action: action.unwrap_or(AccountAction::Read), + }) +} diff --git a/crates/didbot-scope/src/action.rs b/crates/didbot-scope/src/action.rs index a74f3a18..096b11a4 100644 --- a/crates/didbot-scope/src/action.rs +++ b/crates/didbot-scope/src/action.rs @@ -8,13 +8,14 @@ use crate::ScopeParseError; /// One action an `action=` query may name. /// /// `Create`, `Update` and `Delete` are `repo:`'s vocabulary. `Manage` is -/// `identity:` and `account:`'s — those resources are not written record by -/// record, so the grammar has one verb for "may change this" rather than -/// three. Sharing one enum across both keeps [`ActionSet`]'s containment and -/// intersection logic single, and a `repo:` scope requesting `manage` (or an -/// `identity:`/`account:` scope requesting `create`) is nonsensical but -/// harmless: containment against a ceiling that never grants the -/// mismatched verb simply always fails, same as any other action mismatch. +/// `identity:`'s — an identity is not written record by record, so the +/// grammar has one verb for "may change this" rather than three. Sharing one +/// enum across both keeps [`ActionSet`]'s containment and intersection logic +/// single, and a `repo:` scope requesting `manage` (or an `identity:` scope +/// requesting `create`) is nonsensical but harmless: containment against a +/// ceiling that never grants the mismatched verb simply always fails, same +/// as any other action mismatch. `account:` has its own, in +/// [`crate::AccountAction`]. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum Action { /// `repo:`'s "may create a new record". @@ -23,7 +24,7 @@ pub enum Action { Update, /// `repo:`'s "may delete a record". Delete, - /// `identity:`/`account:`'s "may change this resource". + /// `identity:`'s "may change this resource". Manage, } @@ -48,7 +49,7 @@ impl Action { } } -/// The set of actions a `repo:` (or `identity:`/`account:`) scope grants. +/// The set of actions a `repo:` (or `identity:`) scope grants. /// /// `All` is distinct from an explicit enumeration of every action: it is /// what an atom with no `action=` query means, and it is what containment diff --git a/crates/didbot-scope/src/error.rs b/crates/didbot-scope/src/error.rs index ecd705de..43726563 100644 --- a/crates/didbot-scope/src/error.rs +++ b/crates/didbot-scope/src/error.rs @@ -63,9 +63,16 @@ pub enum ScopeParseError { /// The MIME value is not `type/subtype`, with either half a `*`. #[error("`{0}` is not a well-formed MIME type or MIME wildcard")] MalformedMime(String), - /// An `action=` value named something other than create, update, delete. + /// An `action=` value the atom's kind does not define. #[error("`{0}` is not a recognised action")] UnknownAction(String), + /// An `account:` atom named an attribute the permission spec does not + /// define. + #[error("`{0}` is not an account attribute")] + UnknownAttribute(String), + /// A parameter the atom's kind does not take, or one given twice. + #[error("`{0}` is not a parameter `{1}` takes, or is given twice")] + UnexpectedParameter(String, String), /// A `transition:` atom named something other than the three defined /// legacy scopes. #[error("`{0}` is not a recognised transition scope")] diff --git a/crates/didbot-scope/src/lib.rs b/crates/didbot-scope/src/lib.rs index 62ccbf41..aae9b35a 100644 --- a/crates/didbot-scope/src/lib.rs +++ b/crates/didbot-scope/src/lib.rs @@ -40,8 +40,10 @@ //! limit. //! - `identity:[?action=]` — `plan/scope-policy.md`'s //! hard-blocked identity capability (handle changes, key rotation). -//! - `account:[?action=]` — the other hard-blocked -//! capability (email, account status, migration). +//! - `account:[?action=]` — the other hard-blocked capability: +//! [`AccountAttr`] names what, and [`AccountAction`] how much, `read` when +//! the atom names none. The attribute may also be written as a parameter, +//! `account?attr=repo&action=manage`. //! - `include:[?aud=%23]` — a permission set, published //! as a lexicon. As an atom it admits only itself. [`Include`] reads it, //! and [`Include::grants`] turns the set's published permissions into the @@ -74,6 +76,7 @@ #![forbid(unsafe_code)] +mod account; mod action; mod error; mod include; @@ -86,6 +89,7 @@ mod transition; #[cfg(test)] mod tests; +pub use account::{AccountAction, AccountAttr}; pub use action::{Action, ActionSet}; pub use error::{ ScopeParseError, MAX_ATOM_BYTES, MAX_GRANT_ATOMS, MAX_GRANT_BYTES, MAX_SCOPE_ATOMS, diff --git a/crates/didbot-scope/src/scope.rs b/crates/didbot-scope/src/scope.rs index 5a3e3942..97193376 100644 --- a/crates/didbot-scope/src/scope.rs +++ b/crates/didbot-scope/src/scope.rs @@ -7,7 +7,8 @@ use percent_encoding::{percent_decode_str, utf8_percent_encode, AsciiSet, CONTRO use crate::parse::{parse_actions, split_kind, split_query}; use crate::{ - Action, ActionSet, MimePattern, NsidPattern, ScopeParseError, Transition, MAX_ATOM_BYTES, + AccountAction, AccountAttr, Action, ActionSet, MimePattern, NsidPattern, ScopeParseError, + Transition, MAX_ATOM_BYTES, }; /// One parsed scope atom. @@ -46,12 +47,12 @@ pub enum Scope { /// Which actions are covered. actions: ActionSet, }, - /// `account:[?action=...]` — also hard-blocked. + /// `account:[?action=...]` — also hard-blocked. Account { - /// The account resource this scope names, e.g. `email`. - resource: String, - /// Which actions are covered. - actions: ActionSet, + /// The part of the account's hosting this scope names. + attr: AccountAttr, + /// How much control over it this scope grants. + action: AccountAction, }, /// `include:[?aud=...]` — a permission set, kept as written. As an /// atom it admits only itself; [`crate::Include`] reads it. @@ -129,16 +130,7 @@ impl Scope { actions: parse_actions(&pairs)?, }) } - "account" => { - let (value, pairs) = split_query(rest)?; - if value.is_empty() { - return Err(ScopeParseError::MissingValue(atom.to_owned())); - } - Ok(Scope::Account { - resource: value.to_owned(), - actions: parse_actions(&pairs)?, - }) - } + "account" => crate::account::parse(rest, atom), other => Err(ScopeParseError::UnknownKind(other.to_owned())), } } @@ -189,14 +181,14 @@ impl Scope { ) => resource_contains(r1, r2) && a1.contains(a2), ( Scope::Account { - resource: r1, - actions: a1, + attr: a1, + action: x1, }, Scope::Account { - resource: r2, - actions: a2, + attr: a2, + action: x2, }, - ) => resource_contains(r1, r2) && a1.contains(a2), + ) => a1 == a2 && x1.includes(*x2), _ => false, } } @@ -268,19 +260,6 @@ impl Scope { resource: r1.clone(), actions: a1.intersect(a2)?, }), - ( - Scope::Account { - resource: r1, - actions: a1, - }, - Scope::Account { - resource: r2, - actions: a2, - }, - ) if r1 == r2 => Some(Scope::Account { - resource: r1.clone(), - actions: a1.intersect(a2)?, - }), _ => None, } } @@ -413,7 +392,13 @@ impl fmt::Display for Scope { } }, Scope::Identity { resource, actions } => write!(f, "identity:{resource}{actions}"), - Scope::Account { resource, actions } => write!(f, "account:{resource}{actions}"), + Scope::Account { attr, action } => { + write!(f, "account:{attr}")?; + if *action == AccountAction::Manage { + write!(f, "?action=manage")?; + } + Ok(()) + } Scope::Include(name) => write!(f, "include:{name}"), } } diff --git a/crates/didbot-scope/src/tests.rs b/crates/didbot-scope/src/tests.rs index c0dce439..fb023969 100644 --- a/crates/didbot-scope/src/tests.rs +++ b/crates/didbot-scope/src/tests.rs @@ -100,10 +100,6 @@ fn a_comma_list_of_actions_is_read_and_printed_as_repeated_parameters() { scope("repo:app.bsky.feed.post?action=create,update").to_string(), "repo:app.bsky.feed.post?action=create&action=update" ); - assert_eq!( - scope("account:email?action=manage,manage").to_string(), - "account:email?action=manage" - ); assert!(matches!( Scope::parse("repo:app.bsky.feed.post?action=create,"), Err(ScopeParseError::UnknownAction(_)) @@ -219,11 +215,52 @@ fn transition_chat_bsky_is_narrower_than_generic() { assert!(!chat.contains(&scope("rpc:app.bsky.feed.getTimeline"))); } +/// The OAuth spec's `transition:email` is "access to the account email +/// address": reading it, which is `account:email`, and not changing it. #[test] -fn transition_email_covers_account_email_only() { +fn transition_email_covers_reading_the_account_email_only() { let email = scope("transition:email"); - assert!(email.contains(&scope("account:email?action=manage"))); - assert!(!email.contains(&scope("account:repo?action=manage"))); + assert!(email.contains(&scope("account:email"))); + assert!(!email.contains(&scope("account:email?action=manage"))); + assert!(!email.contains(&scope("account:repo"))); +} + +/// The permission spec's `account:`: an attribute, `email` or `repo`, and an +/// action, `read` unless it says `manage`. The attribute may be positional +/// or named, and `manage` includes `read`. +#[test] +fn account_reads_the_spec_attributes_and_actions() { + let read = Scope::Account { + attr: AccountAttr::Email, + action: AccountAction::Read, + }; + assert_eq!(scope("account:email"), read); + assert_eq!(scope("account:email?action=read"), read); + assert_eq!( + scope("account:email?action=read").to_string(), + "account:email" + ); + let import = scope("account:repo?action=manage"); + assert_eq!(scope("account?action=manage&attr=repo"), import); + assert_eq!(import.to_string(), "account:repo?action=manage"); + + assert!(scope("account:email?action=manage").contains(&read)); + assert!(!read.contains(&scope("account:email?action=manage"))); + assert!(!import.contains(&scope("account:email"))); + + for refused in [ + "account:*", + "account:status", + "account:email?action=create", + "account:email?action=manage,manage", + "account:email?action=read&action=manage", + "account:email?attr=repo", + "account?action=read", + "account:email?aud=did:web:a.example", + "account:", + ] { + assert!(Scope::parse(refused).is_err(), "`{refused}` parsed"); + } } #[test] diff --git a/crates/didbot-scope/src/transition.rs b/crates/didbot-scope/src/transition.rs index afb27d89..ed172d09 100644 --- a/crates/didbot-scope/src/transition.rs +++ b/crates/didbot-scope/src/transition.rs @@ -3,7 +3,7 @@ use std::fmt; use crate::pattern::under_prefix; -use crate::{NsidPattern, Scope, ScopeParseError}; +use crate::{AccountAction, AccountAttr, NsidPattern, Scope, ScopeParseError}; /// The three legacy `transition:*` scopes. /// @@ -19,7 +19,7 @@ pub enum Transition { Generic, /// The same, narrowed to the `chat.bsky.*` namespace. ChatBsky, - /// Account email read/write. + /// Reading the account's email address, as `account:email` does. Email, } @@ -65,7 +65,13 @@ impl Transition { .. }, ) => under_prefix("chat.bsky", nsid) || nsid == "chat.bsky", - (Transition::Email, Scope::Account { resource, .. }) => resource == "email", + ( + Transition::Email, + Scope::Account { + attr: AccountAttr::Email, + action: AccountAction::Read, + }, + ) => true, _ => false, } } diff --git a/crates/didbot-scope/tests/ceiling_boundary.rs b/crates/didbot-scope/tests/ceiling_boundary.rs index e61b5753..2802461f 100644 --- a/crates/didbot-scope/tests/ceiling_boundary.rs +++ b/crates/didbot-scope/tests/ceiling_boundary.rs @@ -62,7 +62,8 @@ const CORPUS: &[&str] = &[ "identity:handle?action=manage", "account:email", "account:email?action=manage", - "account:status", + "account:repo", + "account:repo?action=manage", ]; fn corpus() -> Vec { @@ -258,19 +259,19 @@ fn a_ceiling_refuses_the_scope_next_door() { // MIME halves are independent, and neither is a prefix test. ("blob:image/png", "blob:image/jpeg"), ("blob:image/*", "blob:video/mp4"), - // A different named resource. `identity:`/`account:` resources - // are opaque strings in this grammar, so a resource whose name - // merely *starts with* a granted one is exactly the input a - // prefix comparison would wave through — and nothing in the - // grammar stops a client asking for one. - ("account:email", "account:status"), + // A different named resource. `identity:` resources are opaque + // strings in this grammar, so a resource whose name merely + // *starts with* a granted one is exactly the input a prefix + // comparison would wave through — and nothing in the grammar stops + // a client asking for one. + ("account:email", "account:repo"), ("identity:handle", "account:email"), - ("account:email", "account:emailConfirmed"), ("identity:handle", "identity:handleHistory"), // `transition:` covers what it covers and no more. ("transition:chat.bsky", "repo:app.bsky.feed.post"), ("transition:chat.bsky", "repo:chat.bskyfoo.convo"), - ("transition:email", "account:status"), + ("transition:email", "account:repo"), + ("transition:email", "account:email?action=manage"), ("transition:generic", "identity:handle"), ("transition:generic", "account:email"), // The base scope grants no capability of its own. diff --git a/crates/didbot-serve/src/oauth/decision.rs b/crates/didbot-serve/src/oauth/decision.rs index dc4a0ddb..d3c5dbb8 100644 --- a/crates/didbot-serve/src/oauth/decision.rs +++ b/crates/didbot-serve/src/oauth/decision.rs @@ -1122,7 +1122,7 @@ mod tests { "identity:*", "identity:handle", "account:email", - "account:status", + "account:repo?action=manage", ] { let requested = ScopeSet::parse(&format!("atproto {atom}")).unwrap(); let verdict = verdict_for(&requested, &ceiling, CEILING_RULE); diff --git a/crates/didbot-serve/tests/permission_sets.rs b/crates/didbot-serve/tests/permission_sets.rs index 5107e4f5..ba36478d 100644 --- a/crates/didbot-serve/tests/permission_sets.rs +++ b/crates/didbot-serve/tests/permission_sets.rs @@ -17,12 +17,14 @@ use std::collections::HashMap; use std::net::SocketAddr; use std::sync::Arc; +use axum::body::Body; use axum::extract::Query; -use axum::http::{header, StatusCode}; +use axum::http::{header, Request, StatusCode}; use axum::response::IntoResponse; use axum::routing::get; use axum::{Json, Router}; use serde_json::{json, Value}; +use tower::ServiceExt; use didbot_http::PrivateAddresses; use didbot_key::SigningKey; @@ -263,15 +265,16 @@ fn scope(s: &str) -> ScopeSet { ScopeSet::parse(s).expect("a scope") } -/// The decision record a push of `scope` mints, as the agent reads it. -async fn pending(harness: &Harness, scope: &str) -> Value { +/// A push of `scope`, and the decision record it mints as the agent reads +/// it. +async fn push(harness: &Harness, scope: &str) -> (Value, Value) { let (status, pushed) = harness.push(APP, scope, CHALLENGE).await; assert_eq!(status, StatusCode::CREATED, "{pushed}"); let (status, listed) = harness .xrpc_get("bot.did.listPendingAuthorizations", &harness.account_token) .await; assert_eq!(status, StatusCode::OK, "{listed}"); - listed["pending"] + let record = listed["pending"] .as_array() .and_then(|pending| { pending @@ -279,7 +282,35 @@ async fn pending(harness: &Harness, scope: &str) -> Value { .find(|record| record["requestUri"] == pushed["request_uri"]) }) .cloned() - .unwrap_or_else(|| panic!("the push is pending: {listed}")) + .unwrap_or_else(|| panic!("the push is pending: {listed}")); + (pushed, record) +} + +/// What `GET /oauth/authorize` answers the client for a push. +async fn authorize(harness: &Harness, pushed: &Value) -> (StatusCode, Value) { + let query = url::form_urlencoded::Serializer::new(String::new()) + .append_pair("client_id", APP) + .append_pair( + "request_uri", + pushed["request_uri"].as_str().expect("a request_uri"), + ) + .finish(); + let response = harness + .app + .clone() + .oneshot( + Request::builder() + .uri(format!("/oauth/authorize?{query}")) + .body(Body::empty()) + .expect("a request"), + ) + .await + .expect("the router answers"); + let status = response.status(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("a body"); + (status, serde_json::from_slice(&body).unwrap_or(Value::Null)) } fn atoms(record: &Value, field: &str) -> Vec { @@ -341,7 +372,7 @@ async fn a_set_with_unacceptable_parts_is_narrowed_to_the_acceptable_ones() { let requested = "atproto include:com.example.authMixed?aud=did:web:api.example.com%23svc_appview"; - let record = pending(&harness, requested).await; + let (_, record) = push(&harness, requested).await; assert_eq!( atoms(&record, "requested"), [ @@ -395,32 +426,24 @@ async fn an_unresolvable_set_grants_nothing() { assert_eq!(status, StatusCode::OK, "{created}"); } -/// Leaflet's own scope. Verbatim it does not parse: this grammar has no -/// `read` action for `account:`. Without that atom, `transition:email` -/// refuses the request whole. Without both, its four sets expand to what -/// they publish. +/// Leaflet's own scope. It parses, and `transition:email` refuses it whole: +/// the push mints a record that denies it, and the authorize step answers +/// the client `access_denied`. Without the two atoms refused whole, its four +/// sets expand to what they publish. #[tokio::test] async fn leaflets_real_scope_string() { let authority = Authority::start().await; let harness = Harness::build_with_sets(&[APP], Arc::new(GrantAnyScope), authority.sets()); - let (status, refused) = harness.push(APP, LEAFLET, CHALLENGE).await; - assert_eq!(status, StatusCode::BAD_REQUEST, "{refused}"); - assert_eq!(refused["error"], "invalid_request"); - assert!( - refused["error_description"] - .as_str() - .is_some_and(|why| why.contains("`read`")), - "{refused}" - ); - - let parses = LEAFLET.replace("account:email?action=read ", ""); - let record = pending(&harness, &parses).await; + let (pushed, record) = push(&harness, LEAFLET).await; assert_eq!(record["verdict"]["kind"], "deny", "{record}"); assert_eq!(record["verdict"]["rule"], "hard-blocked: transition:email"); + let (status, refused) = authorize(&harness, &pushed).await; + assert_eq!(status, StatusCode::FORBIDDEN, "{refused}"); + assert_eq!(refused["error"], "access_denied", "{refused}"); - let granted = parses.replace("transition:email ", ""); - let record = pending(&harness, &granted).await; + let granted = LEAFLET.replace("transition:email account:email?action=read ", ""); + let (_, record) = push(&harness, &granted).await; assert_eq!(record["verdict"]["kind"], "allow", "{record}"); let requested = atoms(&record, "requested"); let count = |prefix: &str| {