From f6ac35fd7094c3c036d2c81802f5f62fba5570ae Mon Sep 17 00:00:00 2001 From: mdecimus Date: Fri, 19 Jul 2024 16:11:00 +0200 Subject: [PATCH] Improved error handling - all tests passing --- .../directory/src/backend/internal/manage.rs | 2 +- crates/imap/src/core/session.rs | 4 +- crates/jmap/src/api/http.rs | 2 +- crates/jmap/src/auth/oauth/token.rs | 41 ++++++++------- crates/jmap/src/services/ingest.rs | 4 +- crates/pop3/src/session.rs | 2 +- crates/trc/src/imple.rs | 50 +++++++++++++++++++ crates/trc/src/macros.rs | 2 +- tests/src/directory/internal.rs | 47 +++++++---------- tests/src/directory/mod.rs | 4 +- tests/src/jmap/mod.rs | 3 ++ tests/src/jmap/purge.rs | 5 +- 12 files changed, 105 insertions(+), 61 deletions(-) diff --git a/crates/directory/src/backend/internal/manage.rs b/crates/directory/src/backend/internal/manage.rs index de88d7fa..6edc3497 100644 --- a/crates/directory/src/backend/internal/manage.rs +++ b/crates/directory/src/backend/internal/manage.rs @@ -140,7 +140,7 @@ impl ManageDirectory for Store { ) -> trc::Result { // Make sure the principal has a name if principal.name.is_empty() { - return Err(not_found(PrincipalField::Name)); + return Err(err_missing(PrincipalField::Name)); } // Map group names diff --git a/crates/imap/src/core/session.rs b/crates/imap/src/core/session.rs index ca13778f..d242a25e 100644 --- a/crates/imap/src/core/session.rs +++ b/crates/imap/src/core/session.rs @@ -205,7 +205,7 @@ impl Session { pub async fn write_error(&self, err: trc::Error) -> bool { tracing::warn!(parent: &self.span, event = "error", reason = %err, "IMAP error."); - if !err.matches(trc::Cause::Network) { + if err.should_write_err() { let disconnect = err.must_disconnect(); if let Err(err) = self.write_bytes(err.serialize()).await { @@ -247,7 +247,7 @@ impl super::SessionData { pub async fn write_error(&self, err: trc::Error) -> trc::Result<()> { tracing::warn!(parent: &self.span, event = "error", reason = %err, "IMAP error."); - if !err.matches(trc::Cause::Network) { + if err.should_write_err() { self.write_bytes(err.serialize()).await } else { Ok(()) diff --git a/crates/jmap/src/api/http.rs b/crates/jmap/src/api/http.rs index 9a54406d..45168480 100644 --- a/crates/jmap/src/api/http.rs +++ b/crates/jmap/src/api/http.rs @@ -598,7 +598,7 @@ impl ToRequestError for trc::Error { trc::AuthCause::TooManyAttempts | trc::AuthCause::Banned => { RequestError::too_many_auth_attempts() } - trc::AuthCause::Error => RequestError::internal_server_error(), + trc::AuthCause::Error => RequestError::unauthorized(), }, trc::Cause::Resource(cause) => match cause { trc::ResourceCause::NotFound => RequestError::not_found(), diff --git a/crates/jmap/src/auth/oauth/token.rs b/crates/jmap/src/auth/oauth/token.rs index 88512a30..38569aaf 100644 --- a/crates/jmap/src/auth/oauth/token.rs +++ b/crates/jmap/src/auth/oauth/token.rs @@ -132,25 +132,29 @@ impl JMAP { } } else if grant_type.eq_ignore_ascii_case("refresh_token") { if let Some(refresh_token) = params.get("refresh_token") { - let (account_id, client_id, time_left) = self + response = match self .validate_access_token("refresh_token", refresh_token) - .await?; - - // TODO: implement revoking client ids - response = self - .issue_token( - account_id, - &client_id, - time_left <= self.core.jmap.oauth_expiry_refresh_token_renew, - ) .await - .map(TokenResponse::Granted) - .map_err(|err| { - trc::AuthCause::Error - .into_err() - .details(err) - .caused_by(trc::location!()) - })?; + { + Ok((account_id, client_id, time_left)) => self + .issue_token( + account_id, + &client_id, + time_left <= self.core.jmap.oauth_expiry_refresh_token_renew, + ) + .await + .map(TokenResponse::Granted) + .map_err(|err| { + trc::AuthCause::Error + .into_err() + .details(err) + .caused_by(trc::location!()) + })?, + Err(err) => { + tracing::warn!("Failed to validate refresh token: {:?}", err); + TokenResponse::error(ErrorType::InvalidGrant) + } + }; } else { response = TokenResponse::error(ErrorType::InvalidRequest); } @@ -281,6 +285,7 @@ impl JMAP { trc::AuthCause::Error .into_err() .ctx(trc::Key::Reason, "Failed to decode token") + .caused_by(trc::location!()) .details(token_.to_string()) })?; let (account_id, expiry, client_id) = token @@ -298,6 +303,7 @@ impl JMAP { trc::AuthCause::Error .into_err() .ctx(trc::Key::Reason, "Failed to decode token") + .caused_by(trc::location!()) .details(token_.to_string()) })?; @@ -349,6 +355,7 @@ impl JMAP { trc::AuthCause::Error .into_err() .ctx(trc::Key::Details, "Failed to decode token") + .caused_by(trc::location!()) .reason(err) })?; diff --git a/crates/jmap/src/services/ingest.rs b/crates/jmap/src/services/ingest.rs index bbf12f84..db52740f 100644 --- a/crates/jmap/src/services/ingest.rs +++ b/crates/jmap/src/services/ingest.rs @@ -161,12 +161,12 @@ impl JMAP { trc::Cause::Ingest => { *status = DeliveryResult::PermanentFailure { code: err - .value(trc::Key::Reason) + .value(trc::Key::Code) .and_then(|v| v.to_uint()) .map(|n| { [(n / 100) as u8, ((n % 100) / 10) as u8, (n % 10) as u8] }) - .unwrap(), + .unwrap_or([5, 5, 0]), reason: err .take_value(trc::Key::Reason) .and_then(|v| v.into_string()) diff --git a/crates/pop3/src/session.rs b/crates/pop3/src/session.rs index 6837cd68..c984b720 100644 --- a/crates/pop3/src/session.rs +++ b/crates/pop3/src/session.rs @@ -163,7 +163,7 @@ impl Session { tracing::error!(parent: &self.span, "POP3 error: {}", err); let disconnect = err.must_disconnect(); - if !err.matches(trc::Cause::Network) { + if err.should_write_err() { if let Err(err) = self.write_bytes(err.serialize()).await { tracing::debug!(parent: &self.span, "Failed to write error: {}", err); return false; diff --git a/crates/trc/src/imple.rs b/crates/trc/src/imple.rs index 3dffa825..e4a448ef 100644 --- a/crates/trc/src/imple.rs +++ b/crates/trc/src/imple.rs @@ -435,6 +435,11 @@ impl Error { | Cause::Limit(LimitCause::ConcurrentRequest | LimitCause::TooManyRequests) ) } + + #[inline(always)] + pub fn should_write_err(&self) -> bool { + !matches!(self.inner, Cause::Network | Cause::Auth(AuthCause::Banned)) + } } impl Value { @@ -495,3 +500,48 @@ impl Display for Context { } impl std::error::Error for Error {} + +impl PartialEq for Value { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Self::Static(l0), Self::Static(r0)) => l0 == r0, + (Self::String(l0), Self::String(r0)) => l0 == r0, + (Self::String(l0), Self::Static(r0)) => l0 == r0, + (Self::Static(l0), Self::String(r0)) => l0 == r0, + (Self::UInt(l0), Self::UInt(r0)) => l0 == r0, + (Self::Int(l0), Self::Int(r0)) => l0 == r0, + (Self::Float(l0), Self::Float(r0)) => l0 == r0, + (Self::Bytes(l0), Self::Bytes(r0)) => l0 == r0, + (Self::Bool(l0), Self::Bool(r0)) => l0 == r0, + (Self::Ipv4(l0), Self::Ipv4(r0)) => l0 == r0, + (Self::Ipv6(l0), Self::Ipv6(r0)) => l0 == r0, + (Self::Protocol(l0), Self::Protocol(r0)) => l0 == r0, + (Self::Error(l0), Self::Error(r0)) => l0 == r0, + (Self::Array(l0), Self::Array(r0)) => l0 == r0, + _ => false, + } + } +} + +impl Eq for Value {} + +impl PartialEq for Context +where + T: Eq, +{ + fn eq(&self, other: &Self) -> bool { + if self.inner == other.inner && self.keys_size == other.keys_size { + for kv in self.keys.iter().take(self.keys_size) { + if !other.keys.iter().take(other.keys_size).any(|okv| kv == okv) { + return false; + } + } + + true + } else { + false + } + } +} + +impl Eq for Context where T: Eq {} diff --git a/crates/trc/src/macros.rs b/crates/trc/src/macros.rs index cd9c2b8a..b49f4e30 100644 --- a/crates/trc/src/macros.rs +++ b/crates/trc/src/macros.rs @@ -34,7 +34,7 @@ macro_rules! error { #[macro_export] macro_rules! location { () => {{ - concat!(file!(), ":", line!(), " (", module_path!(), ")") + concat!(file!(), ":", line!()) }}; } diff --git a/tests/src/directory/internal.rs b/tests/src/directory/internal.rs index 7efc2017..81b2f215 100644 --- a/tests/src/directory/internal.rs +++ b/tests/src/directory/internal.rs @@ -7,8 +7,9 @@ use ahash::AHashSet; use directory::{ backend::internal::{ - lookup::DirectoryStore, manage::ManageDirectory, PrincipalField, PrincipalUpdate, - PrincipalValue, + lookup::DirectoryStore, + manage::{self, ManageDirectory}, + PrincipalField, PrincipalUpdate, PrincipalValue, }, Principal, QueryBy, Type, }; @@ -33,9 +34,7 @@ async fn internal_directory() { // A principal without name should fail assert_eq!( store.create_account(Principal::default(), vec![]).await, - Err(DirectoryError::Management(ManagementError::MissingField( - PrincipalField::Name - ))) + Err(manage::err_missing(PrincipalField::Name)) ); // Basic account creation @@ -63,10 +62,7 @@ async fn internal_directory() { vec![] ) .await, - Err(DirectoryError::Management(ManagementError::AlreadyExists { - field: PrincipalField::Name, - value: "john".to_string() - })) + Err(manage::err_exists(PrincipalField::Name, "john".to_string())) ); // An account using a non-existent domain should fail @@ -81,9 +77,7 @@ async fn internal_directory() { vec![] ) .await, - Err(DirectoryError::Management(ManagementError::NotFound( - "example.org".to_string() - ))) + Err(manage::not_found("example.org".to_string())) ); // Create a domain name @@ -121,9 +115,7 @@ async fn internal_directory() { )], ) .await, - Err(DirectoryError::Management(ManagementError::NotFound( - "otherdomain.org".to_string() - ))) + Err(manage::not_found("otherdomain.org".to_string())) ); // Create an account with an email address @@ -197,10 +189,10 @@ async fn internal_directory() { vec![] ) .await, - Err(DirectoryError::Management(ManagementError::AlreadyExists { - field: PrincipalField::Emails, - value: "jane@example.org".to_string() - })) + Err(manage::err_exists( + PrincipalField::Emails, + "jane@example.org".to_string() + )) ); // Create a mailing list @@ -348,9 +340,7 @@ async fn internal_directory() { )], ) .await, - Err(DirectoryError::Management(ManagementError::NotFound( - "accounting".to_string() - ))) + Err(manage::not_found("accounting".to_string())) ); // Remove a member from a group @@ -502,10 +492,7 @@ async fn internal_directory() { ),], ) .await, - Err(DirectoryError::Management(ManagementError::AlreadyExists { - field: PrincipalField::Name, - value: "jane".to_string() - })) + Err(manage::err_exists(PrincipalField::Name, "jane".to_string())) ); assert_eq!( store @@ -517,10 +504,10 @@ async fn internal_directory() { ),], ) .await, - Err(DirectoryError::Management(ManagementError::AlreadyExists { - field: PrincipalField::Emails, - value: "jane@example.org".to_string() - })) + Err(manage::err_exists( + PrincipalField::Emails, + "jane@example.org".to_string() + )) ); // List accounts diff --git a/tests/src/directory/mod.rs b/tests/src/directory/mod.rs index 2b36b8a2..d6efc3e2 100644 --- a/tests/src/directory/mod.rs +++ b/tests/src/directory/mod.rs @@ -5,7 +5,7 @@ */ pub mod imap; -//pub mod internal; +pub mod internal; pub mod ldap; pub mod smtp; pub mod sql; @@ -392,8 +392,6 @@ pub fn dummy_tls_acceptor() -> Arc { let cert_file = &mut BufReader::new(CERT.as_bytes()); let key_file = &mut BufReader::new(PK.as_bytes()); - let todo = "fix interkal"; - // convert files to key/cert objects let cert_chain = certs(cert_file).map(|r| r.unwrap()).collect(); let mut keys: Vec = pkcs8_private_keys(key_file) diff --git a/tests/src/jmap/mod.rs b/tests/src/jmap/mod.rs index 73f53f12..faa3afb7 100644 --- a/tests/src/jmap/mod.rs +++ b/tests/src/jmap/mod.rs @@ -253,6 +253,9 @@ email = "address" quota = "quota" class = "type" +[imap.auth] +allow-plain-text = true + [oauth] key = "parerga_und_paralipomena" diff --git a/tests/src/jmap/purge.rs b/tests/src/jmap/purge.rs index 464e443b..1d8315ce 100644 --- a/tests/src/jmap/purge.rs +++ b/tests/src/jmap/purge.rs @@ -32,7 +32,7 @@ pub async fn test(params: &mut JMAPTest) { // Connect to IMAP params .directory - .create_test_user_with_email("jdoe@example.com", "secret", "John Doe") + .create_test_user_with_email("jdoe@example.com", "12345", "John Doe") .await; let account_id = server .core @@ -43,8 +43,7 @@ pub async fn test(params: &mut JMAPTest) { .unwrap(); let mut imap = ImapConnection::connect(b"_x ").await; imap.assert_read(Type::Untagged, ResponseType::Ok).await; - imap.send("AUTHENTICATE PLAIN {32+}\r\nAGpkb2VAZXhhbXBsZS5jb20Ac2VjcmV0") - .await; + imap.send("LOGIN \"jdoe@example.com\" \"12345\"").await; imap.assert_read(Type::Tagged, ResponseType::Ok).await; imap.send("STATUS INBOX (UIDNEXT MESSAGES UNSEEN)").await; imap.assert_read(Type::Tagged, ResponseType::Ok)