Identity cleanup (#544)

* Start clean up of errors

* Fix todo

* Improve device code response

* More docs

* Remove unnecessary traits

* Fix clippy error

Author: rylev

sdk/identity/src/authorization_code_flow.rs

@@ -2,7 +2,6 @@
 //!
 //! You can learn more about the OAuth2 authorization code flow [here](https://docs.microsoft.com/azure/active-directory/develop/v2-oauth2-auth-code-flow).
 
-use log::debug;
 use oauth2::basic::BasicClient;
 use oauth2::reqwest::async_http_client;
 use oauth2::{ClientId, ClientSecret};
@@ -93,7 +92,6 @@ impl AuthorizationCodeFlow {
             .request_async(async_http_client)
             .await?;
 
-        debug!("\nMS Graph returned the following token:\n{:?}\n", token);
         Ok(token)
     }
 }

sdk/identity/src/client_credentials_flow/login_response.rs

@@ -1,8 +1,6 @@
-use crate::Error;
 use chrono::{DateTime, TimeZone, Utc};
 use oauth2::AccessToken;
 use serde::{de, Deserialize, Deserializer};
-use std::str::FromStr;
 
 #[derive(Debug, Clone, Deserialize)]
 struct _LoginResponse {
@@ -26,14 +24,6 @@ pub struct LoginResponse {
     pub access_token: AccessToken,
 }
 
-impl FromStr for LoginResponse {
-    type Err = Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        Ok(serde_json::from_str(s)?)
-    }
-}
-
 impl<'de> Deserialize<'de> for LoginResponse {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
     where
@@ -49,7 +39,7 @@ impl LoginResponse {
         &self.access_token
     }
 
-    fn from_base_response(r: _LoginResponse) -> Result<LoginResponse, Error> {
+    fn from_base_response(r: _LoginResponse) -> Result<LoginResponse, std::num::ParseIntError> {
         let expires_on: Option<DateTime<Utc>> = match r.expires_on {
             Some(d) => Some(Utc.timestamp(d.parse()?, 0)),
             None => None,

sdk/identity/src/client_credentials_flow/mod.rs

@@ -1,4 +1,4 @@
-//! Authorize using the client credentials flow
+//! Authorize using the OAuth 2.0 client credentials flow
 //!
 //! For example:
 //!
@@ -39,7 +39,6 @@
 
 mod login_response;
 
-use crate::Error;
 use login_response::LoginResponse;
 use url::form_urlencoded;
 
@@ -50,7 +49,7 @@ pub async fn perform(
     client_secret: &oauth2::ClientSecret,
     scopes: &[&str],
     tenant_id: &str,
-) -> Result<LoginResponse, Error> {
+) -> Result<LoginResponse, ClientCredentialError> {
     let encoded: String = form_urlencoded::Serializer::new(String::new())
         .append_pair("client_id", client_id.as_str())
         .append_pair("scope", &scopes.join(" "))
@@ -61,19 +60,46 @@ pub async fn perform(
     let url = url::Url::parse(&format!(
         "https://login.microsoftonline.com/{}/oauth2/v2.0/token",
         tenant_id
-    ))?;
+    ))
+    .map_err(|_| ClientCredentialError::InvalidTenantId(tenant_id.to_owned()))?;
 
-    client
+    let response = client
         .post(url)
         .header("ContentType", "Application / WwwFormUrlEncoded")
         .body(encoded)
         .send()
-        .await?
+        .await
+        .map_err(|e| ClientCredentialError::RequestError(Box::new(e)))?;
+
+    if !response.status().is_success() {
+        return Err(ClientCredentialError::UnsuccessfulResponse(
+            response.status().as_u16(),
+            response.text().await.ok(),
+        ));
+    }
+
+    let b = response
         .text()
         .await
-        .map(|s| -> Result<LoginResponse, Error> {
-            Ok(serde_json::from_str::<LoginResponse>(&s)?)
-        })?
-    // TODO The HTTP status code should be checked to deserialize an error response.
-    // serde_json::from_str::<crate::errors::ErrorResponse>(&s).map(Error::ErrorResponse)
+        .map_err(|e| ClientCredentialError::RequestError(Box::new(e)))?;
+
+    serde_json::from_str::<LoginResponse>(&b)
+        .map_err(|_| ClientCredentialError::InvalidResponseBody(b))
+}
+
+/// Errors when performing the client credential flow
+#[derive(thiserror::Error, Debug)]
+pub enum ClientCredentialError {
+    /// The http response was unsuccessful
+    #[error("The http response was unsuccessful with status {0}: {}", .1.as_deref().unwrap_or("<NO UTF-8 BODY>"))]
+    UnsuccessfulResponse(u16, Option<String>),
+    /// The http response body was could not be turned into a client credential response
+    #[error("The http response body could not be turned into a client credential response: {0}")]
+    InvalidResponseBody(String),
+    /// The tenant id could not be url encoded
+    #[error("The supplied tenant id could not be url encoded: {0}")]
+    InvalidTenantId(String),
+    /// An error occurred when trying to make a request
+    #[error("An error occurred when trying to make a request: {0}")]
+    RequestError(Box<dyn std::error::Error + Send + Sync>),
 }

sdk/identity/src/device_code_flow/device_code_responses.rs

@@ -44,20 +44,33 @@ impl DeviceCodeAuthorization {
     }
 }
 
+/// Errors when performing the device code flow
 #[derive(Error, Debug)]
 pub enum DeviceCodeError {
-    #[error("Authorization declined")]
+    /// The authorization response returned a "declined" response
+    #[error("authorization declined: {0}")]
     AuthorizationDeclined(DeviceCodeErrorResponse),
-    #[error("Bad verification code")]
+    /// The authorization response returned a "bad verification" response
+    #[error("bad verification code: {0}")]
     BadVerificationCode(DeviceCodeErrorResponse),
-    #[error("Expired token")]
+    /// The authorization response returned a "expired" response
+    #[error("expired token: {0}")]
     ExpiredToken(DeviceCodeErrorResponse),
-    #[error("Unrecognized error: {0}")]
+    /// The authorization response returned an unrecognized error
+    #[error("unrecognized device code error response error kind: {0}")]
     UnrecognizedError(DeviceCodeErrorResponse),
-    #[error("Unhandled error: {0}. {1}")]
-    UnhandledError(String, String),
-    #[error("Reqwest error: {0}")]
-    ReqwestError(reqwest::Error),
+    /// The supplied tenant id could not be url encoded
+    #[error("the supplied tenant id could not be url encoded: {0}")]
+    InvalidTenantId(String),
+    /// The HTTP response returned an unsuccessful HTTP status code
+    #[error("the http response was unsuccesful with status {0}: {}", .1.as_deref().unwrap_or("<NO UTF-8 BODY>"))]
+    UnsuccessfulResponse(u16, Option<String>),
+    /// The response body could not be turned into a device code response
+    #[error("the http response body could not be turned into a device code response: {0}")]
+    InvalidResponseBody(String),
+    /// An error occurred when trying to make a request
+    #[error("an error occurred when trying to make a request: {0}")]
+    RequestError(Box<dyn std::error::Error + Send + Sync>),
 }
 
 #[derive(Debug, Clone)]
@@ -101,7 +114,7 @@ impl TryInto<DeviceCodeResponse> for String {
                         }
                     }
                     // If we cannot, we bail out giving the full error as string
-                    Err(error) => Err(DeviceCodeError::UnhandledError(error.to_string(), self)),
+                    Err(_) => Err(DeviceCodeError::InvalidResponseBody(self)),
                 }
             }
         }

