Re: [PATCH proxmox v2 02/16] notify: smtp: Introduce state management

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

On Thu, Apr 09, 2026 at 11:51:43AM +0200, Lukas Wagner wrote:
Thanks for the review :)
> Looking good!
> 
> With the trivial suggestions implemented:
> 
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> 
[...]
> > +use crate::endpoints::smtp::State;
> 
> I think this import must be feature gated, otherwise a 
> 
> cargo build --no-default-features
> 
> does not compile
> 
[...]
> > +use crate::endpoints::smtp::State;
> 
> This one here as well with
> 
> cargo build --no-default-features --features=pbs-context
> 
> 
[...]
> > +use crate::endpoints::smtp::State;
> 
> Also this one with
> 
> cargo build --no-default-features --features=pve-context
> 
> 
Argh, should have tested these again sorry about that
> >  use crate::renderer::TemplateSource;
> >  use crate::Error;
> >  
> > @@ -125,6 +126,19 @@ impl Context for PBSContext {
> >              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> >          Ok(template_string)
> >      }
> > +
> > +    fn load_oauth_state(&self, endpoint_name: &str) -> Result<State, Error> {
> > +        let path = format!("/var/lib/proxmox-backup-priv/notifications/state-{endpoint_name}.json");
> > +        State::load(path)
> > +    }
> > +
> > +    fn save_oauth_state(&self, endpoint_name: &str, state: Option<State>) -> Result<(), Error> {
> > +        let path = format!("/var/lib/proxmox-backup-priv/notifications/state-{endpoint_name}.json");
> 
> Sorry for the back and forth with the path. As discussed off-list, we
> now agreed on the following paths for the oauth state files:
> 
> PBS: /var/lib/proxmox-backup/notifications/oauth-state/<endpoint>.json
> PDM: /var/lib/proxmox-datacenter-manager/notifications/oauth-state/<endpoint>.json
> PVE: /etc/pve/priv/notifications/oauth-state/<endpoint>.json
> 
ACK
[...]
> > +#[derive(Serialize, Deserialize, Clone, Debug, Default)]
> > +#[serde(rename_all = "kebab-case")]
> 
> No big deal, but this struct could use a doc comment.
> 
You're right, will add one, thanks!

> > +pub struct State {
> > +    #[serde(skip_serializing_if = "Option::is_none")]
> > +    pub oauth2_refresh_token: Option<String>,
> > +    pub last_refreshed: i64,
> > +}
> > +
> > +impl From<Option<String>> for State {
> > +    fn from(value: Option<String>) -> Self {
> > +        Self {
> > +            oauth2_refresh_token: value,
> > +            last_refreshed: proxmox_time::epoch_i64(),
> > +        }
> > +    }
> > +}
> 
> This `impl From` feels a bit off to me, due to it being not 100%
> functional (due to the timestamp). Also `From<Option<...>>`
> feels a bit odd to me. Maybe just have a `State::new` instead?
> 
> In the end-result of this series, you only ever call this From
> in the api handler, and there only if the refresh-token is provided.
> I guess you could have a State::new(refresh_token: String, timestamp:
> i64) then?
> 
Yea you're right that a `new` method would be cleaner here, will update
this.
> > +
> > +/// Attempt to create a directory at `path` with `mode`, returning `Ok(())` if the directory either already
> > +/// exists, or was successfully created.
> > +///
> > +/// `pmxcfs` automatically sets the x-bit in directory permissions, however it does not allow the user
> > +/// to set it, the `fchmod` fails with `EPERM` on directories with `0o700`.
> > +///
> > +/// The `proxmox_sys` version of this function unconditionally logs mode mismatches even with
> > +/// `enforce_permissions == false` AND calls `fchmod`. This means that if using that version, we would
> > +/// always get a permission mismatch warning in the `pvedaemon` logs, even though we do not need the
> > +/// `fchmod` call.
> > +fn ensure_dir_exists<P: AsRef<Path>>(path: P, mode: nix::sys::stat::Mode) -> Result<(), Error> {
> > +    match nix::unistd::mkdir(path.as_ref(), mode) {
> > +        Ok(()) | Err(nix::errno::Errno::EEXIST) => Ok(()),
> > +        Err(e) => Err(Error::StatePersistence(
> > +            path.as_ref().to_string_lossy().into(),
> > +            e.into(),
> > +        )),
> > +    }
> > +}
> > +
> > +impl State {
> > +    /// Load the state for the endpoint identified by `endpoint_name`, instantiating a default object
> > +    /// if yes state exists.
> 
> the 'yes' should not be here, I think? :)
> 
It should be 'no'...
> > +    ///
> > +    /// # Errors
> > +    /// An [`Error`] is returned if deserialization of the state object fails.
> > +    pub(crate) fn load<P: AsRef<Path>>(path: P) -> Result<State, Error> {
> 
> This one should be `pub`, since it is supposed to be called from the
> context implementation (which should be moved out of this crate at some
> point).
> 
[...]
> > +    /// Persist the state for the endpoint identified by `endpoint_name`.
> > +    ///
> > +    /// # Errors
> > +    /// An [`Error`] is returned if serialization of the state object, or the final write, fail.
> > +    pub(crate) fn save<P: AsRef<Path>>(
> 
> Same here.
> 
Yes, I meant to change that for v2 but this somehow slipped through my 
TODOs, thanks!
> > +        self,
> > +        path: P,
> > +        mode: nix::sys::stat::Mode,
> 
> I'd rather use CreateOptions here and move the CreateOptions::new to the
> caller
> 
Yes, the reason I did not do that was because ensure_dir_exists expects
a Mode, which is a private field in CreateOptions. Since the directories we
need to create for the state files are now deeper though, we need to use
create_path instead, which does take CreateOptions structs directly, so
that is solved :)

> > +    ) -> Result<(), Error> {
> > +        let path_str = path.as_ref().to_string_lossy();
> > +        let parent = path.as_ref().parent().unwrap();
> > +
> > +        debug!("attempting to persist state at {path_str}");
> > +
> > +        ensure_dir_exists(parent, mode)?;
> > +
> > +        let s = serde_json::to_string_pretty(&self)
> > +            .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))?;
> > +
> > +        proxmox_sys::fs::replace_file(
> > +            &path,
> > +            s.as_bytes(),
> > +            proxmox_sys::fs::CreateOptions::new().perm(mode),
> > +            false,
> > +        )
> > +        .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))
> > +    }
> > +
> > +    /// Delete the state for the endpoint identitied by `endpoint_name`.
> > +    ///
> > +    /// # Errors
> > +    /// An [`Error`] is returned if the state file cannot be deleted.
> 
> This seems to be wrong?
> 
Yes, copy-paste error, will be fixed
> > +    pub(crate) fn delete<P: AsRef<Path>>(path: P) {
> > +        let _ = std::fs::remove_file(&path);
> 
> Might make sense to the following here (untested):
> 
>         if let Err(e) = std::fs::remove_file(&path) {
>             if e.kind() != std::io::ErrorKind::NotFound {
>                 // log error
>             }
>         }
> 
ACK, will do that

[...]