Re: [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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.