From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 3B2A71FF13F for ; Thu, 09 Apr 2026 13:35:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4D46E1FE83; Thu, 9 Apr 2026 13:36:02 +0200 (CEST) Date: Thu, 9 Apr 2026 13:35:26 +0200 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox v2 02/16] notify: smtp: Introduce state management Message-ID: References: <20260325131444.366808-1-a.bied-charreton@proxmox.com> <20260325131444.366808-3-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: 1775734458729 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.798 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: 4POUZVCNBKEADJZ7HQOVKGJZ454GLYVY X-Message-ID-Hash: 4POUZVCNBKEADJZ7HQOVKGJZ454GLYVY 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 Thu, Apr 09, 2026 at 11:51:43AM +0200, Lukas Wagner wrote: Thanks for the review :) > Looking good! > > With the trivial suggestions implemented: > > Reviewed-by: Lukas Wagner > [...] > > +use crate::endpoints::smtp::State; > > I think this import must be feature gated, otherwise a > > cargo build --no-default-features > > does not compile > [...] > > +use crate::endpoints::smtp::State; > > This one here as well with > > cargo build --no-default-features --features=pbs-context > > [...] > > +use crate::endpoints::smtp::State; > > Also this one with > > cargo build --no-default-features --features=pve-context > > Argh, should have tested these again sorry about that > > use crate::renderer::TemplateSource; > > use crate::Error; > > > > @@ -125,6 +126,19 @@ impl Context for PBSContext { > > .map_err(|err| Error::Generic(format!("could not load template: {err}")))?; > > Ok(template_string) > > } > > + > > + fn load_oauth_state(&self, endpoint_name: &str) -> Result { > > + let path = format!("/var/lib/proxmox-backup-priv/notifications/state-{endpoint_name}.json"); > > + State::load(path) > > + } > > + > > + fn save_oauth_state(&self, endpoint_name: &str, state: Option) -> Result<(), Error> { > > + let path = format!("/var/lib/proxmox-backup-priv/notifications/state-{endpoint_name}.json"); > > Sorry for the back and forth with the path. As discussed off-list, we > now agreed on the following paths for the oauth state files: > > PBS: /var/lib/proxmox-backup/notifications/oauth-state/.json > PDM: /var/lib/proxmox-datacenter-manager/notifications/oauth-state/.json > PVE: /etc/pve/priv/notifications/oauth-state/.json > ACK [...] > > +#[derive(Serialize, Deserialize, Clone, Debug, Default)] > > +#[serde(rename_all = "kebab-case")] > > No big deal, but this struct could use a doc comment. > You're right, will add one, thanks! > > +pub struct State { > > + #[serde(skip_serializing_if = "Option::is_none")] > > + pub oauth2_refresh_token: Option, > > + pub last_refreshed: i64, > > +} > > + > > +impl From> for State { > > + fn from(value: Option) -> Self { > > + Self { > > + oauth2_refresh_token: value, > > + last_refreshed: proxmox_time::epoch_i64(), > > + } > > + } > > +} > > This `impl From` feels a bit off to me, due to it being not 100% > functional (due to the timestamp). Also `From>` > feels a bit odd to me. Maybe just have a `State::new` instead? > > In the end-result of this series, you only ever call this From > in the api handler, and there only if the refresh-token is provided. > I guess you could have a State::new(refresh_token: String, timestamp: > i64) then? > Yea you're right that a `new` method would be cleaner here, will update this. > > + > > +/// Attempt to create a directory at `path` with `mode`, returning `Ok(())` if the directory either already > > +/// exists, or was successfully created. > > +/// > > +/// `pmxcfs` automatically sets the x-bit in directory permissions, however it does not allow the user > > +/// to set it, the `fchmod` fails with `EPERM` on directories with `0o700`. > > +/// > > +/// The `proxmox_sys` version of this function unconditionally logs mode mismatches even with > > +/// `enforce_permissions == false` AND calls `fchmod`. This means that if using that version, we would > > +/// always get a permission mismatch warning in the `pvedaemon` logs, even though we do not need the > > +/// `fchmod` call. > > +fn ensure_dir_exists>(path: P, mode: nix::sys::stat::Mode) -> Result<(), Error> { > > + match nix::unistd::mkdir(path.as_ref(), mode) { > > + Ok(()) | Err(nix::errno::Errno::EEXIST) => Ok(()), > > + Err(e) => Err(Error::StatePersistence( > > + path.as_ref().to_string_lossy().into(), > > + e.into(), > > + )), > > + } > > +} > > + > > +impl State { > > + /// Load the state for the endpoint identified by `endpoint_name`, instantiating a default object > > + /// if yes state exists. > > the 'yes' should not be here, I think? :) > It should be 'no'... > > + /// > > + /// # Errors > > + /// An [`Error`] is returned if deserialization of the state object fails. > > + pub(crate) fn load>(path: P) -> Result { > > This one should be `pub`, since it is supposed to be called from the > context implementation (which should be moved out of this crate at some > point). > [...] > > + /// Persist the state for the endpoint identified by `endpoint_name`. > > + /// > > + /// # Errors > > + /// An [`Error`] is returned if serialization of the state object, or the final write, fail. > > + pub(crate) fn save>( > > Same here. > Yes, I meant to change that for v2 but this somehow slipped through my TODOs, thanks! > > + self, > > + path: P, > > + mode: nix::sys::stat::Mode, > > I'd rather use CreateOptions here and move the CreateOptions::new to the > caller > Yes, the reason I did not do that was because ensure_dir_exists expects a Mode, which is a private field in CreateOptions. Since the directories we need to create for the state files are now deeper though, we need to use create_path instead, which does take CreateOptions structs directly, so that is solved :) > > + ) -> Result<(), Error> { > > + let path_str = path.as_ref().to_string_lossy(); > > + let parent = path.as_ref().parent().unwrap(); > > + > > + debug!("attempting to persist state at {path_str}"); > > + > > + ensure_dir_exists(parent, mode)?; > > + > > + let s = serde_json::to_string_pretty(&self) > > + .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))?; > > + > > + proxmox_sys::fs::replace_file( > > + &path, > > + s.as_bytes(), > > + proxmox_sys::fs::CreateOptions::new().perm(mode), > > + false, > > + ) > > + .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into())) > > + } > > + > > + /// Delete the state for the endpoint identitied by `endpoint_name`. > > + /// > > + /// # Errors > > + /// An [`Error`] is returned if the state file cannot be deleted. > > This seems to be wrong? > Yes, copy-paste error, will be fixed > > + pub(crate) fn delete>(path: P) { > > + let _ = std::fs::remove_file(&path); > > Might make sense to the following here (untested): > > if let Err(e) = std::fs::remove_file(&path) { > if e.kind() != std::io::ErrorKind::NotFound { > // log error > } > } > ACK, will do that [...]