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 C86001FF179 for ; Wed, 29 Oct 2025 18:47:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 126BA10FFB; Wed, 29 Oct 2025 18:48:32 +0100 (CET) Message-ID: <0708164e-1fc4-4056-a086-6465288f92f8@proxmox.com> Date: Wed, 29 Oct 2025 18:47:58 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Dominik Csapak , pmg-devel@lists.proxmox.com References: <20251029144543.124324-1-d.csapak@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20251029144543.124324-1-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761760065328 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.027 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_MSPIKE_H2 0.001 Average reputation (+2) 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 Subject: Re: [pmg-devel] [PATCH pmg-yew-quarantine-gui] execute actions when opening gui from a link from quarantine mails X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pmg-devel-bounces@lists.proxmox.com Sender: "pmg-devel" Am 29.10.25 um 15:45 schrieb Dominik Csapak: > The PMG quarantine mails can contain links that should do some actions > directly, e.g. deliver or delete a mail. This works for our desktop gui > and worked in the old Framework7 mobile ui, but was forgotten to be > implemented in the new rust based gui. > > This patch implements it in `SpamList` in the 'create' method. > > To make it work properly we have to omit the `location.replace` when > doing a login, otherwise we lose the get parameters and refresh the > page. Instead remove the ticket parameter (when we extrat that) with > the browsers `history` api, and the rest when we execute the action. > > A SnackBar is also added on successful execution of an action. > > Parts of this patch are take from Stoiko's first approach[0]. > > Note that while this patch enables the actions when entering from a > quarantine link, it won't show a SnackBar (neither on error or success) > due to our `CatalogLoader` behavior. A fix for that is on the yew-devel > list [1]. > > 0: https://lore.proxmox.com/pmg-devel/20251028163628.79739-1-s.ivanov@proxmox.com/ > 1: https://lore.proxmox.com/yew-devel/20251029143617.18134-1-d.csapak@proxmox.com/T/#u Looks OK to me in general principle, some comments (mostly nits and naming) inline though, and would also like to get a T-b, e.g. from Stoiko (but I'm not picky ;-)) > > Co-developed-off-by: Stoiko Ivanov > Signed-off-by: Dominik Csapak > --- > src/main.rs | 22 +++++++++--- > src/page_login.rs | 5 +++ > src/spam_list.rs | 89 +++++++++++++++++++++++++++++++++++++++-------- > 3 files changed, 97 insertions(+), 19 deletions(-) > > diff --git a/src/main.rs b/src/main.rs > index 8cd8a6b..bb0ecc1 100644 > --- a/src/main.rs > +++ b/src/main.rs > @@ -14,8 +14,8 @@ pub use page_not_found::PageNotFound; > mod page_login; > pub use page_login::PageLogin; > > -use anyhow::Error; > -use gloo_utils::{document, format::JsValueSerdeExt}; > +use anyhow::{format_err, Error}; > +use gloo_utils::format::JsValueSerdeExt; > use serde::Deserialize; > use serde_json::{json, Value}; > use wasm_bindgen::JsValue; > @@ -158,9 +158,6 @@ impl Component for PmgQuarantineApp { > match msg { > Msg::Login(info) => { > self.login_info = Some(info.clone()); > - let document = document(); > - let location = document.location().unwrap(); > - let _ = location.replace(&location.pathname().unwrap()); > } > Msg::Logout => self.login_info = None, > } > @@ -187,6 +184,21 @@ impl std::fmt::Display for MailAction { > } > } > > +impl std::str::FromStr for MailAction { > + type Err = anyhow::Error; > + fn from_str(s: &str) -> Result { > + match s { > + "deliver" => Ok(MailAction::Deliver), > + "delete" => Ok(MailAction::Delete), > + "welcomelist" => Ok(MailAction::Welcomelist), > + "whitelist" => Ok(MailAction::Welcomelist), tiny nit: could be written as alternatives to better clarify that it's the same, i.e.: "welcomelist" | "whitelist" => Ok(MailAction::Welcomelist), > + "blocklist" => Ok(MailAction::Blocklist), > + "blacklist" => Ok(MailAction::Blocklist), similar nit above. > + _ => Err(format_err!("unknown quarantine action")), > + } > + } > +} > + > pub(crate) async fn mail_action(id: &str, action: MailAction) -> Result { > let param = json!({ > "action": action.to_string(), > diff --git a/src/page_login.rs b/src/page_login.rs > index d285209..f1e3406 100644 > --- a/src/page_login.rs > +++ b/src/page_login.rs > @@ -20,6 +20,8 @@ use proxmox_yew_comp::{ > http_login, start_ticket_refresh_loop, stop_ticket_refresh_loop, LoginPanel, > }; > > +use crate::spam_list::remove_get_parameter; > + > #[derive(Properties, PartialEq)] > pub struct PageLogin { > #[prop_or_default] > @@ -90,6 +92,9 @@ impl Component for PmgPageLogin { > Self::ticket_login(ctx, username.to_string(), ticket.to_string()); > stop_ticket_refresh_loop(); > } > + if let Err(err) = remove_get_parameter(Some("ticket")) { > + log::error!("could not remove ticket parameter: {err}"); > + } > } > } > } > diff --git a/src/spam_list.rs b/src/spam_list.rs > index b870e12..0af31c6 100644 > --- a/src/spam_list.rs > +++ b/src/spam_list.rs > @@ -1,10 +1,14 @@ > -use anyhow::Error; > -use wasm_bindgen::JsValue; > +use std::{rc::Rc, str::FromStr}; > > -use core::clone::Clone; > +use anyhow::{format_err, Error}; > +use gloo_utils::{document, window}; > use js_sys::Date; > +use serde::{Deserialize, Serialize}; > use serde_json::Value; > -use std::rc::Rc; > +use url::{form_urlencoded, Url}; > +use wasm_bindgen::JsValue; > +use yew::html::{IntoEventCallback, IntoPropValue}; > +use yew::virtual_dom::{VComp, VNode}; > > use pwt::{ > css::{AlignItems, ColorScheme, FlexFit, Opacity, Overflow}, > @@ -12,17 +16,10 @@ use pwt::{ > touch::{Slidable, SlidableAction, SnackBar, SnackBarContextExt}, > widget::{error_message, Container, Fa, List, ListTile, Progress, Row}, > }; > -use yew::{ > - html::{IntoEventCallback, IntoPropValue}, > - virtual_dom::{VComp, VNode}, > -}; > -//use yew::html::IntoEventCallback; > > use proxmox_yew_comp::http_get; > use pwt::widget::Column; > > -use serde::{Deserialize, Serialize}; > - > use crate::{mail_action, MailAction}; > > #[derive(Copy, Clone, Serialize, Default, PartialEq)] > @@ -110,6 +107,19 @@ impl Component for PmgSpamList { > > fn create(ctx: &Context) -> Self { > let me = Self { data: None }; > + > + match extract_mail_action_from_get() { > + Ok(None) => {} > + Ok(Some((id, action))) => { > + ctx.link().send_message(Msg::Action(id, action)); > + } > + Err(err) => { > + ctx.link().show_snackbar( > + SnackBar::new().message(format!("could not call action: {err}")), Would do s/call/execute/, as calling is IMO not that easy to understand. > + ); > + } > + } > + > me.load(ctx); > me > } > @@ -145,9 +155,11 @@ impl Component for PmgSpamList { > Msg::Action(id, action) => { > let link = ctx.link().clone(); > wasm_bindgen_futures::spawn_local(async move { > - if let Err(err) = mail_action(&id, action).await { > - link.show_snackbar(SnackBar::new().message(err.to_string())); > - } > + let msg = match mail_action(&id, action).await { > + Ok(_) => tr!("Action '{0}' successful", action), > + Err(err) => err.to_string(), > + }; > + link.show_snackbar(SnackBar::new().message(msg)); > link.send_message(Msg::Reload); > }); > return false; > @@ -299,3 +311,52 @@ fn epoch_to_date(epoch: i64) -> String { > date.get_date() > ) > } > + > +fn extract_mail_action_from_get() -> Result, Error> { > + let document = document(); > + let location = document.location().unwrap(); > + let search = location.search().unwrap(); > + let param = web_sys::UrlSearchParams::new_with_str(&search).unwrap(); > + > + remove_get_parameter(None)?; > + > + if let (Some(id), Some(action)) = (param.get("cselect"), param.get("action")) { > + let action = MailAction::from_str(&action)?; > + return Ok(Some((id, action))); > + } > + Ok(None) > +} > + > +/// Removes `name` parameter from the get values via the browser `history` object. > +/// > +/// If no name is given, removes all parameters. > +pub fn remove_get_parameter(name: Option<&str>) -> Result<(), Error> { "get" is a bit ambiguous here IMO as these parameters in the URL/location can be send on any request. Maybe better use "search" or "query". Also why delete here and extract above? In the backend we also got a quite frequently used "extract_param" method for something that deletes and returns a specific parameter, so might be fitting here. E.g.: fn extract_query_parameter Or even fn extract_location_query_parameter And for above extract_mail_action_from_get then either extract_mail_action_from_query or extract_mail_action_from_location_query, respectively. Also s/get/query/ in the error log message on call site. > + let location = window().location(); > + let history = window().history().unwrap(); > + > + let mut url = Url::parse( > + &location > + .href() > + .map_err(|err| format_err!("could not get location: {err:?}"))?, > + )?; > + if let Some(name) = name { > + let mut query = form_urlencoded::Serializer::new(String::new()); > + for (key, value) in url.query_pairs() { > + if key == name { > + continue; > + } > + query.append_pair(&key, &value); > + } > + > + url.set_query(Some(&query.finish())); > + } else { > + url.set_query(None); > + } > + > + // build the new url without get parameters > + history > + .replace_state_with_url(&JsValue::null(), "", Some(url.as_str())) > + .map_err(|err| format_err!("could not set url: {err:?}"))?; > + > + Ok(()) > +} _______________________________________________ pmg-devel mailing list pmg-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel