Re: [PATCH proxmox 2/7] notify (smtp): Introduce state module

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Mon, Mar 23, 2026 at 01:26:05PM +0100, Lukas Wagner wrote:
> Hi Arthur!
> 
> Generally we'd write the commit subject rather like this:
> 
>  "notify: smtp: ...."
> 
>  instead of
> 
>  "notify (smtp): ..."
> 
>  (but not a big deal IMO, just mentioning it)
> 
ACK, got my conventions confused, my bad
> 
[...]
> > @@ -32,6 +34,10 @@ pub trait Context: Send + Sync + Debug {
> >          namespace: Option<&str>,
> >          source: TemplateSource,
> >      ) -> Result<Option<String>, Error>;
> > +    /// Return the path to the state file for this context.
> > +    fn state_file_path(&self, name: &str) -> String;
> 
> I think this should rather return a PathBuf. And rename `name` ->
> `endpoint_name`?
> 
Thanks for catching that, I could not do the usual `P: AsRef<Path>` and
somehow fell back to `String` directly..

> > +    /// Create options to be used when writing files containing secrets.
> > +    fn secret_create_options(&self) -> CreateOptions;
> 
> Due to CreateOptions being part of the `Context` trait now, we need to
> pull in proxmox-sys unconditionally:
> 
> diff --git i/proxmox-notify/Cargo.toml w/proxmox-notify/Cargo.toml
> index d816c695..35d5b8cb 100644
> --- i/proxmox-notify/Cargo.toml
> +++ w/proxmox-notify/Cargo.toml
> @@ -36,17 +36,15 @@ proxmox-schema = { workspace = true, features = ["api-macro", "api-types"] }
>  proxmox-section-config = { workspace = true }
>  proxmox-serde.workspace = true
>  proxmox-sendmail = { workspace = true, optional = true }
> -proxmox-sys = { workspace = true, optional = true }
> +proxmox-sys = { workspace = true }
>  proxmox-time.workspace = true
>  proxmox-uuid = { workspace = true, features = ["serde"] }
>  nix.workspace = true
>  
>  [features]
>  default = ["sendmail", "gotify", "smtp", "webhook"]
> -mail-forwarder = ["dep:mail-parser", "dep:proxmox-sys", "proxmox-sendmail/mail-forwarder"]
> -sendmail = ["dep:proxmox-sys", "dep:proxmox-sendmail"]
> +mail-forwarder = ["dep:mail-parser", "proxmox-sendmail/mail-forwarder"]
> +sendmail = ["dep:proxmox-sendmail"]
>  gotify = ["dep:proxmox-http", "dep:http"]
> -pve-context = ["dep:proxmox-sys"]
> -pbs-context = ["dep:proxmox-sys"]
> -smtp = ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http", "dep:proxmox-sys"]
> +smtp = ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http"]
>  webhook = ["dep:http", "dep:percent-encoding", "dep:proxmox-base64", "dep:proxmox-http"]
> 
> 
ACK
> 
> >  }
> >  
> >  #[cfg(not(test))]
> > diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
> > index 3e5da59c..4f93b45d 100644
> > --- a/proxmox-notify/src/context/pbs.rs
> > +++ b/proxmox-notify/src/context/pbs.rs
> > @@ -125,6 +125,14 @@ impl Context for PBSContext {
> >              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> >          Ok(template_string)
> >      }
> > +
> > +    fn state_file_path(&self, name: &str) -> String {
> > +        format!("/var/lib/proxmox-backup/priv/notifications/{name}.json")
> > +    }
> > +
> > +    fn secret_create_options(&self) -> proxmox_sys::fs::CreateOptions {
> > +        proxmox_sys::fs::CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o600))
> 
> (1) In the trait impl, you could get the appropriate CreateOptions via
> proxmox-product-config...
> 
> > +    }
> >  }
> >  
> >  #[cfg(test)]
> > diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
> > index a97cce26..e30f7b49 100644
> > --- a/proxmox-notify/src/context/pve.rs
> > +++ b/proxmox-notify/src/context/pve.rs
> > @@ -74,6 +74,14 @@ impl Context for PVEContext {
> >              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> >          Ok(template_string)
> >      }
> > +
> > +    fn state_file_path(&self, name: &str) -> String {
> > +        format!("/etc/pve/priv/notifications/{name}.json")
> 
> I think `state` should be somewhere in the path -- so maybe one of
> 
>   - priv/notification-endpoint-state/{name}.json
>   - priv/notification-state/{name}.json
>   - priv/notifications/state-{name}.json
> 
Makes sense, thanks for the suggestion. Will use the third one, 
it allows using the already existing /priv/notifications directory.
> > +    }
> > +
> > +    fn secret_create_options(&self) -> proxmox_sys::fs::CreateOptions {
> > +        proxmox_sys::fs::CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o600))
> 
> (2) ... here as well, but you probably need to initialize
> proxmox-product-config in the proxmox-perl-rs bindings with the correct
> UIDs.
> 
ACK this and the other proxmox-product-config comment, will integrate
that crate in v2, thanks

[...]
> > +    /// Persist the state for the endpoint identified by `name`.
> > +    ///
> > +    /// # Errors
> > +    /// An [`Error`] is returned if serialization of the state object, or the final write, fail.
> > +    pub(crate) fn store(self, name: &str) -> Result<(), Error> {
> > +        let path = context().state_file_path(name);
> > +        let parent = std::path::Path::new(&path).parent().unwrap();
> > +
> > +        proxmox_sys::fs::ensure_dir_exists(parent, &context().secret_create_options(), false)
> > +            .map_err(|e| Error::ConfigSerialization(e.into()))?;
> 
> I'm getting a "bad permissions on "/etc/pve/priv/notifications" (0o700
> != 0o600)" error in the system/task logs when the token is refreshed.
> The CreateOptions provided by the context are 0o600, but I think pmxcfs
> automatically enforces the x-bit on directories, leading to 0o700 perms
> on the '/etc/pve/priv/notifications/' directory, which ultimately gives
> us this error. I think you either have to add a separate trait method
> for the directory permissions/CreateOptions, or add the x-bit to the
> existing CreateOptions yourself here at the callsite.
> 
Thanks for catching that! Will fix in v2
> > +
> > +        let s = serde_json::to_string_pretty(&self)
> > +            .map_err(|e| Error::ConfigSerialization(e.into()))?;
> > +
> > +        proxmox_sys::fs::replace_file(path, s.as_bytes(), context().secret_create_options(), true)
> > +            .map_err(|e| Error::ConfigSerialization(e.into()))
> > +    }
> > +
> > +    /// Set `last_refreshed`.
> > +    pub(crate) fn set_last_refreshed(mut self, last_refreshed: i64) -> Self {
> > +        self.last_refreshed = last_refreshed;
> > +        self
> > +    }
> > +
> > +    /// Set `oauth2_refresh_token`.
> > +    pub(crate) fn set_oauth2_refresh_token(mut self, oauth2_refresh_token: Option<String>) -> Self {
> > +        self.oauth2_refresh_token = oauth2_refresh_token;
> > +        self
> > +    }
> > +}
>