From 35430125750b554443b79124efe8d349651c005a Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 20 Sep 2017 19:26:35 -0400 Subject: [PATCH 1/5] Reorganize routes to group them by function --- src/lib.rs | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 47c48742e6e..b8325bf5ff0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -137,11 +137,25 @@ pub enum Replica { pub fn middleware(app: Arc) -> MiddlewareBuilder { let mut api_router = RouteBuilder::new(); + // Route used by both `cargo search` and the frontend api_router.get("/crates", C(krate::index)); - api_router.get("/crates/:crate_id", C(krate::show)); + + // Routes used by `cargo` api_router.put("/crates/new", C(krate::new)); - api_router.get("/crates/:crate_id/:version", C(version::show)); + api_router.get("/crates/:crate_id/owners", C(krate::owners)); + api_router.put("/crates/:crate_id/owners", C(krate::add_owners)); + api_router.delete("/crates/:crate_id/owners", C(krate::remove_owners)); + api_router.delete("/crates/:crate_id/:version/yank", C(version::yank)); + api_router.put("/crates/:crate_id/:version/unyank", C(version::unyank)); api_router.get("/crates/:crate_id/:version/download", C(krate::download)); + + // Routes that appear to be unused + api_router.get("/versions", C(version::index)); + api_router.get("/versions/:version_id", C(version::show)); + + // Routes used by the frontend + api_router.get("/crates/:crate_id", C(krate::show)); + api_router.get("/crates/:crate_id/:version", C(version::show)); api_router.get("/crates/:crate_id/:version/readme", C(krate::readme)); api_router.get( "/crates/:crate_id/:version/dependencies", @@ -152,27 +166,17 @@ pub fn middleware(app: Arc) -> MiddlewareBuilder { C(version::downloads), ); api_router.get("/crates/:crate_id/:version/authors", C(version::authors)); - // Used to generate download graphs api_router.get("/crates/:crate_id/downloads", C(krate::downloads)); api_router.get("/crates/:crate_id/versions", C(krate::versions)); api_router.put("/crates/:crate_id/follow", C(krate::follow)); api_router.delete("/crates/:crate_id/follow", C(krate::unfollow)); api_router.get("/crates/:crate_id/following", C(krate::following)); - // This endpoint may now be redundant, check frontend to see if it is - // being used - api_router.get("/crates/:crate_id/owners", C(krate::owners)); api_router.get("/crates/:crate_id/owner_team", C(krate::owner_team)); api_router.get("/crates/:crate_id/owner_user", C(krate::owner_user)); - api_router.put("/crates/:crate_id/owners", C(krate::add_owners)); - api_router.delete("/crates/:crate_id/owners", C(krate::remove_owners)); - api_router.delete("/crates/:crate_id/:version/yank", C(version::yank)); - api_router.put("/crates/:crate_id/:version/unyank", C(version::unyank)); api_router.get( "/crates/:crate_id/reverse_dependencies", C(krate::reverse_dependencies), ); - api_router.get("/versions", C(version::index)); - api_router.get("/versions/:version_id", C(version::show)); api_router.get("/keywords", C(keyword::index)); api_router.get("/keywords/:keyword_id", C(keyword::show)); api_router.get("/categories", C(category::index)); From ffc9af3159adcde261bb8e75898d6eef4acaf072 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 20 Sep 2017 19:33:09 -0400 Subject: [PATCH 2/5] Move `krate` and `version` api modules into subdirectories These modules are moved into directories so that they can be split into smaller units for `cargo` and `frontend` functionality. --- src/{ => krate}/krate_reverse_dependencies.sql | 0 src/{krate.rs => krate/mod.rs} | 0 src/{version.rs => version/mod.rs} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename src/{ => krate}/krate_reverse_dependencies.sql (100%) rename src/{krate.rs => krate/mod.rs} (100%) rename src/{version.rs => version/mod.rs} (100%) diff --git a/src/krate_reverse_dependencies.sql b/src/krate/krate_reverse_dependencies.sql similarity index 100% rename from src/krate_reverse_dependencies.sql rename to src/krate/krate_reverse_dependencies.sql diff --git a/src/krate.rs b/src/krate/mod.rs similarity index 100% rename from src/krate.rs rename to src/krate/mod.rs diff --git a/src/version.rs b/src/version/mod.rs similarity index 100% rename from src/version.rs rename to src/version/mod.rs From 6793f4a9b6153316c140ba6c24caa05fb156a187 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 20 Sep 2017 19:41:20 -0400 Subject: [PATCH 3/5] Move `categories::sync` functionality under a new `boot` module --- src/bin/server.rs | 4 ++-- src/{ => boot}/categories.rs | 0 src/{ => boot}/categories.toml | 0 src/boot/mod.rs | 1 + src/lib.rs | 2 +- src/tests/categories.rs | 11 ++++++----- 6 files changed, 10 insertions(+), 8 deletions(-) rename src/{ => boot}/categories.rs (100%) rename src/{ => boot}/categories.toml (100%) create mode 100644 src/boot/mod.rs diff --git a/src/bin/server.rs b/src/bin/server.rs index 818cfa87e95..99eccba50a4 100644 --- a/src/bin/server.rs +++ b/src/bin/server.rs @@ -50,8 +50,8 @@ fn main() { // On every server restart, ensure the categories available in the database match // the information in *src/categories.toml*. - let categories_toml = include_str!("../categories.toml"); - cargo_registry::categories::sync(&categories_toml).unwrap(); + let categories_toml = include_str!("../boot/categories.toml"); + cargo_registry::boot::categories::sync(&categories_toml).unwrap(); let heroku = env::var("HEROKU").is_ok(); let port = if heroku { diff --git a/src/categories.rs b/src/boot/categories.rs similarity index 100% rename from src/categories.rs rename to src/boot/categories.rs diff --git a/src/categories.toml b/src/boot/categories.toml similarity index 100% rename from src/categories.toml rename to src/boot/categories.toml diff --git a/src/boot/mod.rs b/src/boot/mod.rs new file mode 100644 index 00000000000..79af7712dfa --- /dev/null +++ b/src/boot/mod.rs @@ -0,0 +1 @@ +pub mod categories; diff --git a/src/lib.rs b/src/lib.rs index b8325bf5ff0..ef896f5e4cf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -75,7 +75,7 @@ use util::{R404, C, R}; pub mod app; pub mod badge; -pub mod categories; +pub mod boot; pub mod category; pub mod config; pub mod crate_owner_invitation; diff --git a/src/tests/categories.rs b/src/tests/categories.rs index 68228143b71..c8a112db75a 100644 --- a/src/tests/categories.rs +++ b/src/tests/categories.rs @@ -60,7 +60,7 @@ fn select_slugs(conn: &PgConnection) -> Vec { fn sync_adds_new_categories() { let conn = pg_connection(); - ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap(); + ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap(); let categories = select_slugs(&conn); assert_eq!(categories, vec!["algorithms", "algorithms::such"]); @@ -70,8 +70,8 @@ fn sync_adds_new_categories() { fn sync_removes_missing_categories() { let conn = pg_connection(); - ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap(); - ::cargo_registry::categories::sync_with_connection(ALGORITHMS, &conn).unwrap(); + ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap(); + ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS, &conn).unwrap(); let categories = select_slugs(&conn); assert_eq!(categories, vec!["algorithms"]); @@ -81,8 +81,9 @@ fn sync_removes_missing_categories() { fn sync_adds_and_removes() { let conn = pg_connection(); - ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap(); - ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_ANOTHER, &conn).unwrap(); + ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap(); + ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_ANOTHER, &conn) + .unwrap(); let categories = select_slugs(&conn); assert_eq!(categories, vec!["algorithms", "another"]); From 52cea71546117949d46de4062e4838957114aac6 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 20 Sep 2017 19:53:53 -0400 Subject: [PATCH 4/5] Pull github interaction out of `http` module into a `github` module --- src/github.rs | 94 +++++++++++++++++++++++++++++++++++++++++++++++++ src/http.rs | 93 ------------------------------------------------ src/lib.rs | 1 + src/owner.rs | 18 +++++----- src/user/mod.rs | 6 ++-- 5 files changed, 107 insertions(+), 105 deletions(-) create mode 100644 src/github.rs diff --git a/src/github.rs b/src/github.rs new file mode 100644 index 00000000000..586871470d5 --- /dev/null +++ b/src/github.rs @@ -0,0 +1,94 @@ +use curl; +use curl::easy::{Easy, List}; + +use oauth2::*; + +use serde_json; +use serde::Deserialize; + +use std::str; + +use app::App; +use util::{human, internal, CargoResult, ChainError}; + +/// 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), curl::Error> { + 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)) +} + +/// Checks for normal responses +pub fn parse_github_response<'de, 'a: 'de, T: Deserialize<'de>>( + mut resp: Easy, + data: &'a [u8], +) -> CargoResult { + match resp.response_code().unwrap() { + 200 => {} + // Ok! + 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", + )); + } + 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 + ))); + } + } + + 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 +/// other info null'd out. Generally, just to be fed to the `github` fn. +pub fn token(token: String) -> Token { + Token { + access_token: token, + scopes: Vec::new(), + token_type: String::new(), + } +} diff --git a/src/http.rs b/src/http.rs index 4f847a2e775..be66909723d 100644 --- a/src/http.rs +++ b/src/http.rs @@ -1,104 +1,11 @@ use conduit::{Request, Response}; use conduit_middleware::Middleware; -use curl; -use curl::easy::{Easy, List}; - -use oauth2::*; - -use serde_json; -use serde::Deserialize; - -use std::str; use std::error::Error; use std::collections::HashMap; -use app::App; -use util::{human, internal, CargoResult, ChainError}; use Uploader; -/// 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), curl::Error> { - 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)) -} - -/// Checks for normal responses -pub fn parse_github_response<'de, 'a: 'de, T: Deserialize<'de>>( - mut resp: Easy, - data: &'a [u8], -) -> CargoResult { - match resp.response_code().unwrap() { - 200 => {} - // Ok! - 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", - )); - } - 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 - ))); - } - } - - 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 -/// other info null'd out. Generally, just to be fed to the `github` fn. -pub fn token(token: String) -> Token { - Token { - access_token: token, - scopes: Vec::new(), - token_type: String::new(), - } -} - #[derive(Clone, Debug)] pub struct SecurityHeadersMiddleware { headers: HashMap>, diff --git a/src/lib.rs b/src/lib.rs index ef896f5e4cf..1ebdea4b4a8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -84,6 +84,7 @@ pub mod dependency; pub mod dist; pub mod download; pub mod git; +pub mod github; pub mod http; pub mod keyword; pub mod krate; diff --git a/src/owner.rs b/src/owner.rs index 6069727487d..589e1862cf6 100644 --- a/src/owner.rs +++ b/src/owner.rs @@ -1,7 +1,7 @@ use diesel::prelude::*; use app::App; -use http; +use github; use schema::*; use util::{human, CargoResult}; use {Crate, User}; @@ -184,9 +184,9 @@ impl Team { // FIXME: we just set per_page=100 and don't bother chasing pagination // links. A hundred teams should be enough for any org, right? let url = format!("/orgs/{}/teams?per_page=100", org_name); - let token = http::token(req_user.gh_access_token.clone()); - let (handle, data) = http::github(app, &url, &token)?; - let teams: Vec = http::parse_github_response(handle, &data)?; + let token = github::token(req_user.gh_access_token.clone()); + let (handle, data) = github::github(app, &url, &token)?; + let teams: Vec = github::parse_github_response(handle, &data)?; let team = teams .into_iter() @@ -209,8 +209,8 @@ impl Team { } let url = format!("/orgs/{}", org_name); - let (handle, resp) = http::github(app, &url, &token)?; - let org: Org = http::parse_github_response(handle, &resp)?; + let (handle, resp) = github::github(app, &url, &token)?; + let org: Org = github::parse_github_response(handle, &resp)?; NewTeam::new(login, team.id, team.name, org.avatar_url).create_or_update(conn) } @@ -276,15 +276,15 @@ 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 = http::token(user.gh_access_token.clone()); - let (mut handle, resp) = http::github(app, &url, &token)?; + 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 = http::parse_github_response(handle, &resp)?; + let membership: Membership = github::parse_github_response(handle, &resp)?; // There is also `state: pending` for which we could possibly give // some feedback, but it's not obvious how that should work. diff --git a/src/user/mod.rs b/src/user/mod.rs index 2cbeb82f5f0..6e15b4e111c 100644 --- a/src/user/mod.rs +++ b/src/user/mod.rs @@ -14,7 +14,7 @@ use pagination::Paginate; use schema::*; use util::{bad_request, human, CargoResult, RequestUtils}; use version::EncodableVersion; -use {http, Version}; +use {github, Version}; use owner::{CrateOwner, Owner, OwnerKind}; use krate::Crate; use email; @@ -375,8 +375,8 @@ pub fn github_access_token(req: &mut Request) -> CargoResult { .exchange(code.clone()) .map_err(|s| human(&s))?; - let (handle, resp) = http::github(req.app(), "/user", &token)?; - let ghuser: GithubUser = http::parse_github_response(handle, &resp)?; + let (handle, resp) = github::github(req.app(), "/user", &token)?; + let ghuser: GithubUser = github::parse_github_response(handle, &resp)?; let user = NewUser::new( ghuser.id, From 7e88ba5de99692e83c7879ad443711fa0aa17774 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 26 Sep 2017 22:02:55 -0400 Subject: [PATCH 5/5] Address review comments [skip ci] --- src/github.rs | 2 ++ src/http.rs | 3 +++ src/owner.rs | 2 +- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/github.rs b/src/github.rs index 586871470d5..e47a46736a0 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1,3 +1,5 @@ +//! This module implements functionality for interacting with GitHub. + use curl; use curl::easy::{Easy, List}; diff --git a/src/http.rs b/src/http.rs index be66909723d..7bcadef7637 100644 --- a/src/http.rs +++ b/src/http.rs @@ -1,3 +1,6 @@ +//! This module implements middleware for adding secuirty headers to +//! http responses in production. + use conduit::{Request, Response}; use conduit_middleware::Middleware; diff --git a/src/owner.rs b/src/owner.rs index 589e1862cf6..85f3b7fb3b9 100644 --- a/src/owner.rs +++ b/src/owner.rs @@ -147,7 +147,7 @@ impl Team { /// Tries to create a Github Team from scratch. Assumes `org` and `team` are /// correctly parsed out of the full `name`. `name` is passed as a /// convenience to avoid rebuilding it. - pub fn create_github_team( + fn create_github_team( app: &App, conn: &PgConnection, login: &str,