Hey Arthur!
Great work, looking really good so far. Left some comments inline.
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Add Google & Microsoft OAuth2 support for SMTP notification targets. The
> SmtpEndpoint implements refresh_state() to allow proactively refreshing
> tokens through pveupdate.
>
> The SMTP API functions are updated to handle OAuth2 state. The refresh
> token initially comes from the frontend, and it is more state than it is
> configuration, therefore it is passed as a single parameter in
> {add,update}_endpoint and persisted separately.
>
> Signed-off-by: Arthur Bied-Charreton <[email protected]>
> ---
> proxmox-notify/src/api/smtp.rs | 144 ++++++++++++++----
> proxmox-notify/src/endpoints/smtp.rs | 210 +++++++++++++++++++++++++--
> 2 files changed, 315 insertions(+), 39 deletions(-)
>
> diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp.rs
> index 470701bf..9bd4826d 100644
> --- a/proxmox-notify/src/api/smtp.rs
> +++ b/proxmox-notify/src/api/smtp.rs
> @@ -1,11 +1,14 @@
> +use std::time::{SystemTime, UNIX_EPOCH};
> +
> use proxmox_http_error::HttpError;
>
> use crate::api::{http_bail, http_err};
> +use crate::context::context;
> use crate::endpoints::smtp::{
> DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPrivateConfig,
> - SmtpPrivateConfigUpdater, SMTP_TYPENAME,
> + SmtpPrivateConfigUpdater, SmtpState, SMTP_TYPENAME,
> };
> -use crate::Config;
> +use crate::{Config, State};
>
> /// Get a list of all smtp endpoints.
> ///
> @@ -30,6 +33,30 @@ pub fn get_endpoint(config: &Config, name: &str) ->
> Result<SmtpConfig, HttpError
> .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found"))
> }
>
> +/// Update the state for the endpoint `name`, and persist it at
> `context().state_file_path()`.
> +fn update_state(name: &str, entry: Option<SmtpState>) -> Result<(),
> HttpError> {
> + let mut state =
> State::from_path(context().state_file_path()).unwrap_or_default();
> +
> + match entry {
> + Some(entry) => state.set(name, &entry).map_err(|e| {
> + http_err!(
> + INTERNAL_SERVER_ERROR,
> + "could not update state for endpoint '{}': {e}",
> + name
> + )
> + })?,
> + None => state.remove(name),
> + }
> +
> + state.persist(context().state_file_path()).map_err(|e| {
> + http_err!(
> + INTERNAL_SERVER_ERROR,
> + "could not update state for endpoint '{}': {e}",
> + name
> + )
> + })
> +}
> +
> /// Add a new smtp endpoint.
> ///
> /// The caller is responsible for any needed permission checks.
> @@ -38,10 +65,15 @@ pub fn get_endpoint(config: &Config, name: &str) ->
> Result<SmtpConfig, HttpError
> /// - an entity with the same name already exists (`400 Bad request`)
> /// - the configuration could not be saved (`500 Internal server error`)
> /// - mailto *and* mailto_user are both set to `None`
> +///
> +/// `oauth2_refresh_token` is initially passed through the API when an OAuth2
> +/// endpoint is created/updated, however its state is not managed through a
> +/// config, which is why it is passed separately.
> pub fn add_endpoint(
> config: &mut Config,
> endpoint_config: SmtpConfig,
> private_endpoint_config: SmtpPrivateConfig,
> + oauth2_refresh_token: Option<String>,
> ) -> Result<(), HttpError> {
> if endpoint_config.name != private_endpoint_config.name {
> // Programming error by the user of the crate, thus we panic
> @@ -64,6 +96,17 @@ pub fn add_endpoint(
> &endpoint_config.name,
> )?;
>
> + update_state(
> + &endpoint_config.name,
> + Some(SmtpState {
> + oauth2_refresh_token,
> + last_refreshed: SystemTime::now()
> + .duration_since(UNIX_EPOCH)
We already have a nice helper for this - proxmox_time::epoch_i64
> + .unwrap()
> + .as_secs(),
> + }),
> + )?;
> +
> config
> .config
> .set_data(&endpoint_config.name, SMTP_TYPENAME, &endpoint_config)
> @@ -76,6 +119,28 @@ pub fn add_endpoint(
> })
> }
>
> +/// Apply `updater` to the private config identified by `name`, and set
> +/// the private config entry afterwards.
> +pub fn update_private_config(
This function does not need to be `pub`, I think?
> + config: &mut Config,
> + name: &str,
> + updater: impl FnOnce(&mut SmtpPrivateConfig),
nice idea btw, using a closure like this
> +) -> Result<(), HttpError> {
> + let mut private_config: SmtpPrivateConfig = config
> + .private_config
> + .lookup(SMTP_TYPENAME, name)
> + .map_err(|e| {
> + http_err!(
> + INTERNAL_SERVER_ERROR,
> + "no private config found for SMTP endpoint: {e}"
> + )
> + })?;
> +
> + updater(&mut private_config);
> +
> + super::set_private_config_entry(config, private_config, SMTP_TYPENAME,
> name)
> +}
> +
> /// Update existing smtp endpoint
> ///
> /// The caller is responsible for any needed permission checks.
> @@ -83,11 +148,16 @@ pub fn add_endpoint(
> /// Returns a `HttpError` if:
> /// - the configuration could not be saved (`500 Internal server error`)
> /// - mailto *and* mailto_user are both set to `None`
> +///
> +/// `oauth2_refresh_token` is initially passed through the API when an OAuth2
> +/// endpoint is created/updated, however its state is not managed through a
> +/// config, which is why it is passed separately.
> pub fn update_endpoint(
> config: &mut Config,
> name: &str,
> updater: SmtpConfigUpdater,
> private_endpoint_config_updater: SmtpPrivateConfigUpdater,
> + oauth2_refresh_token: Option<String>,
> delete: Option<&[DeleteableSmtpProperty]>,
> digest: Option<&[u8]>,
> ) -> Result<(), HttpError> {
> @@ -103,20 +173,20 @@ pub fn update_endpoint(
> DeleteableSmtpProperty::Disable => endpoint.disable = None,
> DeleteableSmtpProperty::Mailto => endpoint.mailto.clear(),
> DeleteableSmtpProperty::MailtoUser =>
> endpoint.mailto_user.clear(),
> - DeleteableSmtpProperty::Password =>
> super::set_private_config_entry(
> - config,
> - SmtpPrivateConfig {
> - name: name.to_string(),
> - password: None,
> - },
> - SMTP_TYPENAME,
> - name,
> - )?,
> + DeleteableSmtpProperty::Password => {
> + update_private_config(config, name, |c| c.password =
> None)?
> + }
> + DeleteableSmtpProperty::AuthMethod => endpoint.auth_method =
> None,
> + DeleteableSmtpProperty::OAuth2ClientId =>
> endpoint.oauth2_client_id = None,
> + DeleteableSmtpProperty::OAuth2ClientSecret => {
> + update_private_config(config, name, |c|
> c.oauth2_client_secret = None)?
> + }
> + DeleteableSmtpProperty::OAuth2TenantId =>
> endpoint.oauth2_tenant_id = None,
> DeleteableSmtpProperty::Port => endpoint.port = None,
> DeleteableSmtpProperty::Username => endpoint.username = None,
> }
> }
> - }
> + };
>
> if let Some(mailto) = updater.mailto {
> endpoint.mailto = mailto;
> @@ -139,29 +209,24 @@ pub fn update_endpoint(
> if let Some(mode) = updater.mode {
> endpoint.mode = Some(mode);
> }
> - if let Some(password) = private_endpoint_config_updater.password {
> - super::set_private_config_entry(
> - config,
> - SmtpPrivateConfig {
> - name: name.into(),
> - password: Some(password),
> - },
> - SMTP_TYPENAME,
> - name,
> - )?;
> + if let Some(auth_method) = updater.auth_method {
> + endpoint.auth_method = Some(auth_method);
> }
> -
> if let Some(author) = updater.author {
> endpoint.author = Some(author);
> }
> -
> if let Some(comment) = updater.comment {
> endpoint.comment = Some(comment);
> }
> -
> if let Some(disable) = updater.disable {
> endpoint.disable = Some(disable);
> }
> + if let Some(oauth2_client_id) = updater.oauth2_client_id {
> + endpoint.oauth2_client_id = Some(oauth2_client_id);
> + }
> + if let Some(oauth2_tenant_id) = updater.oauth2_tenant_id {
> + endpoint.oauth2_tenant_id = Some(oauth2_tenant_id);
> + }
>
> if endpoint.mailto.is_empty() && endpoint.mailto_user.is_empty() {
> http_bail!(
> @@ -170,6 +235,25 @@ pub fn update_endpoint(
> );
> }
>
> + let private_config = SmtpPrivateConfig {
> + name: name.into(),
> + password: private_endpoint_config_updater.password,
> + oauth2_client_secret:
> private_endpoint_config_updater.oauth2_client_secret,
> + };
> +
> + super::set_private_config_entry(config, private_config, SMTP_TYPENAME,
> name)?;
> +
> + update_state(
> + name,
> + Some(SmtpState {
> + oauth2_refresh_token,
> + last_refreshed: SystemTime::now()
> + .duration_since(UNIX_EPOCH)
> + .unwrap()
> + .as_secs(),
same here regarding proxmox_time::epoch_i64
> + }),
> + )?;
> +
> config
> .config
> .set_data(name, SMTP_TYPENAME, &endpoint)
> @@ -196,6 +280,7 @@ pub fn delete_endpoint(config: &mut Config, name: &str)
> -> Result<(), HttpError>
>
> super::remove_private_config_entry(config, name)?;
> config.config.sections.remove(name);
> + update_state(name, None)?;
>
> Ok(())
> }
> @@ -204,7 +289,7 @@ pub fn delete_endpoint(config: &mut Config, name: &str)
> -> Result<(), HttpError>
> pub mod tests {
> use super::*;
> use crate::api::test_helpers::*;
> - use crate::endpoints::smtp::SmtpMode;
> + use crate::endpoints::smtp::{SmtpAuthMethod, SmtpMode};
>
> pub fn add_smtp_endpoint_for_test(config: &mut Config, name: &str) ->
> Result<(), HttpError> {
> add_endpoint(
> @@ -217,6 +302,7 @@ pub mod tests {
> author: Some("root".into()),
> comment: Some("Comment".into()),
> mode: Some(SmtpMode::StartTls),
> + auth_method: Some(SmtpAuthMethod::Plain),
> server: "localhost".into(),
> port: Some(555),
> username: Some("username".into()),
> @@ -225,7 +311,9 @@ pub mod tests {
> SmtpPrivateConfig {
> name: name.into(),
> password: Some("password".into()),
> + oauth2_client_secret: None,
> },
> + None,
> )?;
>
> assert!(get_endpoint(config, name).is_ok());
> @@ -256,6 +344,7 @@ pub mod tests {
> Default::default(),
> None,
> None,
> + None,
> )
> .is_err());
>
> @@ -273,6 +362,7 @@ pub mod tests {
> Default::default(),
> Default::default(),
> None,
> + None,
> Some(&[0; 32]),
> )
> .is_err());
> @@ -304,6 +394,7 @@ pub mod tests {
> },
> Default::default(),
> None,
> + None,
> Some(&digest),
> )?;
>
> @@ -327,6 +418,7 @@ pub mod tests {
> "smtp-endpoint",
> Default::default(),
> Default::default(),
> + None,
> Some(&[
> DeleteableSmtpProperty::Author,
> DeleteableSmtpProperty::MailtoUser,
> diff --git a/proxmox-notify/src/endpoints/smtp.rs
> b/proxmox-notify/src/endpoints/smtp.rs
> index 69c4048c..b7194fff 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};
>
> @@ -80,11 +83,21 @@ pub struct SmtpConfig {
> pub port: Option<u16>,
> #[serde(skip_serializing_if = "Option::is_none")]
> pub mode: Option<SmtpMode>,
> + /// Method to be used for authentication.
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub auth_method: Option<SmtpAuthMethod>,
> /// Username to use during authentication.
> /// If no username is set, no authentication will be performed.
> /// The PLAIN and LOGIN authentication methods are supported
> #[serde(skip_serializing_if = "Option::is_none")]
> pub username: Option<String>,
> + /// Client ID for XOAUTH2 authentication method.
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub oauth2_client_id: Option<String>,
> + /// Tenant ID for XOAUTH2 authentication method. Only required for
> + /// Microsoft Exchange Online OAuth2.
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub oauth2_tenant_id: Option<String>,
> /// Mail address to send a mail to.
> #[serde(default, skip_serializing_if = "Vec::is_empty")]
> #[updater(serde(skip_serializing_if = "Option::is_none"))]
> @@ -131,12 +144,39 @@ pub enum DeleteableSmtpProperty {
> MailtoUser,
> /// Delete `password`
> Password,
> + /// Delete `auth_method`
> + AuthMethod,
> + /// Delete `oauth2_client_id`
> + #[serde(rename = "oauth2-client-id")]
> + OAuth2ClientId,
> + /// Delete `oauth2_client_secret`
> + #[serde(rename = "oauth2-client-secret")]
> + OAuth2ClientSecret,
> + /// Delete `oauth2_tenant_id`
> + #[serde(rename = "oauth2-tenant-id")]
> + OAuth2TenantId,
> /// Delete `port`
> Port,
> /// Delete `username`
> Username,
> }
>
> +/// Authentication mode to use for SMTP.
> +#[api]
> +#[derive(Serialize, Deserialize, Clone, Debug, Default)]
You could also derive `Copy` here, then you don't need to clone it in
some places in the code.
> +#[serde(rename_all = "kebab-case")]
> +pub enum SmtpAuthMethod {
> + /// Username + password
> + #[default]
> + Plain,
> + /// Google OAuth2
> + #[serde(rename = "google-oauth2")]
> + GoogleOAuth2,
> + /// Microsoft OAuth2
> + #[serde(rename = "microsoft-oauth2")]
> + MicrosoftOAuth2,
> +}
> +
> #[derive(Serialize, Deserialize, Clone, Debug, Default)]
> #[serde(rename_all = "kebab-case")]
> pub struct SmtpState {
> @@ -157,9 +197,14 @@ pub struct SmtpPrivateConfig {
> /// Name of the endpoint
> #[updater(skip)]
> pub name: String,
> +
> /// The password to use during authentication.
> #[serde(skip_serializing_if = "Option::is_none")]
> pub password: Option<String>,
> +
> + /// OAuth2 client secret
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub oauth2_client_secret: Option<String>,
> }
>
> /// A sendmail notification endpoint.
> @@ -168,6 +213,60 @@ pub struct SmtpEndpoint {
> pub private_config: SmtpPrivateConfig,
> }
>
> +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())),
> + }
> + }
> +
> + /// Infer the auth method based on the presence of a password field in
> the private config.
> + ///
> + /// This is required for backwards compatibility for configs created
> before the `auth_method`
> + /// field was added, i.e., the presence of a password implicitly meant
> plain authentication
> + /// was to be used.
> + fn auth_method(&self) -> Option<SmtpAuthMethod> {
> + self.config.auth_method.clone().or_else(|| {
> + if self.private_config.password.is_some() {
> + Some(SmtpAuthMethod::Plain)
> + } else {
> + None
> + }
> + })
> + }
> +}
> +
> impl Endpoint for SmtpEndpoint {
> fn send(&self, notification: &Notification, state: &mut State) ->
> Result<(), Error> {
> let mut endpoint_state =
> state.get_or_default::<SmtpState>(self.name())?;
> @@ -190,23 +289,58 @@ impl Endpoint for SmtpEndpoint {
> }
> };
>
> - let mut transport_builder =
> SmtpTransport::builder_dangerous(&self.config.server)
> + let transport_builder =
> SmtpTransport::builder_dangerous(&self.config.server)
> .tls(tls)
> .port(port)
> .timeout(Some(Duration::from_secs(SMTP_TIMEOUT.into())));
>
> - if let Some(username) = self.config.username.as_deref() {
> - if let Some(password) = self.private_config.password.as_deref() {
> - transport_builder = transport_builder.credentials((username,
> password).into());
> - } else {
> - return Err(Error::NotifyFailed(
> - self.name().into(),
> - Box::new(Error::Generic(
> - "username is set but no password was
> provided".to_owned(),
> - )),
> - ));
> + let transport_builder = match &self.auth_method() {
> + None => transport_builder,
> + Some(SmtpAuthMethod::Plain) => match (
> + self.config.username.as_deref(),
> + self.private_config.password.as_deref(),
> + ) {
> + (Some(username), Some(password)) => {
> + transport_builder.credentials((username,
> password).into())
> + }
> + (Some(_), None) => {
> + return Err(Error::NotifyFailed(
> + self.name().into(),
> + Box::new(Error::Generic(
> + "username is set but no password was
> provided".to_owned(),
> + )),
> + ))
> + }
> + _ => transport_builder,
> + },
> + Some(method) => {
> + let token_exchange_result = self.get_access_token(
> + endpoint_state
> + .oauth2_refresh_token
> + .as_ref()
> + .ok_or(Error::NotifyFailed(
> + self.name().into(),
> + Box::new(Error::Generic("no refresh token found
> for endpoint".into())),
> + ))?,
> + method,
> + )?;
> +
> + if let Some(new_refresh_token) =
> token_exchange_result.refresh_token {
> + endpoint_state.oauth2_refresh_token =
> Some(new_refresh_token.into_secret());
> + }
> + endpoint_state.last_refreshed = SystemTime::now()
> + .duration_since(UNIX_EPOCH)
> + .unwrap()
> + .as_secs();
Same here regarding proxmox_time::epoch_i64
> +
> + transport_builder
> + .credentials(Credentials::new(
> + self.config.from_address.clone(),
> + token_exchange_result.access_token.into_secret(),
> + ))
> + .authentication(vec![Mechanism::Xoauth2])
> }
> - }
> + };
>
> let transport = transport_builder.build();
I think it could be nice to move everything above this line to a
separate method `build_smtp_tansport` - this method is getting rather
long and this part is a very distinct step of the things performed here.
>
> @@ -298,6 +432,56 @@ impl Endpoint for SmtpEndpoint {
> fn disabled(&self) -> bool {
> self.config.disable.unwrap_or_default()
> }
> +
> + fn refresh_state(&self, state: &mut State) -> Result<bool, Error> {
> + let endpoint_state = match state.get::<SmtpState>(self.name())? {
> + None => return Ok(false),
> + Some(s) => s,
> + };
> +
> + let Some(refresh_token) = endpoint_state.oauth2_refresh_token else {
> + return Ok(false);
> + };
> +
> + // 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(endpoint_state.last_refreshed))
> + .map_err(|e| Error::Generic(e.to_string()))?
> + < Duration::from_secs(60 * 60 * 12)
same here regarding proxmox_time::epoch_i64
Also the cut-off duration should be const, not a magic number
> + {
> + return Ok(false);
> + }
> +
> + let Some(auth_method) = self.config.auth_method.as_ref() else {
> + return Ok(false);
> + };
> +
> + let new_state = match self
> + .get_access_token(&refresh_token, auth_method)?
> + .refresh_token
> + {
> + // Microsoft OAuth2, new token was returned
> + Some(new_refresh_token) => SmtpState {
> + oauth2_refresh_token: Some(new_refresh_token.into_secret()),
> + last_refreshed: SystemTime::now()
> + .duration_since(UNIX_EPOCH)
> + .unwrap()
> + .as_secs(),
> + },
> + // Google OAuth2, refresh token's lifetime was extended
> + None => SmtpState {
> + oauth2_refresh_token: Some(refresh_token),
> + last_refreshed: SystemTime::now()
> + .duration_since(UNIX_EPOCH)
> + .unwrap()
> + .as_secs(),
> + },
> + };
> +
> + state.set(self.name(), &new_state)?;
> + Ok(true)
It seems like you return Ok(true) in case that the state changed (token
was refreshed) and Ok(false) if nothing changed, is this correct?
The boolean parameter should be documented in the trait doc-comments.
Also at the moment you don't seem to use the boolean part anywhere? I
guess we could use it to determine whether we need to replace the
existing state file at all (we don't if all endpoints returned `false`).
> + }
> }
>
> /// Construct a lettre `Message` from a raw email message.
