From 2d2b502422cf807883a27a83771a1aad4c12eb23 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Wed, 3 Jan 2024 14:40:52 +0100 Subject: [PATCH 1/7] Add basic `TeamRepo` trait --- Cargo.lock | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ Cargo.toml | 1 + src/lib.rs | 1 + src/team_repo.rs | 50 +++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+) create mode 100644 src/team_repo.rs diff --git a/Cargo.lock b/Cargo.lock index 4e6ed709576..717c880bd94 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -964,6 +964,7 @@ dependencies = [ "ipnetwork", "lettre", "minijinja", + "mockall", "moka", "oauth2", "object_store", @@ -1426,6 +1427,12 @@ version = "0.15.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1aaf95b3e5c8f23aa320147307562d361db0ae0d51242340f558153b4eb2439b" +[[package]] +name = "downcast" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" + [[package]] name = "ecdsa" version = "0.16.9" @@ -1614,6 +1621,12 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fragile" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c2141d6d6c8512188a7891b4b01590a45f6dac67afb4f255c4124dbb86d4eaa" + [[package]] name = "futf" version = "0.1.5" @@ -2526,6 +2539,33 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "mockall" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "43766c2b5203b10de348ffe19f7e54564b64f3d6018ff7648d1e2d6d3a0f0a48" +dependencies = [ + "cfg-if", + "downcast", + "fragile", + "lazy_static", + "mockall_derive", + "predicates", + "predicates-tree", +] + +[[package]] +name = "mockall_derive" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af7cbce79ec385a1d4f54baa90a76401eb15d9cab93685f62e7e9f942aa00ae2" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 2.0.48", +] + [[package]] name = "moka" version = "0.12.2" @@ -3003,6 +3043,33 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c" +[[package]] +name = "predicates" +version = "3.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6dfc28575c2e3f19cb3c73b93af36460ae898d426eba6fc15b9bd2a5220758a0" +dependencies = [ + "anstyle", + "itertools 0.11.0", + "predicates-core", +] + +[[package]] +name = "predicates-core" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b794032607612e7abeb4db69adb4e33590fa6cf1149e95fd7cb00e634b92f174" + +[[package]] +name = "predicates-tree" +version = "1.0.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "368ba315fb8c5052ab692e68a0eefec6ec57b23a36959c14496f0b0df2c0cecf" +dependencies = [ + "predicates-core", + "termtree", +] + [[package]] name = "primeorder" version = "0.13.6" @@ -3978,6 +4045,12 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "termtree" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3369f5ac52d5eb6ab48c6b4ffdc8efbcad6b89c765749064ba298f2c68a16a76" + [[package]] name = "thiserror" version = "1.0.56" diff --git a/Cargo.toml b/Cargo.toml index dd34eacdb69..bc7a12d28ea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -90,6 +90,7 @@ ipnetwork = "=0.20.0" tikv-jemallocator = { version = "=0.5.4", features = ['unprefixed_malloc_on_supported_platforms', 'profiling'] } lettre = { version = "=0.11.3", default-features = false, features = ["file-transport", "smtp-transport", "native-tls", "hostname", "builder"] } minijinja = "=1.0.11" +mockall = "=0.12.1" moka = { version = "=0.12.2", features = ["future"] } oauth2 = { version = "=4.4.2", default-features = false, features = ["reqwest"] } object_store = { version = "=0.9.0", features = ["aws"] } diff --git a/src/lib.rs b/src/lib.rs index 0243b761a70..c35c04302db 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,6 +55,7 @@ pub mod sql; pub mod ssh; pub mod storage; pub mod tasks; +pub mod team_repo; mod test_util; pub mod typosquat; pub mod util; diff --git a/src/team_repo.rs b/src/team_repo.rs new file mode 100644 index 00000000000..19beb6830f8 --- /dev/null +++ b/src/team_repo.rs @@ -0,0 +1,50 @@ +//! The code in this module interacts with the +//! repository. +//! +//! The [TeamRepo] trait is used to abstract away the HTTP client for testing +//! purposes. The [TeamRepoImpl] struct is the actual implementation of +//! the trait. + +use async_trait::async_trait; +use mockall::automock; +use reqwest::Client; + +#[automock] +#[async_trait] +pub trait TeamRepo { + async fn get_team(&self, name: &str) -> anyhow::Result; +} + +#[derive(Debug, Clone, Deserialize)] +pub struct Team { + pub name: String, + pub kind: String, + pub members: Vec, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct Member { + pub name: String, + pub github: String, + pub github_id: i32, + pub is_lead: bool, +} + +pub struct TeamRepoImpl { + client: Client, +} + +impl TeamRepoImpl { + pub fn new(client: Client) -> Self { + TeamRepoImpl { client } + } +} + +#[async_trait] +impl TeamRepo for TeamRepoImpl { + async fn get_team(&self, name: &str) -> anyhow::Result { + let url = format!("https://team-api.infra.rust-lang.org/v1/teams/{name}.json"); + let response = self.client.get(url).send().await?.error_for_status()?; + Ok(response.json().await?) + } +} From 00e831c0caa391a626e65a54d584f53249417fee Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Wed, 3 Jan 2024 14:41:37 +0100 Subject: [PATCH 2/7] worker/environment: Add `team_repo` field --- src/bin/background-worker.rs | 5 ++++- src/tests/util/test_app.rs | 10 ++++++++++ src/worker/environment.rs | 2 ++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/bin/background-worker.rs b/src/bin/background-worker.rs index ebc1c4ce965..79fd9f400e8 100644 --- a/src/bin/background-worker.rs +++ b/src/bin/background-worker.rs @@ -18,6 +18,7 @@ use crates_io::cloudfront::CloudFront; use crates_io::db::DieselPool; use crates_io::fastly::Fastly; use crates_io::storage::Storage; +use crates_io::team_repo::TeamRepoImpl; use crates_io::worker::{Environment, RunnerExt}; use crates_io::{config, Emails}; use crates_io::{db, ssh}; @@ -76,7 +77,8 @@ fn main() -> anyhow::Result<()> { .expect("Couldn't build client"); let emails = Emails::from_environment(&config); - let fastly = Fastly::from_environment(client); + let fastly = Fastly::from_environment(client.clone()); + let team_repo = TeamRepoImpl::new(client); let connection_pool = r2d2::Pool::builder() .max_size(10) @@ -90,6 +92,7 @@ fn main() -> anyhow::Result<()> { .storage(storage) .connection_pool(DieselPool::new_background_worker(connection_pool.clone())) .emails(emails) + .team_repo(Box::new(team_repo)) .build()?; let environment = Arc::new(environment); diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index e434c28684d..0106dd604b3 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -7,6 +7,7 @@ use crates_io::middleware::cargo_compat::StatusCodeConfig; use crates_io::models::token::{CrateScope, EndpointScope}; use crates_io::rate_limiter::{LimitedAction, RateLimiterConfig}; use crates_io::storage::StorageConfig; +use crates_io::team_repo::MockTeamRepo; use crates_io::worker::{Environment, RunnerExt}; use crates_io::{App, Emails, Env}; use crates_io_index::testing::UpstreamIndex; @@ -80,6 +81,7 @@ impl TestApp { index: None, build_job_runner: false, use_chaos_proxy: false, + team_repo: MockTeamRepo::new(), } } @@ -204,6 +206,7 @@ pub struct TestAppBuilder { index: Option, build_job_runner: bool, use_chaos_proxy: bool, + team_repo: MockTeamRepo, } impl TestAppBuilder { @@ -259,11 +262,13 @@ impl TestAppBuilder { index_location: index.url(), credentials: Credentials::Missing, }; + let environment = Environment::builder() .repository_config(repository_config) .storage(app.storage.clone()) .connection_pool(app.primary_database.clone()) .emails(app.emails.clone()) + .team_repo(Box::new(self.team_repo)) .build() .unwrap(); @@ -351,6 +356,11 @@ impl TestAppBuilder { self } + pub fn with_team_repo(mut self, team_repo: MockTeamRepo) -> Self { + self.team_repo = team_repo; + self + } + pub fn with_replica(mut self) -> Self { let primary = &self.config.db.primary; diff --git a/src/worker/environment.rs b/src/worker/environment.rs index bc1e651cbbc..94143a36944 100644 --- a/src/worker/environment.rs +++ b/src/worker/environment.rs @@ -2,6 +2,7 @@ use crate::cloudfront::CloudFront; use crate::db::DieselPool; use crate::fastly::Fastly; use crate::storage::Storage; +use crate::team_repo::TeamRepo; use crate::typosquat; use crate::Emails; use crates_io_index::{Repository, RepositoryConfig}; @@ -25,6 +26,7 @@ pub struct Environment { pub storage: Arc, pub connection_pool: DieselPool, pub emails: Emails, + pub team_repo: Box, /// A lazily initialised cache of the most popular crates ready to use in typosquatting checks. #[builder(default, setter(skip))] From fe2944b8100ce77d84e5821bc61a7896f7ecdd31 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Wed, 3 Jan 2024 14:42:04 +0100 Subject: [PATCH 3/7] worker: Implement `SyncAdmins` background job --- src/tests/worker/mod.rs | 1 + src/tests/worker/sync_admins.rs | 83 +++++++++++++++++++++++++++++ src/worker/jobs/mod.rs | 2 + src/worker/jobs/sync_admins.rs | 93 +++++++++++++++++++++++++++++++++ src/worker/mod.rs | 1 + 5 files changed, 180 insertions(+) create mode 100644 src/tests/worker/sync_admins.rs create mode 100644 src/worker/jobs/sync_admins.rs diff --git a/src/tests/worker/mod.rs b/src/tests/worker/mod.rs index 08dfb7f4e6c..f8ae161e713 100644 --- a/src/tests/worker/mod.rs +++ b/src/tests/worker/mod.rs @@ -1 +1,2 @@ mod git; +mod sync_admins; diff --git a/src/tests/worker/sync_admins.rs b/src/tests/worker/sync_admins.rs new file mode 100644 index 00000000000..27d5816b003 --- /dev/null +++ b/src/tests/worker/sync_admins.rs @@ -0,0 +1,83 @@ +use crate::util::TestApp; +use crates_io::schema::users; +use crates_io::team_repo::{Member, MockTeamRepo, Team}; +use crates_io::worker::jobs::SyncAdmins; +use crates_io_worker::BackgroundJob; +use diesel::prelude::*; +use diesel::{PgConnection, QueryResult, RunQueryDsl}; + +#[test] +fn test_sync_admins_job() { + let mock_response = mock_team( + "crates-io", + vec![ + mock_member("existing-admin", 1), + mock_member("new-admin", 3), + ], + ); + + let mut team_repo = MockTeamRepo::new(); + team_repo + .expect_get_team() + .with(mockall::predicate::eq("crates-io-admins")) + .returning(move |_| Ok(mock_response.clone())); + + let (app, _) = TestApp::full().with_team_repo(team_repo).empty(); + + app.db(|conn| create_user("existing-admin", 1, true, conn).unwrap()); + app.db(|conn| create_user("obsolete-admin", 2, true, conn).unwrap()); + app.db(|conn| create_user("new-admin", 3, false, conn).unwrap()); + app.db(|conn| create_user("unrelated-user", 42, false, conn).unwrap()); + + let admins = app.db(|conn| get_admins(conn).unwrap()); + let expected_admins = vec![("existing-admin".into(), 1), ("obsolete-admin".into(), 2)]; + assert_eq!(admins, expected_admins); + + app.db(|conn| SyncAdmins.enqueue(conn).unwrap()); + app.run_pending_background_jobs(); + + let admins = app.db(|conn| get_admins(conn).unwrap()); + let expected_admins = vec![("existing-admin".into(), 1), ("new-admin".into(), 3)]; + assert_eq!(admins, expected_admins); +} + +fn mock_team(name: impl Into, members: Vec) -> Team { + Team { + name: name.into(), + kind: "marker-team".to_string(), + members, + } +} + +fn mock_member(name: impl Into, github_id: i32) -> Member { + let name = name.into(); + let github = name.clone(); + Member { + name, + github, + github_id, + is_lead: false, + } +} + +fn create_user(name: &str, gh_id: i32, is_admin: bool, conn: &mut PgConnection) -> QueryResult<()> { + diesel::insert_into(users::table) + .values(( + users::name.eq(name), + users::gh_login.eq(name), + users::gh_id.eq(gh_id), + users::gh_access_token.eq("some random token"), + users::is_admin.eq(is_admin), + )) + .execute(conn)?; + + Ok(()) +} + +fn get_admins(conn: &mut PgConnection) -> QueryResult> { + users::table + .select((users::gh_login, users::gh_id)) + .filter(users::is_admin.eq(true)) + .order(users::gh_id.asc()) + .get_results(conn) +} diff --git a/src/worker/jobs/mod.rs b/src/worker/jobs/mod.rs index 61d0e263ce7..916bd160222 100644 --- a/src/worker/jobs/mod.rs +++ b/src/worker/jobs/mod.rs @@ -9,6 +9,7 @@ mod daily_db_maintenance; pub mod dump_db; mod git; mod readmes; +mod sync_admins; mod typosquat; mod update_downloads; @@ -16,6 +17,7 @@ pub use self::daily_db_maintenance::DailyDbMaintenance; pub use self::dump_db::DumpDb; pub use self::git::{NormalizeIndex, SquashIndex, SyncToGitIndex, SyncToSparseIndex}; pub use self::readmes::RenderAndUploadReadme; +pub use self::sync_admins::SyncAdmins; pub use self::typosquat::CheckTyposquat; pub use self::update_downloads::UpdateDownloads; diff --git a/src/worker/jobs/sync_admins.rs b/src/worker/jobs/sync_admins.rs new file mode 100644 index 00000000000..6c31ff1d42a --- /dev/null +++ b/src/worker/jobs/sync_admins.rs @@ -0,0 +1,93 @@ +use crate::schema::users; +use crate::tasks::spawn_blocking; +use crate::worker::Environment; +use crates_io_worker::BackgroundJob; +use diesel::prelude::*; +use diesel::RunQueryDsl; +use std::collections::HashSet; +use std::sync::Arc; + +/// See . +const TEAM_NAME: &str = "crates-io-admins"; + +#[derive(Serialize, Deserialize)] +pub struct SyncAdmins; + +impl BackgroundJob for SyncAdmins { + const JOB_NAME: &'static str = "sync_admins"; + + type Context = Arc; + + async fn run(&self, ctx: Self::Context) -> anyhow::Result<()> { + info!("Syncing admins from rust-lang/team repo…"); + + let repo_admins = ctx.team_repo.get_team(TEAM_NAME).await?.members; + let repo_admin_ids = repo_admins + .iter() + .map(|m| m.github_id) + .collect::>(); + + spawn_blocking::<_, _, anyhow::Error>(move || { + let mut conn = ctx.connection_pool.get()?; + + let database_admins = users::table + .select((users::gh_id, users::gh_login)) + .filter(users::is_admin.eq(true)) + .get_results::<(i32, String)>(&mut conn)?; + + let database_admin_ids = database_admins + .iter() + .map(|(gh_id, _)| *gh_id) + .collect::>(); + + let new_admin_ids = repo_admin_ids + .difference(&database_admin_ids) + .collect::>(); + + if new_admin_ids.is_empty() { + debug!("No new admins to add"); + } else { + let new_admins = repo_admins + .iter() + .filter(|m| new_admin_ids.contains(&&m.github_id)) + .map(|m| format!("{} (github_id: {})", m.github, m.github_id)) + .collect::>() + .join(", "); + + info!("Adding new admins: {}", new_admins); + + diesel::update(users::table) + .filter(users::gh_id.eq_any(new_admin_ids)) + .set(users::is_admin.eq(true)) + .execute(&mut conn)?; + } + + let obsolete_admin_ids = database_admin_ids + .difference(&repo_admin_ids) + .collect::>(); + + if obsolete_admin_ids.is_empty() { + debug!("No obsolete admins to remove"); + } else { + let obsolete_admins = database_admins + .iter() + .filter(|(gh_id, _)| obsolete_admin_ids.contains(&gh_id)) + .map(|(gh_id, login)| format!("{} (github_id: {})", login, gh_id)) + .collect::>() + .join(", "); + + info!("Removing obsolete admins: {}", obsolete_admins); + + diesel::update(users::table) + .filter(users::gh_id.eq_any(obsolete_admin_ids)) + .set(users::is_admin.eq(false)) + .execute(&mut conn)?; + } + + Ok(()) + }) + .await?; + + Ok(()) + } +} diff --git a/src/worker/mod.rs b/src/worker/mod.rs index 36e79b41b37..cfe4f5675ff 100644 --- a/src/worker/mod.rs +++ b/src/worker/mod.rs @@ -25,6 +25,7 @@ impl RunnerExt for Runner> { .register_job_type::() .register_job_type::() .register_job_type::() + .register_job_type::() .register_job_type::() .register_job_type::() .register_job_type::() From 1f66a5eeb09d040092d82d9430a4325138943b73 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Wed, 3 Jan 2024 14:43:00 +0100 Subject: [PATCH 4/7] admin/enqueue_job: Add support for `crates-admin enqueue-job sync_admins` --- src/admin/enqueue_job.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/admin/enqueue_job.rs b/src/admin/enqueue_job.rs index 15376d49dbd..d7e2bbc9045 100644 --- a/src/admin/enqueue_job.rs +++ b/src/admin/enqueue_job.rs @@ -30,6 +30,7 @@ pub enum Command { #[arg()] name: String, }, + SyncAdmins, } pub fn run(command: Command) -> Result<()> { @@ -58,6 +59,9 @@ pub fn run(command: Command) -> Result<()> { } => { jobs::DumpDb::new(database_url.expose_secret(), target_name).enqueue(conn)?; } + Command::SyncAdmins => { + jobs::SyncAdmins.enqueue(conn)?; + } Command::DailyDbMaintenance => { jobs::DailyDbMaintenance.enqueue(conn)?; } From 29644859b3781657ca2898760af68b5ad7c0e32a Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Fri, 5 Jan 2024 11:51:50 +0100 Subject: [PATCH 5/7] team_repo: Pin "Let's Encrypt" intermediate certificate https://team-api.infra.rust-lang.org is hosted on GitHub Pages, which uses Let's Encrypt as CA. The certificates for https://team-api.infra.rust-lang.org itself are short-lived and somewhat out of our control, but the intermediate certificate from Let's Encrypt can at least be pinned to reduce the risk of man in the middle attacks. --- src/bin/background-worker.rs | 2 +- src/certs/lets-encrypt.pem | 30 ++++++++++++++++++++++++++++++ src/certs/mod.rs | 1 + src/lib.rs | 1 + src/team_repo.rs | 34 ++++++++++++++++++++++++++++++++-- 5 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 src/certs/lets-encrypt.pem create mode 100644 src/certs/mod.rs diff --git a/src/bin/background-worker.rs b/src/bin/background-worker.rs index 79fd9f400e8..0c6a8218397 100644 --- a/src/bin/background-worker.rs +++ b/src/bin/background-worker.rs @@ -78,7 +78,7 @@ fn main() -> anyhow::Result<()> { let emails = Emails::from_environment(&config); let fastly = Fastly::from_environment(client.clone()); - let team_repo = TeamRepoImpl::new(client); + let team_repo = TeamRepoImpl::default(); let connection_pool = r2d2::Pool::builder() .max_size(10) diff --git a/src/certs/lets-encrypt.pem b/src/certs/lets-encrypt.pem new file mode 100644 index 00000000000..cd44265fe8b --- /dev/null +++ b/src/certs/lets-encrypt.pem @@ -0,0 +1,30 @@ +-----BEGIN CERTIFICATE----- +MIIFFjCCAv6gAwIBAgIRAJErCErPDBinU/bWLiWnX1owDQYJKoZIhvcNAQELBQAw +TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh +cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMjAwOTA0MDAwMDAw +WhcNMjUwOTE1MTYwMDAwWjAyMQswCQYDVQQGEwJVUzEWMBQGA1UEChMNTGV0J3Mg +RW5jcnlwdDELMAkGA1UEAxMCUjMwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK +AoIBAQC7AhUozPaglNMPEuyNVZLD+ILxmaZ6QoinXSaqtSu5xUyxr45r+XXIo9cP +R5QUVTVXjJ6oojkZ9YI8QqlObvU7wy7bjcCwXPNZOOftz2nwWgsbvsCUJCWH+jdx +sxPnHKzhm+/b5DtFUkWWqcFTzjTIUu61ru2P3mBw4qVUq7ZtDpelQDRrK9O8Zutm +NHz6a4uPVymZ+DAXXbpyb/uBxa3Shlg9F8fnCbvxK/eG3MHacV3URuPMrSXBiLxg +Z3Vms/EY96Jc5lP/Ooi2R6X/ExjqmAl3P51T+c8B5fWmcBcUr2Ok/5mzk53cU6cG +/kiFHaFpriV1uxPMUgP17VGhi9sVAgMBAAGjggEIMIIBBDAOBgNVHQ8BAf8EBAMC +AYYwHQYDVR0lBBYwFAYIKwYBBQUHAwIGCCsGAQUFBwMBMBIGA1UdEwEB/wQIMAYB +Af8CAQAwHQYDVR0OBBYEFBQusxe3WFbLrlAJQOYfr52LFMLGMB8GA1UdIwQYMBaA +FHm0WeZ7tuXkAXOACIjIGlj26ZtuMDIGCCsGAQUFBwEBBCYwJDAiBggrBgEFBQcw +AoYWaHR0cDovL3gxLmkubGVuY3Iub3JnLzAnBgNVHR8EIDAeMBygGqAYhhZodHRw +Oi8veDEuYy5sZW5jci5vcmcvMCIGA1UdIAQbMBkwCAYGZ4EMAQIBMA0GCysGAQQB +gt8TAQEBMA0GCSqGSIb3DQEBCwUAA4ICAQCFyk5HPqP3hUSFvNVneLKYY611TR6W +PTNlclQtgaDqw+34IL9fzLdwALduO/ZelN7kIJ+m74uyA+eitRY8kc607TkC53wl +ikfmZW4/RvTZ8M6UK+5UzhK8jCdLuMGYL6KvzXGRSgi3yLgjewQtCPkIVz6D2QQz +CkcheAmCJ8MqyJu5zlzyZMjAvnnAT45tRAxekrsu94sQ4egdRCnbWSDtY7kh+BIm +lJNXoB1lBMEKIq4QDUOXoRgffuDghje1WrG9ML+Hbisq/yFOGwXD9RiX8F6sw6W4 +avAuvDszue5L3sz85K+EC4Y/wFVDNvZo4TYXao6Z0f+lQKc0t8DQYzk1OXVu8rp2 +yJMC6alLbBfODALZvYH7n7do1AZls4I9d1P4jnkDrQoxB3UqQ9hVl3LEKQ73xF1O +yK5GhDDX8oVfGKF5u+decIsH4YaTw7mP3GFxJSqv3+0lUFJoi5Lc5da149p90Ids +hCExroL1+7mryIkXPeFM5TgO9r0rvZaBFOvV2z0gp35Z0+L4WPlbuEjN/lxPFin+ +HlUjr8gRsI3qfJOQFy/9rKIJR0Y/8Omwt/8oTWgy1mdeHmmjk7j1nYsvC9JSQ6Zv +MldlTTKB3zhThV1+XWYp6rjd5JW1zbVWEkLNxE7GJThEUG3szgBVGP7pSWTUTsqX +nLRbwHOoq7hHwg== +-----END CERTIFICATE----- diff --git a/src/certs/mod.rs b/src/certs/mod.rs new file mode 100644 index 00000000000..de2d1f8da58 --- /dev/null +++ b/src/certs/mod.rs @@ -0,0 +1 @@ +pub const LETS_ENCRYPT: &[u8] = include_bytes!("./lets-encrypt.pem"); diff --git a/src/lib.rs b/src/lib.rs index c35c04302db..b0af96db80d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,6 +32,7 @@ pub mod admin; mod app; pub mod auth; pub mod boot; +pub mod certs; pub mod ci; pub mod cloudfront; pub mod config; diff --git a/src/team_repo.rs b/src/team_repo.rs index 19beb6830f8..0764ade747e 100644 --- a/src/team_repo.rs +++ b/src/team_repo.rs @@ -5,9 +5,10 @@ //! purposes. The [TeamRepoImpl] struct is the actual implementation of //! the trait. +use crate::certs; use async_trait::async_trait; use mockall::automock; -use reqwest::Client; +use reqwest::{Certificate, Client}; #[automock] #[async_trait] @@ -35,11 +36,28 @@ pub struct TeamRepoImpl { } impl TeamRepoImpl { - pub fn new(client: Client) -> Self { + fn new(client: Client) -> Self { TeamRepoImpl { client } } } +impl Default for TeamRepoImpl { + fn default() -> Self { + let client = build_client(); + TeamRepoImpl::new(client) + } +} + +fn build_client() -> Client { + let lets_encrypt_cert = Certificate::from_pem(certs::LETS_ENCRYPT).unwrap(); + + Client::builder() + .tls_built_in_root_certs(false) + .add_root_certificate(lets_encrypt_cert) + .build() + .unwrap() +} + #[async_trait] impl TeamRepo for TeamRepoImpl { async fn get_team(&self, name: &str) -> anyhow::Result { @@ -48,3 +66,15 @@ impl TeamRepo for TeamRepoImpl { Ok(response.json().await?) } } + +#[cfg(test)] +mod tests { + use crate::team_repo::build_client; + + /// This test is here to make sure that the client is built + /// correctly without panicking. + #[test] + fn test_build_client() { + let _client = build_client(); + } +} From e580a6dc63cd9bfa8af83cf5d038f1f196fbe0ef Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 8 Jan 2024 12:10:54 +0100 Subject: [PATCH 6/7] admin/enqueue_job: Enqueue `SyncAdmin` job only if none are in progress already --- src/admin/enqueue_job.rs | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/admin/enqueue_job.rs b/src/admin/enqueue_job.rs index d7e2bbc9045..04f55ce67ee 100644 --- a/src/admin/enqueue_job.rs +++ b/src/admin/enqueue_job.rs @@ -3,6 +3,7 @@ use crate::schema::{background_jobs, crates}; use crate::worker::jobs; use anyhow::Result; use crates_io_worker::BackgroundJob; +use diesel::dsl::exists; use diesel::prelude::*; use secrecy::{ExposeSecret, SecretString}; @@ -30,7 +31,11 @@ pub enum Command { #[arg()] name: String, }, - SyncAdmins, + SyncAdmins { + /// Force a sync even if one is already in progress + #[arg(long)] + force: bool, + }, } pub fn run(command: Command) -> Result<()> { @@ -59,7 +64,26 @@ pub fn run(command: Command) -> Result<()> { } => { jobs::DumpDb::new(database_url.expose_secret(), target_name).enqueue(conn)?; } - Command::SyncAdmins => { + Command::SyncAdmins { force } => { + if !force { + // By default, we don't want to enqueue a sync if one is already + // in progress. If a sync fails due to e.g. an expired pinned + // certificate we don't want to keep adding new jobs to the + // queue, since the existing job will be retried until it + // succeeds. + + let query = background_jobs::table + .filter(background_jobs::job_type.eq(jobs::SyncAdmins::JOB_NAME)); + + if diesel::select(exists(query)).get_result(conn)? { + info!( + "Did not enqueue {}, existing job already in progress", + jobs::SyncAdmins::JOB_NAME + ); + return Ok(()); + } + } + jobs::SyncAdmins.enqueue(conn)?; } Command::DailyDbMaintenance => { From 7e4b9f8ea0ea299de2203e67cabfe60237202115 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 8 Jan 2024 12:54:40 +0100 Subject: [PATCH 7/7] worker/jobs/sync_admins: Send notification emails --- Cargo.lock | 1 + Cargo.toml | 1 + ..._worker__sync_admins__sync_admins_job.snap | 8 ++ src/tests/worker/sync_admins.rs | 24 ++++- src/worker/jobs/sync_admins.rs | 102 +++++++++++++++--- 5 files changed, 119 insertions(+), 17 deletions(-) create mode 100644 src/tests/worker/snapshots/all__worker__sync_admins__sync_admins_job.snap diff --git a/Cargo.lock b/Cargo.lock index 717c880bd94..4f6b6e2c429 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -973,6 +973,7 @@ dependencies = [ "parking_lot", "prometheus", "rand", + "regex", "reqwest", "scheduled-thread-pool", "secrecy", diff --git a/Cargo.toml b/Cargo.toml index bc7a12d28ea..63c98f90919 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -129,4 +129,5 @@ crates_io_test_db = { path = "crates_io_test_db" } claims = "=0.7.1" googletest = "=0.10.0" insta = { version = "=1.34.0", features = ["json", "redactions"] } +regex = "=1.10.2" tokio = "=1.35.1" diff --git a/src/tests/worker/snapshots/all__worker__sync_admins__sync_admins_job.snap b/src/tests/worker/snapshots/all__worker__sync_admins__sync_admins_job.snap new file mode 100644 index 00000000000..990ebf240b7 --- /dev/null +++ b/src/tests/worker/snapshots/all__worker__sync_admins__sync_admins_job.snap @@ -0,0 +1,8 @@ +--- +source: src/tests/worker/sync_admins.rs +expression: emails +--- +[ + "To: existing-admin@crates.io\r\nFrom: noreply@crates.io\r\nSubject: crates.io: Admin account changes\r\nContent-Type: text/plain; charset=utf-8\r\nContent-Transfer-Encoding: 7bit\r\n\r\nNew admins have been added:\r\n\r\n- new-admin (github_id: 3)\r\n\r\nAdmin access has been revoked for:\r\n- obsolete-admin (github_id: 2)\r\n", + "To: obsolete-admin@crates.io\r\nFrom: noreply@crates.io\r\nSubject: crates.io: Admin account changes\r\nContent-Type: text/plain; charset=utf-8\r\nContent-Transfer-Encoding: 7bit\r\n\r\nNew admins have been added:\r\n\r\n- new-admin (github_id: 3)\r\n\r\nAdmin access has been revoked for:\r\n- obsolete-admin (github_id: 2)\r\n", +] diff --git a/src/tests/worker/sync_admins.rs b/src/tests/worker/sync_admins.rs index 27d5816b003..68d95d19eac 100644 --- a/src/tests/worker/sync_admins.rs +++ b/src/tests/worker/sync_admins.rs @@ -1,10 +1,12 @@ use crate::util::TestApp; -use crates_io::schema::users; +use crates_io::schema::{emails, users}; use crates_io::team_repo::{Member, MockTeamRepo, Team}; use crates_io::worker::jobs::SyncAdmins; use crates_io_worker::BackgroundJob; use diesel::prelude::*; use diesel::{PgConnection, QueryResult, RunQueryDsl}; +use insta::assert_debug_snapshot; +use regex::Regex; #[test] fn test_sync_admins_job() { @@ -39,6 +41,15 @@ fn test_sync_admins_job() { let admins = app.db(|conn| get_admins(conn).unwrap()); let expected_admins = vec![("existing-admin".into(), 1), ("new-admin".into(), 3)]; assert_eq!(admins, expected_admins); + + let email_header_regex = Regex::new(r"(Message-ID|Date): [^\r\n]+\r\n").unwrap(); + let emails = app.as_inner().emails.mails_in_memory().unwrap(); + let emails = emails + .iter() + .map(|(_, email)| email_header_regex.replace_all(email, "")) + .collect::>(); + + assert_debug_snapshot!(emails); } fn mock_team(name: impl Into, members: Vec) -> Team { @@ -61,7 +72,7 @@ fn mock_member(name: impl Into, github_id: i32) -> Member { } fn create_user(name: &str, gh_id: i32, is_admin: bool, conn: &mut PgConnection) -> QueryResult<()> { - diesel::insert_into(users::table) + let user_id = diesel::insert_into(users::table) .values(( users::name.eq(name), users::gh_login.eq(name), @@ -69,6 +80,15 @@ fn create_user(name: &str, gh_id: i32, is_admin: bool, conn: &mut PgConnection) users::gh_access_token.eq("some random token"), users::is_admin.eq(is_admin), )) + .returning(users::id) + .get_result::(conn)?; + + diesel::insert_into(emails::table) + .values(( + emails::user_id.eq(user_id), + emails::email.eq(format!("{}@crates.io", name)), + emails::verified.eq(true), + )) .execute(conn)?; Ok(()) diff --git a/src/worker/jobs/sync_admins.rs b/src/worker/jobs/sync_admins.rs index 6c31ff1d42a..8819e8381bf 100644 --- a/src/worker/jobs/sync_admins.rs +++ b/src/worker/jobs/sync_admins.rs @@ -1,10 +1,12 @@ -use crate::schema::users; +use crate::email::Email; +use crate::schema::{emails, users}; use crate::tasks::spawn_blocking; use crate::worker::Environment; use crates_io_worker::BackgroundJob; use diesel::prelude::*; use diesel::RunQueryDsl; use std::collections::HashSet; +use std::fmt::{Display, Formatter}; use std::sync::Arc; /// See . @@ -31,57 +33,83 @@ impl BackgroundJob for SyncAdmins { let mut conn = ctx.connection_pool.get()?; let database_admins = users::table - .select((users::gh_id, users::gh_login)) + .left_join(emails::table) + .select((users::gh_id, users::gh_login, emails::email.nullable())) .filter(users::is_admin.eq(true)) - .get_results::<(i32, String)>(&mut conn)?; + .get_results::<(i32, String, Option)>(&mut conn)?; let database_admin_ids = database_admins .iter() - .map(|(gh_id, _)| *gh_id) + .map(|(gh_id, _, _)| *gh_id) .collect::>(); let new_admin_ids = repo_admin_ids .difference(&database_admin_ids) .collect::>(); - if new_admin_ids.is_empty() { + let new_admins = if new_admin_ids.is_empty() { debug!("No new admins to add"); + vec![] } else { let new_admins = repo_admins .iter() .filter(|m| new_admin_ids.contains(&&m.github_id)) .map(|m| format!("{} (github_id: {})", m.github, m.github_id)) - .collect::>() - .join(", "); + .collect::>(); - info!("Adding new admins: {}", new_admins); + info!("Adding new admins: {}", new_admins.join(", ")); diesel::update(users::table) .filter(users::gh_id.eq_any(new_admin_ids)) .set(users::is_admin.eq(true)) .execute(&mut conn)?; - } + + new_admins + }; let obsolete_admin_ids = database_admin_ids .difference(&repo_admin_ids) .collect::>(); - if obsolete_admin_ids.is_empty() { + let obsolete_admins = if obsolete_admin_ids.is_empty() { debug!("No obsolete admins to remove"); + vec![] } else { let obsolete_admins = database_admins .iter() - .filter(|(gh_id, _)| obsolete_admin_ids.contains(&gh_id)) - .map(|(gh_id, login)| format!("{} (github_id: {})", login, gh_id)) - .collect::>() - .join(", "); + .filter(|(gh_id, _, _)| obsolete_admin_ids.contains(&gh_id)) + .map(|(gh_id, login, _)| format!("{} (github_id: {})", login, gh_id)) + .collect::>(); - info!("Removing obsolete admins: {}", obsolete_admins); + info!("Removing obsolete admins: {}", obsolete_admins.join(", ")); diesel::update(users::table) .filter(users::gh_id.eq_any(obsolete_admin_ids)) .set(users::is_admin.eq(false)) .execute(&mut conn)?; + + obsolete_admins + }; + + if !new_admins.is_empty() || !obsolete_admins.is_empty() { + let email = AdminAccountEmail::new(new_admins, obsolete_admins); + + for database_admin in &database_admins { + let (_, _, email_address) = database_admin; + if let Some(email_address) = email_address { + if let Err(error) = ctx.emails.send(email_address, email.clone()) { + warn!( + "Failed to send email to admin {} ({}, github_id: {}): {}", + database_admin.1, email_address, database_admin.0, error + ); + } + } else { + warn!( + "No email address found for admin {} (github_id: {})", + database_admin.1, database_admin.0 + ); + } + } } Ok(()) @@ -91,3 +119,47 @@ impl BackgroundJob for SyncAdmins { Ok(()) } } + +#[derive(Debug, Clone)] +struct AdminAccountEmail { + new_admins: Vec, + obsolete_admins: Vec, +} + +impl AdminAccountEmail { + fn new(new_admins: Vec, obsolete_admins: Vec) -> Self { + Self { + new_admins, + obsolete_admins, + } + } +} + +impl Email for AdminAccountEmail { + const SUBJECT: &'static str = "crates.io: Admin account changes"; + + fn body(&self) -> String { + self.to_string() + } +} + +impl Display for AdminAccountEmail { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + if !self.new_admins.is_empty() { + writeln!(f, "New admins have been added:\n")?; + for new_admin in &self.new_admins { + writeln!(f, "- {}", new_admin)?; + } + writeln!(f)?; + } + + if !self.obsolete_admins.is_empty() { + writeln!(f, "Admin access has been revoked for:")?; + for obsolete_admin in &self.obsolete_admins { + writeln!(f, "- {}", obsolete_admin)?; + } + } + + Ok(()) + } +}