From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 15150821F for ; Wed, 26 Jul 2023 16:19:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E35828C1 for ; Wed, 26 Jul 2023 16:18:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 26 Jul 2023 16:18:45 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8183545A4F for ; Wed, 26 Jul 2023 16:18:45 +0200 (CEST) From: Lukas Wagner To: pve-devel@lists.proxmox.com Date: Wed, 26 Jul 2023 16:18:23 +0200 Message-Id: <20230726141824.813621-4-l.wagner@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230726141824.813621-1-l.wagner@proxmox.com> References: <20230726141824.813621-1-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.105 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 PROLO_LEO2 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pve-devel] [PATCH v2 proxmox 3/4] notify: use HttpError from proxmox-http-error X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Jul 2023 14:19:19 -0000 Also improve API documentation in terms of which HttpError is returned when. Signed-off-by: Lukas Wagner --- Notes: Assumes that the following two patches have been applied before, otherwise there will be a conflict (which is trivial to resolve, though) 1: "notify: fix build warnings if not all features are enabled": https://lists.proxmox.com/pipermail/pve-devel/2023-July/058409.html 2: "notify: fix tests if not all features are enabled" https://lists.proxmox.com/pipermail/pve-devel/2023-July/058410.html proxmox-notify/Cargo.toml | 1 + proxmox-notify/src/api/common.rs | 33 ++++---- proxmox-notify/src/api/filter.rs | 59 ++++++------- proxmox-notify/src/api/gotify.rs | 85 ++++++++++--------- proxmox-notify/src/api/group.rs | 82 +++++++++--------- proxmox-notify/src/api/mod.rs | 130 ++++++++++------------------- proxmox-notify/src/api/sendmail.rs | 93 ++++++++++++--------- 7 files changed, 232 insertions(+), 251 deletions(-) diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml index 5cceb0b..8b36a52 100644 --- a/proxmox-notify/Cargo.toml +++ b/proxmox-notify/Cargo.toml @@ -14,6 +14,7 @@ log.workspace = true once_cell.workspace = true openssl.workspace = true proxmox-http = { workspace = true, features = ["client-sync"], optional = true } +proxmox-http-error.workspace = true proxmox-human-byte.workspace = true proxmox-schema = { workspace = true, features = ["api-macro", "api-types"]} proxmox-section-config = { workspace = true } diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/common.rs index 48761fb..bc7029f 100644 --- a/proxmox-notify/src/api/common.rs +++ b/proxmox-notify/src/api/common.rs @@ -1,15 +1,17 @@ -use crate::api::ApiError; +use super::http_err; use crate::{Bus, Config, Notification}; +use proxmox_http_error::HttpError; + /// Send a notification to a given target. /// /// The caller is responsible for any needed permission checks. -/// Returns an `ApiError` in case of an error. -pub fn send(config: &Config, channel: &str, notification: &Notification) -> Result<(), ApiError> { +/// Returns an `anyhow::Error` in case of an error. +pub fn send(config: &Config, channel: &str, notification: &Notification) -> Result<(), HttpError> { let bus = Bus::from_config(config).map_err(|err| { - ApiError::internal_server_error( - "Could not instantiate notification bus", - Some(Box::new(err)), + http_err!( + INTERNAL_SERVER_ERROR, + "Could not instantiate notification bus: {err}" ) })?; @@ -21,23 +23,20 @@ pub fn send(config: &Config, channel: &str, notification: &Notification) -> Resu /// Test target (group or single endpoint) identified by its `name`. /// /// The caller is responsible for any needed permission checks. -/// Returns an `ApiError` if sending via the endpoint failed. -pub fn test_target(config: &Config, endpoint: &str) -> Result<(), ApiError> { +/// Returns an `anyhow::Error` if sending via the endpoint failed. +pub fn test_target(config: &Config, endpoint: &str) -> Result<(), HttpError> { let bus = Bus::from_config(config).map_err(|err| { - ApiError::internal_server_error( - "Could not instantiate notification bus", - Some(Box::new(err)), + http_err!( + INTERNAL_SERVER_ERROR, + "Could not instantiate notification bus: {err}" ) })?; bus.test_target(endpoint).map_err(|err| match err { crate::Error::TargetDoesNotExist(endpoint) => { - ApiError::not_found(format!("endpoint '{endpoint}' does not exist"), None) + http_err!(NOT_FOUND, "endpoint '{endpoint}' does not exist") } - _ => ApiError::internal_server_error( - format!("Could not test target: {err}"), - Some(Box::new(err)), - ), + _ => http_err!(INTERNAL_SERVER_ERROR, "Could not test target: {err}"), })?; Ok(()) @@ -49,7 +48,7 @@ pub fn test_target(config: &Config, endpoint: &str) -> Result<(), ApiError> { /// the result for 'grp1' would be [grp1, a, b, c, filter1, filter2]. /// The result will always contain the entity that was passed as a parameter. /// If the entity does not exist, the result will only contain the entity. -pub fn get_referenced_entities(config: &Config, entity: &str) -> Result, ApiError> { +pub fn get_referenced_entities(config: &Config, entity: &str) -> Result, HttpError> { let entities = super::get_referenced_entities(config, entity); Ok(Vec::from_iter(entities.into_iter())) } diff --git a/proxmox-notify/src/api/filter.rs b/proxmox-notify/src/api/filter.rs index b5b6284..be0c25b 100644 --- a/proxmox-notify/src/api/filter.rs +++ b/proxmox-notify/src/api/filter.rs @@ -1,63 +1,69 @@ -use crate::api::ApiError; +use crate::api::http_err; use crate::filter::{DeleteableFilterProperty, FilterConfig, FilterConfigUpdater, FILTER_TYPENAME}; use crate::Config; +use proxmox_http_error::HttpError; /// Get a list of all filters /// /// The caller is responsible for any needed permission checks. -/// Returns a list of all filters or an `ApiError` if the config is erroneous. -pub fn get_filters(config: &Config) -> Result, ApiError> { +/// Returns a list of all filters or a `HttpError` if the config is +/// (`500 Internal server error`). +pub fn get_filters(config: &Config) -> Result, HttpError> { config .config .convert_to_typed_array(FILTER_TYPENAME) - .map_err(|e| ApiError::internal_server_error("Could not fetch filters", Some(e.into()))) + .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "Could not fetch filters: {e}")) } /// Get filter with given `name` /// /// The caller is responsible for any needed permission checks. -/// Returns the endpoint or an `ApiError` if the filter was not found. -pub fn get_filter(config: &Config, name: &str) -> Result { +/// Returns the endpoint or a `HttpError` if the filter was not found (`404 Not found`). +pub fn get_filter(config: &Config, name: &str) -> Result { config .config .lookup(FILTER_TYPENAME, name) - .map_err(|_| ApiError::not_found(format!("filter '{name}' not found"), None)) + .map_err(|_| http_err!(NOT_FOUND, "filter '{name}' not found")) } /// Add new notification filter. /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if a filter with the same name already exists or -/// if the filter could not be saved. -pub fn add_filter(config: &mut Config, filter_config: &FilterConfig) -> Result<(), ApiError> { +/// Returns a `HttpError` if: +/// - an entity with the same name already exists (`400 Bad request`) +/// - the configuration could not be saved (`500 Internal server error`) +pub fn add_filter(config: &mut Config, filter_config: &FilterConfig) -> Result<(), HttpError> { super::ensure_unique(config, &filter_config.name)?; config .config .set_data(&filter_config.name, FILTER_TYPENAME, filter_config) .map_err(|e| { - ApiError::internal_server_error( - format!("could not save filter '{}'", filter_config.name), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save filter '{}': {e}", + filter_config.name ) })?; Ok(()) } -/// Update existing filter +/// Update existing notification filter /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the config could not be saved. +/// Returns a `HttpError` if: +/// - the configuration could not be saved (`500 Internal server error`) +/// - an invalid digest was passed (`400 Bad request`) pub fn update_filter( config: &mut Config, name: &str, filter_updater: &FilterConfigUpdater, delete: Option<&[DeleteableFilterProperty]>, digest: Option<&[u8]>, -) -> Result<(), ApiError> { +) -> Result<(), HttpError> { super::verify_digest(config, digest)?; let mut filter = get_filter(config, name)?; @@ -92,12 +98,7 @@ pub fn update_filter( config .config .set_data(name, FILTER_TYPENAME, &filter) - .map_err(|e| { - ApiError::internal_server_error( - format!("could not save filter '{name}'"), - Some(e.into()), - ) - })?; + .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "could not save filter '{name}': {e}"))?; Ok(()) } @@ -106,8 +107,10 @@ pub fn update_filter( /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the filter does not exist. -pub fn delete_filter(config: &mut Config, name: &str) -> Result<(), ApiError> { +/// Returns a `HttpError` if: +/// - the entity does not exist (`404 Not found`) +/// - the filter is still referenced by another entity (`400 Bad request`) +pub fn delete_filter(config: &mut Config, name: &str) -> Result<(), HttpError> { // Check if the filter exists let _ = get_filter(config, name)?; super::ensure_unused(config, name)?; @@ -142,14 +145,14 @@ filter: filter2 } #[test] - fn test_update_not_existing_returns_error() -> Result<(), ApiError> { + fn test_update_not_existing_returns_error() -> Result<(), HttpError> { let mut config = empty_config(); assert!(update_filter(&mut config, "test", &Default::default(), None, None).is_err()); Ok(()) } #[test] - fn test_update_invalid_digest_returns_error() -> Result<(), ApiError> { + fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> { let mut config = config_with_two_filters(); assert!(update_filter( &mut config, @@ -164,7 +167,7 @@ filter: filter2 } #[test] - fn test_filter_update() -> Result<(), ApiError> { + fn test_filter_update() -> Result<(), HttpError> { let mut config = config_with_two_filters(); let digest = config.digest; @@ -215,7 +218,7 @@ filter: filter2 } #[test] - fn test_filter_delete() -> Result<(), ApiError> { + fn test_filter_delete() -> Result<(), HttpError> { let mut config = config_with_two_filters(); delete_filter(&mut config, "filter1")?; diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs index 521dd16..0ec48fd 100644 --- a/proxmox-notify/src/api/gotify.rs +++ b/proxmox-notify/src/api/gotify.rs @@ -1,4 +1,6 @@ -use crate::api::ApiError; +use proxmox_http_error::HttpError; + +use crate::api::http_err; use crate::endpoints::gotify::{ DeleteableGotifyProperty, GotifyConfig, GotifyConfigUpdater, GotifyPrivateConfig, GotifyPrivateConfigUpdater, GOTIFY_TYPENAME, @@ -8,36 +10,41 @@ use crate::Config; /// Get a list of all gotify endpoints. /// /// The caller is responsible for any needed permission checks. -/// Returns a list of all gotify endpoints or an `ApiError` if the config is erroneous. -pub fn get_endpoints(config: &Config) -> Result, ApiError> { +/// Returns a list of all gotify endpoints or a `HttpError` if the config is +/// erroneous (`500 Internal server error`). +pub fn get_endpoints(config: &Config) -> Result, HttpError> { config .config .convert_to_typed_array(GOTIFY_TYPENAME) - .map_err(|e| ApiError::internal_server_error("Could not fetch endpoints", Some(e.into()))) + .map_err(|e| http_err!(NOT_FOUND, "Could not fetch endpoints: {e}")) } /// Get gotify endpoint with given `name` /// /// The caller is responsible for any needed permission checks. -/// Returns the endpoint or an `ApiError` if the endpoint was not found. -pub fn get_endpoint(config: &Config, name: &str) -> Result { +/// Returns the endpoint or a `HttpError` if the endpoint was not found (`404 Not found`). +pub fn get_endpoint(config: &Config, name: &str) -> Result { config .config .lookup(GOTIFY_TYPENAME, name) - .map_err(|_| ApiError::not_found(format!("endpoint '{name}' not found"), None)) + .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found")) } /// Add a new gotify endpoint. /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if an endpoint with the same name already exists, -/// or if the endpoint could not be saved. +/// Returns a `HttpError` if: +/// - an entity with the same name already exists (`400 Bad request`) +/// - a referenced filter does not exist (`400 Bad request`) +/// - the configuration could not be saved (`500 Internal server error`) +/// +/// Panics if the names of the private config and the public config do not match. pub fn add_endpoint( config: &mut Config, endpoint_config: &GotifyConfig, private_endpoint_config: &GotifyPrivateConfig, -) -> Result<(), ApiError> { +) -> Result<(), HttpError> { if endpoint_config.name != private_endpoint_config.name { // Programming error by the user of the crate, thus we panic panic!("name for endpoint config and private config must be identical"); @@ -56,20 +63,22 @@ pub fn add_endpoint( .config .set_data(&endpoint_config.name, GOTIFY_TYPENAME, endpoint_config) .map_err(|e| { - ApiError::internal_server_error( - format!("could not save endpoint '{}'", endpoint_config.name), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save endpoint '{}': {e}", + endpoint_config.name ) - })?; - - Ok(()) + }) } /// Update existing gotify endpoint /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the config could not be saved. +/// Returns a `HttpError` if: +/// - an entity with the same name already exists (`400 Bad request`) +/// - a referenced filter does not exist (`400 Bad request`) +/// - the configuration could not be saved (`500 Internal server error`) pub fn update_endpoint( config: &mut Config, name: &str, @@ -77,7 +86,7 @@ pub fn update_endpoint( private_endpoint_config_updater: &GotifyPrivateConfigUpdater, delete: Option<&[DeleteableGotifyProperty]>, digest: Option<&[u8]>, -) -> Result<(), ApiError> { +) -> Result<(), HttpError> { super::verify_digest(config, digest)?; let mut endpoint = get_endpoint(config, name)?; @@ -120,21 +129,21 @@ pub fn update_endpoint( .config .set_data(name, GOTIFY_TYPENAME, &endpoint) .map_err(|e| { - ApiError::internal_server_error( - format!("could not save endpoint '{name}'"), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save endpoint '{name}': {e}" ) - })?; - - Ok(()) + }) } /// Delete existing gotify endpoint /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the endpoint does not exist. -pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), ApiError> { +/// Returns a `HttpError` if: +/// - the entity does not exist (`404 Not found`) +/// - the endpoint is still referenced by another entity (`400 Bad request`) +pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError> { // Check if the endpoint exists let _ = get_endpoint(config, name)?; super::ensure_unused(config, name)?; @@ -148,22 +157,20 @@ pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), Api fn set_private_config_entry( config: &mut Config, private_config: &GotifyPrivateConfig, -) -> Result<(), ApiError> { +) -> Result<(), HttpError> { config .private_config .set_data(&private_config.name, GOTIFY_TYPENAME, private_config) .map_err(|e| { - ApiError::internal_server_error( - format!( - "could not save private config for endpoint '{}'", - private_config.name - ), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save private config for endpoint '{}': {e}", + private_config.name ) }) } -fn remove_private_config_entry(config: &mut Config, name: &str) -> Result<(), ApiError> { +fn remove_private_config_entry(config: &mut Config, name: &str) -> Result<(), HttpError> { config.private_config.sections.remove(name); Ok(()) } @@ -173,7 +180,7 @@ mod tests { use super::*; use crate::api::test_helpers::empty_config; - pub fn add_default_gotify_endpoint(config: &mut Config) -> Result<(), ApiError> { + pub fn add_default_gotify_endpoint(config: &mut Config) -> Result<(), HttpError> { add_endpoint( config, &GotifyConfig { @@ -193,7 +200,7 @@ mod tests { } #[test] - fn test_update_not_existing_returns_error() -> Result<(), ApiError> { + fn test_update_not_existing_returns_error() -> Result<(), HttpError> { let mut config = empty_config(); assert!(update_endpoint( @@ -210,7 +217,7 @@ mod tests { } #[test] - fn test_update_invalid_digest_returns_error() -> Result<(), ApiError> { + fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> { let mut config = empty_config(); add_default_gotify_endpoint(&mut config)?; @@ -228,7 +235,7 @@ mod tests { } #[test] - fn test_gotify_update() -> Result<(), ApiError> { + fn test_gotify_update() -> Result<(), HttpError> { let mut config = empty_config(); add_default_gotify_endpoint(&mut config)?; @@ -279,7 +286,7 @@ mod tests { } #[test] - fn test_gotify_endpoint_delete() -> Result<(), ApiError> { + fn test_gotify_endpoint_delete() -> Result<(), HttpError> { let mut config = empty_config(); add_default_gotify_endpoint(&mut config)?; diff --git a/proxmox-notify/src/api/group.rs b/proxmox-notify/src/api/group.rs index d6b8f38..6fc71ea 100644 --- a/proxmox-notify/src/api/group.rs +++ b/proxmox-notify/src/api/group.rs @@ -1,43 +1,47 @@ -use crate::api::ApiError; +use proxmox_http_error::HttpError; + +use crate::api::{http_bail, http_err}; use crate::group::{DeleteableGroupProperty, GroupConfig, GroupConfigUpdater, GROUP_TYPENAME}; use crate::Config; /// Get all notification groups /// /// The caller is responsible for any needed permission checks. -/// Returns a list of all groups or an `ApiError` if the config is erroneous. -pub fn get_groups(config: &Config) -> Result, ApiError> { +/// Returns a list of all groups or a `HttpError` if the config is +/// erroneous (`500 Internal server error`). +pub fn get_groups(config: &Config) -> Result, HttpError> { config .config .convert_to_typed_array(GROUP_TYPENAME) - .map_err(|e| ApiError::internal_server_error("Could not fetch groups", Some(e.into()))) + .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "Could not fetch groups: {e}")) } /// Get group with given `name` /// /// The caller is responsible for any needed permission checks. -/// Returns the endpoint or an `ApiError` if the group was not found. -pub fn get_group(config: &Config, name: &str) -> Result { +/// Returns the endpoint or an `HttpError` if the group was not found (`404 Not found`). +pub fn get_group(config: &Config, name: &str) -> Result { config .config .lookup(GROUP_TYPENAME, name) - .map_err(|_| ApiError::not_found(format!("group '{name}' not found"), None)) + .map_err(|_| http_err!(NOT_FOUND, "group '{name}' not found")) } /// Add a new group. /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if a group with the same name already exists, or -/// if the group could not be saved -pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(), ApiError> { +/// Returns a `HttpError` if: +/// - an entity with the same name already exists (`400 Bad request`) +/// - a referenced filter does not exist (`400 Bad request`) +/// - no endpoints were passed (`400 Bad request`) +/// - referenced endpoints do not exist (`404 Not found`) +/// - the configuration could not be saved (`500 Internal server error`) +pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(), HttpError> { super::ensure_unique(config, &group_config.name)?; if group_config.endpoint.is_empty() { - return Err(ApiError::bad_request( - "group must contain at least one endpoint", - None, - )); + http_bail!(BAD_REQUEST, "group must contain at least one endpoint",); } if let Some(filter) = &group_config.filter { @@ -51,27 +55,31 @@ pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(), .config .set_data(&group_config.name, GROUP_TYPENAME, group_config) .map_err(|e| { - ApiError::internal_server_error( - format!("could not save group '{}'", group_config.name), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save group '{}': {e}", + group_config.name ) - })?; - - Ok(()) + }) } /// Update existing group /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the config could not be saved. +/// Returns a `HttpError` if: +/// - a referenced filter does not exist (`400 Bad request`) +/// - an invalid digest was passed (`400 Bad request`) +/// - no endpoints were passed (`400 Bad request`) +/// - referenced endpoints do not exist (`404 Not found`) +/// - the configuration could not be saved (`500 Internal server error`) pub fn update_group( config: &mut Config, name: &str, updater: &GroupConfigUpdater, delete: Option<&[DeleteableGroupProperty]>, digest: Option<&[u8]>, -) -> Result<(), ApiError> { +) -> Result<(), HttpError> { super::verify_digest(config, digest)?; let mut group = get_group(config, name)?; @@ -88,10 +96,7 @@ pub fn update_group( if let Some(endpoints) = &updater.endpoint { super::ensure_endpoints_exist(config, endpoints)?; if endpoints.is_empty() { - return Err(ApiError::bad_request( - "group must contain at least one endpoint", - None, - )); + http_bail!(BAD_REQUEST, "group must contain at least one endpoint",); } group.endpoint = endpoints.iter().map(Into::into).collect() } @@ -109,22 +114,15 @@ pub fn update_group( config .config .set_data(name, GROUP_TYPENAME, &group) - .map_err(|e| { - ApiError::internal_server_error( - format!("could not save group '{name}'"), - Some(e.into()), - ) - })?; - - Ok(()) + .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "could not save group '{name}': {e}")) } /// Delete existing group /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the group does not exist. -pub fn delete_group(config: &mut Config, name: &str) -> Result<(), ApiError> { +/// Returns a `HttpError` if the group does not exist (`404 Not found`). +pub fn delete_group(config: &mut Config, name: &str) -> Result<(), HttpError> { // Check if the group exists let _ = get_group(config, name)?; @@ -141,7 +139,7 @@ mod tests { use crate::api::sendmail::tests::add_sendmail_endpoint_for_test; use crate::api::test_helpers::*; - fn add_default_group(config: &mut Config) -> Result<(), ApiError> { + fn add_default_group(config: &mut Config) -> Result<(), HttpError> { add_sendmail_endpoint_for_test(config, "test")?; add_group( @@ -173,14 +171,14 @@ mod tests { } #[test] - fn test_add_group() -> Result<(), ApiError> { + fn test_add_group() -> Result<(), HttpError> { let mut config = empty_config(); assert!(add_default_group(&mut config).is_ok()); Ok(()) } #[test] - fn test_update_group_fails_if_endpoint_does_not_exist() -> Result<(), ApiError> { + fn test_update_group_fails_if_endpoint_does_not_exist() -> Result<(), HttpError> { let mut config = empty_config(); add_default_group(&mut config)?; @@ -199,7 +197,7 @@ mod tests { } #[test] - fn test_update_group_fails_if_digest_invalid() -> Result<(), ApiError> { + fn test_update_group_fails_if_digest_invalid() -> Result<(), HttpError> { let mut config = empty_config(); add_default_group(&mut config)?; @@ -215,7 +213,7 @@ mod tests { } #[test] - fn test_update_group() -> Result<(), ApiError> { + fn test_update_group() -> Result<(), HttpError> { let mut config = empty_config(); add_default_group(&mut config)?; @@ -249,7 +247,7 @@ mod tests { } #[test] - fn test_group_delete() -> Result<(), ApiError> { + fn test_group_delete() -> Result<(), HttpError> { let mut config = empty_config(); add_default_group(&mut config)?; diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs index 9372443..cb0e085 100644 --- a/proxmox-notify/src/api/mod.rs +++ b/proxmox-notify/src/api/mod.rs @@ -1,9 +1,6 @@ -use std::collections::HashSet; -use std::error::Error as StdError; -use std::fmt::Display; - use crate::Config; -use serde::Serialize; +use proxmox_http_error::HttpError; +use std::collections::HashSet; pub mod common; pub mod filter; @@ -13,81 +10,44 @@ pub mod group; #[cfg(feature = "sendmail")] pub mod sendmail; -#[derive(Debug, Serialize)] -pub struct ApiError { - /// HTTP Error code - code: u16, - /// Error message - message: String, - #[serde(skip_serializing)] - /// The underlying cause of the error - source: Option>, +// We have our own, local versions of http_err and http_bail, because +// we don't want to wrap the error in anyhow::Error. If we were to do that, +// we would need to downcast in the perlmod bindings, since we need +// to return `HttpError` from there. +#[macro_export] +macro_rules! http_err { + ($status:ident, $($fmt:tt)+) => {{ + proxmox_http_error::HttpError::new( + proxmox_http_error::StatusCode::$status, + format!($($fmt)+) + ) + }}; } -impl ApiError { - fn new>( - message: S, - code: u16, - source: Option>, - ) -> Self { - Self { - message: message.as_ref().into(), - code, - source, - } - } - - pub fn bad_request>( - message: S, - source: Option>, - ) -> Self { - Self::new(message, 400, source) - } - - pub fn not_found>( - message: S, - source: Option>, - ) -> Self { - Self::new(message, 404, source) - } - - pub fn internal_server_error>( - message: S, - source: Option>, - ) -> Self { - Self::new(message, 500, source) - } +#[macro_export] +macro_rules! http_bail { + ($status:ident, $($fmt:tt)+) => {{ + return Err($crate::api::http_err!($status, $($fmt)+)); + }}; } -impl Display for ApiError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&format!("{} {}", self.code, self.message)) - } -} +pub use http_bail; +pub use http_err; -impl StdError for ApiError { - fn source(&self) -> Option<&(dyn StdError + 'static)> { - match &self.source { - None => None, - Some(source) => Some(&**source), - } - } -} - -fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), ApiError> { +fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), HttpError> { if let Some(digest) = digest { if config.digest != *digest { - return Err(ApiError::bad_request( - "detected modified configuration - file changed by other user? Try again.", - None, - )); + http_bail!( + BAD_REQUEST, + "detected modified configuration - file changed by other user? Try again." + ); } } Ok(()) } -fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Result<(), ApiError> { +fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Result<(), HttpError> { #[allow(unused_mut)] let mut exists = false; @@ -101,16 +61,16 @@ fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Resul } if !exists { - Err(ApiError::not_found( - format!("endpoint '{name}' does not exist"), - None, - )) + http_bail!(NOT_FOUND, "endpoint '{name}' does not exist") } else { Ok(()) } } -fn ensure_endpoints_exist>(config: &Config, endpoints: &[T]) -> Result<(), ApiError> { +fn ensure_endpoints_exist>( + config: &Config, + endpoints: &[T], +) -> Result<(), HttpError> { for endpoint in endpoints { ensure_endpoint_exists(config, endpoint.as_ref())?; } @@ -118,18 +78,18 @@ fn ensure_endpoints_exist>(config: &Config, endpoints: &[T]) -> Re Ok(()) } -fn ensure_unique(config: &Config, entity: &str) -> Result<(), ApiError> { +fn ensure_unique(config: &Config, entity: &str) -> Result<(), HttpError> { if config.config.sections.contains_key(entity) { - return Err(ApiError::bad_request( - format!("Cannot create '{entity}', an entity with the same name already exists"), - None, - )); + http_bail!( + BAD_REQUEST, + "Cannot create '{entity}', an entity with the same name already exists" + ); } Ok(()) } -fn get_referrers(config: &Config, entity: &str) -> Result, ApiError> { +fn get_referrers(config: &Config, entity: &str) -> Result, HttpError> { let mut referrers = HashSet::new(); for group in group::get_groups(config)? { @@ -165,16 +125,16 @@ fn get_referrers(config: &Config, entity: &str) -> Result, ApiEr Ok(referrers) } -fn ensure_unused(config: &Config, entity: &str) -> Result<(), ApiError> { +fn ensure_unused(config: &Config, entity: &str) -> Result<(), HttpError> { let referrers = get_referrers(config, entity)?; if !referrers.is_empty() { let used_by = referrers.into_iter().collect::>().join(", "); - return Err(ApiError::bad_request( - format!("cannot delete '{entity}', referenced by: {used_by}"), - None, - )); + http_bail!( + BAD_REQUEST, + "cannot delete '{entity}', referenced by: {used_by}" + ); } Ok(()) @@ -240,7 +200,7 @@ mod tests { use crate::filter::FilterConfig; use crate::group::GroupConfig; - fn prepare_config() -> Result { + fn prepare_config() -> Result { let mut config = super::test_helpers::empty_config(); filter::add_filter( @@ -316,7 +276,7 @@ mod tests { } #[test] - fn test_get_referrers_for_entity() -> Result<(), ApiError> { + fn test_get_referrers_for_entity() -> Result<(), HttpError> { let config = prepare_config().unwrap(); assert_eq!( diff --git a/proxmox-notify/src/api/sendmail.rs b/proxmox-notify/src/api/sendmail.rs index 59a57b4..ac8737c 100644 --- a/proxmox-notify/src/api/sendmail.rs +++ b/proxmox-notify/src/api/sendmail.rs @@ -1,4 +1,6 @@ -use crate::api::ApiError; +use proxmox_http_error::HttpError; + +use crate::api::{http_bail, http_err}; use crate::endpoints::sendmail::{ DeleteableSendmailProperty, SendmailConfig, SendmailConfigUpdater, SENDMAIL_TYPENAME, }; @@ -7,32 +9,36 @@ use crate::Config; /// Get a list of all sendmail endpoints. /// /// The caller is responsible for any needed permission checks. -/// Returns a list of all sendmail endpoints or an `ApiError` if the config is erroneous. -pub fn get_endpoints(config: &Config) -> Result, ApiError> { +/// Returns a list of all sendmail endpoints or a `HttpError` if the config is +/// erroneous (`500 Internal server error`). +pub fn get_endpoints(config: &Config) -> Result, HttpError> { config .config .convert_to_typed_array(SENDMAIL_TYPENAME) - .map_err(|e| ApiError::internal_server_error("Could not fetch endpoints", Some(e.into()))) + .map_err(|e| http_err!(NOT_FOUND, "Could not fetch endpoints: {e}")) } /// Get sendmail endpoint with given `name`. /// /// The caller is responsible for any needed permission checks. -/// Returns the endpoint or an `ApiError` if the endpoint was not found. -pub fn get_endpoint(config: &Config, name: &str) -> Result { +/// Returns the endpoint or a `HttpError` if the endpoint was not found (`404 Not found`). +pub fn get_endpoint(config: &Config, name: &str) -> Result { config .config .lookup(SENDMAIL_TYPENAME, name) - .map_err(|_| ApiError::not_found(format!("endpoint '{name}' not found"), None)) + .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found")) } /// Add a new sendmail endpoint. /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if an endpoint with the same name already exists, -/// or if the endpoint could not be saved. -pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<(), ApiError> { +/// Returns a `HttpError` if: +/// - an entity with the same name already exists (`400 Bad request`) +/// - a referenced filter does not exist (`400 Bad request`) +/// - the configuration could not be saved (`500 Internal server error`) +/// - mailto *and* mailto_user are both set to `None` +pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<(), HttpError> { super::ensure_unique(config, &endpoint.name)?; if let Some(filter) = &endpoint.filter { @@ -41,37 +47,39 @@ pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<() } if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() { - return Err(ApiError::bad_request( - "must at least provide one recipient, either in mailto or in mailto-user", - None, - )); + http_bail!( + BAD_REQUEST, + "must at least provide one recipient, either in mailto or in mailto-user" + ); } config .config .set_data(&endpoint.name, SENDMAIL_TYPENAME, endpoint) .map_err(|e| { - ApiError::internal_server_error( - format!("could not save endpoint '{}'", endpoint.name), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save endpoint '{}': {e}", + endpoint.name ) - })?; - - Ok(()) + }) } /// Update existing sendmail endpoint /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the config could not be saved. +/// Returns a `HttpError` if: +/// - a referenced filter does not exist (`400 Bad request`) +/// - the configuration could not be saved (`500 Internal server error`) +/// - mailto *and* mailto_user are both set to `None` pub fn update_endpoint( config: &mut Config, name: &str, updater: &SendmailConfigUpdater, delete: Option<&[DeleteableSendmailProperty]>, digest: Option<&[u8]>, -) -> Result<(), ApiError> { +) -> Result<(), HttpError> { super::verify_digest(config, digest)?; let mut endpoint = get_endpoint(config, name)?; @@ -115,31 +123,33 @@ pub fn update_endpoint( } if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() { - return Err(ApiError::bad_request( - "must at least provide one recipient, either in mailto or in mailto-user", - None, - )); + http_bail!( + BAD_REQUEST, + "must at least provide one recipient, either in mailto or in mailto-user" + ); } config .config .set_data(name, SENDMAIL_TYPENAME, &endpoint) .map_err(|e| { - ApiError::internal_server_error( - format!("could not save endpoint '{name}'"), - Some(e.into()), + http_err!( + INTERNAL_SERVER_ERROR, + "could not save endpoint '{}': {e}", + endpoint.name ) - })?; - - Ok(()) + }) } /// Delete existing sendmail endpoint /// /// The caller is responsible for any needed permission checks. /// The caller also responsible for locking the configuration files. -/// Returns an `ApiError` if the endpoint does not exist. -pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), ApiError> { +/// Returns a `HttpError` if: +/// - an entity with the same name already exists (`400 Bad request`) +/// - a referenced filter does not exist (`400 Bad request`) +/// - the configuration could not be saved (`500 Internal server error`) +pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError> { // Check if the endpoint exists let _ = get_endpoint(config, name)?; super::ensure_unused(config, name)?; @@ -154,7 +164,10 @@ pub mod tests { use super::*; use crate::api::test_helpers::*; - pub fn add_sendmail_endpoint_for_test(config: &mut Config, name: &str) -> Result<(), ApiError> { + pub fn add_sendmail_endpoint_for_test( + config: &mut Config, + name: &str, + ) -> Result<(), HttpError> { add_endpoint( config, &SendmailConfig { @@ -173,7 +186,7 @@ pub mod tests { } #[test] - fn test_sendmail_create() -> Result<(), ApiError> { + fn test_sendmail_create() -> Result<(), HttpError> { let mut config = empty_config(); assert_eq!(get_endpoints(&config)?.len(), 0); @@ -186,7 +199,7 @@ pub mod tests { } #[test] - fn test_update_not_existing_returns_error() -> Result<(), ApiError> { + fn test_update_not_existing_returns_error() -> Result<(), HttpError> { let mut config = empty_config(); assert!(update_endpoint(&mut config, "test", &Default::default(), None, None,).is_err()); @@ -195,7 +208,7 @@ pub mod tests { } #[test] - fn test_update_invalid_digest_returns_error() -> Result<(), ApiError> { + fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> { let mut config = empty_config(); add_sendmail_endpoint_for_test(&mut config, "sendmail-endpoint")?; @@ -219,7 +232,7 @@ pub mod tests { } #[test] - fn test_sendmail_update() -> Result<(), ApiError> { + fn test_sendmail_update() -> Result<(), HttpError> { let mut config = empty_config(); add_sendmail_endpoint_for_test(&mut config, "sendmail-endpoint")?; @@ -275,7 +288,7 @@ pub mod tests { } #[test] - fn test_sendmail_delete() -> Result<(), ApiError> { + fn test_sendmail_delete() -> Result<(), HttpError> { let mut config = empty_config(); add_sendmail_endpoint_for_test(&mut config, "sendmail-endpoint")?; -- 2.39.2