Re: [pdm-devel] [PATCH datacenter-manager 2/3] views: add 'include-all' param; change semantics when there are no includes

["Lukas Wagner" <l.wagner@proxmox.com>]

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 <t.lamprecht@proxmox.com>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>   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<bool>,
>
> 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