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 <[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.
>
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).