sdk/identity/src/device_code_flow/mod.rs

@@ -4,24 +4,25 @@
 //!
 //! You can learn more about this authorization flow [here](https://docs.microsoft.com/azure/active-directory/develop/v2-oauth2-device-code).
 mod device_code_responses;
-use crate::Error;
-use async_timer::timer::new_timer;
+
 pub use device_code_responses::*;
+
+use async_timer::timer::new_timer;
 use futures::stream::unfold;
-use log::debug;
 use oauth2::ClientId;
 use serde::Deserialize;
+use url::form_urlencoded;
+
 use std::borrow::Cow;
 use std::convert::TryInto;
 use std::time::Duration;
-use url::form_urlencoded;
 
 pub async fn start<'a, 'b, T>(
     client: &'a reqwest::Client,
     tenant_id: T,
     client_id: &'a ClientId,
     scopes: &'b [&'b str],
-) -> Result<DeviceCodePhaseOneResponse<'a>, Error>
+) -> Result<DeviceCodePhaseOneResponse<'a>, DeviceCodeError>
 where
     T: Into<Cow<'a, str>>,
 {
@@ -32,41 +33,48 @@ where
 
     let tenant_id = tenant_id.into();
 
-    debug!("encoded ==> {}", encoded);
-
     let url = url::Url::parse(&format!(
         "https://login.microsoftonline.com/{}/oauth2/v2.0/devicecode",
         tenant_id
-    ))?;
+    ))
+    .map_err(|_| DeviceCodeError::InvalidTenantId(tenant_id.clone().into_owned()))?;
 
-    client
+    let response = client
         .post(url)
         .header("ContentType", "application/x-www-form-urlencoded")
         .body(encoded)
         .send()
-        .await?
+        .await
+        .map_err(|e| DeviceCodeError::RequestError(Box::new(e)))?;
+
+    if !response.status().is_success() {
+        return Err(DeviceCodeError::UnsuccessfulResponse(
+            response.status().as_u16(),
+            response.text().await.ok(),
+        ));
+    }
+    let s = response
         .text()
         .await
