Skip to content

Commit d1d85f0

Browse files
gustavoavenameta-codesync[bot]
authored andcommitted
restricted_paths: test read_only manifest-id recording
Summary: ## This stack Decouple restricted_paths config from the manifest_id store (T277178795). Replace `path_acls` (path -> ACL) with per-path `PathRestrictionMetadata` carrying the REPO_REGION ACL and optional permission-request group, so a future `read_only` flag can freeze manifest-id-store growth without dropping enforcement. Thrift `path_acls` is retained as a parser fallback. The configerator thrift source is the separately-published D109697491. ## This diff (no-op) Test-only: end-to-end coverage for the `read_only` behaviour introduced earlier in the stack (D109827082). No production code changes. Two tests in the `restricted_paths` integration suite. First, that `read_only` stops new manifest-id-store recording: a contrast test runs the same fixture writable (derivation records one entry per manifest type -- Hg, HgAugmented, Fsnode, ContentManifest) and then with every config path `read_only` (records nothing), exercising all four per-entry derivation gates end-to-end. Second, that `read_only` paths still count as restricted: with all config paths `read_only`, both `may_have_restricted_paths()` and `has_restricted_paths()` stay true across every `AclManifestMode` (Disabled/Shadow/Both/Authoritative), since `read_only` gates recording only, never detection or enforcement. Small test-infra additions support these: a `with_config_paths_read_only` builder toggle (default false, so existing tests are unchanged), `manifest_id_store_entries` exposed on the scenario result for structural assertions, and a `build_repo_only` helper for predicate-only tests. Follow-up: the `mapping.rs` `track_all_restricted_paths` backfill `!read_only` filter is not covered here -- it is reachable only via the untopological `derive_from_predecessor` path. `AclManifest` derivation does not branch on `read_only`. ___ Differential Revision: D109838120 fbshipit-source-id: 31719d168a4b9c68e1a96112e78d4f2cbe975f91
1 parent 8ed22cb commit d1d85f0

2 files changed

Lines changed: 161 additions & 2 deletions

File tree

‎eden/mononoke/repo_attributes/restricted_paths/test/main.rs‎

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

8+
use std::collections::HashSet;
89
use std::str::FromStr;
910

1011
use anyhow::Result;
@@ -1391,6 +1392,120 @@ async fn test_single_dir_restricted_and_unrestricted(fb: FacebookInit) -> Result
13911392
Ok(())
13921393
}
13931394

