Re: [PATCH proxmox 5/7] notify (smtp): Add state handling logic

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

I think this misses removing the endpoint state file in
`delete_endpoint`. Some more notes inline.

On Fri Feb 13, 2026 at 5:04 PM CET, Arthur Bied-Charreton wrote:
> Create new state file in add_endpoint, and create/update existing one in
> update_endpoint.
>
> Add trigger_state_refresh to the Endpoint trait, with no-op default
> implementation. Override trigger_state_refresh in SmtpEndpoint's
> Endpoint impl to trigger an OAuth2 token exchange, in order to rotate
> an existing token, or extend its lifetime.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  proxmox-notify/src/api/common.rs     | 16 ++++++
>  proxmox-notify/src/api/smtp.rs       | 29 ++++++++++-
>  proxmox-notify/src/endpoints/smtp.rs | 75 +++++++++++++++++++++++++++-
>  proxmox-notify/src/lib.rs            | 19 +++++++
>  4 files changed, 137 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs
> index fa2356e2..1e6b7d46 100644
> --- a/proxmox-notify/src/api/common.rs
> +++ b/proxmox-notify/src/api/common.rs
> @@ -3,6 +3,22 @@ use proxmox_http_error::HttpError;
>  use super::http_err;
>  use crate::{Bus, Config, Notification};
>  
> +/// Refresh all notification targets' internal state.
> +///
> +/// The caller is responsible for any needed permission checks.
> +pub fn trigger_state_refresh(config: &Config) -> Result<(), HttpError> {
> +    let bus = Bus::from_config(config).map_err(|err| {
> +        http_err!(
> +            INTERNAL_SERVER_ERROR,
> +            "Could not instantiate notification bus: {err}"
> +        )
> +    })?;
> +
> +    bus.trigger_state_refresh();
> +
> +    Ok(())
> +}
> +
>  /// Send a notification to a given target.
>  ///
>  /// The caller is responsible for any needed permission checks.
> diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp.rs
> index 4231cdae..8992e789 100644
> --- a/proxmox-notify/src/api/smtp.rs
> +++ b/proxmox-notify/src/api/smtp.rs
> @@ -2,7 +2,7 @@ use proxmox_http_error::HttpError;
>  
>  use crate::api::{http_bail, http_err};
>  use crate::endpoints::smtp::{
> -    DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPrivateConfig,
> +    self, DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPrivateConfig,
>      SmtpPrivateConfigUpdater, SMTP_TYPENAME,
>  };
>  use crate::Config;
> @@ -69,6 +69,16 @@ pub fn add_endpoint(
>          &endpoint_config.name,
>      )?;
>  
> +    smtp::State::new(oauth2_refresh_token)
> +        .store(&endpoint_config.name)
> +        .map_err(|e| {
> +            http_err!(
> +                INTERNAL_SERVER_ERROR,
> +                "could not create state file for endpoint '{}': {e}",
> +                endpoint_config.name
> +            )
> +        })?;
> +
>      config
>          .config
>          .set_data(&endpoint_config.name, SMTP_TYPENAME, &endpoint_config)
> @@ -206,6 +216,23 @@ pub fn update_endpoint(
>          }
>      })?;
>  
> +    smtp::State::load(name)
> +        .map_err(|e| {
> +            http_err!(
> +                INTERNAL_SERVER_ERROR,
> +                "could not load state for endpoint '{name}': {e}"
> +            )
> +        })?
> +        .set_oauth2_refresh_token(oauth2_refresh_token)
> +        .set_last_refreshed(proxmox_time::epoch_i64())
> +        .store(name)
> +        .map_err(|e| {
> +            http_err!(
> +                INTERNAL_SERVER_ERROR,
> +                "could not persist state for endpoint '{name}': {e}"
> +            )
> +        })?;
> +
>      config
>          .config
>          .set_data(name, SMTP_TYPENAME, &endpoint)
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index 361c4da9..244799fd 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -1,12 +1,15 @@
>  use std::borrow::Cow;
> -use std::time::Duration;
> +use std::time::{Duration, SystemTime, UNIX_EPOCH};
>  
>  use lettre::message::header::{HeaderName, HeaderValue};
>  use lettre::message::{Mailbox, MultiPart, SinglePart};
> +use lettre::transport::smtp::authentication::{Credentials, Mechanism};
>  use lettre::transport::smtp::client::{Tls, TlsParameters};
>  use lettre::{message::header::ContentType, Message, SmtpTransport, Transport};
>  use serde::{Deserialize, Serialize};
>  
> +use oauth2::{ClientId, ClientSecret, RefreshToken};
> +
>  use proxmox_schema::api_types::COMMENT_SCHEMA;
>  use proxmox_schema::{api, Updater};
>  
> @@ -22,6 +25,7 @@ const SMTP_PORT: u16 = 25;
>  const SMTP_SUBMISSION_STARTTLS_PORT: u16 = 587;
>  const SMTP_SUBMISSION_TLS_PORT: u16 = 465;
>  const SMTP_TIMEOUT: u16 = 5;
> +const SMTP_STATE_REFRESH_CUTOFF_SECONDS: u64 = 60 * 60 * 12;