-        .map(|s| -> Result<DeviceCodePhaseOneResponse, Error> {
-            serde_json::from_str::<DeviceCodePhaseOneResponse>(&s)
-                // we need to capture some variables that will be useful in
-                // the second phase (the client, the tenant_id and the client_id)
-                .map(|device_code_reponse| {
-                    Ok(DeviceCodePhaseOneResponse {
-                        device_code: device_code_reponse.device_code,
-                        user_code: device_code_reponse.user_code,
-                        verification_uri: device_code_reponse.verification_uri,
-                        expires_in: device_code_reponse.expires_in,
-                        interval: device_code_reponse.interval,
-                        message: device_code_reponse.message,
-                        client: Some(client),
-                        tenant_id,
-                        client_id: client_id.as_str().to_string(),
-                    })
-                })?
-            // TODO The HTTP status code should be checked to deserialize an error response.
-            // serde_json::from_str::<crate::errors::ErrorResponse>(&s).map(Error::ErrorResponse)
-        })?
+        .map_err(|e| DeviceCodeError::RequestError(Box::new(e)))?;
+
+    serde_json::from_str::<DeviceCodePhaseOneResponse>(&s)
+        // we need to capture some variables that will be useful in
+        // the second phase (the client, the tenant_id and the client_id)
+        .map(|device_code_reponse| {
+            Ok(DeviceCodePhaseOneResponse {
+                device_code: device_code_reponse.device_code,
+                user_code: device_code_reponse.user_code,
+                verification_uri: device_code_reponse.verification_uri,
+                expires_in: device_code_reponse.expires_in,
+                interval: device_code_reponse.interval,
+                message: device_code_reponse.message,
+                client: Some(client),
+                tenant_id,
+                client_id: client_id.as_str().to_string(),
+            })
+        })
+        .map_err(|_| DeviceCodeError::InvalidResponseBody(s))?
 }
 
 #[derive(Debug, Clone, Deserialize)]
@@ -77,17 +85,14 @@ pub struct DeviceCodePhaseOneResponse<'a> {
     expires_in: u64,
     interval: u64,
     message: String,
-    // the skipped fields below do not come
-    // from the Azure answer. They will be added
-    // manually after deserialization
+    // The skipped fields below do not come from the Azure answer.
+    // They will be added manually after deserialization
     #[serde(skip)]
     client: Option<&'a reqwest::Client>,
     #[serde(skip)]
     tenant_id: Cow<'a, str>,
-    // we store the ClientId as string instead of
-    // the original type because it does not
-    // implement Default and it's in another
-    // create
+    // We store the ClientId as string instead of the original type, because it
+    // does not implement Default, and it's in another crate
     #[serde(skip)]
     client_id: String,
 }
@@ -97,9 +102,9 @@ impl<'a> DeviceCodePhaseOneResponse<'a> {
         &self.message
     }
 
-    pub fn stream<'b>(
-        &'b self,
-    ) -> impl futures::Stream<Item = Result<DeviceCodeResponse, DeviceCodeError>> + 'b + '_ {
+    pub fn stream(
+        &self,
+    ) -> impl futures::Stream<Item = Result<DeviceCodeResponse, DeviceCodeError>> + '_ {
         #[derive(Debug, Clone, PartialEq)]
         enum NextState {
             Continue,
@@ -114,12 +119,10 @@ impl<'a> DeviceCodePhaseOneResponse<'a> {
                         self.tenant_id,
                     );
 
-                    // throttle down as specified by Azure. This could be
+                    // Throttle down as specified by Azure. This could be
                     // smarter: we could calculate the elapsed time since the
-                    // last poll and wait only the delta. For now we do not
-                    // need such precision.
+                    // last poll and wait only the delta.
                     new_timer(Duration::from_secs(self.interval)).await;
-                    debug!("posting to {}", &uri);
 
                     let mut encoded = form_urlencoded::Serializer::new(String::new());
                     let encoded = encoded
@@ -136,22 +139,24 @@ impl<'a> DeviceCodePhaseOneResponse<'a> {
                         .body(encoded)
                         .send()
                         .await
-                        .map_err(DeviceCodeError::ReqwestError)
+                        .map_err(|e| DeviceCodeError::RequestError(Box::new(e)))
                     {
                         Ok(result) => result,
                         Err(error) => return Some((Err(error), NextState::Finish)),
                     };
-                    debug!("result (raw) ==> {:?}", result);
 
-                    let result = match result.text().await.map_err(DeviceCodeError::ReqwestError) {
+                    let result = match result
+                        .text()
+                        .await
+                        .map_err(|e| DeviceCodeError::RequestError(Box::new(e)))
+                    {
                         Ok(result) => result,
                         Err(error) => return Some((Err(error), NextState::Finish)),
                     };
-                    debug!("result (as text) ==> {}", result);
 
-                    // here either we get an error response from Azure
+                    // Here either we get an error response from Azure
                     // or we get a success. A success can be either "Pending" or
-                    // "Completed". We finish the loop only on "Completed" (ie Success)
+                    // "Completed". We finish the loop only on "Completed"
                     match result.try_into() {
                         Ok(device_code_response) => {
                             let next_state = match &device_code_response {