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 87C321FF179 for ; Wed, 26 Nov 2025 15:59:39 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2939BF69E; Wed, 26 Nov 2025 15:59:52 +0100 (CET) Mime-Version: 1.0 Date: Wed, 26 Nov 2025 15:59:47 +0100 Message-Id: From: "Lukas Wagner" To: "Dominik Csapak" , "Proxmox Datacenter Manager development discussion" , "Lukas Wagner" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20251117141122.328559-1-l.wagner@proxmox.com> <20251117141122.328559-3-l.wagner@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1764169151383 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 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. [views.rs, mod.rs, tests.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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" On Wed Nov 26, 2025 at 3:25 PM CET, Dominik Csapak wrote: > 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 > I mean yes, you are right and I actually came to the same conclusion ... after the work was already finished :). First I wanted to do the test refactor in a separate followup, since it's not really important at the moment. But in the end I included it here and didn't think ahead as much as I should have... Would be even more additional work to shuffle it around now, so I'll leave it as is - I hope the slightly higher review burden for those superfluous changes is not too bad. > 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? > Ah yes, thx! Will include it in v2. >> + >> /// 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