From 684ffacdf9cc8991ccd183e99010bd0b464331f6 Mon Sep 17 00:00:00 2001 From: Alexander Zeidler Date: Fri, 21 Mar 2025 14:33:41 +0100 Subject: [PATCH] fix #6143: notify: allow overriding notification templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, notification templates could be modified by the user, but these were overwritten again with installing newer package versions of pve-manager and proxmox-backup. Now override templates can be created cluster-wide in the path “/etc/{pve,proxmox-backup}/notification-templates/{namespace}”, which are used with priority. The folder structure has to be created and populated manually (e.g. /etc/pve/notification-templates/default). If override templates are not existing or their rendering fails, the vendor templates in "/usr/share/{pve-manager,proxmox-backup}/templates/default/" are used. Sequence: [override html -> vendor html ->] override txt -> vendor txt An error is only returned if none of the template candidates could be used. Using an override template gets not logged. Signed-off-by: Alexander Zeidler Reviewed-by: Lukas Wagner Tested-by: Lukas Wagner Link: https://lore.proxmox.com/20250321133341.151340-1-a.zeidler@proxmox.com --- proxmox-notify/src/context/mod.rs | 4 +- proxmox-notify/src/context/pbs.rs | 9 +- proxmox-notify/src/context/pve.rs | 10 +- proxmox-notify/src/context/test.rs | 2 + proxmox-notify/src/renderer/mod.rs | 152 ++++++++++++++++++++--------- 5 files changed, 128 insertions(+), 49 deletions(-) diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs index c0a5a13b..8b6e2c43 100644 --- a/proxmox-notify/src/context/mod.rs +++ b/proxmox-notify/src/context/mod.rs @@ -1,6 +1,7 @@ use std::fmt::Debug; use std::sync::Mutex; +use crate::renderer::TemplateSource; use crate::Error; #[cfg(any(feature = "pve-context", feature = "pbs-context"))] @@ -24,11 +25,12 @@ pub trait Context: Send + Sync + Debug { fn http_proxy_config(&self) -> Option; /// Return default config for built-in targets/matchers. fn default_config(&self) -> &'static str; - /// Lookup a template in a certain (optional) namespace + /// Return the path of `filename` from `source` and a certain (optional) `namespace` fn lookup_template( &self, filename: &str, namespace: Option<&str>, + source: TemplateSource, ) -> Result, Error>; } diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs index e8106c57..3e5da59c 100644 --- a/proxmox-notify/src/context/pbs.rs +++ b/proxmox-notify/src/context/pbs.rs @@ -7,6 +7,7 @@ use proxmox_schema::{ObjectSchema, Schema, StringSchema}; use proxmox_section_config::{SectionConfig, SectionConfigPlugin}; use crate::context::{common, Context}; +use crate::renderer::TemplateSource; use crate::Error; const PBS_USER_CFG_FILENAME: &str = "/etc/proxmox-backup/user.cfg"; @@ -109,8 +110,14 @@ impl Context for PBSContext { &self, filename: &str, namespace: Option<&str>, + source: TemplateSource, ) -> Result, Error> { - let path = Path::new("/usr/share/proxmox-backup/templates") + let path = match source { + TemplateSource::Vendor => "/usr/share/proxmox-backup/templates", + TemplateSource::Override => "/etc/proxmox-backup/notification-templates", + }; + + let path = Path::new(&path) .join(namespace.unwrap_or("default")) .join(filename); diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs index d49ab27c..a97cce26 100644 --- a/proxmox-notify/src/context/pve.rs +++ b/proxmox-notify/src/context/pve.rs @@ -1,4 +1,5 @@ use crate::context::{common, Context}; +use crate::renderer::TemplateSource; use crate::Error; use std::path::Path; @@ -58,10 +59,17 @@ impl Context for PVEContext { &self, filename: &str, namespace: Option<&str>, + source: TemplateSource, ) -> Result, Error> { - let path = Path::new("/usr/share/pve-manager/templates") + let path = match source { + TemplateSource::Vendor => "/usr/share/pve-manager/templates", + TemplateSource::Override => "/etc/pve/notification-templates", + }; + + let path = Path::new(&path) .join(namespace.unwrap_or("default")) .join(filename); + let template_string = proxmox_sys::fs::file_read_optional_string(path) .map_err(|err| Error::Generic(format!("could not load template: {err}")))?; Ok(template_string) diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs index 5df25d05..13ef6eae 100644 --- a/proxmox-notify/src/context/test.rs +++ b/proxmox-notify/src/context/test.rs @@ -1,4 +1,5 @@ use crate::context::Context; +use crate::renderer::TemplateSource; use crate::Error; #[derive(Debug)] @@ -29,6 +30,7 @@ impl Context for TestContext { &self, _filename: &str, _namespace: Option<&str>, + _source: TemplateSource, ) -> Result, Error> { Ok(Some(String::new())) } diff --git a/proxmox-notify/src/renderer/mod.rs b/proxmox-notify/src/renderer/mod.rs index e058ea22..b37cf438 100644 --- a/proxmox-notify/src/renderer/mod.rs +++ b/proxmox-notify/src/renderer/mod.rs @@ -1,6 +1,6 @@ //! Module for rendering notification templates. -use std::time::Duration; +use std::{fmt::Display, time::Duration}; use handlebars::{ Context, Handlebars, Helper, HelperDef, HelperResult, Output, RenderContext, @@ -190,11 +190,29 @@ impl ValueRenderFunction { } } -/// Available template types +/// Choose between the provided `vendor` template or its by the user optionally created `override` #[derive(Copy, Clone)] +pub enum TemplateSource { + Vendor, + Override, +} + +impl Display for TemplateSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + TemplateSource::Vendor => f.write_str("vendor"), + TemplateSource::Override => f.write_str("override"), + } + } +} + +/// Available template types +#[derive(Copy, Clone, PartialEq)] pub enum TemplateType { /// HTML body template HtmlBody, + /// Fallback HTML body, based on the `PlaintextBody` template + HtmlBodyFromPlaintext, /// Plaintext body template PlaintextBody, /// Subject template @@ -205,14 +223,24 @@ impl TemplateType { fn file_suffix(&self) -> &'static str { match self { TemplateType::HtmlBody => "body.html.hbs", + TemplateType::HtmlBodyFromPlaintext => "body.txt.hbs", TemplateType::PlaintextBody => "body.txt.hbs", TemplateType::Subject => "subject.txt.hbs", } } fn postprocess(&self, mut rendered: String) -> String { - if let Self::Subject = self { - rendered = rendered.replace('\n', " "); + match self { + TemplateType::HtmlBodyFromPlaintext => { + rendered = format!( + "
{}
", + handlebars::html_escape(&rendered) + ) + } + TemplateType::Subject => { + rendered = rendered.replace('\n', " "); + } + _ => {} } rendered @@ -221,6 +249,7 @@ impl TemplateType { fn block_render_fns(&self) -> BlockRenderFunctions { match self { TemplateType::HtmlBody => html::block_render_functions(), + TemplateType::HtmlBodyFromPlaintext => plaintext::block_render_functions(), TemplateType::Subject => plaintext::block_render_functions(), TemplateType::PlaintextBody => plaintext::block_render_functions(), } @@ -231,6 +260,7 @@ impl TemplateType { TemplateType::PlaintextBody => handlebars::no_escape, TemplateType::Subject => handlebars::no_escape, TemplateType::HtmlBody => handlebars::html_escape, + TemplateType::HtmlBodyFromPlaintext => handlebars::no_escape, } } } @@ -250,70 +280,100 @@ impl BlockRenderFunctions { } fn render_template_impl( - template: &str, data: &Value, renderer: TemplateType, -) -> Result { - let mut handlebars = Handlebars::new(); - handlebars.register_escape_fn(renderer.escape_fn()); + filename: &str, + source: TemplateSource, +) -> Result, Error> { + let template_string = context::context().lookup_template(&filename, None, source)?; - let block_render_fns = renderer.block_render_fns(); - block_render_fns.register_helpers(&mut handlebars); + if let Some(template_string) = template_string { + let mut handlebars = Handlebars::new(); + handlebars.register_escape_fn(renderer.escape_fn()); - ValueRenderFunction::register_helpers(&mut handlebars); + let block_render_fns = renderer.block_render_fns(); + block_render_fns.register_helpers(&mut handlebars); - handlebars.register_helper( - "relative-percentage", - Box::new(handlebars_relative_percentage_helper), - ); + ValueRenderFunction::register_helpers(&mut handlebars); - let rendered_template = handlebars - .render_template(template, data) - .map_err(|err| Error::RenderError(err.into()))?; + handlebars.register_helper( + "relative-percentage", + Box::new(handlebars_relative_percentage_helper), + ); - Ok(rendered_template) + let rendered_template = handlebars + .render_template(&template_string, data) + .map_err(|err| Error::RenderError(err.into()))?; + + let rendered_template = renderer.postprocess(rendered_template); + + Ok(Some(rendered_template)) + } else { + Ok(None) + } } /// Render a template string. /// -/// The output format can be chosen via the `renderer` parameter (see [TemplateType] -/// for available options). +/// The output format is chosen via the `ty` parameter (see [TemplateType] for +/// available options). If an override template is found and renderable, it is +/// used instead of the vendor one. If the [TemplateType] is `HtmlBody` but no +/// HTML template is found or renderable, it falls back to use a plaintext +/// template encapsulated in a pre-formatted HTML block (
).
 pub fn render_template(
     mut ty: TemplateType,
     template: &str,
     data: &Value,
 ) -> Result {
-    let filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+    let mut source = TemplateSource::Override;
 
-    let template_string = context::context().lookup_template(&filename, None)?;
+    loop {
+        let filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+        let result = render_template_impl(data, ty, &filename, source);
 
-    let (template_string, fallback) = match (template_string, ty) {
-        (None, TemplateType::HtmlBody) => {
-            ty = TemplateType::PlaintextBody;
-            let plaintext_filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
-            (
-                context::context().lookup_template(&plaintext_filename, None)?,
-                true,
-            )
+        match result {
+            Ok(Some(s)) => {
+                return Ok(s);
+            }
+            Ok(None) => {}
+            Err(err) => {
+                tracing::error!("failed to render {source} template '{filename}': {err}");
+            }
         }
-        (template_string, _) => (template_string, false),
-    };
 
-    let template_string = template_string.ok_or(Error::Generic(format!(
-        "could not load template '{template}'"
-    )))?;
-
-    let mut rendered = render_template_impl(&template_string, data, ty)?;
-    rendered = ty.postprocess(rendered);
-
-    if fallback {
-        rendered = format!(
-            "
{}
",
-            handlebars::html_escape(&rendered)
-        );
+        match (ty, source) {
+            (
+                TemplateType::HtmlBody
+                | TemplateType::HtmlBodyFromPlaintext
+                | TemplateType::PlaintextBody
+                | TemplateType::Subject,
+                TemplateSource::Override,
+            ) => {
+                // Override template not found or renderable, try the vendor one instead
+                source = TemplateSource::Vendor;
+            }
+            (TemplateType::HtmlBody, TemplateSource::Vendor) => {
+                // Override and vendor HTML templates not found or renderable,
+                // try next the override plaintext as fallback
+                ty = TemplateType::HtmlBodyFromPlaintext;
+                source = TemplateSource::Override;
+            }
+            (
+                TemplateType::HtmlBodyFromPlaintext
+                | TemplateType::PlaintextBody
+                | TemplateType::Subject,
+                TemplateSource::Vendor,
+            ) => {
+                // Return error, no suitable templates found or renderable
+                break;
+            }
+        }
     }
 
-    Ok(rendered)
+    Err(Error::Generic(
+        "failed to render notification template, all template candidates are erroneous or missing"
+            .into(),
+    ))
 }
 
 #[cfg(test)]