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 <[email protected]>
> > [...]
> > + 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.