1395+
// With every config restricted path flipped to read_only, derivation must not
1396+
// record new manifest-id-store entries -- while the same writable paths still do.
1397+
#[mononoke::fbinit_test]
1398+
async fn test_read_only_config_paths_record_no_new_manifest_id_entries(
1399+
fb: FacebookInit,
1400+
) -> Result<()> {
1401+
let restricted_paths = vec![(
1402+
NonRootMPath::new("restricted/dir")?,
1403+
MononokeIdentity::from_str("REPO_REGION:restricted_acl")?,
1404+
)];
1405+
// A file directly under the restricted root, so the directory is materialized
1406+
// in the derived manifests and is eligible for recording.
1407+
let file_path_changes = vec![("restricted/dir/a", None)];
1408+
1409+
// Baseline: writable config paths record one manifest-id-store entry per
1410+
// manifest type for the restricted root.
1411+
let writable = RestrictedPathsTestDataBuilder::new()
1412+
.with_config_restricted_paths(restricted_paths.clone())
1413+
.with_file_path_changes(file_path_changes.clone())
1414+
.build(fb)
1415+
.await?
1416+
.observe_restricted_paths_scenario(&[])
1417+
.await?;
1418+
1419+
let recorded_types: HashSet<ManifestType> = writable
1420+
.manifest_id_store_entries
1421+
.iter()
1422+
.map(|entry| entry.manifest_type.clone())
1423+
.collect();
1424+
assert_eq!(
1425+
recorded_types,
1426+
HashSet::from([
1427+
ManifestType::Hg,
1428+
ManifestType::HgAugmented,
1429+
ManifestType::Fsnode,
1430+
ManifestType::ContentManifest,
1431+
]),
1432+
"writable restricted paths should record an entry for every manifest type, got {:?}",
1433+
writable.manifest_id_store_entries,
1434+
);
1435+
1436+
// Same fixture with every config path marked read_only: no new entries are
1437+
// recorded. The store stays empty because every recording site (the four
1438+
// per-entry derivation gates) consults read_only via
1439+
// should_record_manifest_id_entry.
1440+
let read_only = RestrictedPathsTestDataBuilder::new()
1441+
.with_config_restricted_paths(restricted_paths)
1442+
.with_config_paths_read_only(true)
1443+
.with_file_path_changes(file_path_changes)
1444+
.build(fb)
1445+
.await?
1446+
.observe_restricted_paths_scenario(&[])
1447+
.await?;
1448+
assert!(
1449+
read_only.manifest_id_store_entries.is_empty(),
1450+
"read_only restricted paths must not record new manifest-id entries, got {:?}",
1451+
read_only.manifest_id_store_entries,
1452+
);
1453+
1454+
Ok(())
1455+
}
1456+
1457+
// read_only gates derivation recording only; it must NOT change whether a repo
1458+
// is considered to have restricted paths. With every config path read_only,
1459+
// both may_have_restricted_paths and has_restricted_paths must stay true across
1460+
// every AclManifest mode. (read_only is deliberately true here precisely to
1461+
// prove these predicates ignore it.)
1462+
#[mononoke::fbinit_test]
1463+
async fn test_read_only_config_paths_still_count_as_restricted(fb: FacebookInit) -> Result<()> {
1464+
let restricted_paths = vec![
1465+
(
1466+
NonRootMPath::new("restricted/dir")?,
1467+
MononokeIdentity::from_str("REPO_REGION:restricted_acl")?,
1468+
),
1469+
(
1470+
NonRootMPath::new("other/secret")?,
1471+
MononokeIdentity::from_str("REPO_REGION:other_acl")?,
1472+
),
1473+
];
1474+
1475+
for mode in [
1476+
AclManifestMode::Disabled,
1477+
AclManifestMode::Shadow,
1478+
AclManifestMode::Both,
1479+
AclManifestMode::Authoritative,
1480+
] {
1481+
let repo = RestrictedPathsTestDataBuilder::new()
1482+
.with_config_restricted_paths(restricted_paths.clone())
1483+
.with_config_paths_read_only(true)
1484+
.with_acl_manifest_mode(mode)
1485+
.build(fb)
1486+
.await?
1487+
.build_repo_only()
1488+
.await?;
1489+
1490+
// `has_restricted_paths` is the load-bearing, read_only-agnostic check in
1491+
// every mode. `may_have_restricted_paths` additionally confirms no mode
1492+
// starts reporting the repo as unrestricted; it short-circuits on the
1493+
// mode for non-Disabled, so it is strongest in Disabled mode.
1494+
assert!(
1495+
repo.restricted_paths().may_have_restricted_paths(),
1496+
"may_have_restricted_paths must be true for read-only paths in mode {mode:?}",
1497+
);
1498+
assert!(
1499+
repo.restricted_paths()
1500+
.config_based()
1501+
.has_restricted_paths(),
1502+
"has_restricted_paths must be true for read-only paths in mode {mode:?}",
1503+
);
1504+
}
1505+
1506+
Ok(())
1507+
}
1508+
13941509
// Multiple restricted directories generate multiple entries in the manifest
13951510
#[mononoke::fbinit_test]
13961511
async fn test_multiple_restricted_dirs(fb: FacebookInit) -> Result<()> {

‎eden/mononoke/repo_attributes/restricted_paths/test/utils.rs‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ pub const TEST_CLIENT_MAIN_ID: &str = "user:myusername0";
7070

7171
pub struct RestrictedPathsScenarioResult {
7272
pub scuba_logs: Vec<ScubaAccessLogSample>,
73+
/// All entries present in the manifest-id store after the scenario ran.
74+
pub manifest_id_store_entries: Vec<RestrictedPathManifestIdEntry>,
7375
}
7476

7577
struct RestrictedPathsScenario {
@@ -93,6 +95,9 @@ pub struct RestrictedPathsTestData {
9395
enforcement_scenarios: Vec<(Vec<EnforcementConditionSet>, bool)>,
9496
/// Config-backed restrictions written into `RestrictedPathsConfig.path_acls`.
9597
config_restricted_paths: Vec<(NonRootMPath, MononokeIdentity)>,
98+
/// When true, every config restricted path is marked `read_only` in its
99+
/// `PathRestrictionMetadata`.
100+
config_paths_read_only: bool,
96101
/// AclManifest-backed restrictions materialized as `.slacl` files.
97102
acl_manifest_restricted_paths: Vec<(NonRootMPath, MononokeIdentity)>,
98103
acl_manifest_mode: AclManifestMode,
@@ -105,6 +110,7 @@ pub struct RestrictedPathsTestData {
105110
#[derive(Clone)]
106111
pub struct RestrictedPathsTestDataBuilder {
107112
config_restricted_paths: Vec<(NonRootMPath, MononokeIdentity)>,
113+
config_paths_read_only: bool,
108114
acl_manifest_restricted_paths: Vec<(NonRootMPath, MononokeIdentity)>,
109115
acl_manifest_mode: AclManifestMode,
110116
tooling_allowlist_group: Option<String>,
@@ -341,6 +347,7 @@ impl RestrictedPathsTestDataBuilder {
341347
pub fn new() -> Self {
342348
Self {
343349
config_restricted_paths: vec![],
350+
config_paths_read_only: false,
344351
acl_manifest_restricted_paths: vec![],
345352
acl_manifest_mode: AclManifestMode::Disabled,
346353
tooling_allowlist_group: None,
@@ -373,6 +380,13 @@ impl RestrictedPathsTestDataBuilder {
373380
self
374381
}
375382

383+
/// Mark every config restricted path as `read_only` in its
384+
/// `PathRestrictionMetadata`. Defaults to false.
385+
pub fn with_config_paths_read_only(mut self, read_only: bool) -> Self {
386+
self.config_paths_read_only = read_only;
387+
self
388+
}
389+
376390
pub fn with_acl_manifest_restricted_paths(
377391
mut self,
378392
restricted_paths: Vec<(NonRootMPath, MononokeIdentity)>,
@@ -534,6 +548,7 @@ impl RestrictedPathsTestDataBuilder {
534548
expected_scuba_logs: self.expected_scuba_logs,
535549
enforcement_scenarios: self.enforcement_scenarios,
536550
config_restricted_paths: self.config_restricted_paths,
551+
config_paths_read_only: self.config_paths_read_only,
537552
acl_manifest_restricted_paths: self.acl_manifest_restricted_paths,
538553
acl_manifest_mode: self.acl_manifest_mode,
539554
repo_regions_config: self.repo_regions_config,
@@ -589,6 +604,13 @@ impl RestrictedPathsTestData {
589604
Ok(())
590605
}
591606

607+
/// Build a scenario repo without running derivation or access operations.
608+
/// For tests that only inspect config-derived predicates such as
609+
/// `may_have_restricted_paths`.
610+
pub async fn build_repo_only(&self) -> Result<TestRepo> {
611+
Ok(self.setup_scenario_repo(&[]).await?.repo)
612+
}
613+
592614
/// Runs the standard access scenario and returns the Scuba rows it emitted.
593615
pub async fn observe_restricted_paths_scenario(
594616
&self,
@@ -614,8 +636,16 @@ impl RestrictedPathsTestData {
614636
.await?;
615637
tokio::time::sleep(std::time::Duration::from_millis(200)).await;
616638

639+
let manifest_id_store_entries = scenario
640+
.repo
641+
.restricted_paths()
642+
.config_based()
643+
.manifest_id_store()
644+
.get_all_entries(&self.ctx)
645+
.await?;
617646
Ok(RestrictedPathsScenarioResult {
618647
scuba_logs: deserialize_scuba_log_file(&scenario.log_path)?,
648+
manifest_id_store_entries,
619649
})
620650
}
621651

@@ -645,8 +675,16 @@ impl RestrictedPathsTestData {
645675
.await?;
646676
tokio::time::sleep(std::time::Duration::from_millis(200)).await;
647677

678+
let manifest_id_store_entries = scenario
679+
.repo
680+
.restricted_paths()
681+
.config_based()
682+
.manifest_id_store()
683+
.get_all_entries(&self.ctx)
684+
.await?;
648685
Ok(RestrictedPathsScenarioResult {
649686
scuba_logs: deserialize_scuba_log_file(&scenario.log_path)?,
687+
manifest_id_store_entries,
650688
})
651689
}
652690

@@ -996,6 +1034,7 @@ impl RestrictedPathsTestData {
9961034
.sorted()
9971035
.collect::<Vec<_>>(),
9981036
manifest_id_store_entries
1037+
.clone()
9991038
.into_iter()
10001039
.sorted()
10011040
.collect::<Vec<_>>()
@@ -1018,7 +1057,10 @@ impl RestrictedPathsTestData {
10181057
println!(
10191058
"Scenario {scenario_idx} finished SUCCESSFULLY with expect_enforcement: {expect_enforcement} and enforcement_condition_sets: {enforcement_condition_sets:#?}"
10201059
);
1021-
Ok(RestrictedPathsScenarioResult { scuba_logs })
1060+
Ok(RestrictedPathsScenarioResult {
1061+
scuba_logs,
1062+
manifest_id_store_entries,
1063+
})
10221064
}
10231065

10241066
async fn setup_scenario_repo(
@@ -1031,6 +1073,7 @@ impl RestrictedPathsTestData {
10311073
let repo = setup_test_repo(
10321074
&self.ctx,
10331075
self.config_restricted_paths.clone(),
1076+
self.config_paths_read_only,
10341077
self.acl_manifest_mode,
10351078
self.tooling_allowlist_group.clone(),
10361079
acls,
@@ -1245,6 +1288,7 @@ fn setup_acl_file(acls: Acls) -> Result<std::path::PathBuf> {
12451288
async fn setup_test_repo(
12461289
ctx: &CoreContext,
12471290
config_restricted_paths: Vec<(NonRootMPath, MononokeIdentity)>,
1291+
config_paths_read_only: bool,
12481292
acl_manifest_mode: AclManifestMode,
12491293
tooling_allowlist_group: Option<String>,
12501294
acls: Acls,
@@ -1267,7 +1311,7 @@ async fn setup_test_repo(
12671311
PathRestrictionMetadata {
12681312
repo_region_acl: acl,
12691313
permission_request_group: None,
1270-
read_only: false,
1314+
read_only: config_paths_read_only,
12711315
},
12721316
)
12731317
})

0 commit comments

Comments
 (0)