more/better error handling

src/error.rs

@@ -31,6 +31,9 @@ impl LoginRequired {
         self.return_to = Some(dest);
         self
     }
+    pub fn get_return(&self) -> Option<&Url> {
+        self.return_to.as_ref()
+    }
 }
 
 impl Display for LoginRequired {
@@ -61,15 +64,15 @@ pub enum MultipartFieldError {
         err: toml::value::DatetimeParseError,
     },
     /// File filed has no associated file_ame
-    #[error("missing filename for attachment")]
+    #[error("missing filename for attachment in field {field}")]
-    NotAFile,
+    NotAFile { field: &'static str },
     /// A field that can only be specified once occurred a second time
     #[error(
         "the field {field} occurred more often than expected, expected at most {limit} occurrences"
     )]
     TooManyOccurrences { field: &'static str, limit: u8 },
-    #[error("a required filed was missing: {0}")]
+    #[error("a required filed was missing: {field}")]
-    MissingField(&'static str),
+    MissingField { field: &'static str },
     #[error("{0}")]
     IOError(#[from] std::io::Error),
     #[error("{0}")]
@@ -83,16 +86,16 @@ pub enum MultipartFieldError {
 impl ResponseError for MultipartFieldError {
     fn status_code(&self) -> StatusCode {
         match self {
-            MultipartFieldError::ContentTooLarge { .. } => StatusCode::PAYLOAD_TOO_LARGE,
             MultipartFieldError::IOError(_) | MultipartFieldError::Runtime(_) => {
                 StatusCode::INTERNAL_SERVER_ERROR
             }
-            MultipartFieldError::Date { .. }
+            MultipartFieldError::ContentTooLarge { .. }
+            | MultipartFieldError::Date { .. }
             | MultipartFieldError::MultipartError(_)
-            | MultipartFieldError::NotAFile
+            | MultipartFieldError::NotAFile { field: _ }
             | MultipartFieldError::TooManyOccurrences { .. }
             | MultipartFieldError::UTF8Error(_)
-            | MultipartFieldError::MissingField(_)
+            | MultipartFieldError::MissingField { field: _ }
             | MultipartFieldError::InvalidAddress(_) => StatusCode::BAD_REQUEST,
         }
     }
@@ -135,7 +138,17 @@ pub(crate) fn default_error_response(
     error: &impl Error,
     status: StatusCode,
 ) -> HttpResponse<BoxBody> {
-    warn!("Some error occurred {}", error);
+    if status.is_server_error() {
+        error!("A Server-Side Error Occurred: {}", error)
+    } else if status.is_client_error() {
+        warn!("A Client-Side Error Occurred: {}", error)
+    } else {
+        error!(
+            "An error occurred, but a non-error status code was generated {}: {}",
+            status, error
+        )
+    }
+
     HttpResponse::build(status)
         .insert_header((header::CONTENT_TYPE, mime::TEXT_PLAIN_UTF_8))
         .body(error.to_string())

src/job_offers/error.rs

@@ -74,19 +74,13 @@ pub(crate) enum EmailError {
 
 impl ResponseError for EmailError {
     fn status_code(&self) -> StatusCode {
-        match self {
+        // technically the email address could be invalid,
-            EmailError::Email(_) => StatusCode::BAD_REQUEST,
+        // but an un-parsable email address should have been caught at form validation time
-            EmailError::SendMail(_) => StatusCode::INTERNAL_SERVER_ERROR,
+        StatusCode::INTERNAL_SERVER_ERROR
-            EmailError::Template(_) => StatusCode::INTERNAL_SERVER_ERROR,
-        }
     }
     fn error_response(&self) -> HttpResponse<BoxBody> {
         let status_code = self.status_code();
-        match self {
+        default_error_response(self, status_code)
-            EmailError::Email(inner) => default_error_response(inner, status_code),
-            EmailError::SendMail(inner) => default_error_response(inner, status_code),
-            EmailError::Template(inner) => default_error_response(inner, status_code),
-        }
     }
 }
 

src/main.rs

@@ -16,6 +16,7 @@ use listenfd::ListenFd;
 #[cfg(feature = "dev_mode")]
 use log::info;
 use log::{error, LevelFilter, SetLoggerError};
+use route::error_handler;
 
 mod auth;
 mod error;
@@ -106,14 +107,25 @@ async fn run() -> std::result::Result<(), error::SeverInitializationError> {
                 ErrorHandlers::default()
                     .handler(
                         StatusCode::INTERNAL_SERVER_ERROR,
-                        route::internal_server_error_handler,
+                        error_handler::internal_server_error_handler,
                     )
-                    .handler(StatusCode::NOT_FOUND, route::not_found_error_handler)
+                    .handler(
-                    .handler(StatusCode::UNAUTHORIZED, route::unauthorized_error_handler),
+                        StatusCode::NOT_FOUND,
+                        error_handler::not_found_error_handler,
+                    )
+                    .handler(
+                        StatusCode::UNAUTHORIZED,
+                        error_handler::unauthorized_error_handler,
+                    )
+                    .handler(StatusCode::BAD_REQUEST, error_handler::bad_request)
+                    .handler(
+                        StatusCode::TOO_MANY_REQUESTS,
+                        error_handler::too_many_requests,
+                    ),
             )
+            .wrap(session_store)
             .wrap(NormalizePath::new(TrailingSlash::Trim))
             .wrap(logger)
-            .wrap(session_store)
             .app_data(hb_ref.clone())
             .app_data(jobs_ref.clone())
             .app_data(bundle_ref.clone())

src/route.rs

@@ -1,22 +1,16 @@
 use std::borrow::Cow;
 
 use actix_files::NamedFile;
-use actix_session::SessionExt;
-use actix_web::dev::ServiceResponse;
 use actix_web::error::UrlGenerationError;
-use actix_web::http::header::LOCATION;
+use actix_web::http::header;
-use actix_web::http::{header, Method};
+use actix_web::web::{self, ServiceConfig};
-use actix_web::middleware::ErrorHandlerResponse;
+use actix_web::{get, HttpRequest, HttpResponse};
-use actix_web::web::{self, Data, ServiceConfig};
-use actix_web::{get, HttpRequest, HttpResponse, ResponseError};
-use handlebars::Handlebars;
 use http::HeaderValue;
 use serde::{Serialize, Serializer};
-use serde_json::json;
-use thiserror::private::DisplayAsDisplay;
 use url::Url;
 
 mod auth;
+pub(crate) mod error_handler;
 mod job_offer;
 mod license;
 
@@ -29,7 +23,6 @@ pub(crate) use job_offer::{
 };
 pub(crate) use license::{LICENSES_ROUTE, LICENSE_BUNDLE};
 
-use crate::auth::User;
 use crate::error::PresentationError;
 use crate::route::job_offer::action::JOBOFFER_DELETE_EXPIRED_ROUTE;
 use crate::server_config::OperationMode;
@@ -59,79 +52,6 @@ async fn static_index_css() -> Result<NamedFile, actix_web::Error> {
     Ok(file.use_last_modified(true))
 }
 
-#[derive(Debug, thiserror::Error)]
-#[error("Some error occurred while attempting to display an error page")]
-pub struct ErrorHandlerError;
-impl ResponseError for ErrorHandlerError {}
-
-pub(crate) fn generic_server_error_handler<B>(
-    res: ServiceResponse,
-    template: &str,
-    title: &str,
-) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
-    let hb: &Data<Handlebars> = res.request().app_data().ok_or(ErrorHandlerError)?;
-    let config: &Data<ServerConfig> = res.request().app_data().ok_or(ErrorHandlerError)?;
-
-    let base = base(res.request(), config, title)?;
-    let session = res.get_session();
-    let user = User::current(&session).ok();
-
-    let data = json!({
-        "base": base,
-        "user": user,
-    });
-
-    let body = hb
-        .render(template, &data)
-        .map_err(PresentationError::Render)?;
-
-    let response = HttpResponse::with_body(res.status(), body)
-        .map_into_boxed_body()
-        .map_into_right_body();
-    let response = res.into_response(response);
-
-    Ok(ErrorHandlerResponse::Response(response))
-}
-
-pub(crate) fn internal_server_error_handler<B>(
-    res: ServiceResponse,
-) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
-    generic_server_error_handler(res, "error/500", "Internal Server Error")
-}
-
-pub(crate) fn not_found_error_handler<B>(
-    res: ServiceResponse,
-) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
-    generic_server_error_handler(res, "error/404", "Not Found")
-}
-
-pub(crate) fn unauthorized_error_handler<B>(
-    res: ServiceResponse,
-) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
-    let mut login_url = res.request().url_for_static(LOGIN_ROUTE)?;
-
-    if res.request().method() == Method::GET {
-        let req_uri = res.request().uri().as_display().to_string();
-
-        login_url
-            .query_pairs_mut()
-            .append_pair("return_to", &req_uri);
-        let response = HttpResponse::SeeOther()
-            .insert_header((LOCATION, login_url.as_str()))
-            .body("")
-            .map_into_boxed_body()
-            .map_into_right_body();
-
-        Ok(ErrorHandlerResponse::Response(res.into_response(response)))
-    } else {
-        // we do not want to keep the 401 Unauthorized status-code as we will not set the WWW-Authenticate header.
-        // which the standard requires for 401 responses
-        let (req, res) = res.into_parts();
-        let res = ServiceResponse::new(req, HttpResponse::BadRequest().body(res.into_body()));
-        generic_server_error_handler(res, "error/401", "Unauthorized")
-    }
-}
-
 #[derive(Serialize)]
 struct BaseData<'a> {
     title: Cow<'a, str>,

src/route/error_handler.rs

@@ -0,0 +1,254 @@
+use crate::auth::User;
+use crate::error::{AuthenticationError, LoginRequired, MultipartFieldError, PresentationError};
+use crate::job_offers::error::{DeleteError, SaveError};
+use crate::route::LOGIN_ROUTE;
+use crate::{route, ServerConfig};
+
+use crate::route::job_offer::error::{ConfirmationError, SubmissionError};
+use actix_session::SessionExt;
+use actix_web::dev::ServiceResponse;
+use actix_web::error::UrlGenerationError;
+use actix_web::middleware::ErrorHandlerResponse;
+use actix_web::web::Data;
+use actix_web::{HttpRequest, HttpResponse, ResponseError};
+use handlebars::Handlebars;
+use http::header::LOCATION;
+use http::Method;
+use lettre::address::AddressError;
+use log::{error, warn};
+use serde_json::json;
+use thiserror::private::DisplayAsDisplay;
+use url::Url;
+
+#[derive(Debug, thiserror::Error)]
+#[error("Some error occurred while attempting to display an error page")]
+pub struct ErrorHandlerError;
+
+impl ResponseError for ErrorHandlerError {}
+
+pub(crate) fn generic_server_error_handler<B>(
+    res: ServiceResponse,
+    template: &str,
+    title: &str,
+    msg: Option<&str>,
+) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
+    let hb: &Data<Handlebars> = res.request().app_data().ok_or(ErrorHandlerError)?;
+    let config: &Data<ServerConfig> = res.request().app_data().ok_or(ErrorHandlerError)?;
+
+    let base = route::base(res.request(), config, title)?;
+    let session = res.get_session();
+    let user = User::current(&session).ok();
+
+    let data = json!({
+        "base": base,
+        "user": user,
+        "msg": msg,
+    });
+
+    let body = hb
+        .render(template, &data)
+        .map_err(PresentationError::Render)?;
+
+    let response = HttpResponse::with_body(res.status(), body)
+        .map_into_boxed_body()
+        .map_into_right_body();
+    let response = res.into_response(response);
+
+    Ok(ErrorHandlerResponse::Response(response))
+}
+
+pub(crate) fn internal_server_error_handler<B>(
+    res: ServiceResponse,
+) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
+    if let Some(err) = res.response().error() {
+        if let Some(err) = err.as_error::<SaveError>() {
+            error!("Internal Server Error due to SaveError: {}", err)
+        } else if let Some(err) = err.as_error::<DeleteError>() {
+            error!("Internal Server Error due to DeleteError: {}", err)
+        } else if let Some(err) = err.as_error::<ErrorHandlerError>() {
+            error!("Internal Server Error due to ErrorHandlerError: {}", err)
+        } else if let Some(err) = err.as_error::<MultipartFieldError>() {
+            error!("Internal Server Error due to MultipartFieldError: {}", err)
+        } else if let Some(err) = err.as_error::<PresentationError>() {
+            error!("Internal Server Error due to PresentationError: {}", err)
+        } else if let Some(err) = err.as_error::<AuthenticationError>() {
+            error!("Internal Server Error due to AuthenticationError: {}", err)
+        } else if let Some(err) = err.as_error::<SubmissionError>() {
+            error!("Internal Server Error due to SubmissionError: {}", err)
+        } else {
+            error!("Unknown Error Type for Internal Server Error")
+        }
+    }
+    // we only generate a generic error page as we don't want to (accidentally) leak interna
+    generic_server_error_handler(res, "error/500", "Internal Server Error", None)
+}
+
+pub(crate) fn not_found_error_handler<B>(
+    res: ServiceResponse,
+) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
+    generic_server_error_handler(res, "error/404", "Not Found", None)
+}
+
+pub(crate) fn too_many_requests<B>(
+    res: ServiceResponse,
+) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
+    generic_server_error_handler(res, "error/429", "Too Many Requests", None)
+}
+
+pub(crate) fn bad_request<B>(
+    res: ServiceResponse,
+) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
+    let msg;
+    let msg = if let Some(err) = res.response().error() {
+        if let Some(err) = err.as_error::<SubmissionError>() {
+            match err {
+                SubmissionError::MissingLinkOrAttachment => {
+                    Some("Es muss mindestens ein Link oder ein Anhang angegeben werden.")
+                }
+                SubmissionError::Form(MultipartFieldError::Date { err:_ }) => {
+                    Some("Eine Datumsangabe entsprach nicht dem erwarteten Format.")
+                }
+                SubmissionError::Form(MultipartFieldError::ContentTooLarge {
+                    field,
+                    max_byte_size,
+                }) => {
+                    msg = format!(
+                        "Der Inhalt des Feldes mit ID {field} war zu lang, maximal {max_byte_size} Bytes sind gestated."
+                    );
+                    Some(msg.as_str())
+                }
+                SubmissionError::Form(MultipartFieldError::TooManyOccurrences { field, limit }) => {
+                    msg = format!(
+                        "Das Feld mit der ID {field} wurde zu oft vorhanden, erlaubt sind {limit} vorkommen."
+                    );
+                    Some(msg.as_str())
+                }
+                SubmissionError::Form(MultipartFieldError::MultipartError(mpe)) => {
+                    warn!("{}", mpe);
+                    msg = format!("{}", mpe);
+                    Some(msg.as_str())
+                }
+                SubmissionError::Form(MultipartFieldError::NotAFile { field }) => {
+                    msg = format!(
+                        "Das Feld mit der ID {field} erwartet eine Datei, aber der zugehörige ContentDisposition-Header enthielt keinen Dateinamen."
+                    );
+                    Some(msg.as_str())
+                }
+                SubmissionError::Form(MultipartFieldError::MissingField { field }) => {
+                    msg =
+                        format!("Das Feld mit der ID {field} fehlt obwohl es nicht optional ist.");
+                    Some(msg.as_str())
+                }
+                SubmissionError::Form(MultipartFieldError::UTF8Error(_err)) => {
+                    Some("Ein Feld das im UTF-8 format erwartet wurde konnte nicht als UTF-8 geparst werden.")
+                }
+                SubmissionError::Form(MultipartFieldError::InvalidAddress(reason)) => {
+                    Some(match reason {
+                        AddressError::MissingParts => { "Unvollständige E-Mail Address" }
+                        AddressError::Unbalanced => { "Unausgeglichene Klammern '<' & '>' in E-Mail Address. " }
+                        AddressError::InvalidUser => { "Der Local/User Teil (vor dem @) der E-Mail Address ist ungültig." }
+                        AddressError::InvalidDomain => { "Der Host/Domain Teil (nach dem @) der E-Mail Address ist ungültig." }
+                    })
+                }
+                SubmissionError::Save(_)
+                | SubmissionError::TooManyRequests
+                | SubmissionError::Render(_)
+                | SubmissionError::Form(MultipartFieldError::IOError(_))
+                | SubmissionError::Form(MultipartFieldError::Runtime(_)) => {
+                    error!(
+                        "Response Status Code (Bad Request) and Error appear to disagree : {}",
+                        err
+                    );
+                    None
+                }
+            }
+        } else if let Some(err) = err.as_error::<ConfirmationError>() {
+            match err {
+                ConfirmationError::InvalidRequest => {
+                    Some("Die Stellenanzeige erwartet keine Bestätigung, die Stellenanzeigen ID ist ungültig oder der Bestätigungstoken is ungültig.")
+                }
+                ConfirmationError::Save(_)
+                | ConfirmationError::Delete(_)
+                | ConfirmationError::SuccessRenderError(_)
+                | ConfirmationError::RenderError(_)
+                | ConfirmationError::Url(_)
+                | ConfirmationError::Presentation(_) => {
+                    error!(
+                        "Response Status Code (Bad Request) and Error appear to disagree : {}",
+                        err
+                    );
+                    None
+                }
+            }
+        } else if let Some(err) = err.as_error::<MultipartFieldError>() {
+            warn!("Unexpected MultipartFieldError as top level error!");
+            msg = err.to_string();
+            Some(msg.as_str())
+        } else {
+            warn!("Bad Request Error of unknown type!");
+            None
+        }
+    } else {
+        warn!("Bad Request does not have an associated error!");
+        None
+    };
+    generic_server_error_handler(res, "error/400", "Bad Request", msg)
+}
+
+fn login_url_with_return(req: &HttpRequest, return_to: &str) -> Result<Url, UrlGenerationError> {
+    let mut login_url = req.url_for_static(LOGIN_ROUTE)?;
+
+    login_url
+        .query_pairs_mut()
+        .append_pair("return_to", return_to);
+
+    Ok(login_url)
+}
+
+pub(crate) fn unauthorized_error_handler<B>(
+    res: ServiceResponse,
+) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
+    if let Some(return_url) = res
+        .response()
+        .error()
+        .and_then(|err| err.as_error::<LoginRequired>())
+        .and_then(|err| err.get_return())
+    {
+        // we have a LoginRequired error type with a return_to URL set, redirect the user to the login page with the
+        // return_to url parameter set
+
+        let login_url = login_url_with_return(res.request(), return_url.as_str())?;
+
+        let response = HttpResponse::SeeOther()
+            .insert_header((LOCATION, login_url.as_str()))
+            .body("")
+            .map_into_boxed_body()
+            .map_into_right_body();
+
+        Ok(ErrorHandlerResponse::Response(res.into_response(response)))
+    } else if res.request().method() == Method::GET {
+        // for a get request we can just return the user to the originally requested page after login,
+        // so redirect them to the login page and set the return_to url parameter to the target of the current request
+
+        let req_uri = res.request().uri().as_display().to_string();
+
+        let login_url = login_url_with_return(res.request(), &req_uri)?;
+
+        let response = HttpResponse::SeeOther()
+            .insert_header((LOCATION, login_url.as_str()))
+            .body("")
+            .map_into_boxed_body()
+            .map_into_right_body();
+
+        Ok(ErrorHandlerResponse::Response(res.into_response(response)))
+    } else {
+        // we have neither a known return path nor is the current request of method get, which we could retry after login
+
+        // we do not want to keep the 401 Unauthorized status-code as we will not set the WWW-Authenticate header
+        // and the standard requires 401 responses to carry that header.
+
+        let (req, res) = res.into_parts();
+        let res = ServiceResponse::new(req, HttpResponse::BadRequest().body(res.into_body()));
+        generic_server_error_handler(res, "error/401", "Unauthorized", None)
+    }
+}

