* [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets
@ 2026-02-04 16:13 Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 1/5] notify: Introduce xoauth2 module Arthur Bied-Charreton
` (15 more replies)
0 siblings, 16 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Sending this as an RFC to get early feedback on the overall direction.
This series adds OAuth2 support for SMTP notification targets, motivated by Microsoft's
upcoming deprecation of basic authentication for SMTP [1]. Google and Microsoft are supported as
OAuth2 providers.
The main architectural decisions are:
- OAuth2 refresh tokens are treated as state, not config. They are persisted in a separate JSON file and managed
entirely from the Rust side via standard I/O.
- The oauth2 crate is used with a local ureq backend (newtype over ureq::Agent), since the upstream ureq feature
is currently patched out in Debian due to a ureq 2/3 version mismatch [2].
- Token refresh is triggered both proactively via pveupdate and when sending a notification to handle idle periods
and providers like Microsoft that rotate refresh tokens on every use.
Known issues:
- Microsoft is untested (no test tenant, somehow impossible to create a free test account)
[1] https://techcommunity.microsoft.com/blog/exchange/updated-exchange-online-smtp-auth-basic-authentication-deprecation-timeline/4489835
[2] https://git.proxmox.com/?p=debcargo-conf.git;a=blob;f=src/oauth2/debian/patches/disable-ureq.patch;h=828b883a83a86927c5cd32df055226a5e78e8bea;hb=refs/heads/proxmox/trixie
proxmox:
Arthur Bied-Charreton (5):
notify: Introduce xoauth2 module
notify: Add state file handling
notify: Update Endpoint trait and Bus to use State
notify: smtp: add OAuth2/XOAUTH2 authentication support
notify: Add test for State
proxmox-notify/Cargo.toml | 5 +
proxmox-notify/debian/control | 12 +-
proxmox-notify/src/api/common.rs | 70 ++++++-
proxmox-notify/src/api/smtp.rs | 144 +++++++++++---
proxmox-notify/src/context/mod.rs | 2 +
proxmox-notify/src/context/pbs.rs | 4 +
proxmox-notify/src/context/pve.rs | 4 +
proxmox-notify/src/context/test.rs | 4 +
proxmox-notify/src/endpoints/gotify.rs | 4 +-
proxmox-notify/src/endpoints/sendmail.rs | 4 +-
proxmox-notify/src/endpoints/smtp.rs | 227 +++++++++++++++++++++--
proxmox-notify/src/endpoints/webhook.rs | 4 +-
proxmox-notify/src/lib.rs | 157 ++++++++++++++--
proxmox-notify/src/xoauth2.rs | 146 +++++++++++++++
14 files changed, 718 insertions(+), 69 deletions(-)
create mode 100644 proxmox-notify/src/xoauth2.rs
proxmox-perl-rs:
Arthur Bied-Charreton (1):
notify: update bindings with new OAuth2 parameters
common/src/bindings/notify.rs | 44 +++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
proxmox-widget-toolkit:
Arthur Bied-Charreton (2):
utils: Add OAuth2 flow handlers
notifications: Add opt-in OAuth2 support for SMTP targets
src/Utils.js | 84 +++++++++++++++
src/panel/SmtpEditPanel.js | 191 +++++++++++++++++++++++++++++++--
src/window/EndpointEditBase.js | 1 +
3 files changed, 265 insertions(+), 11 deletions(-)
pve-manager:
Arthur Bied-Charreton (5):
notifications: Add OAuth2 parameters to schema and add/update
endpoints
notifications: Add refresh-targets endpoint
notifications: Trigger notification target refresh in pveupdate
notifications: Handle OAuth2 callback in login handler
notifications: Opt into OAuth2 authentication
PVE/API2/Cluster/Notifications.pm | 89 +++++++++++++++++++++++++++++++
bin/pveupdate | 9 ++++
www/manager6/Utils.js | 10 ++++
www/manager6/Workspace.js | 20 +++++++
4 files changed, 128 insertions(+)
pve-cluster:
Arthur Bied-Charreton (1):
notifications: Add refresh_targets subroutine to PVE::Notify
src/PVE/Notify.pm | 6 ++++++
1 file changed, 6 insertions(+)
pve-docs:
Arthur Bied-Charreton (1):
notifications: Add section about OAuth2 to SMTP targets docs
notifications.adoc | 44 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
Summary over all repositories:
24 files changed, 1197 insertions(+), 88 deletions(-)
--
Generated by murpp 0.9.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox 1/5] notify: Introduce xoauth2 module
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-06 15:00 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH proxmox 2/5] notify: Add state file handling Arthur Bied-Charreton
` (14 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Prepare proxmox-notify to use the oauth2 for SMTP XOAUTH2 support.
The xoauth2 module handles some of the implementation details related to
supporting XOAUTH2 for SMTP notification targets.
* Add a ureq::Agent newtype wrapper implementing the SyncHttpClient
trait to allow using ureq as oauth2 backend, since OAuth2 dropped the
ureq feature. Debian seems to have patched it out due to a ureq 2/3
version
mismatch [1].
* Add get_{google,microsoft}_token functions
[1]
https://git.proxmox.com/?p=debcargo-conf.git;a=blob;f=src/oauth2/debian/patches/disable-ureq.patch;h=828b883a83a86927c5cd32df055226a5e78e8bea;hb=refs/heads/proxmox/trixie
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
proxmox-notify/Cargo.toml | 4 +
proxmox-notify/debian/control | 10 ++-
proxmox-notify/src/lib.rs | 1 +
proxmox-notify/src/xoauth2.rs | 146 ++++++++++++++++++++++++++++++++++
4 files changed, 159 insertions(+), 2 deletions(-)
create mode 100644 proxmox-notify/src/xoauth2.rs
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index bc63e19d..52493ef7 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
lettre = { workspace = true, optional = true }
tracing.workspace = true
mail-parser = { workspace = true, optional = true }
+oauth2 = { version = "5.0.0" }
+ureq = { version = "3.0.11", features = ["platform-verifier"] }
+
+
openssl.workspace = true
percent-encoding = { workspace = true, optional = true }
regex.workspace = true
diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
index e588e485..7770f5ee 100644
--- a/proxmox-notify/debian/control
+++ b/proxmox-notify/debian/control
@@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
librust-handlebars-5+default-dev <!nocheck>,
librust-http-1+default-dev <!nocheck>,
librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
+ librust-oauth2-5+default-dev <!nocheck>,
librust-openssl-0.10+default-dev <!nocheck>,
librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
librust-proxmox-base64-1+default-dev <!nocheck>,
@@ -33,7 +34,9 @@ Build-Depends-Arch: cargo:native <!nocheck>,
librust-serde-1+default-dev <!nocheck>,
librust-serde-1+derive-dev <!nocheck>,
librust-serde-json-1+default-dev <!nocheck>,
- librust-tracing-0.1+default-dev <!nocheck>
+ librust-tracing-0.1+default-dev <!nocheck>,
+ librust-ureq-3+default-dev (>= 3.0.11-~~) <!nocheck>,
+ librust-ureq-3+platform-verifier-dev (>= 3.0.11-~~) <!nocheck>
Maintainer: Proxmox Support Team <support@proxmox.com>
Standards-Version: 4.7.2
Vcs-Git: git://git.proxmox.com/git/proxmox.git
@@ -49,6 +52,7 @@ Depends:
librust-anyhow-1+default-dev,
librust-const-format-0.2+default-dev,
librust-handlebars-5+default-dev,
+ librust-oauth2-5+default-dev,
librust-openssl-0.10+default-dev,
librust-proxmox-http-error-1+default-dev,
librust-proxmox-human-byte-1+default-dev,
@@ -65,7 +69,9 @@ Depends:
librust-serde-1+default-dev,
librust-serde-1+derive-dev,
librust-serde-json-1+default-dev,
- librust-tracing-0.1+default-dev
+ librust-tracing-0.1+default-dev,
+ librust-ureq-3+default-dev (>= 3.0.11-~~),
+ librust-ureq-3+platform-verifier-dev (>= 3.0.11-~~)
Recommends:
librust-proxmox-notify+default-dev (= ${binary:Version})
Suggests:
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 879f8326..1134027c 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -24,6 +24,7 @@ pub mod context;
pub mod endpoints;
pub mod renderer;
pub mod schema;
+pub mod xoauth2;
#[derive(Debug)]
pub enum Error {
diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
new file mode 100644
index 00000000..66faabfa
--- /dev/null
+++ b/proxmox-notify/src/xoauth2.rs
@@ -0,0 +1,146 @@
+use oauth2::{
+ basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
+ TokenUrl,
+};
+
+use crate::Error;
+
+/// `oauth2` dropped support for the `ureq` backend, this newtype circumvents this
+/// by implementing the `SyncHttpClient` trait. This allows us to avoid pulling in
+/// a different backend like `reqwest`.
+pub struct UreqSyncHttpClient(ureq::Agent);
+
+impl Default for UreqSyncHttpClient {
+ /// Set `max_redirects` to 0 to prevent SSRF, see
+ /// https://docs.rs/oauth2/latest/oauth2/#security-warning
+ fn default() -> Self {
+ Self(ureq::Agent::new_with_config(
+ ureq::Agent::config_builder().max_redirects(0).build(),
+ ))
+ }
+}
+
+impl oauth2::SyncHttpClient for UreqSyncHttpClient {
+ type Error = oauth2::HttpClientError<ureq::Error>;
+
+ fn call(&self, request: oauth2::HttpRequest) -> Result<oauth2::HttpResponse, Self::Error> {
+ let uri = request.uri().to_string();
+
+ let response = match request.method() {
+ &http::Method::POST => {
+ let req = request
+ .headers()
+ .iter()
+ .fold(self.0.post(&uri), |req, (name, value)| {
+ req.header(name, value)
+ });
+ req.send(request.body()).map_err(Box::new)?
+ }
+ &http::Method::GET => {
+ let req = request
+ .headers()
+ .iter()
+ .fold(self.0.get(&uri), |req, (name, value)| {
+ req.header(name, value)
+ });
+ req.call().map_err(Box::new)?
+ }
+ m => {
+ return Err(oauth2::HttpClientError::Other(format!(
+ "unexpected method: {m}"
+ )));
+ }
+ };
+
+ let mut builder = http::Response::builder().status(response.status());
+
+ if let Some(content_type) = response.headers().get(http::header::CONTENT_TYPE) {
+ builder = builder.header(http::header::CONTENT_TYPE, content_type);
+ }
+
+ let (_, mut body) = response.into_parts();
+
+ let body = body.read_to_vec().map_err(Box::new)?;
+
+ builder.body(body).map_err(oauth2::HttpClientError::Http)
+ }
+}
+
+pub struct TokenExchangeResult {
+ pub access_token: AccessToken,
+ pub refresh_token: Option<RefreshToken>,
+}
+
+/// Microsoft Identity Platform refresh tokens replace themselves
+/// upon every use, however the old one does *not* get revoked.
+///
+/// This means that every access token request yields both an access
+/// token AND a new refresh token, which should replace the old one.
+/// The old one should then be discarded.
+///
+/// https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
+pub fn get_microsoft_token(
+ client_id: ClientId,
+ client_secret: ClientSecret,
+ tenant_id: &str,
+ refresh_token: RefreshToken,
+) -> Result<TokenExchangeResult, Error> {
+ let client = BasicClient::new(client_id)
+ .set_client_secret(client_secret)
+ .set_auth_uri(
+ AuthUrl::new("https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into())
+ .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
+ )
+ .set_token_uri(
+ TokenUrl::new(format!(
+ "https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token"
+ ))
+ .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
+ );
+
+ let token_result = client
+ .exchange_refresh_token(&refresh_token)
+ .request(&UreqSyncHttpClient::default())
+ .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
+
+ Ok(TokenExchangeResult {
+ access_token: token_result.access_token().clone(),
+ refresh_token: token_result.refresh_token().cloned(),
+ })
+}
+
+/// Google refresh tokens' TTL is extended at every use. As long as
+/// a token has been used at least once in the past 6 months, and no
+/// other expiration reason applies, we can keep the same token.
+///
+/// To make sure the token does not expire, it should be enough to periodically
+/// make an access token request. If the token becomes invalid for whatever
+/// other reason, we need user intervention to get a new one.
+///
+/// https://developers.google.com/identity/protocols/oauth2#expiration
+pub fn get_google_token(
+ client_id: ClientId,
+ client_secret: ClientSecret,
+ refresh_token: RefreshToken,
+) -> Result<TokenExchangeResult, Error> {
+ let client = BasicClient::new(client_id)
+ .set_client_secret(client_secret)
+ .set_auth_uri(
+ AuthUrl::new("https://accounts.google.com/o/oauth2/v2/auth".into())
+ .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
+ )
+ .set_token_uri(
+ TokenUrl::new("https://oauth2.googleapis.com/token".into())
+ .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
+ );
+
+ let token_result = client
+ .exchange_refresh_token(&refresh_token)
+ .request(&UreqSyncHttpClient::default())
+ .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
+
+ Ok(TokenExchangeResult {
+ access_token: token_result.access_token().clone(),
+ refresh_token: token_result.refresh_token().cloned(),
+ })
+}
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox 2/5] notify: Add state file handling
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 1/5] notify: Introduce xoauth2 module Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-10 15:51 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State Arthur Bied-Charreton
` (13 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Add State struct abstracting state file deserialization, updates and
persistence, as well as an EndpointState marker trait stateful endpoints
may implement.
Also add a state_file_path method to the crate's Context trait, which
allows
tests to build their own context instead of depending on statics.
As far as SMTP endpoints are concerned, file locks are not necessary.
Old Microsoft tokens stay valid for 90 days after refreshes [1], and
Google
tokens' lifetime is just extended at every use [2], so concurrent reads
should not
be an issue here.
[1]
https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
[2]
https://stackoverflow.com/questions/8953983/do-google-refresh-tokens-expire
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
proxmox-notify/Cargo.toml | 1 +
proxmox-notify/debian/control | 2 +
proxmox-notify/src/context/mod.rs | 2 +
proxmox-notify/src/context/pbs.rs | 4 ++
proxmox-notify/src/context/pve.rs | 4 ++
proxmox-notify/src/context/test.rs | 4 ++
proxmox-notify/src/lib.rs | 60 ++++++++++++++++++++++++++++++
7 files changed, 77 insertions(+)
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 52493ef7..daa10296 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -40,6 +40,7 @@ proxmox-sendmail = { workspace = true, optional = true }
proxmox-sys = { workspace = true, optional = true }
proxmox-time.workspace = true
proxmox-uuid = { workspace = true, features = ["serde"] }
+nix.workspace = true
[features]
default = ["sendmail", "gotify", "smtp", "webhook"]
diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
index 7770f5ee..76b8a1fa 100644
--- a/proxmox-notify/debian/control
+++ b/proxmox-notify/debian/control
@@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
librust-handlebars-5+default-dev <!nocheck>,
librust-http-1+default-dev <!nocheck>,
librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
+ librust-nix-0.29+default-dev <!nocheck>,
librust-oauth2-5+default-dev <!nocheck>,
librust-openssl-0.10+default-dev <!nocheck>,
librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
@@ -52,6 +53,7 @@ Depends:
librust-anyhow-1+default-dev,
librust-const-format-0.2+default-dev,
librust-handlebars-5+default-dev,
+ librust-nix-0.29+default-dev,
librust-oauth2-5+default-dev,
librust-openssl-0.10+default-dev,
librust-proxmox-http-error-1+default-dev,
diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
index 8b6e2c43..86130409 100644
--- a/proxmox-notify/src/context/mod.rs
+++ b/proxmox-notify/src/context/mod.rs
@@ -32,6 +32,8 @@ pub trait Context: Send + Sync + Debug {
namespace: Option<&str>,
source: TemplateSource,
) -> Result<Option<String>, Error>;
+ /// Return the state file, or None if no state file exists for this context.
+ fn state_file_path(&self) -> &'static str;
}
#[cfg(not(test))]
diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
index 3e5da59c..67010060 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -125,6 +125,10 @@ impl Context for PBSContext {
.map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
Ok(template_string)
}
+
+ fn state_file_path(&self) -> &'static str {
+ "/etc/proxmox-backup/notifications.state.json"
+ }
}
#[cfg(test)]
diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
index a97cce26..0dffbb11 100644
--- a/proxmox-notify/src/context/pve.rs
+++ b/proxmox-notify/src/context/pve.rs
@@ -74,6 +74,10 @@ impl Context for PVEContext {
.map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
Ok(template_string)
}
+
+ fn state_file_path(&self) -> &'static str {
+ "/etc/pve/priv/notifications.state.json"
+ }
}
pub static PVE_CONTEXT: PVEContext = PVEContext;
diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
index 2c236b4c..e0236b9c 100644
--- a/proxmox-notify/src/context/test.rs
+++ b/proxmox-notify/src/context/test.rs
@@ -40,4 +40,8 @@ impl Context for TestContext {
) -> Result<Option<String>, Error> {
Ok(Some(String::new()))
}
+
+ fn state_file_path(&self) -> &'static str {
+ "/tmp/notifications.state.json"
+ }
}
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 1134027c..a40342cc 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -6,6 +6,7 @@ use std::fmt::Display;
use std::str::FromStr;
use context::context;
+use serde::de::DeserializeOwned;
use serde::{Deserialize, Serialize};
use serde_json::json;
use serde_json::Value;
@@ -272,6 +273,65 @@ impl Notification {
}
}
+#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)]
+pub struct State {
+ #[serde(flatten)]
+ pub sections: HashMap<String, Value>,
+}
+
+impl FromStr for State {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ serde_json::from_str(s).map_err(|e| Error::ConfigDeserialization(e.into()))
+ }
+}
+
+/// Marker trait to be implemented by the state structs for stateful endpoints.
+pub trait EndpointState: Serialize + DeserializeOwned + Default {}
+
+impl State {
+ pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Result<Self, Error> {
+ let contents = proxmox_sys::fs::file_read_string(path)
+ .map_err(|e| Error::ConfigDeserialization(e.into()))?;
+ Self::from_str(&contents)
+ }
+
+ pub fn persist<P: AsRef<std::path::Path>>(&self, path: P) -> Result<(), Error> {
+ let state_str =
+ serde_json::to_string_pretty(self).map_err(|e| Error::ConfigSerialization(e.into()))?;
+
+ let mode = nix::sys::stat::Mode::from_bits_truncate(0o600);
+ let options = proxmox_sys::fs::CreateOptions::new().perm(mode);
+
+ proxmox_sys::fs::replace_file(path, state_str.as_bytes(), options, true)
+ .map_err(|e| Error::ConfigSerialization(e.into()))
+ }
+
+ pub fn get<S: EndpointState>(&self, name: &str) -> Result<Option<S>, Error> {
+ match self.sections.get(name) {
+ Some(v) => Ok(Some(
+ S::deserialize(v).map_err(|e| Error::ConfigDeserialization(e.into()))?,
+ )),
+ None => Ok(None),
+ }
+ }
+
+ pub fn get_or_default<S: EndpointState>(&self, name: &str) -> Result<S, Error> {
+ Ok(self.get(name)?.unwrap_or_default())
+ }
+
+ pub fn set<S: EndpointState>(&mut self, name: &str, state: &S) -> Result<(), Error> {
+ let v = serde_json::to_value(state).map_err(|e| Error::ConfigSerialization(e.into()))?;
+ self.sections.insert(name.to_string(), v);
+ Ok(())
+ }
+
+ pub fn remove(&mut self, name: &str) {
+ self.sections.remove(name);
+ }
+}
+
/// Notification configuration
#[derive(Debug, Clone)]
pub struct Config {
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 1/5] notify: Introduce xoauth2 module Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 2/5] notify: Add state file handling Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-10 15:52 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support Arthur Bied-Charreton
` (12 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
This lays the groundwork for handling OAuth2 state in proxmox-notify by
updating the crate's internal API to pass around a mutable State struct.
The state can be updated by its consumers and will be persisted
afterwards, much like the Config struct, with the difference that it is
fully internal to the crate. External consumers of the API do not need
to know/care about it.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
proxmox-notify/src/api/common.rs | 70 +++++++++++++++++++++---
proxmox-notify/src/endpoints/gotify.rs | 4 +-
proxmox-notify/src/endpoints/sendmail.rs | 4 +-
proxmox-notify/src/endpoints/smtp.rs | 17 +++++-
proxmox-notify/src/endpoints/webhook.rs | 4 +-
proxmox-notify/src/lib.rs | 42 +++++++++-----
6 files changed, 113 insertions(+), 28 deletions(-)
diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs
index fa2356e2..3483be9a 100644
--- a/proxmox-notify/src/api/common.rs
+++ b/proxmox-notify/src/api/common.rs
@@ -1,7 +1,52 @@
use proxmox_http_error::HttpError;
use super::http_err;
-use crate::{Bus, Config, Notification};
+use crate::{context::context, Bus, Config, Notification, State};
+
+use tracing::error;
+
+fn get_state() -> State {
+ let path_str = context().state_file_path();
+ let path = std::path::Path::new(path_str);
+
+ match path.exists() {
+ true => match State::from_path(path) {
+ Ok(s) => s,
+ Err(e) => {
+ // We don't want to block notifications on all endpoints only
+ // because the stateful ones are broken.
+ error!("could not instantiate state from {path_str}: {e}",);
+ State::default()
+ }
+ },
+ false => State::default(),
+ }
+}
+
+fn persist_state(state: &State) {
+ let path_str = context().state_file_path();
+
+ if let Err(e) = state.persist(path_str) {
+ error!("could not persist state at '{path_str}': {e}");
+ }
+}
+
+pub fn refresh_targets(config: &Config) -> Result<(), HttpError> {
+ let bus = Bus::from_config(config).map_err(|err| {
+ http_err!(
+ INTERNAL_SERVER_ERROR,
+ "Could not instantiate notification bus: {err}"
+ )
+ })?;
+
+ let mut state = get_state();
+
+ bus.refresh_targets(&mut state);
+
+ persist_state(&state);
+
+ Ok(())
+}
/// Send a notification to a given target.
///
@@ -15,7 +60,11 @@ pub fn send(config: &Config, notification: &Notification) -> Result<(), HttpErro
)
})?;
- bus.send(notification);
+ let mut state = get_state();
+
+ bus.send(notification, &mut state);
+
+ persist_state(&state);
Ok(())
}
@@ -32,12 +81,17 @@ pub fn test_target(config: &Config, endpoint: &str) -> Result<(), HttpError> {
)
})?;
- bus.test_target(endpoint).map_err(|err| match err {
- crate::Error::TargetDoesNotExist(endpoint) => {
- http_err!(NOT_FOUND, "endpoint '{endpoint}' does not exist")
- }
- _ => http_err!(INTERNAL_SERVER_ERROR, "Could not test target: {err}"),
- })?;
+ let mut state = get_state();
+
+ bus.test_target(endpoint, &mut state)
+ .map_err(|err| match err {
+ crate::Error::TargetDoesNotExist(endpoint) => {
+ http_err!(NOT_FOUND, "endpoint '{endpoint}' does not exist")
+ }
+ _ => http_err!(INTERNAL_SERVER_ERROR, "Could not test target: {err}"),
+ })?;
+
+ persist_state(&state);
Ok(())
}
diff --git a/proxmox-notify/src/endpoints/gotify.rs b/proxmox-notify/src/endpoints/gotify.rs
index 3e12a60e..5f48fc0a 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -12,7 +12,7 @@ use proxmox_schema::{api, Updater};
use crate::context::context;
use crate::renderer::TemplateType;
use crate::schema::ENTITY_NAME_SCHEMA;
-use crate::{renderer, Content, Endpoint, Error, Notification, Origin, Severity};
+use crate::{renderer, Content, Endpoint, Error, Notification, Origin, Severity, State};
const HTTP_TIMEOUT: Duration = Duration::from_secs(10);
@@ -96,7 +96,7 @@ pub enum DeleteableGotifyProperty {
}
impl Endpoint for GotifyEndpoint {
- fn send(&self, notification: &Notification) -> Result<(), Error> {
+ fn send(&self, notification: &Notification, _: &mut State) -> Result<(), Error> {
let (title, message) = match ¬ification.content {
Content::Template {
template_name,
diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index 70b0f111..d95a6872 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -4,10 +4,10 @@ use serde::{Deserialize, Serialize};
use proxmox_schema::api_types::COMMENT_SCHEMA;
use proxmox_schema::{api, Updater};
-use crate::context;
use crate::endpoints::common::mail;
use crate::renderer::TemplateType;
use crate::schema::{EMAIL_SCHEMA, ENTITY_NAME_SCHEMA, USER_SCHEMA};
+use crate::{context, State};
use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
pub(crate) const SENDMAIL_TYPENAME: &str = "sendmail";
@@ -104,7 +104,7 @@ pub struct SendmailEndpoint {
}
impl Endpoint for SendmailEndpoint {
- fn send(&self, notification: &Notification) -> Result<(), Error> {
+ fn send(&self, notification: &Notification, _: &mut State) -> Result<(), Error> {
let recipients = mail::get_recipients(
self.config.mailto.as_slice(),
self.config.mailto_user.as_slice(),
diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index c888dee7..69c4048c 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -15,6 +15,7 @@ use crate::endpoints::common::mail;
use crate::renderer::TemplateType;
use crate::schema::{EMAIL_SCHEMA, ENTITY_NAME_SCHEMA, USER_SCHEMA};
use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
+use crate::{xoauth2, EndpointState, State};
pub(crate) const SMTP_TYPENAME: &str = "smtp";
@@ -136,6 +137,16 @@ pub enum DeleteableSmtpProperty {
Username,
}
+#[derive(Serialize, Deserialize, Clone, Debug, Default)]
+#[serde(rename_all = "kebab-case")]
+pub struct SmtpState {
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub oauth2_refresh_token: Option<String>,
+ pub last_refreshed: u64,
+}
+
+impl EndpointState for SmtpState {}
+
#[api]
#[derive(Serialize, Deserialize, Clone, Updater, Debug)]
#[serde(rename_all = "kebab-case")]
@@ -158,7 +169,9 @@ pub struct SmtpEndpoint {
}
impl Endpoint for SmtpEndpoint {
- fn send(&self, notification: &Notification) -> Result<(), Error> {
+ fn send(&self, notification: &Notification, state: &mut State) -> Result<(), Error> {
+ let mut endpoint_state = state.get_or_default::<SmtpState>(self.name())?;
+
let tls_parameters = TlsParameters::new(self.config.server.clone())
.map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?;
@@ -272,6 +285,8 @@ impl Endpoint for SmtpEndpoint {
.send(&email)
.map_err(|err| Error::NotifyFailed(self.name().into(), err.into()))?;
+ state.set(self.name(), &endpoint_state)?;
+
Ok(())
}
diff --git a/proxmox-notify/src/endpoints/webhook.rs b/proxmox-notify/src/endpoints/webhook.rs
index 0167efcf..ce5bddf4 100644
--- a/proxmox-notify/src/endpoints/webhook.rs
+++ b/proxmox-notify/src/endpoints/webhook.rs
@@ -27,7 +27,7 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, StringSchema, Update
use crate::context::context;
use crate::renderer::TemplateType;
use crate::schema::ENTITY_NAME_SCHEMA;
-use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
+use crate::{renderer, Content, Endpoint, Error, Notification, Origin, State};
/// This will be used as a section type in the public/private configuration file.
pub(crate) const WEBHOOK_TYPENAME: &str = "webhook";
@@ -240,7 +240,7 @@ pub const KEY_AND_BASE64_VALUE_SCHEMA: Schema =
impl Endpoint for WebhookEndpoint {
/// Send a notification to a webhook endpoint.
- fn send(&self, notification: &Notification) -> Result<(), Error> {
+ fn send(&self, notification: &Notification, _: &mut State) -> Result<(), Error> {
let request = self.build_request(notification)?;
self.create_client()?
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index a40342cc..3bd83ce4 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -152,13 +152,18 @@ pub enum Origin {
/// Notification endpoint trait, implemented by all endpoint plugins
pub trait Endpoint {
/// Send a documentation
- fn send(&self, notification: &Notification) -> Result<(), Error>;
+ fn send(&self, notification: &Notification, state: &mut State) -> Result<(), Error>;
/// The name/identifier for this endpoint
fn name(&self) -> &str;
/// Check if the endpoint is disabled
fn disabled(&self) -> bool;
+
+ /// Refresh the endpoint's state if required.
+ fn refresh_state(&self, _: &mut State) -> Result<bool, Error> {
+ Ok(false)
+ }
}
#[derive(Debug, Clone, Serialize, Deserialize)]
@@ -599,7 +604,7 @@ impl Bus {
/// the notification.
///
/// Any errors will not be returned but only logged.
- pub fn send(&self, notification: &Notification) {
+ pub fn send(&self, notification: &Notification, state: &mut State) {
let targets = matcher::check_matches(self.matchers.as_slice(), notification);
for target in targets {
@@ -612,7 +617,7 @@ impl Bus {
continue;
}
- match endpoint.send(notification) {
+ match endpoint.send(notification, state) {
Ok(_) => {
info!("notified via target `{name}`");
}
@@ -631,7 +636,7 @@ impl Bus {
///
/// In contrast to the `send` function, this function will return
/// any errors to the caller.
- pub fn test_target(&self, target: &str) -> Result<(), Error> {
+ pub fn test_target(&self, target: &str, state: &mut State) -> Result<(), Error> {
let notification = Notification {
metadata: Metadata {
severity: Severity::Info,
@@ -647,13 +652,21 @@ impl Bus {
};
if let Some(endpoint) = self.endpoints.get(target) {
- endpoint.send(¬ification)?;
+ endpoint.send(¬ification, state)?;
} else {
return Err(Error::TargetDoesNotExist(target.to_string()));
}
Ok(())
}
+
+ pub fn refresh_targets(&self, state: &mut State) {
+ for (name, endpoint) in &self.endpoints {
+ if let Err(e) = endpoint.refresh_state(state) {
+ error!("could not refresh state for endpoint '{name}': {e}");
+ }
+ }
+ }
}
#[cfg(test)]
@@ -671,7 +684,7 @@ mod tests {
}
impl Endpoint for MockEndpoint {
- fn send(&self, message: &Notification) -> Result<(), Error> {
+ fn send(&self, message: &Notification, _: &mut State) -> Result<(), Error> {
self.messages.borrow_mut().push(message.clone());
Ok(())
@@ -714,12 +727,15 @@ mod tests {
bus.add_matcher(matcher);
// Send directly to endpoint
- bus.send(&Notification::from_template(
- Severity::Info,
- "test",
- Default::default(),
- Default::default(),
- ));
+ bus.send(
+ &Notification::from_template(
+ Severity::Info,
+ "test",
+ Default::default(),
+ Default::default(),
+ ),
+ &mut State::default(),
+ );
let messages = mock.messages();
assert_eq!(messages.len(), 1);
@@ -758,7 +774,7 @@ mod tests {
Default::default(),
);
- bus.send(¬ification);
+ bus.send(¬ification, &mut State::default());
};
send_with_severity(Severity::Info);
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (2 preceding siblings ...)
2026-02-04 16:13 ` [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-10 15:52 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH proxmox 5/5] notify: Add test for State Arthur Bied-Charreton
` (11 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
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 <a.bied-charreton@proxmox.com>
---
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)
+ .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(
+ config: &mut Config,
+ name: &str,
+ updater: impl FnOnce(&mut SmtpPrivateConfig),
+) -> 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(),
+ }),
+ )?;
+
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)]
+#[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();
+
+ 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();
@@ -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)
+ {
+ 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)
+ }
}
/// Construct a lettre `Message` from a raw email message.
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox 5/5] notify: Add test for State
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (3 preceding siblings ...)
2026-02-04 16:13 ` [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox-perl-rs 1/1] notify: update bindings with new OAuth2 parameters Arthur Bied-Charreton
` (10 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Add test verifying basic functionality and state persistence.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
proxmox-notify/src/lib.rs | 56 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 3bd83ce4..f4fd3aa2 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -683,10 +683,25 @@ mod tests {
messages: Rc<RefCell<Vec<Notification>>>,
}
+ #[derive(Default, Clone, Serialize, Deserialize)]
+ struct MockEndpointState {
+ notifications_sent: usize,
+ }
+
+ impl EndpointState for MockEndpointState {}
+
impl Endpoint for MockEndpoint {
- fn send(&self, message: &Notification, _: &mut State) -> Result<(), Error> {
+ fn send(&self, message: &Notification, state: &mut State) -> Result<(), Error> {
+ let endpoint_state = state.get_or_default::<MockEndpointState>(self.name)?;
self.messages.borrow_mut().push(message.clone());
+ state.set(
+ self.name,
+ &MockEndpointState {
+ notifications_sent: endpoint_state.notifications_sent + 1,
+ },
+ )?;
+
Ok(())
}
@@ -791,4 +806,43 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_endpoint_with_state() -> Result<(), Error> {
+ let mut state = State::default();
+ let mock = MockEndpoint::new("endpoint");
+
+ let mut bus = Bus::default();
+ bus.add_endpoint(Box::new(mock.clone()));
+
+ let matcher = MatcherConfig {
+ target: vec!["endpoint".into()],
+ ..Default::default()
+ };
+
+ bus.add_matcher(matcher);
+
+ // Send directly to endpoint
+ bus.send(
+ &Notification::from_template(
+ Severity::Info,
+ "test",
+ Default::default(),
+ Default::default(),
+ ),
+ &mut state,
+ );
+
+ let endpoint_state = state.get::<MockEndpointState>("endpoint")?.unwrap();
+
+ assert_eq!(endpoint_state.notifications_sent, 1);
+
+ state.persist(context().state_file_path())?;
+
+ let persisted_state = State::from_path(context().state_file_path())?;
+
+ assert_eq!(state, persisted_state);
+
+ Ok(())
+ }
}
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox-perl-rs 1/1] notify: update bindings with new OAuth2 parameters
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (4 preceding siblings ...)
2026-02-04 16:13 ` [PATCH proxmox 5/5] notify: Add test for State Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox-widget-toolkit 1/2] utils: Add OAuth2 flow handlers Arthur Bied-Charreton
` (9 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Update existing bindings for compatibility with the OAuth2-related proxmox-notify
API changes, and add binding for refresh_targets.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
common/src/bindings/notify.rs | 44 +++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/common/src/bindings/notify.rs b/common/src/bindings/notify.rs
index 409270a..7d0c54b 100644
--- a/common/src/bindings/notify.rs
+++ b/common/src/bindings/notify.rs
@@ -26,8 +26,8 @@ pub mod proxmox_rs_notify {
DeleteableSendmailProperty, SendmailConfig, SendmailConfigUpdater,
};
use proxmox_notify::endpoints::smtp::{
- DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpMode, SmtpPrivateConfig,
- SmtpPrivateConfigUpdater,
+ DeleteableSmtpProperty, SmtpAuthMethod, SmtpConfig, SmtpConfigUpdater, SmtpMode,
+ SmtpPrivateConfig, SmtpPrivateConfigUpdater,
};
use proxmox_notify::endpoints::webhook::{
DeleteableWebhookProperty, WebhookConfig, WebhookConfigUpdater,
@@ -141,6 +141,17 @@ pub mod proxmox_rs_notify {
api::common::send(&config, ¬ification)
}
+ /// Method: Refresh the state for all endpoints.
+ ///
+ /// This iterates through all configured targets, refreshing their state if needed.
+ ///
+ /// See [`api::common::refresh_targets`]
+ #[export(serialize_error)]
+ pub fn refresh_targets(#[try_from_ref] this: &NotificationConfig) -> Result<(), HttpError> {
+ let config = this.config.lock().unwrap();
+ api::common::refresh_targets(&config)
+ }
+
/// Method: Get a list of all notification targets.
///
/// See [`api::get_targets`].
@@ -396,6 +407,11 @@ pub mod proxmox_rs_notify {
mode: Option<SmtpMode>,
username: Option<String>,
password: Option<String>,
+ auth_method: Option<SmtpAuthMethod>,
+ oauth2_client_id: Option<String>,
+ oauth2_client_secret: Option<String>,
+ oauth2_tenant_id: Option<String>,
+ oauth2_refresh_token: Option<String>,
mailto: Option<Vec<String>>,
mailto_user: Option<Vec<String>>,
from_address: String,
@@ -411,7 +427,10 @@ pub mod proxmox_rs_notify {
server,
port,
mode,
+ auth_method,
username,
+ oauth2_client_id,
+ oauth2_tenant_id,
mailto: mailto.unwrap_or_default(),
mailto_user: mailto_user.unwrap_or_default(),
from_address,
@@ -420,7 +439,12 @@ pub mod proxmox_rs_notify {
disable,
origin: None,
},
- SmtpPrivateConfig { name, password },
+ SmtpPrivateConfig {
+ name,
+ password,
+ oauth2_client_secret,
+ },
+ oauth2_refresh_token,
)
}
@@ -437,6 +461,11 @@ pub mod proxmox_rs_notify {
mode: Option<SmtpMode>,
username: Option<String>,
password: Option<String>,
+ auth_method: Option<SmtpAuthMethod>,
+ oauth2_client_id: Option<String>,
+ oauth2_client_secret: Option<String>,
+ oauth2_tenant_id: Option<String>,
+ oauth2_refresh_token: Option<String>,
mailto: Option<Vec<String>>,
mailto_user: Option<Vec<String>>,
from_address: Option<String>,
@@ -457,6 +486,9 @@ pub mod proxmox_rs_notify {
port,
mode,
username,
+ auth_method,
+ oauth2_client_id,
+ oauth2_tenant_id,
mailto,
mailto_user,
from_address,
@@ -464,7 +496,11 @@ pub mod proxmox_rs_notify {
comment,
disable,
},
- SmtpPrivateConfigUpdater { password },
+ SmtpPrivateConfigUpdater {
+ password,
+ oauth2_client_secret,
+ },
+ oauth2_refresh_token,
delete.as_deref(),
digest.as_deref(),
)
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox-widget-toolkit 1/2] utils: Add OAuth2 flow handlers
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (5 preceding siblings ...)
2026-02-04 16:13 ` [PATCH proxmox-perl-rs 1/1] notify: update bindings with new OAuth2 parameters Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox-widget-toolkit 2/2] notifications: Add opt-in OAuth2 support for SMTP targets Arthur Bied-Charreton
` (8 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Introduce the Proxmox.OAuth2 singleton supporting Google and Microsoft
OAuth2. The flow is handled by opening a new window with the
authorization URL, and expects to receive the resulting authorization
code from the redirect handler via a [BroadcastChannel].
[BroadcastChannel]
https://developer.mozilla.org/en-US/docs/Web/API/BroadcastChannel
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/Utils.js | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/src/Utils.js b/src/Utils.js
index 5457ffa..f59c4a5 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -1723,6 +1723,90 @@ Ext.define('Proxmox.Utils', {
},
});
+Ext.define('Proxmox.OAuth2', {
+ singleton: true,
+
+ handleGoogleFlow: function (clientId, clientSecret) {
+ return this._handleFlow({
+ clientId,
+ clientSecret,
+ authUrl: 'https://accounts.google.com/o/oauth2/v2/auth',
+ tokenUrl: 'https://oauth2.googleapis.com/token',
+ scope: 'https://mail.google.com',
+ extraAuthParams: {
+ access_type: 'offline',
+ prompt: 'consent',
+ },
+ });
+ },
+
+ handleMicrosoftFlow: function(clientId, clientSecret, tenantId) {
+ return this._handleFlow({
+ clientId,
+ clientSecret,
+ authUrl: `https://login.microsoftonline.com/${tenantId}/oauth2/v2.0/authorize`,
+ tokenUrl: `https://login.microsoftonline.com/${tenantId}/oauth2/v2.0/token`,
+ scope: 'https://outlook.office.com/SMTP.Send offline_access',
+ extraAuthParams: {
+ prompt: 'consent',
+ },
+ });
+ },
+
+ _handleFlow: function (config) {
+ return new Promise((resolve, reject) => {
+ const redirectUri = window.location.origin;
+ const channelName = `oauth2_${crypto.randomUUID()}`;
+ const state = encodeURIComponent(JSON.stringify({ channelName }));
+
+ const authParams = new URLSearchParams({
+ client_id: config.clientId,
+ response_type: 'code',
+ redirect_uri: redirectUri,
+ scope: config.scope,
+ state,
+ ...config.extraAuthParams,
+ });
+
+ const authUrl = `${config.authUrl}?${authParams}`;
+
+ // Opens OAuth2 authentication window. The app's redirect handler must
+ // extract the authorization code from the callback URL and send it via:
+ // new BroadcastChannel(state.channelName).postMessage({ code })
+ const channel = new BroadcastChannel(channelName);
+ const popup = window.open(authUrl);
+
+ channel.addEventListener('message', async (event) => {
+ if (popup && !popup.closed) {
+ popup.close();
+ }
+ channel.close();
+
+ try {
+ const response = await fetch(config.tokenUrl, {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/x-www-form-urlencoded',
+ },
+ body: new URLSearchParams({
+ grant_type: 'authorization_code',
+ code: event.data.code,
+ client_id: config.clientId,
+ client_secret: config.clientSecret,
+ redirect_uri: redirectUri,
+ }),
+ });
+
+ const tokens = await response.json();
+ resolve(tokens.refresh_token);
+ } catch (error) {
+ reject(error);
+ }
+ });
+ })
+ }
+})
+
Ext.define('Proxmox.Async', {
singleton: true,
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH proxmox-widget-toolkit 2/2] notifications: Add opt-in OAuth2 support for SMTP targets
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (6 preceding siblings ...)
2026-02-04 16:13 ` [PATCH proxmox-widget-toolkit 1/2] utils: Add OAuth2 flow handlers Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints Arthur Bied-Charreton
` (7 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Add Google & Microsoft OAuth2 authentication methods to SMTP endpoint
config.
The enableOAuth2 pmxSmtpEditPanel config flag allows consumers to opt
into the new feature, so it can be gradually introduced into services.
When disabled, no changes are visible from the UI, and only 'None' and
'Username/Password' are shown as
authentication methods. The flag is passed from the schema config, as it
is done for defaultMailAuthor.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/panel/SmtpEditPanel.js | 191 +++++++++++++++++++++++++++++++--
src/window/EndpointEditBase.js | 1 +
2 files changed, 181 insertions(+), 11 deletions(-)
diff --git a/src/panel/SmtpEditPanel.js b/src/panel/SmtpEditPanel.js
index 37e4d51..21ae883 100644
--- a/src/panel/SmtpEditPanel.js
+++ b/src/panel/SmtpEditPanel.js
@@ -6,10 +6,24 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
type: 'smtp',
+ enableOAuth2: false,
+
+ initConfig: function (config) {
+ this.callParent(arguments);
+ this.getViewModel().set('enableOAuth2', config.enableOAuth2 || false);
+ return config;
+ },
+
viewModel: {
xtype: 'viewmodel',
data: {
+ enableOAuth2: false,
mode: 'tls',
+ authMethod: 'plain',
+ oAuth2ClientId: '',
+ oAuth2ClientSecret: '',
+ oAuth2TenantId: '',
+ oAuth2RefreshToken: '',
authentication: true,
originalAuthentication: true,
},
@@ -39,6 +53,30 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
return shouldShowUnchanged ? gettext('Unchanged') : '';
},
+ isPlainAuthentication: function (get) {
+ return get('authMethod') === 'plain';
+ },
+ isOAuth2Authentication: function (get) {
+ if (!get('enableOAuth2')) { return false };
+ const authMethod = get('authMethod');
+ return authMethod === 'google-oauth2' || authMethod === 'microsoft-oauth2';
+ },
+ isMicrosoftOAuth2Authentication: function (get) {
+ if (!get('enableOAuth2')) { return false };
+ return get('authMethod') === 'microsoft-oauth2';
+ },
+ enableAuthenticate: function (get) {
+ if (!get('enableOAuth2')) { return false };
+ const clientId = get('oAuth2ClientId');
+ const clientSecret = get('oAuth2ClientSecret');
+
+ if (get('authMethod') === 'microsoft-oauth2') {
+ const tenantId = get('oAuth2TenantId');
+ return clientId && clientId.trim() !== '' && clientSecret && clientSecret.trim() !== '' && tenantId && tenantId.trim() !== '';
+ } else {
+ return clientId && clientId.trim() !== '' && clientSecret && clientSecret.trim() !== '';
+ }
+ }
},
},
@@ -102,11 +140,25 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
],
column2: [
{
- xtype: 'proxmoxcheckbox',
- fieldLabel: gettext('Authenticate'),
- name: 'authentication',
- bind: {
- value: '{authentication}',
+ xtype: 'proxmoxKVComboBox',
+ fieldLabel: gettext('Authentication'),
+ name: 'auth-method',
+ comboItems: [
+ ['none', gettext('None')],
+ ['plain', gettext('Username/Password')],
+ ['google-oauth2', gettext('OAuth2 (Google)')],
+ ['microsoft-oauth2', gettext('OAuth2 (Microsoft)')],
+ ],
+ bind: '{authMethod}',
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ listeners: {
+ render: function () {
+ if (!this.up('pmxSmtpEditPanel').enableOAuth2) {
+ this.getStore().filter('key', /^(none|plain)$/);
+ }
+ },
},
},
{
@@ -118,7 +170,8 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
deleteEmpty: '{!isCreate}',
},
bind: {
- disabled: '{!authentication}',
+ hidden: '{!isPlainAuthentication}',
+ disabled: '{!isPlainAuthentication}',
},
},
{
@@ -130,10 +183,93 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
allowBlank: '{!isCreate}',
},
bind: {
- disabled: '{!authentication}',
+ hidden: '{!isPlainAuthentication}',
+ disabled: '{!isPlainAuthentication}',
emptyText: '{passwordEmptyText}',
},
},
+ {
+ xtype: 'proxmoxtextfield',
+ fieldLabel: gettext('Client ID'),
+ name: 'oauth2-client-id',
+ allowBlank: false,
+ bind: {
+ hidden: '{!isOAuth2Authentication}',
+ disabled: '{!isOAuth2Authentication}',
+ value: '{oAuth2ClientId}',
+ },
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
+ {
+ xtype: 'proxmoxtextfield',
+ inputType: 'password',
+ fieldLabel: gettext('Client Secret'),
+ name: 'oauth2-client-secret',
+ allowBlank: false,
+ bind: {
+ hidden: '{!isOAuth2Authentication}',
+ disabled: '{!isOAuth2Authentication}',
+ value: '{oAuth2ClientSecret}',
+ },
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
+ {
+ xtype: 'proxmoxtextfield',
+ fieldLabel: gettext('Tenant ID'),
+ name: 'oauth2-tenant-id',
+ allowBlank: false,
+ bind: {
+ hidden: '{!isMicrosoftOAuth2Authentication}',
+ disabled: '{!isMicrosoftOAuth2Authentication}',
+ value: '{oAuth2TenantId}',
+ },
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
+ {
+ xtype: 'button',
+ text: 'Authenticate',
+ fieldLabel: gettext('Authenticate'),
+ handler: async function () {
+ const panel = this.up('pmxSmtpEditPanel');
+ const form = panel.up('form');
+ const values = form.getValues();
+
+ const refreshToken = await panel.handleOAuth2Flow(values);
+
+ panel.getViewModel().set('oAuth2RefreshToken', refreshToken);
+ },
+ bind: {
+ hidden: '{!isOAuth2Authentication}',
+ disabled: '{!enableAuthenticate}',
+ },
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
+ {
+ xtype: 'hiddenfield',
+ name: 'oauth2-refresh-token',
+ allowBlank: false,
+ bind: {
+ value: '{oAuth2RefreshToken}',
+ disabled: '{!isOAuth2Authentication}',
+ },
+ getErrors: function () {
+ if (this.disabled) {
+ return [];
+ }
+ if (!this.getValue()) {
+ return [''];
+ }
+ return [];
+ }
+ }
],
columnB: [
{
@@ -159,7 +295,6 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
},
},
],
-
advancedColumnB: [
{
xtype: 'proxmoxtextfield',
@@ -172,7 +307,15 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
},
},
],
+ handleOAuth2Flow: function (values) {
+ const authMethod = values['auth-method'];
+ if (authMethod === 'microsoft-oauth2') {
+ return Proxmox.OAuth2.handleMicrosoftFlow(values['oauth2-client-id'], values['oauth2-client-secret'], values['oauth2-tenant-id']);
+ } else if (authMethod === 'google-oauth2') {
+ return Proxmox.OAuth2.handleGoogleFlow(values['oauth2-client-id'], values['oauth2-client-secret']);
+ }
+ },
onGetValues: function (values) {
let me = this;
@@ -180,9 +323,24 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
values.mailto = values.mailto.split(/[\s,;]+/);
}
- if (!values.authentication && !me.isCreate) {
+ if (values['auth-method'] === 'none' && !me.isCreate) {
+ delete values['auth-method'];
Proxmox.Utils.assemble_field_data(values, { delete: 'username' });
Proxmox.Utils.assemble_field_data(values, { delete: 'password' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-client-id' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-client-secret' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-tenant-id' });
+ } else if (values['auth-method'] === 'plain' && !me.isCreate) {
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-client-id' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-client-secret' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-tenant-id' });
+ } else if (values['auth-method'] === 'microsoft-oauth2' && !me.isCreate) {
+ Proxmox.Utils.assemble_field_data(values, { delete: 'username' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'password' });
+ } else if (values['auth-method'] === 'google-oauth2' && !me.isCreate) {
+ Proxmox.Utils.assemble_field_data(values, { delete: 'username' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'password' });
+ Proxmox.Utils.assemble_field_data(values, { delete: 'oauth2-tenant-id' });
}
if (values.enable) {
@@ -199,12 +357,23 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
return values;
},
-
onSetValues: function (values) {
let me = this;
- values.authentication = !!values.username;
values.enable = !values.disable;
+
+ if (values['auth-method'] === undefined) {
+ if (values['oauth2-tenant-id']) {
+ values['auth-method'] = 'microsoft-oauth2';
+ } else if (values['oauth2-client-id']) {
+ values['auth-method'] = 'google-oauth2';
+ } else if (values.username) {
+ values['auth-method'] = 'plain'
+ } else {
+ values['auth-method'] = 'none';
+ }
+ }
+
delete values.disable;
// Fix race condition in chromium-based browsers. Without this, the
diff --git a/src/window/EndpointEditBase.js b/src/window/EndpointEditBase.js
index 8c1bfc1..8df2016 100644
--- a/src/window/EndpointEditBase.js
+++ b/src/window/EndpointEditBase.js
@@ -47,6 +47,7 @@ Ext.define('Proxmox.window.EndpointEditBase', {
baseUrl: me.baseUrl,
type: me.type,
defaultMailAuthor: endpointConfig.defaultMailAuthor,
+ enableOAuth2: endpointConfig.enableOAuth2,
},
],
});
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (7 preceding siblings ...)
2026-02-04 16:13 ` [PATCH proxmox-widget-toolkit 2/2] notifications: Add opt-in OAuth2 support for SMTP targets Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-11 8:55 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint Arthur Bied-Charreton
` (6 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Add auth-method, as well as optional
oauth2-{client-id,client-secret,tenant-id,refresh-token} parameters to
prepare for OAuth2 support.
The auth-method parameter was previously implicit and inferred by
proxmox-notify based on the presence of a password. It is now made
explicit, however still kept optional and explicitly inferred in the
{update,create}_endpoint handlers to avoid breaking the API.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
PVE/API2/Cluster/Notifications.pm | 55 +++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
index 8b455227..a45a15b2 100644
--- a/PVE/API2/Cluster/Notifications.pm
+++ b/PVE/API2/Cluster/Notifications.pm
@@ -941,6 +941,13 @@ my $smtp_properties = {
default => 'tls',
optional => 1,
},
+ 'auth-method' => {
+ description =>
+ 'Determine which authentication method shall be used for the connection.',
+ type => 'string',
+ enum => [qw(google-oauth2 microsoft-oauth2 plain none)],
+ optional => 1,
+ },
username => {
description => 'Username for SMTP authentication',
type => 'string',
@@ -951,6 +958,26 @@ my $smtp_properties = {
type => 'string',
optional => 1,
},
+ 'oauth2-client-id' => {
+ description => 'OAuth2 client ID',
+ type => 'string',
+ optional => 1,
+ },
+ 'oauth2-client-secret' => {
+ description => 'OAuth2 client secret',
+ type => 'string',
+ optional => 1,
+ },
+ 'oauth2-tenant-id' => {
+ description => 'OAuth2 tenant ID',
+ type => 'string',
+ optional => 1,
+ },
+ 'oauth2-refresh-token' => {
+ description => 'OAuth2 refresh token',
+ type => 'string',
+ optional => 1,
+ },
mailto => {
type => 'array',
items => {
@@ -1108,6 +1135,11 @@ __PACKAGE__->register_method({
my $mode = extract_param($param, 'mode');
my $username = extract_param($param, 'username');
my $password = extract_param($param, 'password');
+ my $auth_method = extract_param($param, 'auth-method');
+ my $oauth2_client_secret = extract_param($param, 'oauth2-client-secret');
+ my $oauth2_client_id = extract_param($param, 'oauth2-client-id');
+ my $oauth2_tenant_id = extract_param($param, 'oauth2-tenant-id');
+ my $oauth2_refresh_token = extract_param($param, 'oauth2-refresh-token');
my $mailto = extract_param($param, 'mailto');
my $mailto_user = extract_param($param, 'mailto-user');
my $from_address = extract_param($param, 'from-address');
@@ -1115,6 +1147,10 @@ __PACKAGE__->register_method({
my $comment = extract_param($param, 'comment');
my $disable = extract_param($param, 'disable');
+ if (!defined $auth_method) {
+ $auth_method = defined($password) ? 'plain' : 'none';
+ }
+
eval {
PVE::Notify::lock_config(sub {
my $config = PVE::Notify::read_config();
@@ -1126,6 +1162,11 @@ __PACKAGE__->register_method({
$mode,
$username,
$password,
+ $auth_method,
+ $oauth2_client_id,
+ $oauth2_client_secret,
+ $oauth2_tenant_id,
+ $oauth2_refresh_token,
$mailto,
$mailto_user,
$from_address,
@@ -1187,6 +1228,11 @@ __PACKAGE__->register_method({
my $mode = extract_param($param, 'mode');
my $username = extract_param($param, 'username');
my $password = extract_param($param, 'password');
+ my $auth_method = extract_param($param, 'auth-method');
+ my $oauth2_client_secret = extract_param($param, 'oauth2-client-secret');
+ my $oauth2_client_id = extract_param($param, 'oauth2-client-id');
+ my $oauth2_tenant_id = extract_param($param, 'oauth2-tenant-id');
+ my $oauth2_refresh_token = extract_param($param, 'oauth2-refresh-token');
my $mailto = extract_param($param, 'mailto');
my $mailto_user = extract_param($param, 'mailto-user');
my $from_address = extract_param($param, 'from-address');
@@ -1197,6 +1243,10 @@ __PACKAGE__->register_method({
my $delete = extract_param($param, 'delete');
my $digest = extract_param($param, 'digest');
+ if (!defined $auth_method) {
+ $auth_method = defined($password) ? 'plain' : 'none';
+ }
+
eval {
PVE::Notify::lock_config(sub {
my $config = PVE::Notify::read_config();
@@ -1208,6 +1258,11 @@ __PACKAGE__->register_method({
$mode,
$username,
$password,
+ $auth_method,
+ $oauth2_client_id,
+ $oauth2_client_secret,
+ $oauth2_tenant_id,
+ $oauth2_refresh_token,
$mailto,
$mailto_user,
$from_address,
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (8 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-11 9:49 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH pve-manager 3/5] notifications: Trigger notification target refresh in pveupdate Arthur Bied-Charreton
` (5 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
This endpoint allows triggering a refresh of the notification targets'
state, e.g., to prevent OAuth2 refresh tokens from expiring.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
PVE/API2/Cluster/Notifications.pm | 34 +++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
index a45a15b2..f993817d 100644
--- a/PVE/API2/Cluster/Notifications.pm
+++ b/PVE/API2/Cluster/Notifications.pm
@@ -321,6 +321,40 @@ __PACKAGE__->register_method({
},
});
+__PACKAGE__->register_method({
+ name => "refresh_targets",
+ path => 'refresh-targets',
+ protected => 1,
+ method => 'POST',
+ description => 'Refresh state for all targets',
+ permissions => {
+ check => [
+ 'and',
+ ['perm', '/mapping/notifications', ['Mapping.Modify']],
+ [
+ 'or',
+ ['perm', '/', ['Sys.Audit', 'Sys.Modify']],
+ ['perm', '/', ['Sys.AccessNetwork']],
+ ],
+ ],
+ },
+ parameters => {
+ additionalProperties => 0,
+ properties => {},
+ },
+ returns => { type => 'null' },
+ code => sub {
+ eval {
+ my $config = PVE::Notify::read_config();
+ $config->refresh_targets();
+ };
+
+ raise_api_error($@) if $@;
+
+ return;
+ },
+});
+
__PACKAGE__->register_method({
name => 'test_target',
path => 'targets/{name}/test',
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-manager 3/5] notifications: Trigger notification target refresh in pveupdate
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (9 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-manager 4/5] notifications: Handle OAuth2 callback in login handler Arthur Bied-Charreton
` (4 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Avoid OAuth2 refresh token expiry by forcing a notification target
refresh in PVE's daily-update job.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
bin/pveupdate | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/bin/pveupdate b/bin/pveupdate
index b1960c35..86d3d3cd 100755
--- a/bin/pveupdate
+++ b/bin/pveupdate
@@ -195,4 +195,13 @@ sub cleanup_tasks {
cleanup_tasks();
+eval {
+ my $config = PVE::Notify::read_config();
+ # Refresh internal state for notification targets
+ $config->refresh_targets();
+};
+if (my $err = $@) {
+ syslog('err', "refresh notification targets failed: $err");
+}
+
exit(0);
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-manager 4/5] notifications: Handle OAuth2 callback in login handler
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (10 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-manager 3/5] notifications: Trigger notification target refresh in pveupdate Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-11 9:00 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH pve-manager 5/5] notifications: Opt into OAuth2 authentication Arthur Bied-Charreton
` (3 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
The OAuth2 flow redirects to the service's origin
(window.location.origin) after successful authentication.
The callback handler infers whether the login was triggered as the
result of an OAuth2 redirect based on the presence of the code, scope,
and state URL parameters. It then communicates the authentication
results back to the parent window, which is responsible for closing it.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/Workspace.js | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
index b8061c2a..1e79dd57 100644
--- a/www/manager6/Workspace.js
+++ b/www/manager6/Workspace.js
@@ -150,9 +150,29 @@ Ext.define('PVE.StdWorkspace', {
me.down('pveResourceTree').selectById(nodeid);
},
+ handleOauth2Callback: function (params) {
+ const code = params.get('code');
+ const scope = params.get('scope');
+ const state = params.get('state');
+
+ // If true, this window was opened by the OAuth2 button handler from the
+ // SMTP notification targets edit panel.
+ //
+ // Since we got here through a redirect, this window is not script-closable,
+ // and we rely on the parent window to close it in its broadcast channel's
+ // message handler.
+ if (code && scope && state) {
+ const { channelName } = JSON.parse(decodeURIComponent(state));
+ const bc = new BroadcastChannel(channelName);
+ bc.postMessage({ code, scope });
+ }
+ },
+
onLogin: function (loginData) {
let me = this;
+ me.handleOauth2Callback(new URLSearchParams(window.location.search));
+
me.updateUserInfo();
if (loginData) {
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-manager 5/5] notifications: Opt into OAuth2 authentication
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (11 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-manager 4/5] notifications: Handle OAuth2 callback in login handler Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-cluster 1/1] notifications: Add refresh_targets subroutine to PVE::Notify Arthur Bied-Charreton
` (2 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
The OAuth2 authentication for SMTP is made opt-in in
proxmox-widget-toolkit to allow introducing it repo-by-repo.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/Utils.js | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 2992f655..9735184e 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2185,5 +2185,15 @@ Ext.define('PVE.Utils', {
replication: gettext('Replication job notifications'),
fencing: gettext('Node fencing notifications'),
});
+
+ Proxmox.Schema.overrideEndpointTypes({
+ smtp: {
+ name: 'SMTP',
+ ipanel: 'pmxSmtpEditPanel',
+ iconCls: 'fa-envelope-o',
+ defaultMailAuthor: 'Proxmox VE',
+ enableOAuth2: true,
+ },
+ });
},
});
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-cluster 1/1] notifications: Add refresh_targets subroutine to PVE::Notify
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (12 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-manager 5/5] notifications: Opt into OAuth2 authentication Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs Arthur Bied-Charreton
2026-02-13 16:06 ` superseded: [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Expose API to allow refreshing all notification targets' state from the
PVE backend, as well as from pveupdate.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Notify.pm | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
index b44b5b3..d5bfaea 100644
--- a/src/PVE/Notify.pm
+++ b/src/PVE/Notify.pm
@@ -103,6 +103,12 @@ sub error {
);
}
+sub refresh_targets {
+ my ($config) = @_;
+ $config = read_config() if !defined($config);
+ $config->refresh_targets();
+}
+
sub check_may_use_target {
my ($target, $rpcenv) = @_;
my $user = $rpcenv->get_user();
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (13 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-cluster 1/1] notifications: Add refresh_targets subroutine to PVE::Notify Arthur Bied-Charreton
@ 2026-02-04 16:13 ` Arthur Bied-Charreton
2026-02-11 10:06 ` Lukas Wagner
2026-02-13 16:06 ` superseded: [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
15 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-04 16:13 UTC (permalink / raw)
To: pve-devel
Document the new config entries, and add notes/warnings to communicate
that:
1. User intervention is required for initial OAuth2 target setup, and
2. Microsoft OAuth2 apps *must not* be configured as SPAs by the
user, since it would prevent PVE from automatically extending the
refresh token's lifetime
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
notifications.adoc | 44 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/notifications.adoc b/notifications.adoc
index 801b327..679b19b 100644
--- a/notifications.adoc
+++ b/notifications.adoc
@@ -108,16 +108,23 @@ The configuration for SMTP target plugins has the following options:
* `from-address`: Sets the From-address of the email. SMTP relays might require
that this address is owned by the user in order to avoid spoofing. The `From`
header in the email will be set to `$author <$from-address>`.
+* `auth-method`: Sets the authentication method (`plain`, `google-oauth2` or
+ `microsoft-oauth2`).
* `username`: Username to use during authentication. If no username is set,
no authentication will be performed. The PLAIN and LOGIN authentication
methods are supported.
* `password`: Password to use when authenticating.
+* `oauth2-client-id`: Client ID for the OAuth2 application, if applicable.
+* `oauth2-client-secret`: Client secret for the OAuth2 application, if
+ applicable.
+* `oauth2-tenant-id`: Tenant ID for the OAuth2 application, if applicable.
+ Only required for Microsoft OAuth2.
* `mode`: Sets the encryption mode (`insecure`, `starttls` or `tls`). Defaults
to `tls`.
* `server`: Address/IP of the SMTP relay.
-* `port`: The port to connect to. If not set, the used port .
- Defaults to 25 (`insecure`), 465 (`tls`) or 587 (`starttls`), depending on
- the value of `mode`.
+* `port`: The port to connect to. If not set, the used port defaults to 25
+ (`insecure`), 465 (`tls`) or 587 (`starttls`), depending on the value of
+ `mode`.
* `comment`: Comment for this target
Example configuration (`/etc/pve/notifications.cfg`):
@@ -133,13 +140,42 @@ smtp: example
----
The matching entry in `/etc/pve/priv/notifications.cfg`, containing the
-secret token:
+password:
----
smtp: example
password somepassword
----
+[[notification_targets_smtp_oauth2]]
+===== OAuth2 Authentication
+
+SMTP targets also support OAuth2 authentication via the XOAUTH2 mechanism for
+Google and Microsoft mail providers.
+
+Setting up OAuth2 authentication requires creating an OAuth2 application with
+the chosen provider. The application must be configured with a redirect URI
+pointing to the {pve} web interface, i.e. the URL from which the initial
+authentication request is performed in the UI.
+
+CAUTION: For Microsoft, the application must *not* be registered as Single-Page
+Application (SPA), as the lifetime of refresh tokens granted for SPAs cannot
+be extended automatically by {pve}.
+
+To set up OAuth2 authentication via the web interface, select `OAuth2 (Google)`
+or `OAuth2 (Microsoft)` as the authentication method, fill in the client ID and
+secret (and the tenant ID for Microsoft), then click the *Authenticate* button.
+This opens a new window where you can sign in with the selected provider and
+grant the required permissions. Upon successful authentication, a refresh
+token is obtained and stored automatically.
+
+Token refresh happens automatically, manual intervention is only needed if a
+token is revoked.
+
+NOTE: OAuth2 is currently not configurable through direct configuration file
+editing because the refresh token is managed as dynamic state by {pve}. All
+OAuth2 targets must be configured via the web interface.
+
[[notification_targets_gotify]]
Gotify
~~~~~~
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module
2026-02-04 16:13 ` [PATCH proxmox 1/5] notify: Introduce xoauth2 module Arthur Bied-Charreton
@ 2026-02-06 15:00 ` Lukas Wagner
2026-02-09 8:34 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-06 15:00 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
Hi Arthur, thanks for the patch!
some notes inline.
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Prepare proxmox-notify to use the oauth2 for SMTP XOAUTH2 support.
>
> The xoauth2 module handles some of the implementation details related to
> supporting XOAUTH2 for SMTP notification targets.
>
> * Add a ureq::Agent newtype wrapper implementing the SyncHttpClient
> trait to allow using ureq as oauth2 backend, since OAuth2 dropped the
> ureq feature. Debian seems to have patched it out due to a ureq 2/3
> version
> mismatch [1].
> * Add get_{google,microsoft}_token functions
>
> [1]
> https://git.proxmox.com/?p=debcargo-conf.git;a=blob;f=src/oauth2/debian/patches/disable-ureq.patch;h=828b883a83a86927c5cd32df055226a5e78e8bea;hb=refs/heads/proxmox/trixie
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> proxmox-notify/Cargo.toml | 4 +
> proxmox-notify/debian/control | 10 ++-
> proxmox-notify/src/lib.rs | 1 +
> proxmox-notify/src/xoauth2.rs | 146 ++++++++++++++++++++++++++++++++++
> 4 files changed, 159 insertions(+), 2 deletions(-)
> create mode 100644 proxmox-notify/src/xoauth2.rs
>
> diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> index bc63e19d..52493ef7 100644
> --- a/proxmox-notify/Cargo.toml
> +++ b/proxmox-notify/Cargo.toml
> @@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
> lettre = { workspace = true, optional = true }
> tracing.workspace = true
> mail-parser = { workspace = true, optional = true }
> +oauth2 = { version = "5.0.0" }
This misses a 'default-features = false' here, otherwise it will still
pull in reqwest.
Also, the oauth2 and ureq deps should be optional, since they are only
required by the smtp endpoint, which is gated behind a (default)
feature.
As an example, see how the `lettre` crate is handled, that one is also
only needed for the smtp backend.
In general, make sure that something like
`cargo build --no-default-features --features smtp`
works (also cross-check with other features).
> +ureq = { version = "3.0.11", features = ["platform-verifier"] }
> +
> +
> openssl.workspace = true
> percent-encoding = { workspace = true, optional = true }
> regex.workspace = true
> diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
> index e588e485..7770f5ee 100644
> --- a/proxmox-notify/debian/control
> +++ b/proxmox-notify/debian/control
> @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
> librust-handlebars-5+default-dev <!nocheck>,
> librust-http-1+default-dev <!nocheck>,
> librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
> + librust-oauth2-5+default-dev <!nocheck>,
> librust-openssl-0.10+default-dev <!nocheck>,
> librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
> librust-proxmox-base64-1+default-dev <!nocheck>,
> @@ -33,7 +34,9 @@ Build-Depends-Arch: cargo:native <!nocheck>,
> librust-serde-1+default-dev <!nocheck>,
> librust-serde-1+derive-dev <!nocheck>,
> librust-serde-json-1+default-dev <!nocheck>,
> - librust-tracing-0.1+default-dev <!nocheck>
> + librust-tracing-0.1+default-dev <!nocheck>,
> + librust-ureq-3+default-dev (>= 3.0.11-~~) <!nocheck>,
> + librust-ureq-3+platform-verifier-dev (>= 3.0.11-~~) <!nocheck>
> Maintainer: Proxmox Support Team <support@proxmox.com>
> Standards-Version: 4.7.2
> Vcs-Git: git://git.proxmox.com/git/proxmox.git
> @@ -49,6 +52,7 @@ Depends:
> librust-anyhow-1+default-dev,
> librust-const-format-0.2+default-dev,
> librust-handlebars-5+default-dev,
> + librust-oauth2-5+default-dev,
> librust-openssl-0.10+default-dev,
> librust-proxmox-http-error-1+default-dev,
> librust-proxmox-human-byte-1+default-dev,
> @@ -65,7 +69,9 @@ Depends:
> librust-serde-1+default-dev,
> librust-serde-1+derive-dev,
> librust-serde-json-1+default-dev,
> - librust-tracing-0.1+default-dev
> + librust-tracing-0.1+default-dev,
> + librust-ureq-3+default-dev (>= 3.0.11-~~),
> + librust-ureq-3+platform-verifier-dev (>= 3.0.11-~~)
> Recommends:
> librust-proxmox-notify+default-dev (= ${binary:Version})
> Suggests:
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 879f8326..1134027c 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -24,6 +24,7 @@ pub mod context;
> pub mod endpoints;
> pub mod renderer;
> pub mod schema;
> +pub mod xoauth2;
This module is only needed for the 'smtp' feature, so this should be
gated with a
#[cfg(feature = 'smtp')]
you might need to adapt some of the optional deps in Cargo.toml, e.g.
due to your new module, 'smtp' now also requires 'proxmox-sys', etc.
>
> #[derive(Debug)]
> pub enum Error {
> diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
> new file mode 100644
> index 00000000..66faabfa
> --- /dev/null
> +++ b/proxmox-notify/src/xoauth2.rs
> @@ -0,0 +1,146 @@
> +use oauth2::{
> + basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
> + TokenUrl,
> +};
> +
> +use crate::Error;
> +
> +/// `oauth2` dropped support for the `ureq` backend, this newtype circumvents this
> +/// by implementing the `SyncHttpClient` trait. This allows us to avoid pulling in
> +/// a different backend like `reqwest`.
Maybe clarify here that Debian patched out the ureq backend due to a
version mismatch of the packaged ureq crate and the one pulled in by the
oauth crate. Once this is resolved we might drop this custom client
implementation again. oauth2 uses an old version of ureq; we might be
able to contribute a PR to update it to a newer version.
You mentioned it in the commit message, but this would be useful here as
well.
> +pub struct UreqSyncHttpClient(ureq::Agent);
> +
> +impl Default for UreqSyncHttpClient {
> + /// Set `max_redirects` to 0 to prevent SSRF, see
> + /// https://docs.rs/oauth2/latest/oauth2/#security-warning
> + fn default() -> Self {
> + Self(ureq::Agent::new_with_config(
> + ureq::Agent::config_builder().max_redirects(0).build(),
> + ))
> + }
> +}
> +
> +impl oauth2::SyncHttpClient for UreqSyncHttpClient {
> + type Error = oauth2::HttpClientError<ureq::Error>;
> +
> + fn call(&self, request: oauth2::HttpRequest) -> Result<oauth2::HttpResponse, Self::Error> {
> + let uri = request.uri().to_string();
> +
> + let response = match request.method() {
> + &http::Method::POST => {
> + let req = request
> + .headers()
> + .iter()
> + .fold(self.0.post(&uri), |req, (name, value)| {
> + req.header(name, value)
> + });
> + req.send(request.body()).map_err(Box::new)?
> + }
> + &http::Method::GET => {
> + let req = request
> + .headers()
> + .iter()
> + .fold(self.0.get(&uri), |req, (name, value)| {
> + req.header(name, value)
> + });
> + req.call().map_err(Box::new)?
> + }
> + m => {
> + return Err(oauth2::HttpClientError::Other(format!(
> + "unexpected method: {m}"
> + )));
> + }
> + };
> +
> + let mut builder = http::Response::builder().status(response.status());
> +
> + if let Some(content_type) = response.headers().get(http::header::CONTENT_TYPE) {
> + builder = builder.header(http::header::CONTENT_TYPE, content_type);
> + }
> +
> + let (_, mut body) = response.into_parts();
> +
> + let body = body.read_to_vec().map_err(Box::new)?;
> +
> + builder.body(body).map_err(oauth2::HttpClientError::Http)
> + }
> +}
> +
> +pub struct TokenExchangeResult {
> + pub access_token: AccessToken,
> + pub refresh_token: Option<RefreshToken>,
> +}
In general, please add doc-strings for 'pub' functions. Also - I have
not checked the other patches, yet, but also consider using 'pub(crate)'
if these types are only needed inside this crate (which I think might be
the case for these).
> +
> +/// Microsoft Identity Platform refresh tokens replace themselves
> +/// upon every use, however the old one does *not* get revoked.
> +///
> +/// This means that every access token request yields both an access
> +/// token AND a new refresh token, which should replace the old one.
> +/// The old one should then be discarded.
This actually raises a very interesting question. Consider the case of a
read-only cluster filesystem due to a missing quorum:
- state file is read
- one or more smtp endpoints refresh their tokens while sending or due
to the periodic refresh
- state file is written, but fails due to pmxcfs being R/O
I guess we would need to find some way here to not 'lose' the token.
Some ideas:
- check *before* refreshing if we can actually write -> but this is
still racy
- when the write fails, save the token in some other location that is
not in the pmxcfs and then later, when pmxcfs is writable again,
update the statefile there - this could also be weird, in case
*multiple* nodes tried to send notifications at roughly the same
time - which one is the "correct" token then? Hopefully only one of
these nodes should be able to refresh the token in the first place,
but still something to keep in mind I think.
Maybe there is a better way though, these were just some initial ideas...
> +///
> +/// https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
> +pub fn get_microsoft_token(
> + client_id: ClientId,
> + client_secret: ClientSecret,
> + tenant_id: &str,
> + refresh_token: RefreshToken,
> +) -> Result<TokenExchangeResult, Error> {
> + let client = BasicClient::new(client_id)
> + .set_client_secret(client_secret)
> + .set_auth_uri(
> + AuthUrl::new("https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into())
> + .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
> + )
> + .set_token_uri(
> + TokenUrl::new(format!(
> + "https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token"
> + ))
> + .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
> + );
> +
> + let token_result = client
> + .exchange_refresh_token(&refresh_token)
> + .request(&UreqSyncHttpClient::default())
> + .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
apologies for the annoying error handling in proxmox-notify, I've been
meaning to make it more convenient and rethink the error variants a bit,
but I simply have not gotten to it yet :)
> +
> + Ok(TokenExchangeResult {
> + access_token: token_result.access_token().clone(),
> + refresh_token: token_result.refresh_token().cloned(),
> + })
> +}
> +
> +/// Google refresh tokens' TTL is extended at every use. As long as
> +/// a token has been used at least once in the past 6 months, and no
> +/// other expiration reason applies, we can keep the same token.
> +///
> +/// To make sure the token does not expire, it should be enough to periodically
> +/// make an access token request. If the token becomes invalid for whatever
> +/// other reason, we need user intervention to get a new one.
> +///
> +/// https://developers.google.com/identity/protocols/oauth2#expiration
> +pub fn get_google_token(
> + client_id: ClientId,
> + client_secret: ClientSecret,
> + refresh_token: RefreshToken,
> +) -> Result<TokenExchangeResult, Error> {
> + let client = BasicClient::new(client_id)
> + .set_client_secret(client_secret)
> + .set_auth_uri(
> + AuthUrl::new("https://accounts.google.com/o/oauth2/v2/auth".into())
> + .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
> + )
> + .set_token_uri(
> + TokenUrl::new("https://oauth2.googleapis.com/token".into())
> + .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
> + );
> +
> + let token_result = client
> + .exchange_refresh_token(&refresh_token)
> + .request(&UreqSyncHttpClient::default())
> + .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
> +
> + Ok(TokenExchangeResult {
> + access_token: token_result.access_token().clone(),
> + refresh_token: token_result.refresh_token().cloned(),
> + })
> +}
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module
2026-02-06 15:00 ` Lukas Wagner
@ 2026-02-09 8:34 ` Arthur Bied-Charreton
2026-02-10 8:24 ` Lukas Wagner
0 siblings, 1 reply; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-09 8:34 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Fri, Feb 06, 2026 at 04:00:42PM +0100, Lukas Wagner wrote:
> Hi Arthur, thanks for the patch!
>
> some notes inline.
>
> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> > [...]
> > diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> > index bc63e19d..52493ef7 100644
> > --- a/proxmox-notify/Cargo.toml
> > +++ b/proxmox-notify/Cargo.toml
> > @@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
> > lettre = { workspace = true, optional = true }
> > tracing.workspace = true
> > mail-parser = { workspace = true, optional = true }
> > +oauth2 = { version = "5.0.0" }
>
> This misses a 'default-features = false' here, otherwise it will still
> pull in reqwest.
>
> Also, the oauth2 and ureq deps should be optional, since they are only
> required by the smtp endpoint, which is gated behind a (default)
> feature.
>
> As an example, see how the `lettre` crate is handled, that one is also
> only needed for the smtp backend.
>
> In general, make sure that something like
> `cargo build --no-default-features --features smtp`
> works (also cross-check with other features).
>
> > [...]
> > pub mod endpoints;
> > pub mod renderer;
> > pub mod schema;
> > +pub mod xoauth2;
>
> This module is only needed for the 'smtp' feature, so this should be
> gated with a
>
> #[cfg(feature = 'smtp')]
>
> you might need to adapt some of the optional deps in Cargo.toml, e.g.
> due to your new module, 'smtp' now also requires 'proxmox-sys', etc.
>
Will fix the feature/dependencies issues in v2, thanks for the examples!
> >
> > #[derive(Debug)]
> > pub enum Error {
> > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
> > new file mode 100644
> > index 00000000..66faabfa
> > --- /dev/null
> > +++ b/proxmox-notify/src/xoauth2.rs
> > @@ -0,0 +1,146 @@
> > +use oauth2::{
> > + basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
> > + TokenUrl,
> > +};
> > +
> > +use crate::Error;
> > +
> > +/// `oauth2` dropped support for the `ureq` backend, this newtype circumvents this
> > +/// by implementing the `SyncHttpClient` trait. This allows us to avoid pulling in
> > +/// a different backend like `reqwest`.
>
> Maybe clarify here that Debian patched out the ureq backend due to a
> version mismatch of the packaged ureq crate and the one pulled in by the
> oauth crate. Once this is resolved we might drop this custom client
> implementation again. oauth2 uses an old version of ureq; we might be
> able to contribute a PR to update it to a newer version.
>
> You mentioned it in the commit message, but this would be useful here as
> well.
>
I already opened a PR in oauth2 [0], no answer yet. Will add more
details & a link to it to the doc comment.
[0] https://github.com/ramosbugs/oauth2-rs/pull/338
>
> In general, please add doc-strings for 'pub' functions. Also - I have
> not checked the other patches, yet, but also consider using 'pub(crate)'
> if these types are only needed inside this crate (which I think might be
> the case for these).
>
Will do, thanks :)
> > +
> > +/// Microsoft Identity Platform refresh tokens replace themselves
> > +/// upon every use, however the old one does *not* get revoked.
> > +///
> > +/// This means that every access token request yields both an access
> > +/// token AND a new refresh token, which should replace the old one.
> > +/// The old one should then be discarded.
>
> This actually raises a very interesting question. Consider the case of a
> read-only cluster filesystem due to a missing quorum:
>
> - state file is read
> - one or more smtp endpoints refresh their tokens while sending or due
> to the periodic refresh
> - state file is written, but fails due to pmxcfs being R/O
>
> I guess we would need to find some way here to not 'lose' the token.
>
> Some ideas:
> - check *before* refreshing if we can actually write -> but this is
> still racy
> - when the write fails, save the token in some other location that is
> not in the pmxcfs and then later, when pmxcfs is writable again,
> update the statefile there - this could also be weird, in case
> *multiple* nodes tried to send notifications at roughly the same
> time - which one is the "correct" token then? Hopefully only one of
> these nodes should be able to refresh the token in the first place,
> but still something to keep in mind I think.
>
> Maybe there is a better way though, these were just some initial ideas...
>
I may have formulated this doc comment too conservatively: according to the
Microsoft docs on token lifetime [0], while the refresh token does get
"replaced" at every use, the replaced token is not revoked, and each
token has a static lifetime of 90 days.
This means that in this case, I don't think we really *care* which token is
the correct one, since they are all still valid. We can afford to lose
some and persist the first new token we get after the file system
becomes writable again.
(All of this assuming that the Microsoft docs are correct, and the
implementation does not bear any surprises, could not test it myself yet.)
If the FS stays read-only for 90+ days, we need user interaction again to
get a new refresh token anyway, since even the freshest of tokens would be
expired by then. Now that I think about it, we probably need a way to
request a new authorization from the user in the frontend.
[0] https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
> > +///
> > +/// https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
> > +pub fn get_microsoft_token(
> > + client_id: ClientId,
> > + client_secret: ClientSecret,
> > + tenant_id: &str,
> > + refresh_token: RefreshToken,
> > +) -> Result<TokenExchangeResult, Error> {
> > + let client = BasicClient::new(client_id)
> > + .set_client_secret(client_secret)
> > + .set_auth_uri(
> > + AuthUrl::new("https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into())
> > + .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
> > + )
> > + .set_token_uri(
> > + TokenUrl::new(format!(
> > + "https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token"
> > + ))
> > + .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
> > + );
> > +
> > + let token_result = client
> > + .exchange_refresh_token(&refresh_token)
> > + .request(&UreqSyncHttpClient::default())
> > + .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
>
> apologies for the annoying error handling in proxmox-notify, I've been
> meaning to make it more convenient and rethink the error variants a bit,
> but I simply have not gotten to it yet :)
>
I may not have helped by using Error::Generic everywhere ^^'
Maybe we could add an AuthenticationError variant, and pull in thiserror
to streamline the error handdling a little? Might be something for a
follow-up series though.
> > +
> > + Ok(TokenExchangeResult {
> > + access_token: token_result.access_token().clone(),
> > + refresh_token: token_result.refresh_token().cloned(),
> > + })
> > +}
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module
2026-02-09 8:34 ` Arthur Bied-Charreton
@ 2026-02-10 8:24 ` Lukas Wagner
2026-02-10 10:23 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-10 8:24 UTC (permalink / raw)
To: Arthur Bied-Charreton, Lukas Wagner; +Cc: pve-devel
On Mon Feb 9, 2026 at 9:34 AM CET, Arthur Bied-Charreton wrote:
> On Fri, Feb 06, 2026 at 04:00:42PM +0100, Lukas Wagner wrote:
>> Hi Arthur, thanks for the patch!
>>
>> some notes inline.
>>
>> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
>> > [...]
>> > diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
>> > index bc63e19d..52493ef7 100644
>> > --- a/proxmox-notify/Cargo.toml
>> > +++ b/proxmox-notify/Cargo.toml
>> > @@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
>> > lettre = { workspace = true, optional = true }
>> > tracing.workspace = true
>> > mail-parser = { workspace = true, optional = true }
>> > +oauth2 = { version = "5.0.0" }
>>
>> This misses a 'default-features = false' here, otherwise it will still
>> pull in reqwest.
>>
>> Also, the oauth2 and ureq deps should be optional, since they are only
>> required by the smtp endpoint, which is gated behind a (default)
>> feature.
>>
>> As an example, see how the `lettre` crate is handled, that one is also
>> only needed for the smtp backend.
>>
>> In general, make sure that something like
>> `cargo build --no-default-features --features smtp`
>> works (also cross-check with other features).
>>
>> > [...]
>> > pub mod endpoints;
>> > pub mod renderer;
>> > pub mod schema;
>> > +pub mod xoauth2;
>>
>> This module is only needed for the 'smtp' feature, so this should be
>> gated with a
>>
>> #[cfg(feature = 'smtp')]
>>
>> you might need to adapt some of the optional deps in Cargo.toml, e.g.
>> due to your new module, 'smtp' now also requires 'proxmox-sys', etc.
>>
> Will fix the feature/dependencies issues in v2, thanks for the examples!
>
>> >
>> > #[derive(Debug)]
>> > pub enum Error {
>> > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
>> > new file mode 100644
>> > index 00000000..66faabfa
>> > --- /dev/null
>> > +++ b/proxmox-notify/src/xoauth2.rs
>> > @@ -0,0 +1,146 @@
>> > +use oauth2::{
>> > + basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
>> > + TokenUrl,
>> > +};
>> > +
>> > +use crate::Error;
>> > +
>> > +/// `oauth2` dropped support for the `ureq` backend, this newtype circumvents this
>> > +/// by implementing the `SyncHttpClient` trait. This allows us to avoid pulling in
>> > +/// a different backend like `reqwest`.
>>
>> Maybe clarify here that Debian patched out the ureq backend due to a
>> version mismatch of the packaged ureq crate and the one pulled in by the
>> oauth crate. Once this is resolved we might drop this custom client
>> implementation again. oauth2 uses an old version of ureq; we might be
>> able to contribute a PR to update it to a newer version.
>>
>> You mentioned it in the commit message, but this would be useful here as
>> well.
>>
> I already opened a PR in oauth2 [0], no answer yet. Will add more
> details & a link to it to the doc comment.
>
> [0] https://github.com/ramosbugs/oauth2-rs/pull/338
>
quoting from your PR:
'when trying to use the ureq backend for oauth2, I unfortunately had to
find out that while it is documented as a feature in the crate docs, it
is no longer available. This seems to be due to the API changes
introduced by ureq 3.x.'
This might be a bit confusing for the maintainer, you should probably
explain that you are using the Debian-packaged version of oauth2 which
patched out the support for the ureq backend due to the version
conflict.
>>
>> In general, please add doc-strings for 'pub' functions. Also - I have
>> not checked the other patches, yet, but also consider using 'pub(crate)'
>> if these types are only needed inside this crate (which I think might be
>> the case for these).
>>
> Will do, thanks :)
>> > +
>> > +/// Microsoft Identity Platform refresh tokens replace themselves
>> > +/// upon every use, however the old one does *not* get revoked.
>> > +///
>> > +/// This means that every access token request yields both an access
>> > +/// token AND a new refresh token, which should replace the old one.
>> > +/// The old one should then be discarded.
>>
>> This actually raises a very interesting question. Consider the case of a
>> read-only cluster filesystem due to a missing quorum:
>>
>> - state file is read
>> - one or more smtp endpoints refresh their tokens while sending or due
>> to the periodic refresh
>> - state file is written, but fails due to pmxcfs being R/O
>>
>> I guess we would need to find some way here to not 'lose' the token.
>>
>> Some ideas:
>> - check *before* refreshing if we can actually write -> but this is
>> still racy
>> - when the write fails, save the token in some other location that is
>> not in the pmxcfs and then later, when pmxcfs is writable again,
>> update the statefile there - this could also be weird, in case
>> *multiple* nodes tried to send notifications at roughly the same
>> time - which one is the "correct" token then? Hopefully only one of
>> these nodes should be able to refresh the token in the first place,
>> but still something to keep in mind I think.
>>
>> Maybe there is a better way though, these were just some initial ideas...
>>
> I may have formulated this doc comment too conservatively: according to the
> Microsoft docs on token lifetime [0], while the refresh token does get
> "replaced" at every use, the replaced token is not revoked, and each
> token has a static lifetime of 90 days.
>
> This means that in this case, I don't think we really *care* which token is
> the correct one, since they are all still valid. We can afford to lose
> some and persist the first new token we get after the file system
> becomes writable again.
>
> (All of this assuming that the Microsoft docs are correct, and the
> implementation does not bear any surprises, could not test it myself yet.)
>
> If the FS stays read-only for 90+ days, we need user interaction again to
> get a new refresh token anyway, since even the freshest of tokens would be
> expired by then. Now that I think about it, we probably need a way to
> request a new authorization from the user in the frontend.
Ah, that's good to know - so that is hopefully not be an issue in the
real world (but maybe still warrant a note in the documentation). Also
let's hope that this does not become an issue with any other email
provider we might support in the future.
>
> [0] https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
>
>> > +///
>> > +/// https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
>> > +pub fn get_microsoft_token(
>> > + client_id: ClientId,
>> > + client_secret: ClientSecret,
>> > + tenant_id: &str,
>> > + refresh_token: RefreshToken,
>> > +) -> Result<TokenExchangeResult, Error> {
>> > + let client = BasicClient::new(client_id)
>> > + .set_client_secret(client_secret)
>> > + .set_auth_uri(
>> > + AuthUrl::new("https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into())
>> > + .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
>> > + )
>> > + .set_token_uri(
>> > + TokenUrl::new(format!(
>> > + "https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token"
>> > + ))
>> > + .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
>> > + );
>> > +
>> > + let token_result = client
>> > + .exchange_refresh_token(&refresh_token)
>> > + .request(&UreqSyncHttpClient::default())
>> > + .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
>>
>> apologies for the annoying error handling in proxmox-notify, I've been
>> meaning to make it more convenient and rethink the error variants a bit,
>> but I simply have not gotten to it yet :)
>>
>
> I may not have helped by using Error::Generic everywhere ^^'
> Maybe we could add an AuthenticationError variant, and pull in thiserror
> to streamline the error handdling a little? Might be something for a
> follow-up series though.
>
Definitely something for a future patch series. While this will
*technically* be a breaking change for this crate, none of the existing
callers do anything with the returned errors except logging it anyway.
I have some other ideas and changes planned for proxmox-notify anyway
(like pulling out the static context, as explained my integration
testing proposal on pdm-devel [0]), so the error refactoring might
be something that I would include there.
[0] https://lore.proxmox.com/all/20260129134418.307552-1-l.wagner@proxmox.com/T/#u
>> > +
>> > + Ok(TokenExchangeResult {
>> > + access_token: token_result.access_token().clone(),
>> > + refresh_token: token_result.refresh_token().cloned(),
>> > + })
>> > +}
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module
2026-02-10 8:24 ` Lukas Wagner
@ 2026-02-10 10:23 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-10 10:23 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Tue, Feb 10, 2026 at 09:24:34AM +0100, Lukas Wagner wrote:
> On Mon Feb 9, 2026 at 9:34 AM CET, Arthur Bied-Charreton wrote:
> > On Fri, Feb 06, 2026 at 04:00:42PM +0100, Lukas Wagner wrote:
> >> Hi Arthur, thanks for the patch!
> >>
> >> some notes inline.
> >>
> >> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> >> > [...]
> >> > diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> >> > index bc63e19d..52493ef7 100644
> >> > --- a/proxmox-notify/Cargo.toml
> >> > +++ b/proxmox-notify/Cargo.toml
> >> > @@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
> >> > lettre = { workspace = true, optional = true }
> >> > tracing.workspace = true
> >> > mail-parser = { workspace = true, optional = true }
> >> > +oauth2 = { version = "5.0.0" }
> >>
> >> This misses a 'default-features = false' here, otherwise it will still
> >> pull in reqwest.
> >>
> >> Also, the oauth2 and ureq deps should be optional, since they are only
> >> required by the smtp endpoint, which is gated behind a (default)
> >> feature.
> >>
> >> As an example, see how the `lettre` crate is handled, that one is also
> >> only needed for the smtp backend.
> >>
> >> In general, make sure that something like
> >> `cargo build --no-default-features --features smtp`
> >> works (also cross-check with other features).
> >>
> >> > [...]
> >> > pub mod endpoints;
> >> > pub mod renderer;
> >> > pub mod schema;
> >> > +pub mod xoauth2;
> >>
> >> This module is only needed for the 'smtp' feature, so this should be
> >> gated with a
> >>
> >> #[cfg(feature = 'smtp')]
> >>
> >> you might need to adapt some of the optional deps in Cargo.toml, e.g.
> >> due to your new module, 'smtp' now also requires 'proxmox-sys', etc.
> >>
> > Will fix the feature/dependencies issues in v2, thanks for the examples!
> >
> >> >
> >> > #[derive(Debug)]
> >> > pub enum Error {
> >> > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
> >> > new file mode 100644
> >> > index 00000000..66faabfa
> >> > --- /dev/null
> >> > +++ b/proxmox-notify/src/xoauth2.rs
> >> > @@ -0,0 +1,146 @@
> >> > +use oauth2::{
> >> > + basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
> >> > + TokenUrl,
> >> > +};
> >> > +
> >> > +use crate::Error;
> >> > +
> >> > +/// `oauth2` dropped support for the `ureq` backend, this newtype circumvents this
> >> > +/// by implementing the `SyncHttpClient` trait. This allows us to avoid pulling in
> >> > +/// a different backend like `reqwest`.
> >>
> >> Maybe clarify here that Debian patched out the ureq backend due to a
> >> version mismatch of the packaged ureq crate and the one pulled in by the
> >> oauth crate. Once this is resolved we might drop this custom client
> >> implementation again. oauth2 uses an old version of ureq; we might be
> >> able to contribute a PR to update it to a newer version.
> >>
> >> You mentioned it in the commit message, but this would be useful here as
> >> well.
> >>
> > I already opened a PR in oauth2 [0], no answer yet. Will add more
> > details & a link to it to the doc comment.
> >
> > [0] https://github.com/ramosbugs/oauth2-rs/pull/338
> >
>
> quoting from your PR:
>
> 'when trying to use the ureq backend for oauth2, I unfortunately had to
> find out that while it is documented as a feature in the crate docs, it
> is no longer available. This seems to be due to the API changes
> introduced by ureq 3.x.'
>
> This might be a bit confusing for the maintainer, you should probably
> explain that you are using the Debian-packaged version of oauth2 which
> patched out the support for the ureq backend due to the version
> conflict.
>
Makes sense, I edited my PR, thanks :)
> >> > +
> >> > +/// Microsoft Identity Platform refresh tokens replace themselves
> >> > +/// upon every use, however the old one does *not* get revoked.
> >> > +///
> >> > +/// This means that every access token request yields both an access
> >> > +/// token AND a new refresh token, which should replace the old one.
> >> > +/// The old one should then be discarded.
> >>
> >> This actually raises a very interesting question. Consider the case of a
> >> read-only cluster filesystem due to a missing quorum:
> >>
> >> - state file is read
> >> - one or more smtp endpoints refresh their tokens while sending or due
> >> to the periodic refresh
> >> - state file is written, but fails due to pmxcfs being R/O
> >>
> >> I guess we would need to find some way here to not 'lose' the token.
> >>
> >> Some ideas:
> >> - check *before* refreshing if we can actually write -> but this is
> >> still racy
> >> - when the write fails, save the token in some other location that is
> >> not in the pmxcfs and then later, when pmxcfs is writable again,
> >> update the statefile there - this could also be weird, in case
> >> *multiple* nodes tried to send notifications at roughly the same
> >> time - which one is the "correct" token then? Hopefully only one of
> >> these nodes should be able to refresh the token in the first place,
> >> but still something to keep in mind I think.
> >>
> >> Maybe there is a better way though, these were just some initial ideas...
> >>
> > I may have formulated this doc comment too conservatively: according to the
> > Microsoft docs on token lifetime [0], while the refresh token does get
> > "replaced" at every use, the replaced token is not revoked, and each
> > token has a static lifetime of 90 days.
> >
> > This means that in this case, I don't think we really *care* which token is
> > the correct one, since they are all still valid. We can afford to lose
> > some and persist the first new token we get after the file system
> > becomes writable again.
> >
> > (All of this assuming that the Microsoft docs are correct, and the
> > implementation does not bear any surprises, could not test it myself yet.)
> >
> > If the FS stays read-only for 90+ days, we need user interaction again to
> > get a new refresh token anyway, since even the freshest of tokens would be
> > expired by then. Now that I think about it, we probably need a way to
> > request a new authorization from the user in the frontend.
>
> Ah, that's good to know - so that is hopefully not be an issue in the
> real world (but maybe still warrant a note in the documentation). Also
> let's hope that this does not become an issue with any other email
> provider we might support in the future.
>
I will update the docs to reflect this choice better.
There are some OAuth2 providers like Auth0 where you can configure
stricter token rotation [0] (e.g new token issued at every use and old one
instantly invalidated), but it seems to be more targeted at apps that
store refresh tokens in client memory, not long-term authorization like
the kind we need here.
[0] https://auth0.com/docs/secure/tokens/refresh-tokens/configure-refresh-token-rotation
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 2/5] notify: Add state file handling
2026-02-04 16:13 ` [PATCH proxmox 2/5] notify: Add state file handling Arthur Bied-Charreton
@ 2026-02-10 15:51 ` Lukas Wagner
0 siblings, 0 replies; 34+ messages in thread
From: Lukas Wagner @ 2026-02-10 15:51 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
Hey Arthur,
great work so far. Some comments inline.
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Add State struct abstracting state file deserialization, updates and
> persistence, as well as an EndpointState marker trait stateful endpoints
> may implement.
>
> Also add a state_file_path method to the crate's Context trait, which
> allows
> tests to build their own context instead of depending on statics.
The context trait is more about being able to use proxmox-notify in
different products than making it testable, the latter is rather a
(great) side-effect.
>
> As far as SMTP endpoints are concerned, file locks are not necessary.
> Old Microsoft tokens stay valid for 90 days after refreshes [1], and
> Google
> tokens' lifetime is just extended at every use [2], so concurrent reads
> should not
> be an issue here.
Since this state is now pretty general purpose and *could* be used by
other endpoints for something different than OAuth tokens, it still
would make sense to think about potential race conditions and potential
solutions for them.
>
> [1]
> https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
> [2]
> https://stackoverflow.com/questions/8953983/do-google-refresh-tokens-expire
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> proxmox-notify/Cargo.toml | 1 +
> proxmox-notify/debian/control | 2 +
> proxmox-notify/src/context/mod.rs | 2 +
> proxmox-notify/src/context/pbs.rs | 4 ++
> proxmox-notify/src/context/pve.rs | 4 ++
> proxmox-notify/src/context/test.rs | 4 ++
> proxmox-notify/src/lib.rs | 60 ++++++++++++++++++++++++++++++
> 7 files changed, 77 insertions(+)
>
> diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> index 52493ef7..daa10296 100644
> --- a/proxmox-notify/Cargo.toml
> +++ b/proxmox-notify/Cargo.toml
> @@ -40,6 +40,7 @@ proxmox-sendmail = { workspace = true, optional = true }
> proxmox-sys = { workspace = true, optional = true }
> proxmox-time.workspace = true
> proxmox-uuid = { workspace = true, features = ["serde"] }
> +nix.workspace = true
>
> [features]
> default = ["sendmail", "gotify", "smtp", "webhook"]
> diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
> index 7770f5ee..76b8a1fa 100644
> --- a/proxmox-notify/debian/control
> +++ b/proxmox-notify/debian/control
> @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
> librust-handlebars-5+default-dev <!nocheck>,
> librust-http-1+default-dev <!nocheck>,
> librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
> + librust-nix-0.29+default-dev <!nocheck>,
> librust-oauth2-5+default-dev <!nocheck>,
> librust-openssl-0.10+default-dev <!nocheck>,
> librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
> @@ -52,6 +53,7 @@ Depends:
> librust-anyhow-1+default-dev,
> librust-const-format-0.2+default-dev,
> librust-handlebars-5+default-dev,
> + librust-nix-0.29+default-dev,
> librust-oauth2-5+default-dev,
> librust-openssl-0.10+default-dev,
> librust-proxmox-http-error-1+default-dev,
> diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
> index 8b6e2c43..86130409 100644
> --- a/proxmox-notify/src/context/mod.rs
> +++ b/proxmox-notify/src/context/mod.rs
> @@ -32,6 +32,8 @@ pub trait Context: Send + Sync + Debug {
> namespace: Option<&str>,
> source: TemplateSource,
> ) -> Result<Option<String>, Error>;
> + /// Return the state file, or None if no state file exists for this context.
This does not return an Option, so I guess the doc comment is not
correct here?
> + fn state_file_path(&self) -> &'static str;
> }
>
> #[cfg(not(test))]
> diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
> index 3e5da59c..67010060 100644
> --- a/proxmox-notify/src/context/pbs.rs
> +++ b/proxmox-notify/src/context/pbs.rs
> @@ -125,6 +125,10 @@ impl Context for PBSContext {
> .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> Ok(template_string)
> }
> +
> + fn state_file_path(&self) -> &'static str {
> + "/etc/proxmox-backup/notifications.state.json"
> + }
Since we don't have a cluster filesystem in PBS, it probably would
be good to place this file in an actual state directory, such as
/var/lib/proxmox-backup/.
> }
>
> #[cfg(test)]
> diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
> index a97cce26..0dffbb11 100644
> --- a/proxmox-notify/src/context/pve.rs
> +++ b/proxmox-notify/src/context/pve.rs
> @@ -74,6 +74,10 @@ impl Context for PVEContext {
> .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> Ok(template_string)
> }
> +
> + fn state_file_path(&self) -> &'static str {
> + "/etc/pve/priv/notifications.state.json"
> + }
> }
>
> pub static PVE_CONTEXT: PVEContext = PVEContext;
> diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
> index 2c236b4c..e0236b9c 100644
> --- a/proxmox-notify/src/context/test.rs
> +++ b/proxmox-notify/src/context/test.rs
> @@ -40,4 +40,8 @@ impl Context for TestContext {
> ) -> Result<Option<String>, Error> {
> Ok(Some(String::new()))
> }
> +
> + fn state_file_path(&self) -> &'static str {
> + "/tmp/notifications.state.json"
> + }
> }
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 1134027c..a40342cc 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -6,6 +6,7 @@ use std::fmt::Display;
> use std::str::FromStr;
>
> use context::context;
> +use serde::de::DeserializeOwned;
> use serde::{Deserialize, Serialize};
> use serde_json::json;
> use serde_json::Value;
> @@ -272,6 +273,65 @@ impl Notification {
> }
> }
>
> +#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)]
> +pub struct State {
> + #[serde(flatten)]
> + pub sections: HashMap<String, Value>,
> +}
> +
> +impl FromStr for State {
> + type Err = Error;
> +
> + fn from_str(s: &str) -> Result<Self, Self::Err> {
> + serde_json::from_str(s).map_err(|e| Error::ConfigDeserialization(e.into()))
> + }
> +}
> +
> +/// Marker trait to be implemented by the state structs for stateful endpoints.
> +pub trait EndpointState: Serialize + DeserializeOwned + Default {}
> +
> +impl State {
> + pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Result<Self, Error> {
> + let contents = proxmox_sys::fs::file_read_string(path)
> + .map_err(|e| Error::ConfigDeserialization(e.into()))?;
> + Self::from_str(&contents)
Btw, you could use proxmox_sys::file_get_contents and then deserialize
using serde_json::from_slice; this should be a bit more efficient since
it does not need to check if it is valid utf-8 at all.
There is also promxox_sys::file_get_optional_contents -- see the next
patch for the context why I mention this.
> + }
> +
> + pub fn persist<P: AsRef<std::path::Path>>(&self, path: P) -> Result<(), Error> {
> + let state_str =
> + serde_json::to_string_pretty(self).map_err(|e| Error::ConfigSerialization(e.into()))?;
> +
> + let mode = nix::sys::stat::Mode::from_bits_truncate(0o600);
> + let options = proxmox_sys::fs::CreateOptions::new().perm(mode);
I think it might be better to provide the CreateOptions also via some
method in Context - then the application has full control over the
permissions, user and group for the file.
Since you also provide the path via a parameter, I guess the caller of
this function would retrieve it from the context and then pass it along
with the path.
I think I'd use something generic like "secret_create_options" (name
taken from `proxmox-product-config`), indicating that we want the
CreateOptions for *something* secret/sensitive.
Also you don't need the `nix` crate then, I think.
> +
> + proxmox_sys::fs::replace_file(path, state_str.as_bytes(), options, true)
> + .map_err(|e| Error::ConfigSerialization(e.into()))
> + }
> +
> + pub fn get<S: EndpointState>(&self, name: &str) -> Result<Option<S>, Error> {
> + match self.sections.get(name) {
> + Some(v) => Ok(Some(
> + S::deserialize(v).map_err(|e| Error::ConfigDeserialization(e.into()))?,
> + )),
> + None => Ok(None),
> + }
> + }
> +
> + pub fn get_or_default<S: EndpointState>(&self, name: &str) -> Result<S, Error> {
> + Ok(self.get(name)?.unwrap_or_default())
> + }
> +
> + pub fn set<S: EndpointState>(&mut self, name: &str, state: &S) -> Result<(), Error> {
> + let v = serde_json::to_value(state).map_err(|e| Error::ConfigSerialization(e.into()))?;
> + self.sections.insert(name.to_string(), v);
> + Ok(())
> + }
> +
> + pub fn remove(&mut self, name: &str) {
> + self.sections.remove(name);
> + }
> +}
Also here, same as for the other patch, consider adding doc-comments -
and `pub(crate)` might also be a good fit here.
> +
> /// Notification configuration
> #[derive(Debug, Clone)]
> pub struct Config {
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State
2026-02-04 16:13 ` [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State Arthur Bied-Charreton
@ 2026-02-10 15:52 ` Lukas Wagner
2026-02-12 8:26 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-10 15:52 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
Hey Arthur,
some comments inline.
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> This lays the groundwork for handling OAuth2 state in proxmox-notify by
> updating the crate's internal API to pass around a mutable State struct.
>
> The state can be updated by its consumers and will be persisted
> afterwards, much like the Config struct, with the difference that it is
> fully internal to the crate. External consumers of the API do not need
> to know/care about it.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> proxmox-notify/src/api/common.rs | 70 +++++++++++++++++++++---
> proxmox-notify/src/endpoints/gotify.rs | 4 +-
> proxmox-notify/src/endpoints/sendmail.rs | 4 +-
> proxmox-notify/src/endpoints/smtp.rs | 17 +++++-
> proxmox-notify/src/endpoints/webhook.rs | 4 +-
> proxmox-notify/src/lib.rs | 42 +++++++++-----
> 6 files changed, 113 insertions(+), 28 deletions(-)
>
> diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs
> index fa2356e2..3483be9a 100644
> --- a/proxmox-notify/src/api/common.rs
> +++ b/proxmox-notify/src/api/common.rs
> @@ -1,7 +1,52 @@
> use proxmox_http_error::HttpError;
>
> use super::http_err;
> -use crate::{Bus, Config, Notification};
> +use crate::{context::context, Bus, Config, Notification, State};
> +
> +use tracing::error;
> +
> +fn get_state() -> State {
> + let path_str = context().state_file_path();
> + let path = std::path::Path::new(path_str);
> +
> + match path.exists() {
> + true => match State::from_path(path) {
> + Ok(s) => s,
> + Err(e) => {
> + // We don't want to block notifications on all endpoints only
> + // because the stateful ones are broken.
> + error!("could not instantiate state from {path_str}: {e}",);
> + State::default()
> + }
> + },
> + false => State::default(),
> + }
> +}
This is a good example of TOCTOU (time of check, time of use). First you
check if something exists in the filesystem, and if it does, you read
the file. In theory, it could happen that the file is removed in between
these two lines, leading to an error when reading the file. Now, in
this concrete case here this is not a huge issue, but nevertheless its
better to avoid it if you can. In this case here you can just read the
file and then in case of an error check for ENOENT. See [1]
for a very similar example.
[1] https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=server/src/remote_updates.rs;h=e772eef510218d8da41fe4a88ee47847b2598045;hb=HEAD#l159
In proxmox-sys there are also the file_read_optional_string or
file_get_optional_contents helpers which should do just that.
> +
> +fn persist_state(state: &State) {
> + let path_str = context().state_file_path();
> +
> + if let Err(e) = state.persist(path_str) {
> + error!("could not persist state at '{path_str}': {e}");
> + }
> +}
> +
`refresh_targets` sounds very generic. How about
`periodic_maintenance_task` or something alike?
> +pub fn refresh_targets(config: &Config) -> Result<(), HttpError> {
> + let bus = Bus::from_config(config).map_err(|err| {
> + http_err!(
> + INTERNAL_SERVER_ERROR,
> + "Could not instantiate notification bus: {err}"
> + )
> + })?;
> +
> + let mut state = get_state();
> +
> + bus.refresh_targets(&mut state);
> +
> + persist_state(&state);
> +
> + Ok(())
> +}
>
> /// Send a notification to a given target.
> ///
> @@ -15,7 +60,11 @@ pub fn send(config: &Config, notification: &Notification) -> Result<(), HttpErro
> )
> })?;
>
> - bus.send(notification);
> + let mut state = get_state();
> +
> + bus.send(notification, &mut state);
> +
> + persist_state(&state);
>
> Ok(())
> }
> @@ -32,12 +81,17 @@ pub fn test_target(config: &Config, endpoint: &str) -> Result<(), HttpError> {
> )
> })?;
>
> - bus.test_target(endpoint).map_err(|err| match err {
> - crate::Error::TargetDoesNotExist(endpoint) => {
> - http_err!(NOT_FOUND, "endpoint '{endpoint}' does not exist")
> - }
> - _ => http_err!(INTERNAL_SERVER_ERROR, "Could not test target: {err}"),
> - })?;
> + let mut state = get_state();
> +
> + bus.test_target(endpoint, &mut state)
> + .map_err(|err| match err {
> + crate::Error::TargetDoesNotExist(endpoint) => {
> + http_err!(NOT_FOUND, "endpoint '{endpoint}' does not exist")
> + }
> + _ => http_err!(INTERNAL_SERVER_ERROR, "Could not test target: {err}"),
> + })?;
> +
> + persist_state(&state);
I wonder, should we attempt to persist the state in case of an error
here?
>
> Ok(())
> }
> diff --git a/proxmox-notify/src/endpoints/gotify.rs b/proxmox-notify/src/endpoints/gotify.rs
> index 3e12a60e..5f48fc0a 100644
> --- a/proxmox-notify/src/endpoints/gotify.rs
> +++ b/proxmox-notify/src/endpoints/gotify.rs
> @@ -12,7 +12,7 @@ use proxmox_schema::{api, Updater};
> use crate::context::context;
> use crate::renderer::TemplateType;
> use crate::schema::ENTITY_NAME_SCHEMA;
> -use crate::{renderer, Content, Endpoint, Error, Notification, Origin, Severity};
> +use crate::{renderer, Content, Endpoint, Error, Notification, Origin, Severity, State};
>
> const HTTP_TIMEOUT: Duration = Duration::from_secs(10);
>
> @@ -96,7 +96,7 @@ pub enum DeleteableGotifyProperty {
> }
>
> impl Endpoint for GotifyEndpoint {
> - fn send(&self, notification: &Notification) -> Result<(), Error> {
> + fn send(&self, notification: &Notification, _: &mut State) -> Result<(), Error> {
> let (title, message) = match ¬ification.content {
> Content::Template {
> template_name,
> diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
> index 70b0f111..d95a6872 100644
> --- a/proxmox-notify/src/endpoints/sendmail.rs
> +++ b/proxmox-notify/src/endpoints/sendmail.rs
> @@ -4,10 +4,10 @@ use serde::{Deserialize, Serialize};
> use proxmox_schema::api_types::COMMENT_SCHEMA;
> use proxmox_schema::{api, Updater};
>
> -use crate::context;
> use crate::endpoints::common::mail;
> use crate::renderer::TemplateType;
> use crate::schema::{EMAIL_SCHEMA, ENTITY_NAME_SCHEMA, USER_SCHEMA};
> +use crate::{context, State};
> use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
>
> pub(crate) const SENDMAIL_TYPENAME: &str = "sendmail";
> @@ -104,7 +104,7 @@ pub struct SendmailEndpoint {
> }
>
> impl Endpoint for SendmailEndpoint {
> - fn send(&self, notification: &Notification) -> Result<(), Error> {
> + fn send(&self, notification: &Notification, _: &mut State) -> Result<(), Error> {
> let recipients = mail::get_recipients(
> self.config.mailto.as_slice(),
> self.config.mailto_user.as_slice(),
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index c888dee7..69c4048c 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -15,6 +15,7 @@ use crate::endpoints::common::mail;
> use crate::renderer::TemplateType;
> use crate::schema::{EMAIL_SCHEMA, ENTITY_NAME_SCHEMA, USER_SCHEMA};
> use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
> +use crate::{xoauth2, EndpointState, State};
>
> pub(crate) const SMTP_TYPENAME: &str = "smtp";
>
> @@ -136,6 +137,16 @@ pub enum DeleteableSmtpProperty {
> Username,
> }
>
> +#[derive(Serialize, Deserialize, Clone, Debug, Default)]
> +#[serde(rename_all = "kebab-case")]
> +pub struct SmtpState {
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub oauth2_refresh_token: Option<String>,
> + pub last_refreshed: u64,
> +}
> +
> +impl EndpointState for SmtpState {}
> +
> #[api]
> #[derive(Serialize, Deserialize, Clone, Updater, Debug)]
> #[serde(rename_all = "kebab-case")]
> @@ -158,7 +169,9 @@ pub struct SmtpEndpoint {
> }
>
> impl Endpoint for SmtpEndpoint {
> - fn send(&self, notification: &Notification) -> Result<(), Error> {
> + fn send(&self, notification: &Notification, state: &mut State) -> Result<(), Error> {
> + let mut endpoint_state = state.get_or_default::<SmtpState>(self.name())?;
We talked about this off-list already, but it think would be cool if an
endpoint would not get the entire state container, but only it's *own*
state. I quickly tried this using an associated type in the trait for
the state type and making `send` generic, but unfortunately this did not
really work out - we store all endpoints in a HashMap<String, Box<dyn
Endpoint>.
This could probably be solved with some internal architectural changes,
removing the Box<dyn ...> and replace it with 'manual' dispatching
using an enum wrapper and `match`.
> +
> let tls_parameters = TlsParameters::new(self.config.server.clone())
> .map_err(|err| Error::NotifyFailed(self.name().into(), Box::new(err)))?;
>
> @@ -272,6 +285,8 @@ impl Endpoint for SmtpEndpoint {
> .send(&email)
> .map_err(|err| Error::NotifyFailed(self.name().into(), err.into()))?;
>
> + state.set(self.name(), &endpoint_state)?;
> +
> Ok(())
> }
>
> diff --git a/proxmox-notify/src/endpoints/webhook.rs b/proxmox-notify/src/endpoints/webhook.rs
> index 0167efcf..ce5bddf4 100644
> --- a/proxmox-notify/src/endpoints/webhook.rs
> +++ b/proxmox-notify/src/endpoints/webhook.rs
> @@ -27,7 +27,7 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, StringSchema, Update
> use crate::context::context;
> use crate::renderer::TemplateType;
> use crate::schema::ENTITY_NAME_SCHEMA;
> -use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
> +use crate::{renderer, Content, Endpoint, Error, Notification, Origin, State};
>
> /// This will be used as a section type in the public/private configuration file.
> pub(crate) const WEBHOOK_TYPENAME: &str = "webhook";
> @@ -240,7 +240,7 @@ pub const KEY_AND_BASE64_VALUE_SCHEMA: Schema =
>
> impl Endpoint for WebhookEndpoint {
> /// Send a notification to a webhook endpoint.
> - fn send(&self, notification: &Notification) -> Result<(), Error> {
> + fn send(&self, notification: &Notification, _: &mut State) -> Result<(), Error> {
> let request = self.build_request(notification)?;
>
> self.create_client()?
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index a40342cc..3bd83ce4 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -152,13 +152,18 @@ pub enum Origin {
> /// Notification endpoint trait, implemented by all endpoint plugins
> pub trait Endpoint {
> /// Send a documentation
> - fn send(&self, notification: &Notification) -> Result<(), Error>;
> + fn send(&self, notification: &Notification, state: &mut State) -> Result<(), Error>;
>
> /// The name/identifier for this endpoint
> fn name(&self) -> &str;
>
> /// Check if the endpoint is disabled
> fn disabled(&self) -> bool;
> +
> + /// Refresh the endpoint's state if required.
> + fn refresh_state(&self, _: &mut State) -> Result<bool, Error> {
> + Ok(false)
> + }
> }
>
> #[derive(Debug, Clone, Serialize, Deserialize)]
> @@ -599,7 +604,7 @@ impl Bus {
> /// the notification.
> ///
> /// Any errors will not be returned but only logged.
> - pub fn send(&self, notification: &Notification) {
> + pub fn send(&self, notification: &Notification, state: &mut State) {
> let targets = matcher::check_matches(self.matchers.as_slice(), notification);
>
> for target in targets {
> @@ -612,7 +617,7 @@ impl Bus {
> continue;
> }
>
> - match endpoint.send(notification) {
> + match endpoint.send(notification, state) {
> Ok(_) => {
> info!("notified via target `{name}`");
> }
> @@ -631,7 +636,7 @@ impl Bus {
> ///
> /// In contrast to the `send` function, this function will return
> /// any errors to the caller.
> - pub fn test_target(&self, target: &str) -> Result<(), Error> {
> + pub fn test_target(&self, target: &str, state: &mut State) -> Result<(), Error> {
> let notification = Notification {
> metadata: Metadata {
> severity: Severity::Info,
> @@ -647,13 +652,21 @@ impl Bus {
> };
>
> if let Some(endpoint) = self.endpoints.get(target) {
> - endpoint.send(¬ification)?;
> + endpoint.send(¬ification, state)?;
> } else {
> return Err(Error::TargetDoesNotExist(target.to_string()));
> }
>
> Ok(())
> }
> +
> + pub fn refresh_targets(&self, state: &mut State) {
> + for (name, endpoint) in &self.endpoints {
> + if let Err(e) = endpoint.refresh_state(state) {
> + error!("could not refresh state for endpoint '{name}': {e}");
> + }
> + }
> + }
same here regarding the name of the function
> }
>
> #[cfg(test)]
> @@ -671,7 +684,7 @@ mod tests {
> }
>
> impl Endpoint for MockEndpoint {
> - fn send(&self, message: &Notification) -> Result<(), Error> {
> + fn send(&self, message: &Notification, _: &mut State) -> Result<(), Error> {
> self.messages.borrow_mut().push(message.clone());
>
> Ok(())
> @@ -714,12 +727,15 @@ mod tests {
> bus.add_matcher(matcher);
>
> // Send directly to endpoint
> - bus.send(&Notification::from_template(
> - Severity::Info,
> - "test",
> - Default::default(),
> - Default::default(),
> - ));
> + bus.send(
> + &Notification::from_template(
> + Severity::Info,
> + "test",
> + Default::default(),
> + Default::default(),
> + ),
> + &mut State::default(),
> + );
> let messages = mock.messages();
> assert_eq!(messages.len(), 1);
>
> @@ -758,7 +774,7 @@ mod tests {
> Default::default(),
> );
>
> - bus.send(¬ification);
> + bus.send(¬ification, &mut State::default());
> };
>
> send_with_severity(Severity::Info);
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support
2026-02-04 16:13 ` [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support Arthur Bied-Charreton
@ 2026-02-10 15:52 ` Lukas Wagner
2026-02-11 13:00 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-10 15:52 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
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 <a.bied-charreton@proxmox.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints
2026-02-04 16:13 ` [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints Arthur Bied-Charreton
@ 2026-02-11 8:55 ` Lukas Wagner
2026-02-11 12:47 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-11 8:55 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Add auth-method, as well as optional
> oauth2-{client-id,client-secret,tenant-id,refresh-token} parameters to
> prepare for OAuth2 support.
>
> The auth-method parameter was previously implicit and inferred by
> proxmox-notify based on the presence of a password. It is now made
> explicit, however still kept optional and explicitly inferred in the
> {update,create}_endpoint handlers to avoid breaking the API.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> PVE/API2/Cluster/Notifications.pm | 55 +++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
> index 8b455227..a45a15b2 100644
> --- a/PVE/API2/Cluster/Notifications.pm
> +++ b/PVE/API2/Cluster/Notifications.pm
> @@ -941,6 +941,13 @@ my $smtp_properties = {
> default => 'tls',
> optional => 1,
> },
> + 'auth-method' => {
> + description =>
> + 'Determine which authentication method shall be used for the connection.',
> + type => 'string',
> + enum => [qw(google-oauth2 microsoft-oauth2 plain none)],
> + optional => 1,
> + },
> username => {
> description => 'Username for SMTP authentication',
> type => 'string',
> @@ -951,6 +958,26 @@ my $smtp_properties = {
> type => 'string',
> optional => 1,
> },
> + 'oauth2-client-id' => {
> + description => 'OAuth2 client ID',
> + type => 'string',
> + optional => 1,
> + },
> + 'oauth2-client-secret' => {
> + description => 'OAuth2 client secret',
> + type => 'string',
> + optional => 1,
> + },
> + 'oauth2-tenant-id' => {
> + description => 'OAuth2 tenant ID',
> + type => 'string',
> + optional => 1,
> + },
> + 'oauth2-refresh-token' => {
> + description => 'OAuth2 refresh token',
> + type => 'string',
> + optional => 1,
> + },
> mailto => {
> type => 'array',
> items => {
> @@ -1108,6 +1135,11 @@ __PACKAGE__->register_method({
> my $mode = extract_param($param, 'mode');
> my $username = extract_param($param, 'username');
> my $password = extract_param($param, 'password');
> + my $auth_method = extract_param($param, 'auth-method');
> + my $oauth2_client_secret = extract_param($param, 'oauth2-client-secret');
> + my $oauth2_client_id = extract_param($param, 'oauth2-client-id');
> + my $oauth2_tenant_id = extract_param($param, 'oauth2-tenant-id');
> + my $oauth2_refresh_token = extract_param($param, 'oauth2-refresh-token');
> my $mailto = extract_param($param, 'mailto');
> my $mailto_user = extract_param($param, 'mailto-user');
> my $from_address = extract_param($param, 'from-address');
> @@ -1115,6 +1147,10 @@ __PACKAGE__->register_method({
> my $comment = extract_param($param, 'comment');
> my $disable = extract_param($param, 'disable');
>
> + if (!defined $auth_method) {
> + $auth_method = defined($password) ? 'plain' : 'none';
> + }
> +
> eval {
> PVE::Notify::lock_config(sub {
> my $config = PVE::Notify::read_config();
> @@ -1126,6 +1162,11 @@ __PACKAGE__->register_method({
> $mode,
> $username,
> $password,
> + $auth_method,
> + $oauth2_client_id,
> + $oauth2_client_secret,
> + $oauth2_tenant_id,
> + $oauth2_refresh_token,
> $mailto,
> $mailto_user,
> $from_address,
> @@ -1187,6 +1228,11 @@ __PACKAGE__->register_method({
> my $mode = extract_param($param, 'mode');
> my $username = extract_param($param, 'username');
> my $password = extract_param($param, 'password');
> + my $auth_method = extract_param($param, 'auth-method');
> + my $oauth2_client_secret = extract_param($param, 'oauth2-client-secret');
> + my $oauth2_client_id = extract_param($param, 'oauth2-client-id');
> + my $oauth2_tenant_id = extract_param($param, 'oauth2-tenant-id');
> + my $oauth2_refresh_token = extract_param($param, 'oauth2-refresh-token');
> my $mailto = extract_param($param, 'mailto');
> my $mailto_user = extract_param($param, 'mailto-user');
> my $from_address = extract_param($param, 'from-address');
> @@ -1197,6 +1243,10 @@ __PACKAGE__->register_method({
> my $delete = extract_param($param, 'delete');
> my $digest = extract_param($param, 'digest');
>
> + if (!defined $auth_method) {
> + $auth_method = defined($password) ? 'plain' : 'none';
> + }
> +
> eval {
> PVE::Notify::lock_config(sub {
> my $config = PVE::Notify::read_config();
> @@ -1208,6 +1258,11 @@ __PACKAGE__->register_method({
> $mode,
> $username,
> $password,
> + $auth_method,
> + $oauth2_client_id,
> + $oauth2_client_secret,
> + $oauth2_tenant_id,
> + $oauth2_refresh_token,
> $mailto,
> $mailto_user,
> $from_address,
As already explained off-list, I think it's time to switch from a flat
list of parameters to passing hashes for the parameters for the
`add_smtp_target` and `update_smtp_target` methods. This means, the
bindings in proxmox-perl-rs would then directly take
SmtpConfig/SmtpPrivateConfig and
SmtpConfigUpdater/SmtpPrivateConfigUpdater. Then the call could look
something like (not tested)
$config->add_smtp_endpoint(
$name,
{
server => $server,
port => $port,
...
},
{
password => $password,
...
}
);
This makes it much harder to introduce bugs due to parameter ordering.
Long-term we should do the same for the other endpoints, but no need to
do it in this series, I think.
For changes like these and in general it's pretty important to mention
any breaking changes in the cover letter and maybe patch description,
since the changes done in this series affect multiple packages that
*could* be updated independently by our users. For instance, in the
cover-letter you could write something like:
The patch series requires the following version requirement bumps:
pve-manager requires bumped proxmox-perl-rs
proxmox-perl-rs requires bumped proxmox-notify*
*.) although for this one it's not that critical, since its only a
build-dependency, so there is no chance of customer systems breaking due
to partial system updates
This way the maintainer knows that the version requirements in
debian/control must be adapted at some point after applying the patches.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-manager 4/5] notifications: Handle OAuth2 callback in login handler
2026-02-04 16:13 ` [PATCH pve-manager 4/5] notifications: Handle OAuth2 callback in login handler Arthur Bied-Charreton
@ 2026-02-11 9:00 ` Lukas Wagner
0 siblings, 0 replies; 34+ messages in thread
From: Lukas Wagner @ 2026-02-11 9:00 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> The OAuth2 flow redirects to the service's origin
> (window.location.origin) after successful authentication.
>
> The callback handler infers whether the login was triggered as the
> result of an OAuth2 redirect based on the presence of the code, scope,
> and state URL parameters. It then communicates the authentication
> results back to the parent window, which is responsible for closing it.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> www/manager6/Workspace.js | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
> index b8061c2a..1e79dd57 100644
> --- a/www/manager6/Workspace.js
> +++ b/www/manager6/Workspace.js
> @@ -150,9 +150,29 @@ Ext.define('PVE.StdWorkspace', {
> me.down('pveResourceTree').selectById(nodeid);
> },
>
> + handleOauth2Callback: function (params) {
> + const code = params.get('code');
> + const scope = params.get('scope');
> + const state = params.get('state');
Regarding the use of `const`, check out our JavaScript styleguide [1];
it says:
Avoid using const for everything, but rather in the sense of
constants. JavaScript is too dynamic for it to provide actual benefits
if used as default.
[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
> +
> + // If true, this window was opened by the OAuth2 button handler from the
> + // SMTP notification targets edit panel.
> + //
> + // Since we got here through a redirect, this window is not script-closable,
> + // and we rely on the parent window to close it in its broadcast channel's
> + // message handler.
> + if (code && scope && state) {
> + const { channelName } = JSON.parse(decodeURIComponent(state));
> + const bc = new BroadcastChannel(channelName);
> + bc.postMessage({ code, scope });
> + }
> + },
> +
> onLogin: function (loginData) {
> let me = this;
>
> + me.handleOauth2Callback(new URLSearchParams(window.location.search));
> +
> me.updateUserInfo();
>
> if (loginData) {
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint
2026-02-04 16:13 ` [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint Arthur Bied-Charreton
@ 2026-02-11 9:49 ` Lukas Wagner
2026-02-11 12:44 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-11 9:49 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> This endpoint allows triggering a refresh of the notification targets'
> state, e.g., to prevent OAuth2 refresh tokens from expiring.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> PVE/API2/Cluster/Notifications.pm | 34 +++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
> index a45a15b2..f993817d 100644
> --- a/PVE/API2/Cluster/Notifications.pm
> +++ b/PVE/API2/Cluster/Notifications.pm
> @@ -321,6 +321,40 @@ __PACKAGE__->register_method({
> },
> });
>
> +__PACKAGE__->register_method({
> + name => "refresh_targets",
> + path => 'refresh-targets',
Same note here regarding naming, I think 'refresh-targets' is a bit too
generic for my taste. Either we fully narrow it down to
'refresh-oauth-tokens', or make it rather general, e.g.
'trigger-periodic-maintenance' (or something similar, you get the
general direction), covering the case that *maybe* some other endpoint
could need some periodic action as well.
Also, do have any plans of exposing this in the GUI somehow? It's
definitely nice to have this available via pvesh for manual
troubleshooting by an admin anyways, so the API endpoint makes sense;
but as far as I can tell you do not use this endpoint anywhere in the
GUI code; hence I'm asking.
> + protected => 1,
> + method => 'POST',
> + description => 'Refresh state for all targets',
> + permissions => {
> + check => [
> + 'and',
> + ['perm', '/mapping/notifications', ['Mapping.Modify']],
> + [
> + 'or',
> + ['perm', '/', ['Sys.Audit', 'Sys.Modify']],
> + ['perm', '/', ['Sys.AccessNetwork']],
> + ],
> + ],
> + },
> + parameters => {
> + additionalProperties => 0,
> + properties => {},
> + },
> + returns => { type => 'null' },
> + code => sub {
> + eval {
> + my $config = PVE::Notify::read_config();
> + $config->refresh_targets();
> + };
> +
> + raise_api_error($@) if $@;
> +
> + return;
> + },
> +});
> +
> __PACKAGE__->register_method({
> name => 'test_target',
> path => 'targets/{name}/test',
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs
2026-02-04 16:13 ` [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs Arthur Bied-Charreton
@ 2026-02-11 10:06 ` Lukas Wagner
2026-02-11 13:15 ` Arthur Bied-Charreton
0 siblings, 1 reply; 34+ messages in thread
From: Lukas Wagner @ 2026-02-11 10:06 UTC (permalink / raw)
To: Arthur Bied-Charreton, pve-devel
Hi!
Nice work overall, some hints for some refinements for the next version
inline.
On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Document the new config entries, and add notes/warnings to communicate
> that:
>
> 1. User intervention is required for initial OAuth2 target setup, and
>
> 2. Microsoft OAuth2 apps *must not* be configured as SPAs by the
> user, since it would prevent PVE from automatically extending the
> refresh token's lifetime
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> notifications.adoc | 44 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/notifications.adoc b/notifications.adoc
> index 801b327..679b19b 100644
> --- a/notifications.adoc
> +++ b/notifications.adoc
> @@ -108,16 +108,23 @@ The configuration for SMTP target plugins has the following options:
> * `from-address`: Sets the From-address of the email. SMTP relays might require
> that this address is owned by the user in order to avoid spoofing. The `From`
> header in the email will be set to `$author <$from-address>`.
> +* `auth-method`: Sets the authentication method (`plain`, `google-oauth2` or
> + `microsoft-oauth2`).
> * `username`: Username to use during authentication. If no username is set,
> no authentication will be performed. The PLAIN and LOGIN authentication
> methods are supported.
> * `password`: Password to use when authenticating.
> +* `oauth2-client-id`: Client ID for the OAuth2 application, if applicable.
> +* `oauth2-client-secret`: Client secret for the OAuth2 application, if
> + applicable.
> +* `oauth2-tenant-id`: Tenant ID for the OAuth2 application, if applicable.
> + Only required for Microsoft OAuth2.
> * `mode`: Sets the encryption mode (`insecure`, `starttls` or `tls`). Defaults
> to `tls`.
> * `server`: Address/IP of the SMTP relay.
> -* `port`: The port to connect to. If not set, the used port .
> - Defaults to 25 (`insecure`), 465 (`tls`) or 587 (`starttls`), depending on
> - the value of `mode`.
> +* `port`: The port to connect to. If not set, the used port defaults to 25
> + (`insecure`), 465 (`tls`) or 587 (`starttls`), depending on the value of
> + `mode`.
> * `comment`: Comment for this target
Thanks for fixing this up, but since this is completely unrelated to
your OAuth changes, this should rather be a separate commit (as the
first-patch of the pve-docs part of the series - since then it can be
applied independently while this series as a whole is still in progress)
>
> Example configuration (`/etc/pve/notifications.cfg`):
> @@ -133,13 +140,42 @@ smtp: example
> ----
>
> The matching entry in `/etc/pve/priv/notifications.cfg`, containing the
> -secret token:
> +password:
>
> ----
> smtp: example
> password somepassword
> ----
this here as well
>
> +[[notification_targets_smtp_oauth2]]
> +===== OAuth2 Authentication
> +
> +SMTP targets also support OAuth2 authentication via the XOAUTH2 mechanism for
> +Google and Microsoft mail providers.
> +
> +Setting up OAuth2 authentication requires creating an OAuth2 application with
> +the chosen provider. The application must be configured with a redirect URI
> +pointing to the {pve} web interface, i.e. the URL from which the initial
> +authentication request is performed in the UI.
I guess you could also mention that one could add all cluster nodes as
permitted origins and redirect URIs. It would also be good to maybe add
some concrete examples for sensible origins and redirect URIs,
mentioning common restrictions (e.g. Google not allowing IPs)
> +
> +CAUTION: For Microsoft, the application must *not* be registered as Single-Page
> +Application (SPA), as the lifetime of refresh tokens granted for SPAs cannot
> +be extended automatically by {pve}.
> +
> +To set up OAuth2 authentication via the web interface, select `OAuth2 (Google)`
> +or `OAuth2 (Microsoft)` as the authentication method, fill in the client ID and
> +secret (and the tenant ID for Microsoft), then click the *Authenticate* button.
> +This opens a new window where you can sign in with the selected provider and
> +grant the required permissions. Upon successful authentication, a refresh
> +token is obtained and stored automatically.
> +
> +Token refresh happens automatically, manual intervention is only needed if a
> +token is revoked.
Maybe elaborate what 'manual intervention' means in this case (I assume
re-authorize?) . Also could not hurt to mention the pvesh command to
trigger a manual token refresh.
> +
> +NOTE: OAuth2 is currently not configurable through direct configuration file
> +editing because the refresh token is managed as dynamic state by {pve}. All
> +OAuth2 targets must be configured via the web interface.
Maybe mention that one could also add the endpoint by using the
appropriate API endoint, supplying a token that they requested by other
means.
> +
> [[notification_targets_gotify]]
> Gotify
> ~~~~~~
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint
2026-02-11 9:49 ` Lukas Wagner
@ 2026-02-11 12:44 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-11 12:44 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Wed, Feb 11, 2026 at 10:49:45AM +0100, Lukas Wagner wrote:
> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> > This endpoint allows triggering a refresh of the notification targets'
> > state, e.g., to prevent OAuth2 refresh tokens from expiring.
> >
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> > PVE/API2/Cluster/Notifications.pm | 34 +++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
> > index a45a15b2..f993817d 100644
> > --- a/PVE/API2/Cluster/Notifications.pm
> > +++ b/PVE/API2/Cluster/Notifications.pm
> > @@ -321,6 +321,40 @@ __PACKAGE__->register_method({
> > },
> > });
> >
> > +__PACKAGE__->register_method({
> > + name => "refresh_targets",
> > + path => 'refresh-targets',
>
> Same note here regarding naming, I think 'refresh-targets' is a bit too
> generic for my taste. Either we fully narrow it down to
> 'refresh-oauth-tokens', or make it rather general, e.g.
> 'trigger-periodic-maintenance' (or something similar, you get the
> general direction), covering the case that *maybe* some other endpoint
> could need some periodic action as well.
>
You're right, the naming is kind of a WIP.. Maybe something
like 'trigger-state-refresh' would make sense? 'refresh-oauth-tokens'
is a bit too narrow imo, we would have to change it/add a new one again
if proxmox-notify needs more state in the future
> Also, do have any plans of exposing this in the GUI somehow? It's
> definitely nice to have this available via pvesh for manual
> troubleshooting by an admin anyways, so the API endpoint makes sense;
> but as far as I can tell you do not use this endpoint anywhere in the
> GUI code; hence I'm asking.
>
I don't mean to use it in the UI no, it is intended for pvesh
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints
2026-02-11 8:55 ` Lukas Wagner
@ 2026-02-11 12:47 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-11 12:47 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Wed, Feb 11, 2026 at 09:55:23AM +0100, Lukas Wagner wrote:
> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> > Add auth-method, as well as optional
> > oauth2-{client-id,client-secret,tenant-id,refresh-token} parameters to
> > prepare for OAuth2 support.
> >
> > The auth-method parameter was previously implicit and inferred by
> > proxmox-notify based on the presence of a password. It is now made
> > explicit, however still kept optional and explicitly inferred in the
> > {update,create}_endpoint handlers to avoid breaking the API.
> >
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> > PVE/API2/Cluster/Notifications.pm | 55 +++++++++++++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> > [...]
> > eval {
> > PVE::Notify::lock_config(sub {
> > my $config = PVE::Notify::read_config();
> > @@ -1208,6 +1258,11 @@ __PACKAGE__->register_method({
> > $mode,
> > $username,
> > $password,
> > + $auth_method,
> > + $oauth2_client_id,
> > + $oauth2_client_secret,
> > + $oauth2_tenant_id,
> > + $oauth2_refresh_token,
> > $mailto,
> > $mailto_user,
> > $from_address,
>
> As already explained off-list, I think it's time to switch from a flat
> list of parameters to passing hashes for the parameters for the
> `add_smtp_target` and `update_smtp_target` methods. This means, the
> bindings in proxmox-perl-rs would then directly take
> SmtpConfig/SmtpPrivateConfig and
> SmtpConfigUpdater/SmtpPrivateConfigUpdater. Then the call could look
> something like (not tested)
>
> $config->add_smtp_endpoint(
> $name,
> {
> server => $server,
> port => $port,
> ...
> },
> {
> password => $password,
> ...
> }
> );
>
> This makes it much harder to introduce bugs due to parameter ordering.
> Long-term we should do the same for the other endpoints, but no need to
> do it in this series, I think.
>
That makes a lot of sense, will update it
> For changes like these and in general it's pretty important to mention
> any breaking changes in the cover letter and maybe patch description,
> since the changes done in this series affect multiple packages that
> *could* be updated independently by our users. For instance, in the
> cover-letter you could write something like:
>
> The patch series requires the following version requirement bumps:
>
> pve-manager requires bumped proxmox-perl-rs
> proxmox-perl-rs requires bumped proxmox-notify*
>
>
> *.) although for this one it's not that critical, since its only a
> build-dependency, so there is no chance of customer systems breaking due
> to partial system updates
>
> This way the maintainer knows that the version requirements in
> debian/control must be adapted at some point after applying the patches.
I was wondering how this would work, thanks for the explanation!
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support
2026-02-10 15:52 ` Lukas Wagner
@ 2026-02-11 13:00 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-11 13:00 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Tue, Feb 10, 2026 at 04:52:51PM +0100, Lukas Wagner wrote:
> 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 <a.bied-charreton@proxmox.com>
> > [...]
> > + 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
>
I was looking for something like that, thanks :)
> > + .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
>
> > [...]
> > + 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
>
> > [...]
> > +/// 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.
>
[...]
> 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.
>
Thanks for all the feedback, I will address all of it in v2
> > + {
> > + 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`).
>
The plan was originally to be able to only persist the state if
something changed yes, however with the new approach we discussed
off-list, each endpoint will only be passed its own state (as opposed to
currently, getting the state for all endpoints and being responsible for
looking up its own).
I was thinking about changing send() and refresh_state() to return an
Option<S> instead, that way we avoid the output parameter and the bus
can determine whether it needs to update the global state itself.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs
2026-02-11 10:06 ` Lukas Wagner
@ 2026-02-11 13:15 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-11 13:15 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Wed, Feb 11, 2026 at 11:06:24AM +0100, Lukas Wagner wrote:
> Hi!
>
> Nice work overall, some hints for some refinements for the next version
> inline.
>
> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> > Document the new config entries, and add notes/warnings to communicate
> > that:
> >
> > 1. User intervention is required for initial OAuth2 target setup, and
> >
> > 2. Microsoft OAuth2 apps *must not* be configured as SPAs by the
> > user, since it would prevent PVE from automatically extending the
> > refresh token's lifetime
> >
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> > notifications.adoc | 44 ++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/notifications.adoc b/notifications.adoc
> > index 801b327..679b19b 100644
> > --- a/notifications.adoc
> > +++ b/notifications.adoc
> > @@ -108,16 +108,23 @@ The configuration for SMTP target plugins has the following options:
> > * `from-address`: Sets the From-address of the email. SMTP relays might require
> > that this address is owned by the user in order to avoid spoofing. The `From`
> > header in the email will be set to `$author <$from-address>`.
> > +* `auth-method`: Sets the authentication method (`plain`, `google-oauth2` or
> > + `microsoft-oauth2`).
> > * `username`: Username to use during authentication. If no username is set,
> > no authentication will be performed. The PLAIN and LOGIN authentication
> > methods are supported.
> > * `password`: Password to use when authenticating.
> > +* `oauth2-client-id`: Client ID for the OAuth2 application, if applicable.
> > +* `oauth2-client-secret`: Client secret for the OAuth2 application, if
> > + applicable.
> > +* `oauth2-tenant-id`: Tenant ID for the OAuth2 application, if applicable.
> > + Only required for Microsoft OAuth2.
> > * `mode`: Sets the encryption mode (`insecure`, `starttls` or `tls`). Defaults
> > to `tls`.
> > * `server`: Address/IP of the SMTP relay.
>
>
>
> > -* `port`: The port to connect to. If not set, the used port .
> > - Defaults to 25 (`insecure`), 465 (`tls`) or 587 (`starttls`), depending on
> > - the value of `mode`.
> > +* `port`: The port to connect to. If not set, the used port defaults to 25
> > + (`insecure`), 465 (`tls`) or 587 (`starttls`), depending on the value of
> > + `mode`.
> > * `comment`: Comment for this target
>
> Thanks for fixing this up, but since this is completely unrelated to
> your OAuth changes, this should rather be a separate commit (as the
> first-patch of the pve-docs part of the series - since then it can be
> applied independently while this series as a whole is still in progress)
>
> >
> > Example configuration (`/etc/pve/notifications.cfg`):
> > @@ -133,13 +140,42 @@ smtp: example
> > ----
> >
> > The matching entry in `/etc/pve/priv/notifications.cfg`, containing the
> > -secret token:
> > +password:
> >
> > ----
> > smtp: example
> > password somepassword
> > ----
>
> this here as well
>
Thanks, sent a separate patch:
https://lore.proxmox.com/pve-devel/20260211131323.232299-1-a.bied-charreton@proxmox.com/T/#u
> >
> > +[[notification_targets_smtp_oauth2]]
> > +===== OAuth2 Authentication
> > +
> > +SMTP targets also support OAuth2 authentication via the XOAUTH2 mechanism for
> > +Google and Microsoft mail providers.
> > +
> > +Setting up OAuth2 authentication requires creating an OAuth2 application with
> > +the chosen provider. The application must be configured with a redirect URI
> > +pointing to the {pve} web interface, i.e. the URL from which the initial
> > +authentication request is performed in the UI.
>
> I guess you could also mention that one could add all cluster nodes as
> permitted origins and redirect URIs. It would also be good to maybe add
> some concrete examples for sensible origins and redirect URIs,
> mentioning common restrictions (e.g. Google not allowing IPs)
>
> > +
> > +CAUTION: For Microsoft, the application must *not* be registered as Single-Page
> > +Application (SPA), as the lifetime of refresh tokens granted for SPAs cannot
> > +be extended automatically by {pve}.
> > +
> > +To set up OAuth2 authentication via the web interface, select `OAuth2 (Google)`
> > +or `OAuth2 (Microsoft)` as the authentication method, fill in the client ID and
> > +secret (and the tenant ID for Microsoft), then click the *Authenticate* button.
> > +This opens a new window where you can sign in with the selected provider and
> > +grant the required permissions. Upon successful authentication, a refresh
> > +token is obtained and stored automatically.
> > +
> > +Token refresh happens automatically, manual intervention is only needed if a
> > +token is revoked.
>
> Maybe elaborate what 'manual intervention' means in this case (I assume
> re-authorize?) . Also could not hurt to mention the pvesh command to
> trigger a manual token refresh.
>
> > +
> > +NOTE: OAuth2 is currently not configurable through direct configuration file
> > +editing because the refresh token is managed as dynamic state by {pve}. All
> > +OAuth2 targets must be configured via the web interface.
>
> Maybe mention that one could also add the endpoint by using the
> appropriate API endoint, supplying a token that they requested by other
> means.
>
> > +
> > [[notification_targets_gotify]]
> > Gotify
> > ~~~~~~
>
Thanks, will update the docs accordingly!
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State
2026-02-10 15:52 ` Lukas Wagner
@ 2026-02-12 8:26 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-12 8:26 UTC (permalink / raw)
To: Lukas Wagner; +Cc: pve-devel
On Tue, Feb 10, 2026 at 04:52:14PM +0100, Lukas Wagner wrote:
> Hey Arthur,
>
> some comments inline.
>
> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> > This lays the groundwork for handling OAuth2 state in proxmox-notify by
> > updating the crate's internal API to pass around a mutable State struct.
> >
> > The state can be updated by its consumers and will be persisted
> > afterwards, much like the Config struct, with the difference that it is
> > fully internal to the crate. External consumers of the API do not need
> > to know/care about it.
> >
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> > proxmox-notify/src/api/common.rs | 70 +++++++++++++++++++++---
> > proxmox-notify/src/endpoints/gotify.rs | 4 +-
> > proxmox-notify/src/endpoints/sendmail.rs | 4 +-
> > proxmox-notify/src/endpoints/smtp.rs | 17 +++++-
> > proxmox-notify/src/endpoints/webhook.rs | 4 +-
> > proxmox-notify/src/lib.rs | 42 +++++++++-----
> > 6 files changed, 113 insertions(+), 28 deletions(-)
> >
> > diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs
> > index fa2356e2..3483be9a 100644
> > --- a/proxmox-notify/src/api/common.rs
> > +++ b/proxmox-notify/src/api/common.rs
> > @@ -1,7 +1,52 @@
> > use proxmox_http_error::HttpError;
> >
> > use super::http_err;
> > -use crate::{Bus, Config, Notification};
> > +use crate::{context::context, Bus, Config, Notification, State};
> > +
> > +use tracing::error;
> > +
> > +fn get_state() -> State {
> > + let path_str = context().state_file_path();
> > + let path = std::path::Path::new(path_str);
> > +
> > + match path.exists() {
> > + true => match State::from_path(path) {
> > + Ok(s) => s,
> > + Err(e) => {
> > + // We don't want to block notifications on all endpoints only
> > + // because the stateful ones are broken.
> > + error!("could not instantiate state from {path_str}: {e}",);
> > + State::default()
> > + }
> > + },
> > + false => State::default(),
> > + }
> > +}
>
> This is a good example of TOCTOU (time of check, time of use). First you
> check if something exists in the filesystem, and if it does, you read
> the file. In theory, it could happen that the file is removed in between
> these two lines, leading to an error when reading the file. Now, in
> this concrete case here this is not a huge issue, but nevertheless its
> better to avoid it if you can. In this case here you can just read the
> file and then in case of an error check for ENOENT. See [1]
> for a very similar example.
>
> [1] https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=server/src/remote_updates.rs;h=e772eef510218d8da41fe4a88ee47847b2598045;hb=HEAD#l159
>
> In proxmox-sys there are also the file_read_optional_string or
> file_get_optional_contents helpers which should do just that.
>
Makes sense, thanks!
> > +
> > +fn persist_state(state: &State) {
> > + let path_str = context().state_file_path();
> > +
> > + if let Err(e) = state.persist(path_str) {
> > + error!("could not persist state at '{path_str}': {e}");
> > + }
> > +}
> > +
>
> `refresh_targets` sounds very generic. How about
> `periodic_maintenance_task` or something alike?
>
See
https://lore.proxmox.com/pve-devel/3kqo4fxy4y3lrkhv7exd57ap6llkds2sxrn7gqj6wfxbo5zrvc@pvacwvkdp3zi/
> > +pub fn refresh_targets(config: &Config) -> Result<(), HttpError> {
> > + let bus = Bus::from_config(config).map_err(|err| {
> > + http_err!(
> > + INTERNAL_SERVER_ERROR,
> > + "Could not instantiate notification bus: {err}"
> > + )
> > + })?;
> > +
> > + let mut state = get_state();
> > +
> > + bus.refresh_targets(&mut state);
> > +
> > + persist_state(&state);
> > +
> > + Ok(())
> > +}
>
> I wonder, should we attempt to persist the state in case of an error
> here?
Yes we should, I overlooked that, thanks for catching it
> > +#[derive(Serialize, Deserialize, Clone, Debug, Default)]
> > +#[serde(rename_all = "kebab-case")]
> > +pub struct SmtpState {
> > + #[serde(skip_serializing_if = "Option::is_none")]
> > + pub oauth2_refresh_token: Option<String>,
> > + pub last_refreshed: u64,
> > +}
> > +
> > +impl EndpointState for SmtpState {}
> > +
> > #[api]
> > #[derive(Serialize, Deserialize, Clone, Updater, Debug)]
> > #[serde(rename_all = "kebab-case")]
> > @@ -158,7 +169,9 @@ pub struct SmtpEndpoint {
> > }
> >
> > impl Endpoint for SmtpEndpoint {
> > - fn send(&self, notification: &Notification) -> Result<(), Error> {
> > + fn send(&self, notification: &Notification, state: &mut State) -> Result<(), Error> {
> > + let mut endpoint_state = state.get_or_default::<SmtpState>(self.name())?;
>
> We talked about this off-list already, but it think would be cool if an
> endpoint would not get the entire state container, but only it's *own*
> state. I quickly tried this using an associated type in the trait for
> the state type and making `send` generic, but unfortunately this did not
> really work out - we store all endpoints in a HashMap<String, Box<dyn
> Endpoint>.
>
> This could probably be solved with some internal architectural changes,
> removing the Box<dyn ...> and replace it with 'manual' dispatching
> using an enum wrapper and `match`.
>
>
I am working on that, using an enum seems like the right choice here, it
feels a lot better (modulo the extra dispatching/variant unwrapping
boilerplate, but each concrete endpoint having to get its own state from
a generic pool is not much better).
^ permalink raw reply [flat|nested] 34+ messages in thread
* superseded: [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
` (14 preceding siblings ...)
2026-02-04 16:13 ` [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs Arthur Bied-Charreton
@ 2026-02-13 16:06 ` Arthur Bied-Charreton
15 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-13 16:06 UTC (permalink / raw)
To: pve-devel
Superseded-by: https://lore.proxmox.com/pve-devel/20260213160415.609868-1-a.bied-charreton@proxmox.com/T/#t
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs
2026-02-13 16:03 [PATCH cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/17] " Arthur Bied-Charreton
@ 2026-02-13 16:04 ` Arthur Bied-Charreton
0 siblings, 0 replies; 34+ messages in thread
From: Arthur Bied-Charreton @ 2026-02-13 16:04 UTC (permalink / raw)
To: pve-devel
Document the new config entries and the requirements to use XOAUTH2 for SMTP
notification targets, and add notes/warnings to communicate that:
1. User intervention is required for initial OAuth2 target setup, and
2. Microsoft OAuth2 apps *must not* be configured as SPAs by the user,
since it would prevent PVE from automatically extending the refresh
token's lifetime
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
notifications.adoc | 99 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/notifications.adoc b/notifications.adoc
index 801b327..1ff0552 100644
--- a/notifications.adoc
+++ b/notifications.adoc
@@ -108,10 +108,17 @@ The configuration for SMTP target plugins has the following options:
* `from-address`: Sets the From-address of the email. SMTP relays might require
that this address is owned by the user in order to avoid spoofing. The `From`
header in the email will be set to `$author <$from-address>`.
+* `auth-method`: Sets the authentication method (`plain`, `google-oauth2` or
+ `microsoft-oauth2`).
* `username`: Username to use during authentication. If no username is set,
no authentication will be performed. The PLAIN and LOGIN authentication
methods are supported.
* `password`: Password to use when authenticating.
+* `oauth2-client-id`: Client ID for the OAuth2 application, if applicable.
+* `oauth2-client-secret`: Client secret for the OAuth2 application, if
+ applicable.
+* `oauth2-tenant-id`: Tenant ID for the OAuth2 application, if applicable.
+ Only required for Microsoft OAuth2.
* `mode`: Sets the encryption mode (`insecure`, `starttls` or `tls`). Defaults
to `tls`.
* `server`: Address/IP of the SMTP relay.
@@ -140,6 +147,98 @@ smtp: example
password somepassword
----
+[[notification_targets_smtp_oauth2]]
+==== OAuth2 Authentication
+
+{pve} supports OAuth2 authentication for SMTP targets via the XOAUTH2
+mechanism. This is currently available for Google and Microsoft mail providers.
+
+===== Creating an OAuth2 Application
+
+Before configuring OAuth2 in {pve}, you must register an OAuth2 application
+with your mail provider:
+
+* https://developers.google.com/identity/protocols/oauth2/web-server[Google]
+* https://learn.microsoft.com/en-us/entra/identity-platform/quickstart-register-app[Microsoft Entra ID]
+
+Choose *Web application* as application type.
+
+During registration, add a redirect URI pointing to the {pve} web interface
+URL from which you will perform the authorization flow, for example:
+
+* `https://pve1.yourdomain.com:8006`
+* `https://localhost:8006`
+
+You can add multiple redirect URIs to allow the authorization flow to work from
+any cluster node.
+
+NOTE: Google does not allow bare IP addresses as redirect URIs. See
+<<notification_targets_smtp_oauth2_google,Google-specific setup>> below for a
+workaround.
+
+===== Configuring OAuth2 in {pve}
+
+In the web UI, open the notification target's edit panel and select
+`OAuth2 (Google)` or `OAuth2 (Microsoft)` as the authentication method. Fill in
+the client ID and secret. For Microsoft, also fill in the tenant ID.
+
+Click *Authenticate*. This opens a new window where you can sign in with your
+mail provider and grant the requested permissions. On success, a refresh token
+is obtained and stored.
+
+Token refresh happens automatically, at least once every 24 hours. If the token
+expires due to extended cluster downtime or is revoked, you will need to
+re-authenticate: open the notification target's edit panel, fill in your client
+secret, and click *Authenticate* again.
+
+NOTE: OAuth2 cannot be configured through direct configuration file editing. Use
+the web interface, or alternatively `pvesh`, to configure OAuth2 targets. Note
+that when using `pvesh`, you are responsible for providing the initial refresh
+token.
+
+----
+pvesh create /cluster/notifications/endpoints/smtp \
+ --name oauth2-smtp \
+ --server smtp.example.com \
+ --from-address from@example.com \
+ --mailto-user root@pam \
+ --auth-method google-oauth2 \
+ --oauth2-client-id <client ID> \
+ --oauth2-client-secret <client secret> \
+ --oauth2-refresh-token <refresh token>
+----
+
+For Microsoft, use `--auth-method microsoft-oauth2` and add
+`--oauth2-tenant-id <tenant ID>`.
+
+[[notification_targets_smtp_oauth2_google]]
+===== Google
+
+Google does not allow bare IP addresses as redirect URIs. To work around this,
+add an entry to `/etc/hosts` *on the machine where your browser is running*,
+i.e., your local workstation.
+
+----
+# Replace <IP> with the IP address of your PVE node
+<IP> local.oauth2-redirect.com
+----
+
+You can now register `https://local.oauth2-redirect.com:8006` as a redirect
+URI in your Google OAuth2 application, and use that same URL in the browser
+when accessing the {pve} web interface to perform the authorization flow.
+
+[[notification_targets_smtp_oauth2_microsoft]]
+===== Microsoft
+
+WARNING: For Microsoft, the application must *not* be registered as a
+Single-Page Application (SPA). {pve} requires long-lived refresh tokens, and
+Microsoft does not allow extending the lifetime of refresh tokens granted for
+SPAs.
+
+Register your OAuth2 application as a standard *Web* application in the Entra
+admin center. In addition to the client ID and secret, you will also need the
+*tenant ID* from your application registration.
+
[[notification_targets_gotify]]
Gotify
~~~~~~
--
2.47.3
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2026-02-16 7:40 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-02-04 16:13 [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 1/5] notify: Introduce xoauth2 module Arthur Bied-Charreton
2026-02-06 15:00 ` Lukas Wagner
2026-02-09 8:34 ` Arthur Bied-Charreton
2026-02-10 8:24 ` Lukas Wagner
2026-02-10 10:23 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 2/5] notify: Add state file handling Arthur Bied-Charreton
2026-02-10 15:51 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH proxmox 3/5] notify: Update Endpoint trait and Bus to use State Arthur Bied-Charreton
2026-02-10 15:52 ` Lukas Wagner
2026-02-12 8:26 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support Arthur Bied-Charreton
2026-02-10 15:52 ` Lukas Wagner
2026-02-11 13:00 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox 5/5] notify: Add test for State Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox-perl-rs 1/1] notify: update bindings with new OAuth2 parameters Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox-widget-toolkit 1/2] utils: Add OAuth2 flow handlers Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH proxmox-widget-toolkit 2/2] notifications: Add opt-in OAuth2 support for SMTP targets Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints Arthur Bied-Charreton
2026-02-11 8:55 ` Lukas Wagner
2026-02-11 12:47 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-manager 2/5] notifications: Add refresh-targets endpoint Arthur Bied-Charreton
2026-02-11 9:49 ` Lukas Wagner
2026-02-11 12:44 ` Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-manager 3/5] notifications: Trigger notification target refresh in pveupdate Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-manager 4/5] notifications: Handle OAuth2 callback in login handler Arthur Bied-Charreton
2026-02-11 9:00 ` Lukas Wagner
2026-02-04 16:13 ` [PATCH pve-manager 5/5] notifications: Opt into OAuth2 authentication Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-cluster 1/1] notifications: Add refresh_targets subroutine to PVE::Notify Arthur Bied-Charreton
2026-02-04 16:13 ` [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs Arthur Bied-Charreton
2026-02-11 10:06 ` Lukas Wagner
2026-02-11 13:15 ` Arthur Bied-Charreton
2026-02-13 16:06 ` superseded: [RFC cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/15] fix #7238: Add XOAUTH2 authentication support for SMTP notification targets Arthur Bied-Charreton
2026-02-13 16:03 [PATCH cluster/docs/manager/proxmox{,-perl-rs,-widget-toolkit} 00/17] " Arthur Bied-Charreton
2026-02-13 16:04 ` [PATCH pve-docs 1/1] notifications: Add section about OAuth2 to SMTP targets docs Arthur Bied-Charreton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.