Replace curl with reqwest in src/github.rs

This behavior was split into two functions so that one of the 5 places
we're calling it from could intercept a 404 response code. Since the
happy path otherwise is just calling `.json`, this felt really awkward
to me. Instead I've opted to return `NotFound`, and downcast to it in
the error path for that one place.

I expected to just be able to call `Any::is`, but it turns out this
method is an inherent method not a trait method (which makes sense,
otherwise `Any` wouldn't be object safe). However, a side effect of that
is that even though `Any` is a supertrait of `CargoError`, we can't call
`Any::is` since `dyn CargoError` can't be cast to `dyn Any`. This may
change at some point in the future (see
rust-lang/rfcs#2035), but we have to duplicate
the body of `Any::is` for now.

Except we can't even just duplicate the body of `Any::is`, because the
only trait method for `Any` is unstable so we have to duplicate that
method, too..........

Other notable changes:

- We're no longer sending `Proxy-Connection: Keep-Alive`. According to
  Wikipedia, this is "Implemented as a misunderstanding of the HTTP
  specifications. Common because of mistakes in implementations of early
  HTTP versions". Firefox recently removed it, as have recent versions
  of curl.
- We will accept gzipped responses now. This is good.
- We send an actual user agent, instead of "hello!"

Author: sgrif

src/app.rs

@@ -8,10 +8,12 @@ use std::time::Duration;
 use curl::easy::Easy;
 use diesel::r2d2;
 use git2;
+use reqwest;
 use oauth2;
 use scheduled_thread_pool::ScheduledThreadPool;
 
 use {db, Config, Env};
+use util::CargoResult;
 
 /// The `App` struct holds the main components of the application like
 /// the database connection pool and configurations
@@ -111,4 +113,17 @@ impl App {
         }
         handle
     }
+
+    /// Returns a client for making HTTP requests to upload crate files.
+    ///
+    /// The handle will go through a proxy if the uploader being used has specified one, which
+    /// is only done in test mode in order to be able to record and inspect the HTTP requests
+    /// that tests make.
+    pub fn http_client(&self) -> CargoResult<reqwest::Client> {
+        let mut builder = reqwest::Client::builder();
+        if let Some(proxy) = self.config.uploader.proxy() {
+            builder = builder.proxy(reqwest::Proxy::all(proxy)?);
+        }
+        Ok(builder.build()?)
+    }
 }

src/controllers/user/session.rs