src/route/job_offer/create.rs

@@ -249,16 +249,13 @@ async fn send_confirmation_email(
 ) -> Result<(), EmailError> {
     let to_mailbox = Mailbox::new(None, contact_address);
 
-    let email_body = hb
+    let email_body = hb.render(template::EMAIL_PLAIN, &email_data)?;
-        .render(template::EMAIL_PLAIN, &email_data)
-        .map_err(EmailError::from)?;
 
     let message = lettre::Message::builder()
         .from(email_config.from.to_owned())
         .to(to_mailbox)
         .subject(&email_config.subject)
-        .singlepart(SinglePart::plain(email_body))
+        .singlepart(SinglePart::plain(email_body))?;
-        .map_err(EmailError::from)?;
     lettre::AsyncSendmailTransport::new().send(message).await?;
 
     let message = lettre::Message::builder()
@@ -446,7 +443,7 @@ impl JobOfferSubmitForm {
             })
             .collect();
 
-        let contact = contact.ok_or(MultipartFieldError::MissingField("contact"))?;
+        let contact = contact.ok_or(MultipartFieldError::MissingField { field: "contact" })?;
 
         let expiry_date = match expires.as_deref() {
             None | Some("") => None,
@@ -463,9 +460,10 @@ impl JobOfferSubmitForm {
         };
 
         Ok(JobOfferSubmitForm {
-            title: title.ok_or(MultipartFieldError::MissingField("title"))?,
+            title: title.ok_or(MultipartFieldError::MissingField { field: "title" })?,
-            offering_party: offering_party
+            offering_party: offering_party.ok_or(MultipartFieldError::MissingField {
-                .ok_or(MultipartFieldError::MissingField("offering_party"))?,
+                field: "offering_party",
+            })?,
             contact: Address::from_str(&contact)?,
             public_contact: public_contact.as_deref() == Some(VISIBLE),
             expires: expiry_date,

src/route/job_offer/create/multipart_form.rs

@@ -96,7 +96,7 @@ pub(crate) async fn tmpfile_from_field(
         .content_disposition()
         .get_filename()
         .map(str::to_string)
-        .ok_or(MultipartFieldError::NotAFile)?;
+        .ok_or(MultipartFieldError::NotAFile { field: field_name })?;
 
     let mut remaining = limit;
 

src/server_config/test.rs

@@ -4,3 +4,10 @@ async fn load_dist_config() {
         .await
         .expect("should be able to load config/dist-config.toml ");
 }
+
+#[test]
+fn default_config_toml_serializable() {
+    let default_config = super::ProgramConfig::default();
+    toml::to_string_pretty(&default_config)
+        .expect("successful serialization of default program config");
+}

templates/error/400.hb

@@ -0,0 +1,12 @@
+{{#> base}}
+    <div class="centered">
+        <div>
+            Die Aktion konnte nicht durchgeführt werden. <br />
+            {{#if msg}}
+                {{msg}}
+            {{else}}
+                Es scheint ein un-kategorisiertes Problem mit ihrer Anfrage zu bestehen.
+            {{/if}}
+        </div>
+    </div>
+{{/base}}

templates/error/429.hb

@@ -0,0 +1,9 @@
+{{#> base}}
+    <div class="centered">
+        <div>
+            Die Aktion konnte nicht durchgeführt werden. <br />
+            Es wurden zu viele Anfragen in zu kurzer Zeit gestellt. <br />
+            Bitte warten etwas bevor sie weitere Anfragen stellen.
+        </div>
+    </div>
+{{/base}}