Properly and consistently check server response status everywhere

ROADMAP.md

@@ -11,6 +11,7 @@
 - Release binaries on GitHub
 - Ubuntu PPA package
 - Implement error handling everywhere properly
+- Check all TODOs, solve them when possible
 
 # Future releases
 - Box errors

api/src/action/delete.rs

@@ -1,12 +1,12 @@
-use reqwest::{Client, StatusCode};
+use reqwest::Client;
 
 use api::data::{
     Error as DataError,
     OwnedData,
 };
 use api::nonce::{NonceError, request_nonce};
+use api::request::{ensure_success, ResponseError};
 use api::url::UrlBuilder;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 
 /// An action to delete a remote file.
@@ -66,13 +66,9 @@ impl<'a> Delete<'a> {
             .send()
             .map_err(|_| DeleteError::Request)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(DeleteError::RequestStatus(status, status.err_text()).into());
-        }
-
-        Ok(())
+        // Ensure the status code is succesful
+        ensure_success(&response)
+            .map_err(|err| DeleteError::Response(err))
     }
 }
 
@@ -150,8 +146,7 @@ pub enum DeleteError {
     #[fail(display = "Failed to send file deletion request")]
     Request,
 
-    /// The response for deleting the file indicated an error and wasn't
-    /// successful.
-    #[fail(display = "Bad HTTP response '{}' while deleting the file", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while requesting file deletion.
+    #[fail(display = "Bad response from server while deleting file")]
+    Response(#[cause] ResponseError),
 }

api/src/action/download.rs

@@ -7,14 +7,14 @@ use std::io::{
 use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
 
-use reqwest::{Client, Response, StatusCode};
+use reqwest::{Client, Response};
 use reqwest::header::Authorization;
 use reqwest::header::ContentLength;
 
 use api::url::UrlBuilder;
+use api::request::{ensure_success, ResponseError};
 use crypto::key_set::KeySet;
 use crypto::sig::signature_encoded;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 use reader::{EncryptedFileWriter, ProgressReporter, ProgressWriter};
 use super::metadata::{
@@ -179,11 +179,9 @@ impl<'a> Download<'a> {
             .send()
             .map_err(|_| DownloadError::Request)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(DownloadError::RequestStatus(status, status.err_text()));
-        }
+        // Ensure the response is succesful
+        ensure_success(&response)
+            .map_err(|err| DownloadError::Response(err))?;
 
         // Get the content length
         // TODO: make sure there is enough disk space
@@ -307,9 +305,9 @@ pub enum DownloadError {
     #[fail(display = "Failed to request file download")]
     Request,
 
-    /// The response for downloading the indicated an error and wasn't successful.
-    #[fail(display = "Bad HTTP response '{}' while requesting file download", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while requesting the file download.
+    #[fail(display = "Bad response from server while requesting download")]
+    Response(#[cause] ResponseError),
 
     /// The length of the file is missing, thus the length of the file to download
     /// couldn't be determined.

api/src/action/exists.rs

@@ -1,12 +1,9 @@
-use reqwest::{Client, StatusCode};
+use reqwest::Client;
 
+use api::request::{ensure_success, ResponseError};
 use api::url::UrlBuilder;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 
-/// The HTTP status code that is returned for expired files.
-const FILE_EXPIRED_STATUS: StatusCode = StatusCode::NotFound;
-
 /// An action to check whether a remote file exists.
 /// This aciton returns an `ExistsResponse`, that defines whether the file
 /// exists, and whether it is protected by a password.
@@ -36,15 +33,13 @@ impl<'a> Exists<'a> {
             .send()
             .map_err(|_| Error::Request)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            // Handle expired files
-            if status == FILE_EXPIRED_STATUS {
-                return Ok(ExistsResponse::new(false, false));
-            } else {
-                return Err(Error::RequestStatus(status, status.err_text()).into());
-            }
+        // Ensure the status code is succesful, check the expiry state
+        match ensure_success(&response) {
+            Ok(_) => {},
+            Err(ResponseError::Expired) => return Ok(
+                ExistsResponse::new(false, false)
+            ),
+            Err(err) => return Err(Error::Response(err)),
         }
 
         // Parse the response
@@ -110,10 +105,10 @@ pub enum Error {
     #[fail(display = "Failed to send request whether the file exists")]
     Request,
 
-    /// The response for checking whether the file exists indicated an error
-    /// and wasn't successful.
-    #[fail(display = "Bad HTTP response '{}' while requesting whether the file exists", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while checking whether the file
+    /// exists.
+    #[fail(display = "Bad response from server while checking file existence")]
+    Response(#[cause] ResponseError),
 
     /// The response from the server when checking if the file exists was
     /// malformed.

api/src/action/info.rs

@@ -3,7 +3,6 @@ use std::cmp::max;
 use reqwest::{
     Client,
     Error as ReqwestError,
-    StatusCode,
 };
 
 use api::data::{
@@ -11,8 +10,8 @@ use api::data::{
     OwnedData,
 };
 use api::nonce::{NonceError, request_nonce};
+use api::request::{ensure_success, ResponseError};
 use api::url::UrlBuilder;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 
 /// An action to fetch info of a shared file.
@@ -72,11 +71,9 @@ impl<'a> Info<'a> {
             .send()
             .map_err(|_| InfoError::Request)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(InfoError::RequestStatus(status, status.err_text()).into());
-        }
+        // Ensure the response is successful
+        ensure_success(&response)
+            .map_err(|err| InfoError::Response(err))?;
 
         // Decode the JSON response
         let response: InfoResponse = match response.json() {
@@ -201,10 +198,9 @@ pub enum InfoError {
     #[fail(display = "Failed to send file info request")]
     Request,
 
-    /// The response fetching the file info indicated an error and wasn't
-    /// successful.
-    #[fail(display = "Bad HTTP response '{}' while fetching the file info", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while fetching the file info.
+    #[fail(display = "Bad response from server while fetching file info")]
+    Response(#[cause] ResponseError),
 
     /// Failed to decode the info response from the server.
     /// Maybe the server responded with data from a newer API version.

api/src/action/metadata.rs

@@ -1,15 +1,15 @@
 use failure::Error as FailureError;
 use openssl::symm::decrypt_aead;
-use reqwest::{Client, StatusCode};
+use reqwest::Client;
 use reqwest::header::Authorization;
 use serde_json;
 
 use api::nonce::{header_nonce, NonceError, request_nonce};
+use api::request::{ensure_success, ResponseError};
 use api::url::UrlBuilder;
 use crypto::b64;
 use crypto::key_set::KeySet;
 use crypto::sig::signature_encoded;
-use ext::status_code::StatusCodeExt;
 use file::metadata::Metadata as MetadataData;
 use file::remote_file::RemoteFile;
 use super::exists::{
@@ -17,9 +17,6 @@ use super::exists::{
     Exists as ExistsAction,
 };
 
-/// The HTTP status code that is returned for expired files.
-const FILE_EXPIRED_STATUS: StatusCode = StatusCode::NotFound;
-
 /// An action to fetch file metadata.
 pub struct Metadata<'a> {
     /// The remote file to fetch the metadata for.
@@ -107,13 +104,11 @@ impl<'a> Metadata<'a> {
                 format!("send-v1 {}", sig)
             ))
             .send()
-            .map_err(|_| MetaError::NonceReq)?;
+            .map_err(|_| MetaError::NonceRequest)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(MetaError::NonceReqStatus(status, status.err_text()));
-        }
+        // Ensure the status code is successful
+        ensure_success(&response)
+            .map_err(|err| MetaError::NonceResponse(err))?;
 
         // Get the metadata nonce
         let nonce = header_nonce(&response)
@@ -260,12 +255,12 @@ pub enum MetaError {
 
     /// Sending the request to gather the metadata encryption nonce failed.
     #[fail(display = "Failed to request metadata nonce")]
-    NonceReq,
+    NonceRequest,
 
-    /// The response for fetching the metadata encryption nonce indicated an
-    /// error and wasn't successful.
-    #[fail(display = "Bad HTTP response '{}' while requesting metadata nonce", _1)]
-    NonceReqStatus(StatusCode, String),
+    /// The server responded with an error while fetching the metadata
+    /// encryption nonce.
+    #[fail(display = "Bad response from server while fetching metadata nonce")]
+    NonceResponse(#[cause] ResponseError),
 
     /// Couldn't parse the metadata encryption nonce.
     #[fail(display = "Failed to parse the metadata encryption nonce")]

api/src/action/params.rs

@@ -1,12 +1,12 @@
-use reqwest::{Client, StatusCode};
+use reqwest::Client;
 
 use api::data::{
     Error as DataError,
     OwnedData,
 };
 use api::nonce::{NonceError, request_nonce};
+use api::request::{ensure_success, ResponseError};
 use api::url::UrlBuilder;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 
 /// The default download count.
@@ -88,13 +88,9 @@ impl<'a> Params<'a> {
             .send()
             .map_err(|_| ChangeError::Request)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(ChangeError::RequestStatus(status, status.err_text()).into());
-        }
-
-        Ok(())
+        // Ensure the response is successful
+        ensure_success(&response)
+            .map_err(|err| ChangeError::Response(err))
     }
 }
 
@@ -231,8 +227,7 @@ pub enum ChangeError {
     #[fail(display = "Failed to send parameter change request")]
     Request,
 
-    /// The response for changing the parameters indicated an error and wasn't
-    /// successful.
-    #[fail(display = "Bad HTTP response '{}' while changing the parameters", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while changing the file parameters.
+    #[fail(display = "Bad response from server while changing parameters")]
+    Response(#[cause] ResponseError),
 }

api/src/action/password.rs

@@ -1,13 +1,13 @@
-use reqwest::{Client, StatusCode};
+use reqwest::Client;
 
 use api::data::{
     Error as DataError,
     OwnedData,
 };
 use api::nonce::{NonceError, request_nonce};
+use api::request::{ensure_success, ResponseError};
 use api::url::UrlBuilder;
 use crypto::key_set::KeySet;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 
 /// An action to change a password of an uploaded Send file.
@@ -82,13 +82,9 @@ impl<'a> Password<'a> {
             .send()
             .map_err(|_| ChangeError::Request)?;
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(ChangeError::RequestStatus(status, status.err_text()).into());
-        }
-
-        Ok(())
+        // Ensure the response is successful
+        ensure_success(&response)
+            .map_err(|err| ChangeError::Response(err))
     }
 }
 
@@ -163,7 +159,7 @@ pub enum ChangeError {
     #[fail(display = "Failed to send password change request")]
     Request,
 
-    /// The response for changing the password indicated an error and wasn't successful.
-    #[fail(display = "Bad HTTP response '{}' while changing the password", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while changing the file password.
+    #[fail(display = "Bad response from server while changing password")]
+    Response(#[cause] ResponseError),
 }

api/src/action/upload.rs

@@ -12,7 +12,6 @@ use reqwest::{
     Client, 
     Error as ReqwestError,
     Request,
-    StatusCode,
 };
 use reqwest::header::Authorization;
 use reqwest::mime::APPLICATION_OCTET_STREAM;
@@ -23,8 +22,8 @@ use url::{
 };
 
 use api::nonce::header_nonce;
+use api::request::{ensure_success, ResponseError};
 use crypto::key_set::KeySet;
-use ext::status_code::StatusCodeExt;
 use file::remote_file::RemoteFile;
 use file::metadata::{Metadata, XFileMetadata};
 use reader::{
@@ -112,7 +111,6 @@ impl Upload {
             .start(reader_len);
 
         // Execute the request
-        // TODO: don't fail on nonce error, just don't use it
         let (result, nonce) = self.execute_request(req, client, &key)?;
 
         // Mark the reporter as finished
@@ -217,7 +215,6 @@ impl Upload {
 
         // Configure a form to send
         let part = Part::reader_with_length(reader, len)
-            // TODO: keep this here? .file_name(file.name())
             .mime(APPLICATION_OCTET_STREAM);
         let form = Form::new()
             .part("data", part);
@@ -228,6 +225,7 @@ impl Upload {
             .expect("invalid host");
 
         // Build the request
+        // TODO: create an error for this unwrap
         client.post(url.as_str())
             .header(Authorization(
                 format!("send-v1 {}", key.auth_key_encoded().unwrap())
@@ -250,13 +248,9 @@ impl Upload {
             Err(_) => return Err(UploadError::Request),
         };
 
-        // Validate the status code
-        let status = response.status();
-        if !status.is_success() {
-            return Err(
-                UploadError::RequestStatus(status, status.err_text())
-            );
-        }
+        // Ensure the response is successful
+        ensure_success(&response)
+            .map_err(|err| UploadError::Response(err))?;
 
         // Try to get the nonce, don't error on failure
         let nonce = header_nonce(&response).ok();
@@ -472,9 +466,9 @@ pub enum UploadError {
     #[fail(display = "Failed to request file upload")]
     Request,
 
-    /// The response for downloading the indicated an error and wasn't successful.
-    #[fail(display = "Bad HTTP response '{}' while requesting file upload", _1)]
-    RequestStatus(StatusCode, String),
+    /// The server responded with an error while uploading.
+    #[fail(display = "Bad response from server while uploading")]
+    Response(#[cause] ResponseError),
 
     /// Failed to decode the upload response from the server.
     /// Maybe the server responded with data from a newer API version.

api/src/api/mod.rs

@@ -1,3 +1,4 @@
 pub mod data;
 pub mod nonce;
 pub mod url;
+pub mod request;

api/src/api/request.rs

@@ -0,0 +1,44 @@
+use reqwest::{Response, StatusCode};
+
+use config::{HTTP_STATUS_EXPIRED, HTTP_STATUS_UNAUTHORIZED};
+use ext::status_code::StatusCodeExt;
+
+/// Ensure the given response is successful. IF it isn
+pub fn ensure_success(response: &Response) -> Result<(), ResponseError> {
+    // Get the status
+    let status = response.status();
+
+    // Stop if succesful
+    if status.is_success() {
+        return Ok(());
+    }
+
+    // Handle the expired file error
+    if status == HTTP_STATUS_EXPIRED {
+        return Err(ResponseError::Expired);
+    }
+
+    // Handle the authentication issue error
+    if status == HTTP_STATUS_UNAUTHORIZED {
+        return Err(ResponseError::Unauthorized);
+    }
+
+    // Return the other error
+    Err(ResponseError::Other(status, status.err_text()))
+}
+
+#[derive(Fail, Debug)]
+pub enum ResponseError {
+    /// This request lead to an expired file, or a file that never existed.
+    #[fail(display = "The file has expired or did never exist")]
+    Expired,
+
+    /// We were unauthorized to make this request.
+    /// This is usually because of an incorrect password.
+    #[fail(display = "Unauthorized, are the credentials correct?")]
+    Unauthorized,
+
+    /// Some undefined error occurred with this response.
+    #[fail(display = "Bad HTTP response: {}", _1)]
+    Other(StatusCode, String),
+}

api/src/config.rs

@@ -1,3 +1,11 @@
+use reqwest::StatusCode;
+
+/// The HTTP status code that is returned for expired or non existant files.
+pub const HTTP_STATUS_EXPIRED: StatusCode = StatusCode::NotFound;
+
+/// The HTTP status code that is returned on authentication failure.
+pub const HTTP_STATUS_UNAUTHORIZED: StatusCode = StatusCode::Unauthorized;
+
 /// The recommended maximum upload size in bytes.
 pub const UPLOAD_SIZE_MAX_RECOMMENDED: u64 = 1_073_741_824;