@@ -100,8 +100,7 @@ pub fn github_access_token(req: &mut dyn Request) -> CargoResult<Response> {
         .exchange(code.clone())
         .map_err(|s| human(&s))?;
 
-    let (handle, resp) = github::github(req.app(), "/user", &token)?;
-    let ghuser: GithubUser = github::parse_github_response(handle, &resp)?;
+    let ghuser = github::github::<GithubUser>(req.app(), "/user", &token)?;
 
     let user = NewUser::new(
         ghuser.id,

src/github.rs

@@ -1,88 +1,57 @@
 //! This module implements functionality for interacting with GitHub.
 
-use curl;
-use curl::easy::{Easy, List};
-
+use reqwest::{self, header};
 use oauth2::*;
 
-use serde::Deserialize;
-use serde_json;
+use serde::de::DeserializeOwned;
 
 use std::str;
 
 use app::App;
-use util::{human, internal, CargoResult, ChainError};
+use util::{human, internal, CargoResult, CargoError, errors::NotFound};
 
 /// Does all the nonsense for sending a GET to Github. Doesn't handle parsing
 /// because custom error-code handling may be desirable. Use
 /// `parse_github_response` to handle the "common" processing of responses.
-pub fn github(app: &App, url: &str, auth: &Token) -> Result<(Easy, Vec<u8>), curl::Error> {
+pub fn github<T>(app: &App, url: &str, auth: &Token) -> CargoResult<T>
+where
+    T: DeserializeOwned,
+{
     let url = format!("{}://api.github.com{}", app.config.api_protocol, url);
     info!("GITHUB HTTP: {}", url);
 
-    let mut headers = List::new();
-    headers
-        .append("Accept: application/vnd.github.v3+json")
-        .unwrap();
-    headers.append("User-Agent: hello!").unwrap();
-    headers
-        .append(&format!("Authorization: token {}", auth.access_token))
-        .unwrap();
-
-    let mut handle = app.handle();
-    handle.url(&url).unwrap();
-    handle.get(true).unwrap();
-    handle.http_headers(headers).unwrap();
-
-    let mut data = Vec::new();
-    {
-        let mut transfer = handle.transfer();
-        transfer
-            .write_function(|buf| {
-                data.extend_from_slice(buf);
-                Ok(buf.len())
-            }).unwrap();
-        transfer.perform()?;
-    }
-    Ok((handle, data))
+    let client = app.http_client()?;
+    client.get(&url)
+        .header(header::ACCEPT, "application/vnd.github.v3+json")
+        .header(header::AUTHORIZATION, format!("token {}", auth.access_token))
+        .send()?
+        .error_for_status()
+        .map_err(handle_error_response)?
+        .json()
+        .map_err(Into::into)
 }
 
-/// Checks for normal responses
-pub fn parse_github_response<'de, 'a: 'de, T: Deserialize<'de>>(
-    mut resp: Easy,
-    data: &'a [u8],
-) -> CargoResult<T> {
-    match resp.response_code().unwrap() {
-        // Ok!
-        200 => {}
-        // Unauthorized or Forbidden
-        401 | 403 => {
-            return Err(human(
-                "It looks like you don't have permission \
-                 to query a necessary property from Github \
-                 to complete this request. \
-                 You may need to re-authenticate on \
-                 crates.io to grant permission to read \
-                 github org memberships. Just go to \
-                 https://crates.io/login",
-            ));
-        }
-        // Something else
-        n => {
-            let resp = String::from_utf8_lossy(data);
-            return Err(internal(&format_args!(
-                "didn't get a 200 result from \
-                 github, got {} with: {}",
-                n, resp
-            )));
+fn handle_error_response(error: reqwest::Error) -> Box<dyn CargoError> {
+    use reqwest::StatusCode as Status;
+
+    match error.status() {
+        Some(Status::UNAUTHORIZED) | Some(Status::FORBIDDEN) => human(
+            "It looks like you don't have permission \
+             to query a necessary property from Github \
+             to complete this request. \
+             You may need to re-authenticate on \
+             crates.io to grant permission to read \
+             github org memberships. Just go to \
+             https://crates.io/login",
+        ),
+        Some(Status::NOT_FOUND) => Box::new(NotFound),
+        _ => {
+            internal(&format_args!(
+                "didn't get a 200 result from github: {}",
+                error
+            ))
         }
     }
-
-    let json = str::from_utf8(data)
-        .ok()
-        .chain_error(|| internal("github didn't send a utf8-response"))?;
-
-    serde_json::from_str(json).chain_error(|| internal("github didn't send a valid json response"))
 }
 
 /// Gets a token with the given string as the access token, but all

src/models/team.rs

@@ -2,7 +2,7 @@ use diesel::prelude::*;
 
 use app::App;
 use github;
-use util::{human, CargoResult};
+use util::{human, CargoResult, errors::NotFound};
 
 use models::{Crate, CrateOwner, Owner, OwnerKind, User};
 use schema::{crate_owners, teams};
@@ -142,8 +142,7 @@ impl Team {
         // links. A hundred teams should be enough for any org, right?
         let url = format!("/orgs/{}/teams?per_page=100", org_name);
         let token = github::token(req_user.gh_access_token.clone());
-        let (handle, data) = github::github(app, &url, &token)?;
-        let teams: Vec<GithubTeam> = github::parse_github_response(handle, &data)?;
+        let teams = github::github::<Vec<GithubTeam>>(app, &url, &token)?;
 
         let team = teams
             .into_iter()
@@ -165,8 +164,7 @@ impl Team {
         }
 
         let url = format!("/orgs/{}", org_name);
-        let (handle, resp) = github::github(app, &url, &token)?;
-        let org: Org = github::parse_github_response(handle, &resp)?;
+        let org = github::github::<Org>(app, &url, &token)?;
 
         NewTeam::new(&login.to_lowercase(), team.id, team.name, org.avatar_url)
             .create_or_update(conn)
@@ -225,14 +223,11 @@ fn team_with_gh_id_contains_user(app: &App, github_id: i32, user: &User) -> Carg
 
     let url = format!("/teams/{}/memberships/{}", &github_id, &user.gh_login);
     let token = github::token(user.gh_access_token.clone());
-    let (mut handle, resp) = github::github(app, &url, &token)?;
-
-    // Officially how `false` is returned
-    if handle.response_code().unwrap() == 404 {
-        return Ok(false);
-    }
-
-    let membership: Membership = github::parse_github_response(handle, &resp)?;
+    let membership = match github::github::<Membership>(app, &url, &token) {
+        // Officially how `false` is returned
+        Err(ref e) if e.is::<NotFound>() => return Ok(false),
+        x => x?,
+    };
 
     // There is also `state: pending` for which we could possibly give
     // some feedback, but it's not obvious how that should work.