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 <[email protected]> > --- > 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);
