From c88f69f31d4b81aa89286398dafe1eb02294ea9f Mon Sep 17 00:00:00 2001 From: Louis Escher Date: Sun, 9 Aug 2026 14:16:46 +0200 Subject: [PATCH] fix: Address PR review --- crates/tranquil-auth/src/compress.rs | 18 ++- .../src/endpoints/authorize/consent.rs | 2 +- crates/tranquil-pds/src/delegation/mod.rs | 2 +- crates/tranquil-pds/src/delegation/scopes.rs | 103 ++++++++++++------ .../tests/oauth_permission_sets.rs | 68 +++++++++++- crates/tranquil-pds/tests/scope_edge_cases.rs | 21 ++-- crates/tranquil-scopes/src/coverage.rs | 83 ++++++++++++-- crates/tranquil-scopes/src/lib.rs | 2 +- crates/tranquil-scopes/src/parser.rs | 86 ++++++++++----- crates/tranquil-scopes/src/permission_set.rs | 89 +++++++++------ 10 files changed, 352 insertions(+), 122 deletions(-) diff --git a/crates/tranquil-auth/src/compress.rs b/crates/tranquil-auth/src/compress.rs index 1efc50b..6b75c8d 100644 --- a/crates/tranquil-auth/src/compress.rs +++ b/crates/tranquil-auth/src/compress.rs @@ -73,10 +73,13 @@ pub fn encode_scope(scope: &str) -> Result { 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); diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs index d459421..dae4d86 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize/consent.rs @@ -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 { diff --git a/crates/tranquil-pds/src/delegation/mod.rs b/crates/tranquil-pds/src/delegation/mod.rs index 9ae5f3f..928fd2b 100644 --- a/crates/tranquil-pds/src/delegation/mod.rs +++ b/crates/tranquil-pds/src/delegation/mod.rs @@ -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; diff --git a/crates/tranquil-pds/src/delegation/scopes.rs b/crates/tranquil-pds/src/delegation/scopes.rs index 9ccaaaf..3aa007c 100644 --- a/crates/tranquil-pds/src/delegation/scopes.rs +++ b/crates/tranquil-pds/src/delegation/scopes.rs @@ -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 = - granted.split_whitespace().map(parse_scope).collect(); + let granted_parsed: Vec = 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 = 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::>().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 = - 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 = 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" + )); } } diff --git a/crates/tranquil-pds/tests/oauth_permission_sets.rs b/crates/tranquil-pds/tests/oauth_permission_sets.rs index 7cee66d..ad4ba22 100644 --- a/crates/tranquil-pds/tests/oauth_permission_sets.rs +++ b/crates/tranquil-pds/tests/oauth_permission_sets.rs @@ -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; diff --git a/crates/tranquil-pds/tests/scope_edge_cases.rs b/crates/tranquil-pds/tests/scope_edge_cases.rs index 4319939..bdca3bb 100644 --- a/crates/tranquil-pds/tests/scope_edge_cases.rs +++ b/crates/tranquil-pds/tests/scope_edge_cases.rs @@ -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] diff --git a/crates/tranquil-scopes/src/coverage.rs b/crates/tranquil-scopes/src/coverage.rs index 716da42..54dcbec 100644 --- a/crates/tranquil-scopes/src/coverage.rs +++ b/crates/tranquil-scopes/src/coverage.rs @@ -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 { + if let ParsedScope::Repo(r) = requested { + let actions: HashSet = 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 { + let granted: Vec = 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()) + ); + } } diff --git a/crates/tranquil-scopes/src/lib.rs b/crates/tranquil-scopes/src/lib.rs index 1160e56..e7b6861 100644 --- a/crates/tranquil-scopes/src/lib.rs +++ b/crates/tranquil-scopes/src/lib.rs @@ -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, diff --git a/crates/tranquil-scopes/src/parser.rs b/crates/tranquil-scopes/src/parser.rs index 9193bf7..191dae1 100644 --- a/crates/tranquil-scopes/src/parser.rs +++ b/crates/tranquil-scopes/src/parser.rs @@ -28,7 +28,22 @@ pub struct RepoScope { pub actions: HashSet, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +impl RepoScope { + pub fn to_scope_string(&self) -> String { + let mut actions: Vec = 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 { 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> { }) } +fn parse_repo_actions(params: &HashMap>) -> Option> { + 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 = params - .get("action") - .map(|action_values| { - action_values - .iter() - .filter_map(|s| RepoAction::parse_str(s)) - .collect() - }) - .filter(|set: &HashSet| !set.is_empty()) - .unwrap_or_else(|| { - [RepoAction::Create, RepoAction::Update, RepoAction::Delete] - .into_iter() - .collect() - }); + let Some(actions) = parse_repo_actions(¶ms) 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 = params - .get("action") - .map(|action_values| { - action_values - .iter() - .filter_map(|s| RepoAction::parse_str(s)) - .collect() - }) - .filter(|set: &HashSet| !set.is_empty()) - .unwrap_or_else(|| { - [RepoAction::Create, RepoAction::Update, RepoAction::Delete] - .into_iter() - .collect() - }); + let Some(actions) = parse_repo_actions(¶ms) 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:*/*"); diff --git a/crates/tranquil-scopes/src/permission_set.rs b/crates/tranquil-scopes/src/permission_set.rs index ec7d14b..a5c07a5 100644 --- a/crates/tranquil-scopes/src/permission_set.rs +++ b/crates/tranquil-scopes/src/permission_set.rs @@ -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>) -> Option> { + 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> = BTreeMap::new(); + let mut ungrouped_repo_scopes: BTreeMap> = BTreeMap::new(); let mut rpc_scopes: Vec = 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 = 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 {