From 2f911e3ce897222c54b3fbcc554b94d63ef62a2f Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sat, 30 Jan 2021 23:46:00 +0100 Subject: [PATCH 1/7] App: Rename `github` to `github_oauth` --- src/app.rs | 6 +++--- src/controllers/user/session.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/app.rs b/src/app.rs index 242de0c2fe0..fef408d0d85 100644 --- a/src/app.rs +++ b/src/app.rs @@ -20,7 +20,7 @@ pub struct App { pub read_only_replica_database: Option, /// 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 +47,7 @@ impl App { pub fn new(config: Config, http_client: Option) -> App { use oauth2::{AuthUrl, ClientId, ClientSecret, TokenUrl}; - let github = BasicClient::new( + 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(), @@ -121,7 +121,7 @@ impl App { App { primary_database, read_only_replica_database, - github, + github_oauth, session_key: config.session_key.clone(), config, http_client, diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 9595374cd6e..44bf346df89 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -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,7 +95,7 @@ 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"))?; From d302c1b7540043bdd29258db2834206e11325646 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sun, 31 Jan 2021 00:10:00 +0100 Subject: [PATCH 2/7] controllers::user::session: Move `save_to_database()` out of the `GitHubUser` struct This is glue code that couples the `NewUser` model and the `GithubUser` response struct together. Having this tight coupling in the `impl` block makes it hard to pull the GitHub client into a dedicated module. --- src/controllers/user/session.rs | 56 +++++++++++++++++---------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 44bf346df89..65ae383b341 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -103,7 +103,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { // Fetch the user info from GitHub using the access token we just got and create a user record let ghuser = github::github_api::(req.app(), "/user", token)?; - let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?; + 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() @@ -122,31 +122,33 @@ struct GithubUser { avatar_url: Option, } -impl GithubUser { - fn save_to_database(&self, access_token: &str, conn: &PgConnection) -> AppResult { - 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| { - // If we're in read only mode, we can't update their details - // just look for an existing user - if e.is::() { - 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 { + 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| { + // If we're in read only mode, we can't update their details + // just look for an existing user + if e.is::() { + 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 +177,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(), From 1abafbc7a54474775952f02025289345b3d4c419 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sun, 31 Jan 2021 00:04:10 +0100 Subject: [PATCH 3/7] Extract `GitHubClient` struct --- src/app.rs | 7 ++++ src/config.rs | 2 + src/controllers/user/session.rs | 3 +- src/github.rs | 69 +++++++++++++++++++++++---------- src/models/team.rs | 20 +++++----- src/tests/all.rs | 1 + 6 files changed, 70 insertions(+), 32 deletions(-) diff --git a/src/app.rs b/src/app.rs index fef408d0d85..db099ff2b61 100644 --- a/src/app.rs +++ b/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,6 +20,9 @@ pub struct App { /// The read-only replica database connection pool pub read_only_replica_database: Option, + /// GitHub API client + pub github: GitHubClient, + /// The GitHub OAuth2 configuration pub github_oauth: BasicClient, @@ -47,6 +51,8 @@ impl App { pub fn new(config: Config, http_client: Option) -> App { use oauth2::{AuthUrl, ClientId, ClientSecret, TokenUrl}; + 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())), @@ -121,6 +127,7 @@ impl App { App { primary_database, read_only_replica_database, + github, github_oauth, session_key: config.session_key.clone(), config, diff --git a/src/config.rs b/src/config.rs index 300badf8df4..cdc5753b04f 100644 --- a/src/config.rs +++ b/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, 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, diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 65ae383b341..e54d4284013 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -1,6 +1,5 @@ use crate::controllers::frontend_prelude::*; -use crate::github; use conduit_cookie::RequestSession; use oauth2::reqwest::http_client; use oauth2::{AuthorizationCode, Scope, TokenResponse}; @@ -102,7 +101,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { 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::(req.app(), "/user", token)?; + let ghuser = req.app().github.request::("/user", token)?; let user = save_user_to_database(&ghuser, &token.secret(), &*req.db_conn()?)?; // Log in by setting a cookie and the middleware authentication diff --git a/src/github.rs b/src/github.rs index 924a5c0fe08..5dbe681ab44 100644 --- a/src/github.rs +++ b/src/github.rs @@ -7,29 +7,56 @@ 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(app: &App, url: &str, auth: &AccessToken) -> AppResult -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, +} + +impl GitHubClient { + pub fn new(client: Option, base_url: String) -> Self { + Self { client, base_url } + } + + /// 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(&self, url: &str, auth: &AccessToken) -> AppResult + 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 { diff --git a/src/models/team.rs b/src/models/team.rs index 347ad6bb477..e19abf4acaa 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -1,7 +1,6 @@ use diesel::prelude::*; use crate::app::App; -use crate::github::github_api; use crate::util::errors::{cargo_err, AppResult, NotFound}; use oauth2::AccessToken; @@ -153,12 +152,15 @@ impl Team { let url = format!("/orgs/{}/teams/{}", org_name, team_name); let token = AccessToken::new(req_user.gh_access_token.clone()); - let team = github_api::(app, &url, &token).map_err(|_| { - cargo_err(&format_args!( - "could not find the github team {}/{}", - org_name, team_name - )) - })?; + let team = app + .github + .request::(&url, &token) + .map_err(|_| { + cargo_err(&format_args!( + "could not find the github team {}/{}", + org_name, team_name + )) + })?; let org_id = team.organization.id; @@ -172,7 +174,7 @@ impl Team { } let url = format!("/orgs/{}", org_name); - let org = github_api::(app, &url, &token)?; + let org = app.github.request::(&url, &token)?; NewTeam::new( &login.to_lowercase(), @@ -233,7 +235,7 @@ fn team_with_gh_id_contains_user( &github_org_id, &github_team_id, &user.gh_login ); let token = AccessToken::new(user.gh_access_token.clone()); - let membership = match github_api::(app, &url, &token) { + let membership = match app.github.request::(&url, &token) { // Officially how `false` is returned Err(ref e) if e.is::() => return Ok(false), x => x?, diff --git a/src/tests/all.rs b/src/tests/all.rs index 8c13f3aecfb..feb24754332 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -129,6 +129,7 @@ fn simple_config() -> Config { session_key: "test this has to be over 32 bytes long".to_string(), gh_client_id: dotenv::var("GH_CLIENT_ID").unwrap_or_default(), gh_client_secret: dotenv::var("GH_CLIENT_SECRET").unwrap_or_default(), + gh_base_url: "http://api.github.com".to_string(), db_url: env("TEST_DATABASE_URL"), replica_db_url: None, env: Env::Test, From 561eb48b4caefd7f0096952868fd8d0526f0171e Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sun, 31 Jan 2021 00:14:43 +0100 Subject: [PATCH 4/7] GitHubClient: Extract `current_user()` method --- src/controllers/user/session.rs | 12 ++---------- src/github.rs | 13 +++++++++++++ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index e54d4284013..6095509073c 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -4,6 +4,7 @@ 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; @@ -101,7 +102,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { 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 = req.app().github.request::("/user", token)?; + 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 @@ -112,15 +113,6 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { super::me::me(req) } -#[derive(Deserialize)] -struct GithubUser { - email: Option, - name: Option, - login: String, - id: i32, - avatar_url: Option, -} - fn save_user_to_database( user: &GithubUser, access_token: &str, diff --git a/src/github.rs b/src/github.rs index 5dbe681ab44..3f5ff1f9c86 100644 --- a/src/github.rs +++ b/src/github.rs @@ -21,6 +21,10 @@ impl GitHubClient { Self { client, base_url } } + pub fn current_user(&self, auth: &AccessToken) -> AppResult { + self.request("/user", 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. @@ -79,6 +83,15 @@ fn handle_error_response(error: &reqwest::Error) -> Box { } } +#[derive(Debug, Deserialize)] +pub struct GithubUser { + pub avatar_url: Option, + pub email: Option, + pub id: i32, + pub login: String, + pub name: Option, +} + pub fn team_url(login: &str) -> String { let mut login_pieces = login.split(':'); login_pieces.next(); From 08a3ad261a6cf6bf6e9a1f5bf4bd43804ef4299b Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sun, 31 Jan 2021 00:23:27 +0100 Subject: [PATCH 5/7] GitHubClient: Extract `team_membership()` method --- src/github.rs | 19 +++++++++++++++++++ src/models/team.rs | 23 +++++++++-------------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/github.rs b/src/github.rs index 3f5ff1f9c86..6a7451deb7c 100644 --- a/src/github.rs +++ b/src/github.rs @@ -25,6 +25,20 @@ impl GitHubClient { self.request("/user", auth) } + pub fn team_membership( + &self, + org_id: i32, + team_id: i32, + username: &str, + auth: &AccessToken, + ) -> AppResult { + 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. @@ -92,6 +106,11 @@ pub struct GithubUser { pub name: Option, } +#[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(); diff --git a/src/models/team.rs b/src/models/team.rs index e19abf4acaa..296477293bc 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -225,21 +225,16 @@ fn team_with_gh_id_contains_user( // GET /organizations/:org_id/team/:team_id/memberships/:username // check that "state": "active" - #[derive(Deserialize)] - struct Membership { - state: String, - } - - let url = format!( - "/organizations/{}/team/{}/memberships/{}", - &github_org_id, &github_team_id, &user.gh_login - ); let token = AccessToken::new(user.gh_access_token.clone()); - let membership = match app.github.request::(&url, &token) { - // Officially how `false` is returned - Err(ref e) if e.is::() => return Ok(false), - x => x?, - }; + let membership = + match app + .github + .team_membership(github_org_id, github_team_id, &user.gh_login, &token) + { + // Officially how `false` is returned + Err(ref e) if e.is::() => 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. From 124cddb27332efea6ef3c1282c4f370a8792442f Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sun, 31 Jan 2021 00:28:28 +0100 Subject: [PATCH 6/7] GitHubClient: Extract `team_by_name()` method --- src/github.rs | 22 ++++++++++++++++++++++ src/models/team.rs | 15 +-------------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/github.rs b/src/github.rs index 6a7451deb7c..8199cfbf8ac 100644 --- a/src/github.rs +++ b/src/github.rs @@ -25,6 +25,16 @@ impl GitHubClient { self.request("/user", auth) } + pub fn team_by_name( + &self, + org_name: &str, + team_name: &str, + auth: &AccessToken, + ) -> AppResult { + let url = format!("/orgs/{}/teams/{}", org_name, team_name); + self.request(&url, auth) + } + pub fn team_membership( &self, org_id: i32, @@ -106,6 +116,18 @@ pub struct GithubUser { pub name: Option, } +#[derive(Debug, Deserialize, Copy)] +pub struct GitHubOrganization { + pub id: i32, // unique GH id (needed for membership queries) +} + +#[derive(Debug, Deserialize)] +pub struct GitHubTeam { + pub id: i32, // unique GH id (needed for membership queries) + pub name: Option, // Pretty name + pub organization: GitHubOrganization, +} + #[derive(Debug, Deserialize)] pub struct GitHubTeamMembership { pub state: String, diff --git a/src/models/team.rs b/src/models/team.rs index 296477293bc..c05cf1c7b12 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -138,23 +138,10 @@ impl Team { ))); } - #[derive(Deserialize)] - struct GithubOrganization { - id: i32, // unique GH id (needed for membership queries) - } - - #[derive(Deserialize)] - struct GithubTeam { - id: i32, // unique GH id (needed for membership queries) - name: Option, // Pretty name - organization: GithubOrganization, - } - - let url = format!("/orgs/{}/teams/{}", org_name, team_name); let token = AccessToken::new(req_user.gh_access_token.clone()); let team = app .github - .request::(&url, &token) + .team_by_name(org_name, team_name, &token) .map_err(|_| { cargo_err(&format_args!( "could not find the github team {}/{}", From 73b5b0c67803b59a3046e589647c83978dbe2fca Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sun, 31 Jan 2021 00:31:29 +0100 Subject: [PATCH 7/7] GitHubClient: Extract `org_by_name()` method --- src/github.rs | 8 +++++++- src/models/team.rs | 8 +------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/github.rs b/src/github.rs index 8199cfbf8ac..3e1034f05bf 100644 --- a/src/github.rs +++ b/src/github.rs @@ -25,6 +25,11 @@ impl GitHubClient { self.request("/user", auth) } + pub fn org_by_name(&self, org_name: &str, auth: &AccessToken) -> AppResult { + let url = format!("/orgs/{}", org_name); + self.request(&url, auth) + } + pub fn team_by_name( &self, org_name: &str, @@ -116,9 +121,10 @@ pub struct GithubUser { pub name: Option, } -#[derive(Debug, Deserialize, Copy)] +#[derive(Debug, Deserialize)] pub struct GitHubOrganization { pub id: i32, // unique GH id (needed for membership queries) + pub avatar_url: Option, } #[derive(Debug, Deserialize)] diff --git a/src/models/team.rs b/src/models/team.rs index c05cf1c7b12..b4d4ccc836d 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -155,13 +155,7 @@ impl Team { return Err(cargo_err("only members of a team can add it as an owner")); } - #[derive(Deserialize)] - struct Org { - avatar_url: Option, - } - - let url = format!("/orgs/{}", org_name); - let org = app.github.request::(&url, &token)?; + let org = app.github.org_by_name(org_name, &token)?; NewTeam::new( &login.to_lowercase(),