From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 62C891FF13B for ; Wed, 11 Feb 2026 14:00:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7509A1C1D0; Wed, 11 Feb 2026 14:01:16 +0100 (CET) Date: Wed, 11 Feb 2026 14:00:40 +0100 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support Message-ID: <5opeuvrpig43irxubo5gvck2zugj3uhhwc2dv6tb3bgvu5zcc3@ghsoa2l7ovt4> References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-5-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770814755154 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.845 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: O4TTJYAPTPWVDCJXZ2WDYBOTXSI64NTQ X-Message-ID-Hash: O4TTJYAPTPWVDCJXZ2WDYBOTXSI64NTQ X-MailFrom: a.bied-charreton@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > > [...] > > + 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 instead, that way we avoid the output parameter and the bus can determine whether it needs to update the global state itself.