Duration::from_secs is actually a `const fn`, so you could

const SMTP_STATE_REFRESH_CUTOFF: Duration = Duration::from_secs(12 * 60 * 60);

>  
>  mod state;
>  mod xoauth2;
> @@ -205,6 +209,43 @@ pub struct SmtpEndpoint {
>  }
>  
>  impl SmtpEndpoint {
> +    fn get_access_token(
> +        &self,
> +        refresh_token: &str,
> +        auth_method: &SmtpAuthMethod,
> +    ) -> Result<xoauth2::TokenExchangeResult, Error> {
> +        let client_id = ClientId::new(
> +            self.config
> +                .oauth2_client_id
> +                .as_ref()
> +                .ok_or_else(|| Error::Generic("oauth2-client-id not set".into()))?
> +                .to_string(),
> +        );
> +        let client_secret = ClientSecret::new(
> +            self.private_config
> +                .oauth2_client_secret
> +                .as_ref()
> +                .ok_or_else(|| Error::Generic("oauth2-client-secret not set".into()))?
> +                .to_string(),
> +        );
> +        let refresh_token = RefreshToken::new(refresh_token.into());
> +
> +        match auth_method {
> +            SmtpAuthMethod::GoogleOAuth2 => {
> +                xoauth2::get_google_token(client_id, client_secret, refresh_token)
> +            }
> +            SmtpAuthMethod::MicrosoftOAuth2 => xoauth2::get_microsoft_token(
> +                client_id,
> +                client_secret,
> +                self.config.oauth2_tenant_id.as_ref().ok_or(Error::Generic(
> +                    "tenant ID not set, required for Microsoft OAuth2".into(),
> +                ))?,
> +                refresh_token,
> +            ),
> +            _ => Err(Error::Generic("OAuth2 not configured".into())),
> +        }
> +    }
> +
>      fn build_transport(&self, tls: Tls, port: u16) -> Result<SmtpTransport, Error> {
>          let mut transport_builder = SmtpTransport::builder_dangerous(&self.config.server)
>              .tls(tls)
> @@ -336,6 +377,38 @@ impl Endpoint for SmtpEndpoint {
>      fn disabled(&self) -> bool {
>          self.config.disable.unwrap_or_default()
>      }
> +
> +    fn trigger_state_refresh(&self) -> Result<(), Error> {
> +        let state = State::load(self.name())?;
> +
> +        let Some(refresh_token) = &state.oauth2_refresh_token else {
> +            return Ok(());
> +        };
> +
> +        // The refresh job is configured in pveupdate, which runs once for each node.
> +        // Don't refresh if we already did it recently.
> +        if SystemTime::now()
> +            .duration_since(UNIX_EPOCH + Duration::from_secs(state.last_refreshed as u64))
> +            .map_err(|e| Error::Generic(e.to_string()))?
> +            < Duration::from_secs(SMTP_STATE_REFRESH_CUTOFF_SECONDS)

This reminds me, we deal with "raw" timestamps so often (especially in
PDM), we really need to have some better types/helpers for things like
these... ofc out of scope for this series...

> +        {
> +            return Ok(());
> +        }
> +
> +        let Some(auth_method) = self.config.auth_method.as_ref() else {
> +            return Ok(());
> +        };
> +
> +        match self
> +            .get_access_token(refresh_token, auth_method)?
> +            .refresh_token
> +        {
> +            Some(tok) => state.set_oauth2_refresh_token(Some(tok.into_secret())), // New token was returned, rotate
> +            None => state,
> +        }
> +        .set_last_refreshed(proxmox_time::epoch_i64())
> +        .store(self.name())
> +    }
>  }
>  
>  /// Construct a lettre `Message` from a raw email message.
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 879f8326..c1a5e535 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -157,6 +157,11 @@ pub trait Endpoint {
>  
>      /// Check if the endpoint is disabled
>      fn disabled(&self) -> bool;
> +
> +    /// Refresh endpoint's state
> +    fn trigger_state_refresh(&self) -> Result<(), Error> {
> +        Ok(())
> +    }
>  }
>  
>  #[derive(Debug, Clone, Serialize, Deserialize)]
> @@ -593,6 +598,20 @@ impl Bus {
>  
>          Ok(())
>      }
> +
> +    /// Refresh all endpoints' internal state.
> +    ///
> +    /// This function works on a best effort basis, if an endpoint's state cannot
> +    /// be updated for whatever reason, the error is logged and the next one(s)
> +    /// are attempted.
> +    pub fn trigger_state_refresh(&self) {
> +        for (name, endpoint) in &self.endpoints {
> +            match endpoint.trigger_state_refresh() {
> +                Ok(()) => info!("triggered state refresh for endpoint '{name}'"),

I'd probably rather use `debug!` here and then `info!` only if one endpoint
actually refreshed its token


> +                Err(e) => error!("could not trigger state refresh for endpoint '{name}': {e}"),
> +            };
> +        }
> +    }
>  }
>  
>  #[cfg(test)]