From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 4E5CC1FF179 for ; Wed, 26 Nov 2025 15:26:07 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C9F69E208; Wed, 26 Nov 2025 15:26:19 +0100 (CET) Message-ID: Date: Wed, 26 Nov 2025 15:25:45 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox Datacenter Manager development discussion , Lukas Wagner References: <20251117141122.328559-1-l.wagner@proxmox.com> <20251117141122.328559-3-l.wagner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20251117141122.328559-3-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1764167109981 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, tests.rs, views.rs] Subject: Re: [pdm-devel] [PATCH datacenter-manager 2/3] views: add 'include-all' param; change semantics when there are no includes X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" wouldn't it be less work if this patch (and maybe the first one too) would be ordered after the tests refactoring? just saying this because you have to modify the tests here, which promptly is irrelevant in the next patch no hard feelings from my side though additional comment inline On 11/17/25 3:11 PM, Lukas Wagner wrote: > Previously, having no 'include' rules would mean that *all* resources > are included. Due view permissions being transitively applied to all > included resources, this also has some security consequences. > For instance, if there is only a single include rule which is then > removed by accident, suddenly any AuthID having privileges on the view > ACL object would then be granted these privileges on *all* resources. > > To somewhat mitigate this, the semantics are slightly changed. If there > are no include rules, then no resources will be contained in the view. > A new 'include-all' parameter is added, which aims to emphasize the > intent of *really* including everything. > > Suggested-by: Thomas Lamprecht > Signed-off-by: Lukas Wagner > --- > lib/pdm-api-types/src/views.rs | 4 +++ > server/src/views/mod.rs | 60 +++++++++++++++++++--------------- > server/src/views/tests.rs | 35 ++++++++++++++++++-- > 3 files changed, 69 insertions(+), 30 deletions(-) > > diff --git a/lib/pdm-api-types/src/views.rs b/lib/pdm-api-types/src/views.rs > index 950a3a04..82ab8781 100644 > --- a/lib/pdm-api-types/src/views.rs > +++ b/lib/pdm-api-types/src/views.rs > @@ -58,6 +58,9 @@ pub struct ViewConfig { > #[updater(skip)] > pub id: String, > > + /// Include all resources by default. > + pub include_all: Option, this probably needs at least #[updater(serde(skip_serializing_if = "Option::is_none"))] ? maybe also a serde skip_serializing? > + > /// List of includes. > #[serde(default, skip_serializing_if = "Vec::is_empty")] > #[updater(serde(skip_serializing_if = "Option::is_none"))] > @@ -261,6 +264,7 @@ mod test { > fn config_smoke_test() { > let config = " > view: some-view > + include-all true > include exact:remote=someremote > include remote=someremote > include resource-type=qemu > diff --git a/server/src/views/mod.rs b/server/src/views/mod.rs > index 8b4c70a4..3669ace4 100644 > --- a/server/src/views/mod.rs > +++ b/server/src/views/mod.rs > @@ -64,31 +64,34 @@ impl View { > /// When there are `include remote:<...>` or `exclude remote:<...>` rules, we can use these to > /// check if a remote needs to be considered at all. > pub fn can_skip_remote(&self, remote: &str) -> bool { > - let mut has_any_include_remote = false; > - let mut matches_any_include_remote = false; > - > - let mut any_other = false; > - > - for include in &self.config.include { > - if let FilterRule::Remote(r) = include { > - has_any_include_remote = true; > - if r.matches(remote) { > - matches_any_include_remote = true; > - break; > - } > - } else { > - any_other = true; > - } > - } > - > let matches_any_exclude_remote = self > .config > .exclude > .iter() > .any(|rule| Self::matches_remote_rule(remote, rule)); > > - (has_any_include_remote && !matches_any_include_remote && !any_other) > - || matches_any_exclude_remote > + if matches_any_exclude_remote { > + return true; > + } > + > + if self.config.include_all.unwrap_or_default() { > + return false; > + } > + > + for include in &self.config.include { > + if let FilterRule::Remote(r) = include { > + if r.matches(remote) { > + return false; > + } > + } else { > + // If there is any other type of rule, we cannot safely infer whether we can skip > + // the remote (e.g. for 'tag' matches, we have to check *all* remotes for resources > + // with a given tag) > + return false; > + } > + } > + > + true > } > > /// Check if a remote is *explicitly* included (and not excluded). > @@ -96,18 +99,22 @@ impl View { > /// A subset of the resources of a remote might still be pulled in by other rules, > /// but this function check if the remote as a whole is matched. > pub fn is_remote_explicitly_included(&self, remote: &str) -> bool { > - let matches_include_remote = self > - .config > - .include > - .iter() > - .any(|rule| Self::matches_remote_rule(remote, rule)); > + let included = if self.config.include_all.unwrap_or_default() { > + true > + } else { > + self.config > + .include > + .iter() > + .any(|rule| Self::matches_remote_rule(remote, rule)) > + }; > + > let matches_exclude_remote = self > .config > .exclude > .iter() > .any(|rule| Self::matches_remote_rule(remote, rule)); > > - matches_include_remote && !matches_exclude_remote > + included && !matches_exclude_remote > } > > /// Check if a node is matched by the filter rules. > @@ -131,8 +138,7 @@ impl View { > } > > fn check_if_included(&self, remote: &str, resource: &ResourceData) -> bool { > - if self.config.include.is_empty() { > - // If there are no include rules, any resource is included (unless excluded) > + if self.config.include_all.unwrap_or_default() { > return true; > } > > diff --git a/server/src/views/tests.rs b/server/src/views/tests.rs > index 14d94ac9..0d83ae70 100644 > --- a/server/src/views/tests.rs > +++ b/server/src/views/tests.rs > @@ -135,6 +135,7 @@ fn exclude_remotes() { > FilterRule::Remote(StringMatcher::Exact("remote-a".into())), > FilterRule::Remote(StringMatcher::Exact("remote-b".into())), > ], > + include_all: Some(true), > ..Default::default() > }; > > @@ -184,6 +185,8 @@ fn include_exclude_remotes() { > FilterRule::Remote(StringMatcher::Exact("remote-b".into())), > FilterRule::Remote(StringMatcher::Exact("remote-c".into())), > ], > + > + ..Default::default() > }; > run_test( > config.clone(), > @@ -224,6 +227,7 @@ fn include_exclude_remotes() { > fn empty_config() { > let config = ViewConfig { > id: "empty".into(), > + include_all: Some(true), > ..Default::default() > }; > run_test( > @@ -301,6 +305,7 @@ fn exclude_type() { > FilterRule::ResourceType(EnumMatcher(ResourceType::PveStorage)), > FilterRule::ResourceType(EnumMatcher(ResourceType::PveQemu)), > ], > + include_all: Some(true), > ..Default::default() > }, > &[ > @@ -329,6 +334,7 @@ fn include_exclude_type() { > exclude: vec![FilterRule::ResourceType(EnumMatcher( > ResourceType::PveStorage, > ))], > + ..Default::default() > }, > &[ > ( > @@ -357,6 +363,7 @@ fn include_exclude_tags() { > FilterRule::Tag(StringMatcher::Exact("tag2".to_string())), > ], > exclude: vec![FilterRule::Tag(StringMatcher::Exact("tag3".to_string()))], > + ..Default::default() > }, > &[ > ( > @@ -404,6 +411,7 @@ fn include_exclude_resource_pool() { > exclude: vec![FilterRule::ResourcePool(StringMatcher::Exact( > "pool2".to_string(), > ))], > + ..Default::default() > }, > &[ > ( > @@ -459,6 +467,7 @@ fn include_exclude_resource_id() { > "remote/{REMOTE}/storage/{NODE}/otherstorage" > ))), > ], > + ..Default::default() > }, > &[ > ( > @@ -509,6 +518,7 @@ fn node_included() { > exclude: vec![FilterRule::Remote(StringMatcher::Exact( > "remote-b".to_string(), > ))], > + ..Default::default() > }); > > assert!(view.is_node_included("remote-a", "somenode")); > @@ -528,6 +538,7 @@ fn can_skip_remote_if_excluded() { > exclude: vec![FilterRule::Remote(StringMatcher::Exact( > "remote-b".to_string(), > ))], > + include_all: Some(true), > }); > > assert!(!view.can_skip_remote("remote-a")); > @@ -542,6 +553,7 @@ fn can_skip_remote_if_included() { > "remote-b".to_string(), > ))], > exclude: vec![], > + ..Default::default() > }); > > assert!(!view.can_skip_remote("remote-b")); > @@ -559,6 +571,7 @@ fn can_skip_remote_cannot_skip_if_any_other_include() { > )), > ], > exclude: vec![], > + ..Default::default() > }); > > assert!(!view.can_skip_remote("remote-b")); > @@ -575,6 +588,7 @@ fn can_skip_remote_explicit_remote_exclude() { > exclude: vec![FilterRule::Remote(StringMatcher::Exact( > "remote-a".to_string(), > ))], > + ..Default::default() > }); > > assert!(view.can_skip_remote("remote-a")); > @@ -584,8 +598,19 @@ fn can_skip_remote_explicit_remote_exclude() { > fn can_skip_remote_with_empty_config() { > let view = View::new(ViewConfig { > id: "abc".into(), > - include: vec![], > - exclude: vec![], > + ..Default::default() > + }); > + > + assert!(view.can_skip_remote("remote-a")); > + assert!(view.can_skip_remote("remote-b")); > +} > + > +#[test] > +fn can_skip_remote_cannot_skip_if_all_included() { > + let view = View::new(ViewConfig { > + id: "abc".into(), > + include_all: Some(true), > + ..Default::default() > }); > > assert!(!view.can_skip_remote("remote-a")); > @@ -600,6 +625,7 @@ fn can_skip_remote_with_no_remote_includes() { > "resource/remote-a/guest/100".to_string(), > ))], > exclude: vec![], > + ..Default::default() > }); > > assert!(!view.can_skip_remote("remote-a")); > @@ -614,6 +640,7 @@ fn explicitly_included_remote() { > "remote-b".to_string(), > ))], > exclude: vec![], > + ..Default::default() > }); > > assert!(view.is_remote_explicitly_included("remote-b")); > @@ -629,6 +656,7 @@ fn included_and_excluded_same_remote() { > exclude: vec![FilterRule::Remote(StringMatcher::Exact( > "remote-b".to_string(), > ))], > + ..Default::default() > }); > > assert!(!view.is_remote_explicitly_included("remote-b")); > @@ -640,8 +668,9 @@ fn not_explicitly_included_remote() { > id: "abc".into(), > include: vec![], > exclude: vec![], > + include_all: Some(true), > }); > > // Assert that is not *explicitly* included > - assert!(!view.is_remote_explicitly_included("remote-b")); > + assert!(view.is_remote_explicitly_included("remote-b")); > } _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel