fix: Address PR review

This commit is contained in:
Louis Escher
2026-08-13 16:09:03 +00:00
committed by Tangled
parent b3c314ce66
commit c88f69f31d
10 changed files with 352 additions and 122 deletions
+15 -3
View File
@@ -73,10 +73,13 @@ pub fn encode_scope(scope: &str) -> Result<String, ScopeEncodeError> {
return Err(ScopeEncodeError::TooLarge);
}
let encoded = URL_SAFE_NO_PAD.encode(brotli_compress(scope));
let tagged = format!(
"{COMPRESSED_PREFIX}{}",
URL_SAFE_NO_PAD.encode(brotli_compress(scope))
);
if COMPRESSED_PREFIX.len() + encoded.len() < scope.len() {
Ok(format!("{COMPRESSED_PREFIX}{encoded}"))
if tagged.len() < scope.len() || scope.starts_with(COMPRESSED_PREFIX) {
Ok(tagged)
} else {
Ok(scope.to_owned())
}
@@ -161,6 +164,15 @@ mod tests {
);
}
#[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);
@@ -187,7 +187,7 @@ 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))
grant_scope_str.is_some_and(|g| !tranquil_pds::delegation::grant_permits(g, scope))
};
let make_scope_info = |scope: &str| -> ScopeInfo {
+1 -1
View File
@@ -6,7 +6,7 @@ pub use roles::{
};
pub use scopes::{
EDITOR_FULL_SCOPES, InvalidDelegationScopeError, OWNER_FULL_SCOPES, SCOPE_PRESETS, ScopePreset,
ValidatedDelegationScope, grant_covers, intersect_scopes,
ValidatedDelegationScope, grant_permits, intersect_scopes,
};
pub use tranquil_db_traits::DelegationActionType;
+69 -34
View File
@@ -1,6 +1,6 @@
use std::collections::HashSet;
use std::collections::BTreeSet;
use tranquil_scopes::{covers, parse_scope};
use tranquil_scopes::{ParsedScope, narrow, parse_scope};
pub use tranquil_db_traits::{
DbScope as ValidatedDelegationScope, InvalidScopeError as InvalidDelegationScopeError,
@@ -47,34 +47,40 @@ 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<tranquil_scopes::ParsedScope> =
granted.split_whitespace().map(parse_scope).collect();
let granted_parsed: Vec<ParsedScope> = granted.split_whitespace().map(parse_scope).collect();
let mut scopes: Vec<&str> = requested_set
.iter()
.filter(|requested_scope| {
**requested_scope != "atproto" && any_granted_covers(requested_scope, &granted_parsed)
let scopes: BTreeSet<String> = requested
.split_whitespace()
.filter_map(|requested_scope| {
if requested_scope == "atproto" {
return Some(requested_scope.to_string());
}
let requested_parsed = parse_scope(requested_scope);
let narrowed = narrow(&granted_parsed, &requested_parsed)?;
if narrowed == requested_parsed {
return Some(requested_scope.to_string());
}
match &narrowed {
ParsedScope::Repo(repo) => Some(repo.to_scope_string()),
_ => Some(requested_scope.to_string()),
}
})
.copied()
.chain(requested_set.contains("atproto").then_some("atproto"))
.collect();
scopes.sort();
scopes.join(" ")
scopes.into_iter().collect::<Vec<String>>().join(" ")
}
pub fn grant_covers(granted: &str, scope: &str) -> bool {
pub fn grant_permits(granted: &str, scope: &str) -> bool {
if scope == "atproto" {
return true;
}
let granted_parsed: Vec<tranquil_scopes::ParsedScope> =
granted.split_whitespace().map(parse_scope).collect();
any_granted_covers(scope, &granted_parsed)
}
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))
let granted_parsed: Vec<ParsedScope> = granted.split_whitespace().map(parse_scope).collect();
narrow(&granted_parsed, &parse_scope(scope)).is_some()
}
#[cfg(test)]
@@ -220,12 +226,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]
@@ -262,33 +289,41 @@ mod tests {
}
#[test]
fn test_grant_covers_matches_intersection() {
fn test_grant_permits_matches_intersection() {
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!(grant_covers(
assert!(grant_permits(
granted,
"repo:app.bsky.feed.post?action=create"
));
assert!(grant_covers(granted, "account:*?action=manage"));
assert!(!grant_covers(granted, "identity:*"));
assert!(grant_permits(granted, "account:*?action=manage"));
assert!(!grant_permits(granted, "identity:*"));
assert_eq!(
grant_covers(granted, "identity:*"),
grant_permits(granted, "identity:*"),
intersected.contains("identity")
);
}
#[test]
fn test_grant_covers_atproto_always_true() {
assert!(grant_covers("", "atproto"));
assert!(grant_covers("repo:*", "atproto"));
fn test_grant_permits_atproto_always_true() {
assert!(grant_permits("", "atproto"));
assert!(grant_permits("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_permits_empty_grant_permits_nothing_else() {
assert!(!grant_permits("", "repo:app.bsky.feed.post?action=create"));
assert!(!grant_permits("", "identity:*"));
}
#[test]
fn test_grant_permits_partially_granted_repo_scope() {
assert!(grant_permits(
EDITOR_FULL_SCOPES,
"repo:io.atcr.manifest?action=create&action=delete"
));
}
}
@@ -15,6 +15,9 @@ use wiremock::{Mock, MockServer, ResponseTemplate};
const PERMISSION_SET_NSID: &str = "io.atcr.authFullApp";
const PERMISSION_SET_GRANULAR_SCOPE: &str =
"repo:io.atcr.manifest?action=create rpc:io.atcr.getManifest?aud=*";
const PERMISSION_SET_MULTI_ACTION_SCOPE: &str =
"repo:io.atcr.manifest?action=create&action=update&action=delete";
const EDITOR_SET_NSID: &str = "io.atcr.authEditorApp";
fn disable_rate_limiting_once() {
static ONCE: std::sync::Once = std::sync::Once::new();
@@ -105,6 +108,21 @@ async fn create_delegated_session_with_scope(
handle_prefix: &str,
redirect_uri: &str,
scope: &str,
) -> (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 +137,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 +393,54 @@ 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_grant_row_keeps_include_jwt_carries_expanded() {
seed_permission_set(PERMISSION_SET_NSID, PERMISSION_SET_GRANULAR_SCOPE).await;
+12 -9
View File
@@ -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]
+76 -7
View File
@@ -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,36 @@ 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)
}
pub fn narrow(granted: &[ParsedScope], requested: &ParsedScope) -> Option<ParsedScope> {
if let ParsedScope::Repo(r) = requested {
let actions: HashSet<RepoAction> = 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 (!actions.is_empty()).then(|| {
ParsedScope::Repo(RepoScope {
collection: r.collection.clone(),
actions,
})
});
}
granted
.iter()
.any(|g| covers(g, requested))
.then(|| requested.clone())
}
fn blob_covers(g: &BlobScope, r: &BlobScope) -> bool {
@@ -74,13 +103,23 @@ fn identity_covers(g: &IdentityScope, r: &IdentityScope) -> bool {
#[cfg(test)]
mod tests {
use super::covers;
use crate::parser::parse_scope;
use super::{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<String> {
let granted: Vec<ParsedScope> = 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,
}
}
#[test]
fn repo_wildcard_covers_specific() {
assert!(c("repo:*", "repo:app.bsky.feed.post"));
@@ -193,4 +232,34 @@ 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())
);
}
}
+1 -1
View File
@@ -5,7 +5,7 @@ mod parser;
mod permission_set;
mod permissions;
pub use coverage::covers;
pub use coverage::{covers, narrow};
pub use definitions::{
SCOPE_DEFINITIONS, ScopeCategory, ScopeDefinition, format_scope_for_display,
get_required_scopes, get_scope_definition, is_valid_scope,
+57 -29
View File
@@ -28,7 +28,22 @@ pub struct RepoScope {
pub actions: HashSet<RepoAction>,
}
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
impl RepoScope {
pub fn to_scope_string(&self) -> String {
let mut actions: Vec<RepoAction> = self.actions.iter().copied().collect();
actions.sort();
let rendered: Vec<&str> = actions.iter().map(RepoAction::as_str).collect();
format!(
"repo:{}?action={}",
self.collection.as_deref().unwrap_or("*"),
rendered.join("&action=")
)
}
}
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum RepoAction {
Create,
@@ -37,6 +52,8 @@ pub enum RepoAction {
}
impl RepoAction {
pub const ALL: [RepoAction; 3] = [Self::Create, Self::Update, Self::Delete];
pub fn parse_str(s: &str) -> Option<Self> {
match s {
"create" => Some(Self::Create),
@@ -45,6 +62,14 @@ impl RepoAction {
_ => None,
}
}
pub fn as_str(&self) -> &'static str {
match self {
Self::Create => "create",
Self::Update => "update",
Self::Delete => "delete",
}
}
}
#[derive(Debug, Clone, PartialEq, Eq)]
@@ -150,6 +175,14 @@ fn parse_query_params(query: &str) -> HashMap<String, Vec<String>> {
})
}
fn parse_repo_actions(params: &HashMap<String, Vec<String>>) -> Option<HashSet<RepoAction>> {
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,
@@ -169,20 +202,9 @@ pub fn parse_scope(scope: &str) -> ParsedScope {
Some(rest.to_string())
};
let actions: HashSet<RepoAction> = params
.get("action")
.map(|action_values| {
action_values
.iter()
.filter_map(|s| RepoAction::parse_str(s))
.collect()
})
.filter(|set: &HashSet<RepoAction>| !set.is_empty())
.unwrap_or_else(|| {
[RepoAction::Create, RepoAction::Update, RepoAction::Delete]
.into_iter()
.collect()
});
let Some(actions) = parse_repo_actions(&params) else {
return ParsedScope::Unknown(scope.to_string());
};
return ParsedScope::Repo(RepoScope {
collection,
@@ -191,20 +213,10 @@ pub fn parse_scope(scope: &str) -> ParsedScope {
}
if base == "repo" {
let actions: HashSet<RepoAction> = params
.get("action")
.map(|action_values| {
action_values
.iter()
.filter_map(|s| RepoAction::parse_str(s))
.collect()
})
.filter(|set: &HashSet<RepoAction>| !set.is_empty())
.unwrap_or_else(|| {
[RepoAction::Create, RepoAction::Update, RepoAction::Delete]
.into_iter()
.collect()
});
let Some(actions) = parse_repo_actions(&params) else {
return ParsedScope::Unknown(scope.to_string());
};
return ParsedScope::Repo(RepoScope {
collection: None,
actions,
@@ -340,6 +352,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:*/*");
+53 -36
View File
@@ -1,9 +1,10 @@
use crate::parser::RepoAction;
use hickory_resolver::TokioAsyncResolver;
use hickory_resolver::config::{ResolverConfig, ResolverOpts};
use reqwest::Client;
use serde::{Deserialize, Serialize};
use std::collections::{BTreeMap, HashMap};
use tracing::debug;
use std::collections::{BTreeMap, BTreeSet, HashMap};
use tracing::{debug, warn};
use tranquil_types::{Did, Nsid};
#[derive(Debug, thiserror::Error)]
@@ -332,13 +333,23 @@ fn is_under_authority(target_nsid: &str, authority: &str) -> bool {
.is_some_and(|c| c == '.')
}
const DEFAULT_ACTIONS: &[&str] = &["create", "update", "delete"];
fn action_rank(action: &str) -> usize {
DEFAULT_ACTIONS
.iter()
.position(|known| *known == action)
.unwrap_or(DEFAULT_ACTIONS.len())
fn parse_permission_actions(actions: Option<&Vec<String>>) -> Option<BTreeSet<RepoAction>> {
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(
@@ -346,36 +357,26 @@ fn build_expanded_scopes(
default_aud: Option<&str>,
namespace_authority: &str,
) -> String {
// Key is `repo`, value is array of actions
let mut ungrouped_repo_scopes: BTreeMap<String, Vec<String>> = BTreeMap::new();
let mut ungrouped_repo_scopes: BTreeMap<String, BTreeSet<RepoAction>> = BTreeMap::new();
let mut rpc_scopes: Vec<String> = Vec::new();
permissions
.iter()
.for_each(|perm| match perm.resource.as_str() {
"repo" => {
if let Some(collections) = &perm.collection {
let actions: Vec<&str> = perm
.action
.as_ref()
.map(|a| a.iter().map(String::as_str).collect())
.unwrap_or_else(|| DEFAULT_ACTIONS.to_vec());
if !actions.is_empty() {
collections
.iter()
.filter(|coll| is_under_authority(coll, namespace_authority))
.for_each(|coll| {
let existing =
ungrouped_repo_scopes.entry(coll.to_string()).or_default();
actions.iter().for_each(|action| {
if !existing.iter().any(|seen| seen == action) {
existing.push(action.to_string());
}
});
});
}
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| {
ungrouped_repo_scopes
.entry(coll.to_string())
.or_default()
.extend(actions.iter().copied());
});
}
}
"rpc" => {
@@ -402,10 +403,9 @@ fn build_expanded_scopes(
let grouped_repo_scopes: Vec<String> = ungrouped_repo_scopes
.iter()
.map(|(repo, actions)| {
let mut actions = actions.clone();
actions.sort_by(|a, b| action_rank(a).cmp(&action_rank(b)).then_with(|| a.cmp(b)));
let rendered: Vec<&str> = actions.iter().map(RepoAction::as_str).collect();
format!("repo:{}?action={}", repo, actions.join("&action="))
format!("repo:{}?action={}", repo, rendered.join("&action="))
})
.collect();
@@ -628,6 +628,23 @@ mod tests {
);
}
#[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]
fn test_build_expanded_scopes_rpc() {
let permissions = vec![PermissionEntry {