Skip to content

Commit d3d743a

Browse files
gustavoavenameta-codesync[bot]
authored andcommitted
restricted_paths: use path check primitives
Summary: ## This stack This stack completes the second half of the restricted_paths AclManifest migration after the shadow comparison logging stack. The end result is that restricted_paths no longer relies on legacy construction flags or source-specific call paths: AclManifestMode owns source selection, config and AclManifest fetches are planned explicitly, the fetch results are shared between logging, enforcement, and metadata, and the public callers adapt to the final manifest-aware shape at the API boundary. Visual flow: https://pxl.cl/9GjpF Before this stack: - Behavior was split across the old use_acl_manifest construction state, config-shaped metadata APIs, conditional_enforcement_acls, duplicated source fetches, and unimplemented AclManifest enforcement modes. - Logging and enforcement could ask for overlapping source data independently, and Mononoke API/HG, SCS, and SLAPI still had to bridge around the incomplete manifest restriction shape. After this stack: - Construction and source planning are mode-driven. Disabled, Shadow, Both, and Authoritative modes choose the sources they need; - Config and AclManifest fetches can be killed switched independently by rollout knobs; - Shared typed fetch handles feed both comparison logging and enforcement; - Conditional enforcement uses `enforcement_condition_sets` - `AclManifestMode::Both` mode denies if either source denies; - Authoritative-source errors are surfaced when the authoritative source cannot answer; - Metadata lookup unions config and AclManifest results in Both mode. This makes the codebase better because - Source (config vs acl_manifest) choice is centralized, - Fetch work is not duplicated - Rollout controls are easier to reason about - Restriction check results are typed instead of copied into loosely shaped structs, and API-specific adaptation happens at the edge instead of leaking through the restricted_paths internals. - **The stack also stays reviewable by putting no-op setup and tests before behavior changes** Remaining follow-ups after this stack are - Schematized Scuba source/error parity - The Eden API vector response shape tracked by TODO(T248658346) - Reviewing the both-sources-disabled fail-open behavior also tracked by TODO(T248658346) - Deleting rollout-only fallback paths after AclManifest enforcement is proven. ## This diff (no-op) Changes `ChangesetPathRestrictionContext::restriction_info` to use the typed restricted_paths path check primitive when callers request permission checks. The API behavior is unchanged: callers still receive `PathAccessInfo`, `has_access` remains populated only when requested, and restriction-info-only callers still avoid authorization checks. This removes duplicate ACL parsing/checking from mononoke_api and keeps the permission-checking logic owned by restricted_paths. Reviewed By: lmvasquezg Differential Revision: D103698836 fbshipit-source-id: c624f7b3a917e6e31f5cd15a3b991c7977858af4
1 parent a0ac2ab commit d3d743a

8 files changed

Lines changed: 58 additions & 77 deletions

File tree

