Auto merge of #3218 - Turbo87:github-client, r=locks

Extract `GitHubClient` struct

This PR aims to simplify our GitHub API request code a little bit by extracting a `GitHubClient` struct, and adding several request methods on it. It also allows to set a custom `base_url`, which will be useful in the future when used in combination with https://docs.rs/mockito.

r? `@pietroalbini`

Author: bors

src/app.rs

@@ -3,6 +3,7 @@
 use crate::{db, Config, Env};
 use std::{sync::Arc, time::Duration};
 
+use crate::github::GitHubClient;
 use diesel::r2d2;
 use oauth2::basic::BasicClient;
 use reqwest::blocking::Client;
@@ -19,8 +20,11 @@ pub struct App {
     /// The read-only replica database connection pool
     pub read_only_replica_database: Option<db::DieselPool>,
 
+    /// GitHub API client
+    pub github: GitHubClient,
+
     /// The GitHub OAuth2 configuration
-    pub github: BasicClient,
+    pub github_oauth: BasicClient,
 
     /// A unique key used with conduit_cookie to generate cookies
     pub session_key: String,
@@ -47,7 +51,9 @@ impl App {
     pub fn new(config: Config, http_client: Option<Client>) -> App {
         use oauth2::{AuthUrl, ClientId, ClientSecret, TokenUrl};
 
-        let github = BasicClient::new(
+        let github = GitHubClient::new(http_client.clone(), config.gh_base_url.clone());
+
+        let github_oauth = BasicClient::new(
             ClientId::new(config.gh_client_id.clone()),
             Some(ClientSecret::new(config.gh_client_secret.clone())),
             AuthUrl::new(String::from("https://github.com/login/oauth/authorize")).unwrap(),
@@ -122,6 +128,7 @@ impl App {
             primary_database,
             read_only_replica_database,
             github,
+            github_oauth,
             session_key: config.session_key.clone(),
             config,
             http_client,

src/config.rs

@@ -7,6 +7,7 @@ pub struct Config {
     pub session_key: String,
     pub gh_client_id: String,
     pub gh_client_secret: String,
+    pub gh_base_url: String,
     pub db_url: String,
     pub replica_db_url: Option<String>,
     pub env: Env,
@@ -131,6 +132,7 @@ impl Default for Config {
             session_key: env("SESSION_KEY"),
             gh_client_id: env("GH_CLIENT_ID"),
             gh_client_secret: env("GH_CLIENT_SECRET"),
+            gh_base_url: "https://api.github.com".to_string(),
             db_url: env("DATABASE_URL"),
             replica_db_url: dotenv::var("READ_ONLY_REPLICA_URL").ok(),
             env: cargo_env,

src/controllers/user/session.rs

@@ -1,10 +1,10 @@
 use crate::controllers::frontend_prelude::*;
 
-use crate::github;
 use conduit_cookie::RequestSession;
 use oauth2::reqwest::http_client;
 use oauth2::{AuthorizationCode, Scope, TokenResponse};
 
+use crate::github::GithubUser;
 use crate::middleware::current_user::TrustedUserId;
 use crate::models::{NewUser, User};
 use crate::schema::users;
@@ -28,7 +28,7 @@ use crate::util::errors::ReadOnlyMode;
 pub fn begin(req: &mut dyn RequestExt) -> EndpointResult {
     let (url, state) = req
         .app()
-        .github
+        .github_oauth
         .authorize_url(oauth2::CsrfToken::new_random)
         .add_scope(Scope::new("read:org".to_string()))
         .url();
@@ -95,15 +95,15 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
     let code = AuthorizationCode::new(code);
     let token = req
         .app()
-        .github
+        .github_oauth
         .exchange_code(code)
         .request(http_client)
         .chain_error(|| server_error("Error obtaining token"))?;
     let token = token.access_token();
 
     // Fetch the user info from GitHub using the access token we just got and create a user record
-    let ghuser = github::github_api::<GithubUser>(req.app(), "/user", token)?;
-    let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?;
+    let ghuser = req.app().github.current_user(token)?;
+    let user = save_user_to_database(&ghuser, &token.secret(), &*req.db_conn()?)?;
 
     // Log in by setting a cookie and the middleware authentication
     req.session_mut()
@@ -113,40 +113,33 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
     super::me::me(req)
 }
 
-#[derive(Deserialize)]
-struct GithubUser {
-    email: Option<String>,
-    name: Option<String>,
-    login: String,
-    id: i32,
-    avatar_url: Option<String>,
-}
-
-impl GithubUser {
-    fn save_to_database(&self, access_token: &str, conn: &PgConnection) -> AppResult<User> {
-        NewUser::new(
-            self.id,
-            &self.login,
-            self.name.as_deref(),
-            self.avatar_url.as_deref(),
-            access_token,
-        )
-        .create_or_update(self.email.as_deref(), conn)
-        .map_err(Into::into)
-        .or_else(|e: Box<dyn AppError>| {
-            // If we're in read only mode, we can't update their details
-            // just look for an existing user
-            if e.is::<ReadOnlyMode>() {
-                users::table
-                    .filter(users::gh_id.eq(self.id))
-                    .first(conn)
-                    .optional()?
-                    .ok_or(e)
-            } else {
-                Err(e)
-            }
-        })
-    }
+fn save_user_to_database(
+    user: &GithubUser,
+    access_token: &str,
+    conn: &PgConnection,
+) -> AppResult<User> {
+    NewUser::new(
+        user.id,
+        &user.login,
+        user.name.as_deref(),
+        user.avatar_url.as_deref(),
+        access_token,
+    )
+    .create_or_update(user.email.as_deref(), conn)
+    .map_err(Into::into)
+    .or_else(|e: Box<dyn AppError>| {
+        // If we're in read only mode, we can't update their details
+        // just look for an existing user
+        if e.is::<ReadOnlyMode>() {
+            users::table
+                .filter(users::gh_id.eq(user.id))
+                .first(conn)
+                .optional()?
+                .ok_or(e)
+        } else {
+            Err(e)
+        }
+    })
 }
 
 /// Handles the `DELETE /api/private/session` route.
@@ -175,7 +168,7 @@ mod tests {
             id: -1,
             avatar_url: None,
         };
-        let result = gh_user.save_to_database("arbitrary_token", &conn);
+        let result = save_user_to_database(&gh_user, "arbitrary_token", &conn);
 
         assert!(
             result.is_ok(),

src/github.rs

@@ -7,29 +7,89 @@ use serde::de::DeserializeOwned;
 
 use std::str;
 
-use crate::app::App;
 use crate::util::errors::{cargo_err, internal, not_found, AppError, AppResult};
+use reqwest::blocking::Client;
 
-/// 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_api<T>(app: &App, url: &str, auth: &AccessToken) -> AppResult<T>
-where
-    T: DeserializeOwned,
-{
-    let url = format!("{}://api.github.com{}", app.config.api_protocol, url);
-    info!("GITHUB HTTP: {}", url);
-
-    app.http_client()
-        .get(&url)
-        .header(header::ACCEPT, "application/vnd.github.v3+json")
-        .header(header::AUTHORIZATION, format!("token {}", auth.secret()))
-        .header(header::USER_AGENT, "crates.io (https://crates.io)")
-        .send()?
-        .error_for_status()
-        .map_err(|e| handle_error_response(&e))?
-        .json()
-        .map_err(Into::into)
+#[derive(Debug)]
+pub struct GitHubClient {
+    base_url: String,
+    client: Option<Client>,
+}
+
+impl GitHubClient {
+    pub fn new(client: Option<Client>, base_url: String) -> Self {
+        Self { client, base_url }
+    }
+
+    pub fn current_user(&self, auth: &AccessToken) -> AppResult<GithubUser> {
+        self.request("/user", auth)
+    }
+
+    pub fn org_by_name(&self, org_name: &str, auth: &AccessToken) -> AppResult<GitHubOrganization> {
+        let url = format!("/orgs/{}", org_name);
+        self.request(&url, auth)
+    }
+
+    pub fn team_by_name(
+        &self,
+        org_name: &str,
+        team_name: &str,
+        auth: &AccessToken,
+    ) -> AppResult<GitHubTeam> {
+        let url = format!("/orgs/{}/teams/{}", org_name, team_name);
+        self.request(&url, auth)
+    }
+
+    pub fn team_membership(
+        &self,
+        org_id: i32,
+        team_id: i32,
+        username: &str,
+        auth: &AccessToken,
+    ) -> AppResult<GitHubTeamMembership> {
+        let url = format!(
+            "/organizations/{}/team/{}/memberships/{}",
+            org_id, team_id, username
+        );
+        self.request(&url, auth)
+    }
+
+    /// 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 request<T>(&self, url: &str, auth: &AccessToken) -> AppResult<T>
+    where
+        T: DeserializeOwned,
+    {
+        let url = format!("{}{}", self.base_url, url);
+        info!("GITHUB HTTP: {}", url);
+
+        self.client()
+            .get(&url)
+            .header(header::ACCEPT, "application/vnd.github.v3+json")
+            .header(header::AUTHORIZATION, format!("token {}", auth.secret()))
+            .header(header::USER_AGENT, "crates.io (https://crates.io)")
+            .send()?
+            .error_for_status()
+            .map_err(|e| handle_error_response(&e))?
+            .json()
+            .map_err(Into::into)
+    }
+
+    /// Returns a client for making HTTP requests to upload crate files.
+    ///
+    /// The client will go through a proxy if the application was configured via
+    /// `TestApp::with_proxy()`.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the application was not initialized with a client.  This should only occur in
+    /// tests that were not properly initialized.
+    fn client(&self) -> &Client {
+        self.client
+            .as_ref()
+            .expect("No HTTP client is configured.  In tests, use `TestApp::with_proxy()`.")
+    }
 }
 
 fn handle_error_response(error: &reqwest::Error) -> Box<dyn AppError> {
@@ -52,6 +112,33 @@ fn handle_error_response(error: &reqwest::Error) -> Box<dyn AppError> {
     }
 }
 
+#[derive(Debug, Deserialize)]
+pub struct GithubUser {
+    pub avatar_url: Option<String>,
+    pub email: Option<String>,
+    pub id: i32,
+    pub login: String,
+    pub name: Option<String>,
+}
+
+#[derive(Debug, Deserialize)]
+pub struct GitHubOrganization {
+    pub id: i32, // unique GH id (needed for membership queries)
+    pub avatar_url: Option<String>,
+}
+
+#[derive(Debug, Deserialize)]
+pub struct GitHubTeam {
+    pub id: i32,              // unique GH id (needed for membership queries)
+    pub name: Option<String>, // Pretty name
+    pub organization: GitHubOrganization,
+}
+
+#[derive(Debug, Deserialize)]
+pub struct GitHubTeamMembership {
+    pub state: String,
+}
+
 pub fn team_url(login: &str) -> String {
     let mut login_pieces = login.split(':');
     login_pieces.next();