eden/mononoke/mononoke_api/BUCK

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ rust_library(
2020
"//common/rust/shed/fbinit:fbinit",
2121
"//common/rust/shed/fbinit:fbinit-tokio",
2222
"//eden/mononoke/common/mononoke_macros:mononoke_macros",
23+
"//eden/mononoke/common/permission_checker:permission_checker",
2324
"//eden/mononoke/common/sql_construct:sql_construct",
2425
"//eden/mononoke/repo_attributes/commit_graph/commit_graph_testlib:commit_graph_testlib",
2526
"//eden/mononoke/repo_attributes/repo_blobstore:repo_blobstore",
@@ -61,7 +62,6 @@ rust_library(
6162
"//eden/mononoke/common/acl_regions:acl_regions",
6263
"//eden/mononoke/common/futures_watchdog:futures_watchdog",
6364
"//eden/mononoke/common/mononoke_repos:mononoke_repos",
64-
"//eden/mononoke/common/permission_checker:permission_checker",
6565
"//eden/mononoke/common/scuba_ext:scuba_ext",
6666
"//eden/mononoke/derived_data:basename_suffix_skeleton_manifest_v3",
6767
"//eden/mononoke/derived_data:blame",

eden/mononoke/mononoke_api/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ mutable_blobstore = { version = "0.1.0", path = "../repo_attributes/mutable_blob
8484
mutable_counters = { version = "0.1.0", path = "../repo_attributes/mutable_counters" }
8585
mutable_renames = { version = "0.1.0", path = "../repo_attributes/mutable_renames" }
8686
packfile = { version = "0.1.0", path = "../git/packfile" }
87-
permission_checker = { version = "0.1.0", path = "../common/permission_checker" }
8887
phases = { version = "0.1.0", path = "../repo_attributes/phases" }
8988
protocol = { version = "0.1.0", path = "../git/protocol" }
9089
pushrebase = { version = "0.1.0", path = "../features/pushrebase" }
@@ -136,6 +135,7 @@ fbinit-tokio = { version = "0.1.2", git = "https://github.com/facebookexperiment
136135
fixtures = { version = "0.1.0", path = "../tests/fixtures" }
137136
gix-object = "0.49"
138137
mononoke_macros = { version = "0.1.0", path = "../common/mononoke_macros" }
138+
permission_checker = { version = "0.1.0", path = "../common/permission_checker" }
139139
pretty_assertions = { version = "1.4.1", features = ["alloc"], default-features = false }
140140
sql_construct = { version = "0.1.0", path = "../common/sql_construct" }
141141
test_repo_factory = { version = "0.1.0", path = "../repo_factory/test_repo_factory" }

eden/mononoke/mononoke_api/src/changeset.rs

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ use repo_derived_data::RepoDerivedDataArc;
8181
use repo_derived_data::RepoDerivedDataRef;
8282
use repo_identity::RepoIdentityRef;
8383
use restricted_paths::RestrictedPathsArc;
84+
use restricted_paths::check_path_restriction_infos;
8485
use skeleton_manifest::RootSkeletonManifestId;
8586
use skeleton_manifest_v2::RootSkeletonManifestV2Id;
8687
use smallvec::SmallVec;
@@ -101,7 +102,6 @@ use crate::repo::RepoContext;
101102
use crate::restricted_paths::PathAccessInfo;
102103
use crate::restricted_paths::RestrictedChangeGroup;
103104
use crate::restricted_paths::RestrictedPathsChangesInfo;
104-
use crate::restricted_paths::build_path_access_info;
105105
use crate::specifiers::ChangesetId;
106106
use crate::specifiers::GitSha1;
107107
use crate::specifiers::HgChangesetId;
@@ -1326,7 +1326,7 @@ impl<R: MononokeRepo> ChangesetContext<R> {
13261326
.find_entries(
13271327
self.ctx().clone(),
13281328
other.repo_ctx().repo().repo_blobstore().clone(),
1329-
to_paths.into_iter(),
1329+
to_paths,
13301330
)
13311331
.map_ok(|(path, _)| path)
13321332
.try_collect::<HashSet<_>>();
@@ -2071,13 +2071,26 @@ impl<R: MononokeRepo> ChangesetContext<R> {
20712071
.find_restricted_descendants(self.ctx(), Some(cs_id), roots)
20722072
.await?;
20732073

2074-
build_path_access_info(
2075-
self.ctx(),
2076-
restricted_paths.acl_provider(),
2077-
restriction_infos,
2078-
check_permissions,
2079-
)
2080-
.await
2074+
if check_permissions {
2075+
let restriction_checks =
2076+
check_path_restriction_infos(self.ctx(), &restricted_paths, restriction_infos)
2077+
.await?;
2078+
return Ok(restriction_checks
2079+
.into_iter()
2080+
.map(|restriction_check| PathAccessInfo {
2081+
has_access: Some(restriction_check.has_authorization()),
2082+
restriction: restriction_check.into_restriction_info(),
2083+
})
2084+
.collect());
2085+
}
2086+
2087+
Ok(restriction_infos
2088+
.into_iter()
2089+
.map(|restriction| PathAccessInfo {
2090+
restriction,
2091+
has_access: None,
2092+
})
2093+
.collect())
20812094
}
20822095

20832096
/// Determine which changed files in this changeset touch restricted paths.

eden/mononoke/mononoke_api/src/changeset_path.rs

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ use crate::errors::MononokeError;
6464
use crate::file::FileContext;
6565
use crate::repo::RepoContext;
6666
use crate::restricted_paths::PathAccessInfo;
67-
use crate::restricted_paths::build_path_access_info;
6867
use crate::tree::TreeContext;
6968

7069
pub struct HistoryEntry {
@@ -791,7 +790,7 @@ impl<R: MononokeRepo> ChangesetPathHistoryContext<R> {
791790
repo: &impl history_traversal::Repo,
792791
descendant_id_cs_ids: Vec<(Option<CsAndPath>, Vec<CsAndPath>)>,
793792
) -> Result<(), Error> {
794-
let items = stream::iter(descendant_id_cs_ids.into_iter())
793+
let items = stream::iter(descendant_id_cs_ids)
795794
.map(|(descendant_cs_id, cs_ids)| {
796795
self._visit(ctx, repo, descendant_cs_id.clone(), cs_ids.clone())
797796
.map_ok(move |res| ((descendant_cs_id, cs_ids), res))
@@ -1079,21 +1078,38 @@ impl<R: MononokeRepo> ChangesetPathRestrictionContext<R> {
10791078
let restricted_paths = self.changeset().repo_ctx().repo().restricted_paths_arc();
10801079
let cs_id = self.changeset().id();
10811080

1081+
if check_permissions {
1082+
let restriction_checks = restricted_paths
1083+
.get_path_restriction_check(
1084+
self.changeset().ctx(),
1085+
Some(cs_id),
1086+
std::slice::from_ref(&path),
1087+
)
1088+
.await?;
1089+
return Ok(restriction_checks
1090+
.into_iter()
1091+
.map(|restriction_check| PathAccessInfo {
1092+
has_access: Some(restriction_check.has_authorization()),
1093+
restriction: restriction_check.into_restriction_info(),
1094+
})
1095+
.collect());
1096+
}
1097+
10821098
let restriction_infos = restricted_paths
1083-
.get_path_restriction_info(self.changeset().ctx(), Some(cs_id), &[path])
1099+
.get_path_restriction_info(
1100+
self.changeset().ctx(),
1101+
Some(cs_id),
1102+
std::slice::from_ref(&path),
1103+
)
10841104
.await?;
10851105

1086-
if restriction_infos.is_empty() {
1087-
return Ok(vec![]);
1088-
}
1089-
1090-
build_path_access_info(
1091-
self.changeset().ctx(),
1092-
restricted_paths.acl_provider(),
1093-
restriction_infos,
1094-
check_permissions,
1095-
)
1096-
.await
1106+
Ok(restriction_infos
1107+
.into_iter()
1108+
.map(|restriction| PathAccessInfo {
1109+
restriction,
1110+
has_access: None,
1111+
})
1112+
.collect())
10971113
}
10981114

10991115
/// Find all restricted paths that are descendants of this path.

eden/mononoke/mononoke_api/src/restricted_paths.rs

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,8 @@
55
* GNU General Public License version 2.
66
*/
77

8-
use std::str::FromStr;
9-
use std::sync::Arc;
10-
11-
use anyhow::Context;
12-
use context::CoreContext;
13-
use futures::StreamExt;
14-
use futures::TryStreamExt;
15-
use futures::stream;
168
use mononoke_types::NonRootMPath;
17-
use permission_checker::AclProvider;
18-
use permission_checker::MononokeIdentity;
199
use restricted_paths::PathRestrictionInfo;
20-
use restricted_paths::has_read_access_to_repo_region_acls;
21-
22-
use crate::errors::MononokeError;
2310

2411
/// Access check result for a restricted path.
2512
#[derive(Clone, Debug, PartialEq)]
@@ -48,41 +35,6 @@ impl PathAccessInfo {
4835
}
4936
}
5037

51-
pub(crate) async fn build_path_access_info(
52-
ctx: &CoreContext,
53-
acl_provider: &Arc<dyn AclProvider>,
54-
restriction_infos: Vec<PathRestrictionInfo>,
55-
check_permissions: bool,
56-
) -> Result<Vec<PathAccessInfo>, MononokeError> {
57-
if !check_permissions {
58-
return Ok(restriction_infos
59-
.into_iter()
60-
.map(|restriction| PathAccessInfo {
61-
restriction,
62-
has_access: None,
63-
})
64-
.collect());
65-
}
66-
67-
stream::iter(restriction_infos)
68-
.map(|restriction| {
69-
let acl_provider = Arc::clone(acl_provider);
70-
async move {
71-
let acl = MononokeIdentity::from_str(&restriction.repo_region_acl)
72-
.context("Failed to parse repo_region_acl")?;
73-
let has_access =
74-
has_read_access_to_repo_region_acls(ctx, &acl_provider, &[&acl]).await?;
75-
Ok::<_, MononokeError>(PathAccessInfo {
76-
restriction,
77-
has_access: Some(has_access),
78-
})
79-
}
80-
})
81-
.buffered(100)
82-
.try_collect()
83-
.await
84-
}
85-
8638
/// Information about restricted path changes in a changeset.
8739
#[derive(Clone, Debug, PartialEq)]
8840
pub struct RestrictedPathsChangesInfo {

eden/mononoke/repo_attributes/restricted_paths/src/access_log.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ impl SourceRestrictionCheckResult {
158158
/// would silently bypass an inner restriction.
159159
///
160160
/// Runs checks concurrently and short-circuits on the first deny or error.
161-
pub async fn has_read_access_to_repo_region_acls(
161+
pub(crate) async fn has_read_access_to_repo_region_acls(
162162
ctx: &CoreContext,
163163
acl_provider: &Arc<dyn AclProvider>,
164164
acls: &[&MononokeIdentity],

eden/mononoke/repo_attributes/restricted_paths/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,12 @@ use thiserror::Error;
3737
use tokio::task;
3838

3939
pub use crate::access_log::ACCESS_LOG_SCUBA_TABLE;
40-
pub use crate::access_log::has_read_access_to_repo_region_acls;
4140
use crate::access_log::is_member_of_groups;
4241
use crate::access_log::log_access_to_restricted_path;
4342
pub use crate::restriction_check::ManifestRestrictionCheckResult;
4443
pub use crate::restriction_check::PathRestrictionCheckResult;
4544
pub use crate::restriction_check::RestrictionCheckResult;
45+
pub use crate::restriction_check::check_path_restriction_infos;
4646
pub use crate::restriction_info::ManifestRestrictionInfo;
4747
pub use crate::restriction_info::PathRestrictionInfo;
4848

eden/mononoke/repo_attributes/restricted_paths/src/restriction_check.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ pub(crate) async fn check_manifest_restriction(
359359
check_manifest_authorization(ctx, restricted_paths, restriction_info).await
360360
}
361361

362-
async fn check_config_path_authorization(
362+
pub(crate) async fn check_config_path_authorization(
363363
ctx: &CoreContext,
364364
restricted_paths: &RestrictedPaths,
365365
path: &NonRootMPath,
@@ -385,7 +385,7 @@ async fn check_config_path_authorization(
385385
check_path_authorization(ctx, restricted_paths, restriction_info, restriction_acls).await
386386
}
387387

388-
async fn check_path_restriction_infos(
388+
pub async fn check_path_restriction_infos(
389389
ctx: &CoreContext,
390390
restricted_paths: &RestrictedPaths,
391391
restriction_info: Vec<PathRestrictionInfo>,

0 commit comments

Comments
 (0)