Move the source of truth for auth checks into config::Server.

Author: LawnGnome

src/auth.rs

@@ -1,3 +1,4 @@
+use crate::config::Server;
 use crate::controllers;
 use crate::controllers::util::RequestPartsExt;
 use crate::middleware::log_request::RequestLogExt;
@@ -73,12 +74,17 @@ impl AuthCheck {
     pub fn check<T: RequestPartsExt>(
         &self,
         request: &T,
+        config: &Server,
         conn: &mut PgConnection,
     ) -> AppResult<Authentication> {
-        self.check_authentication(authenticate(request, conn)?)
+        self.check_authentication(authenticate(request, conn)?, config)
     }
 
-    fn check_authentication(&self, auth: Authentication) -> AppResult<Authentication> {
+    fn check_authentication(
+        &self,
+        auth: Authentication,
+        config: &Server,
+    ) -> AppResult<Authentication> {
         if let Some(token) = auth.api_token() {
             if !self.allow_token {
                 let error_message =
@@ -97,8 +103,7 @@ impl AuthCheck {
             }
         }
 
-        // FIXME: better admin detection through chemistry^Wstate.
-        if self.require_admin && auth.user().admin().is_err() {
+        if self.require_admin && !config.gh_admin_user_ids.contains(&auth.user().gh_id) {
             let error_message = "User is unauthorized";
             return Err(internal(error_message).chain(forbidden()));
         }
@@ -269,10 +274,6 @@ fn ensure_not_locked(user: &User) -> AppResult<()> {
 
 #[cfg(test)]
 mod tests {
-    use std::{collections::HashSet, iter::Cycle};
-
-    use crate::models::helpers;
-
     use super::*;
 
     fn cs(scope: &str) -> CrateScope {
@@ -373,81 +374,42 @@ mod tests {
     #[test]
     fn require_admin() {
         let auth_check = AuthCheck::default().require_admin();
+        let config = Server {
+            gh_admin_user_ids: [42, 43].into_iter().collect(),
+            ..Default::default()
+        };
 
-        let mut gen = MockUserGenerator::default();
-        let admin = gen.admin();
-        let non_admin = gen.regular();
+        assert_ok!(auth_check.check_authentication(mock_cookie(42), &config));
+        assert_err!(auth_check.check_authentication(mock_cookie(44), &config));
+        assert_ok!(auth_check.check_authentication(mock_token(43), &config));
+        assert_err!(auth_check.check_authentication(mock_token(45), &config));
+    }
 
-        assert_ok!(auth_check.check_authentication(mock_cookie(admin.clone())));
-        assert_err!(auth_check.check_authentication(mock_cookie(non_admin.clone())));
-        assert_ok!(auth_check.check_authentication(mock_token(admin)));
-        assert_err!(auth_check.check_authentication(mock_token(non_admin)));
+    fn mock_user(gh_id: i32) -> User {
+        User {
+            id: 3,
+            gh_access_token: "arbitrary".into(),
+            gh_login: "literally_anything".into(),
+            name: None,
+            gh_avatar: None,
+            gh_id,
+            account_lock_reason: None,
+            account_lock_until: None,
+        }
     }
 
-    fn mock_cookie(user: User) -> Authentication {
-        Authentication::Cookie(CookieAuthentication { user })
+    fn mock_cookie(gh_id: i32) -> Authentication {
+        Authentication::Cookie(CookieAuthentication {
+            user: mock_user(gh_id),
+        })
     }
 
-    fn mock_token(user: User) -> Authentication {
+    fn mock_token(gh_id: i32) -> Authentication {
         Authentication::Token(TokenAuthentication {
             token: crate::models::token::tests::build_mock()
-                .user_id(user.id)
+                .user_id(gh_id)
                 .token(),
-            user,
+            user: mock_user(gh_id),
         })
     }
-
-    // TODO: cleanup what isn't used in this set of tests.
-    pub struct MockUserGenerator {
-        authorized_uids: HashSet<i32>,
-        authorized_uid_iter: Cycle<std::vec::IntoIter<i32>>,
-        last_unauthorized_uid: i32,
-    }
-
-    impl Default for MockUserGenerator {
-        fn default() -> Self {
-            let authorized_uids = helpers::admin::authorized_user_ids().clone();
-            let authorized_uid_iter = authorized_uids
-                .iter()
-                .copied()
-                .collect::<Vec<i32>>()
-                .into_iter()
-                .cycle();
-
-            Self {
-                authorized_uids,
-                authorized_uid_iter,
-                last_unauthorized_uid: 0,
-            }
-        }
-    }
-
-    impl MockUserGenerator {
-        pub fn admin(&mut self) -> User {
-            Self::mock_user(self.authorized_uid_iter.next().unwrap())
-        }
-
-        pub fn regular(&mut self) -> User {
-            let mut uid = self.last_unauthorized_uid + 1;
-            while self.authorized_uids.contains(&uid) {
-                uid += 1;
-            }
-
-            self.last_unauthorized_uid = uid;
-            Self::mock_user(uid)
-        }
-
-        fn mock_user(gh_id: i32) -> User {
-            User {
-                id: 3,
-                gh_access_token: "arbitrary".into(),
-                gh_login: "literally_anything".into(),
-                name: None,
-                gh_avatar: None,
-                gh_id,
-                account_lock_reason: None,
-                account_lock_until: None,
-            }
-        }
-    }
 }

src/config.rs

@@ -13,12 +13,17 @@ pub use self::base::Base;
 pub use self::database_pools::{DatabasePools, DbPoolConfig};
 pub use crate::config::balance_capacity::BalanceCapacityConfig;
 use http::HeaderValue;
-use std::collections::HashSet;
-use std::time::Duration;
+use std::{collections::HashSet, num::ParseIntError, str::FromStr, time::Duration};
 
 const DEFAULT_VERSION_ID_CACHE_SIZE: u64 = 10_000;
 const DEFAULT_VERSION_ID_CACHE_TTL: u64 = 5 * 60; // 5 minutes
 
+// The defaults correspond to the current crates.io team, which as at the time of writing, is the
+// GitHub user names carols10cents, jtgeibel, Turbo87, JohnTitor, LawnGnome, and mdtro.
+//
+// FIXME: this needs to be removed once we can detect the admins from the Rust teams API.
+const DEFAULT_GH_ADMIN_USER_IDS: [i32; 6] = [193874, 22186, 141300, 25030997, 229984, 20070360];
+
 pub struct Server {
     pub base: Base,
     pub db: DatabasePools,
@@ -47,6 +52,7 @@ pub struct Server {
     pub version_id_cache_ttl: Duration,
     pub cdn_user_agent: String,
     pub balance_capacity: BalanceCapacityConfig,
+    pub gh_admin_user_ids: HashSet<i32>,
 }
 
 impl Default for Server {
@@ -82,6 +88,9 @@ impl Default for Server {
     ///   endpoint even with a healthy database pool.
     /// - `BLOCKED_ROUTES`: A comma separated list of HTTP route patterns that are manually blocked
     ///   by an operator (e.g. `/crates/:crate_id/:version/download`).
+    /// - `GH_ADMIN_USER_IDS`: A comma separated list of GitHub user IDs that will be considered
+    ///   admins. If not set, a default list comprised of the crates.io team as of May 2023 will be
+    ///   used.
     ///
     /// # Panics
     ///
@@ -151,6 +160,9 @@ impl Default for Server {
             cdn_user_agent: dotenvy::var("WEB_CDN_USER_AGENT")
                 .unwrap_or_else(|_| "Amazon CloudFront".into()),
             balance_capacity: BalanceCapacityConfig::from_environment(),
+            gh_admin_user_ids: env_optional("GH_ADMIN_USER_IDS")
+                .map(parse_gh_admin_user_ids)
+                .unwrap_or(DEFAULT_GH_ADMIN_USER_IDS.into_iter().collect()),
         }
     }
 }
@@ -289,3 +301,29 @@ fn parse_ipv6_based_cidr_blocks() {
             .unwrap()
     );
 }
+
+fn parse_gh_admin_user_ids(users: String) -> HashSet<i32> {
+    users
+        .split(|c: char| !(c.is_ascii_digit()))
+        .filter(|uid| !uid.is_empty())
+        .map(i32::from_str)
+        .collect::<Result<HashSet<i32>, ParseIntError>>()
+        .expect("invalid GH_ADMIN_USER_IDS")
+}
+
+#[test]
+fn test_parse_authorized_admin_users() {
+    fn assert_authorized(input: &str, expected: &[i32]) {
+        assert_eq!(
+            parse_gh_admin_user_ids(input.into()),
+            expected.iter().copied().collect()
+        );
+    }
+
+    assert_authorized("", &[]);
+    assert_authorized("12345", &[12345]);
+    assert_authorized("12345, 6789", &[12345, 6789]);
+    assert_authorized("   12345  6789 ", &[12345, 6789]);
+    assert_authorized("12345;6789", &[12345, 6789]);
+    assert_authorized("12345;6789;12345", &[12345, 6789]);
+}

src/controllers/crate_owner_invitation.rs

@@ -19,7 +19,7 @@ use std::collections::{HashMap, HashSet};
 pub async fn list(app: AppState, req: Parts) -> AppResult<Json<Value>> {
     conduit_compat(move || {
         let conn = &mut app.db_read()?;
-        let auth = AuthCheck::only_cookie().check(&req, conn)?;
+        let auth = AuthCheck::only_cookie().check(&req, &app.config, conn)?;
         let user_id = auth.user_id();
 
         let PrivateListResponse {
@@ -59,7 +59,7 @@ pub async fn list(app: AppState, req: Parts) -> AppResult<Json<Value>> {
 pub async fn private_list(app: AppState, req: Parts) -> AppResult<Json<PrivateListResponse>> {
     conduit_compat(move || {
         let conn = &mut app.db_read()?;
-        let auth = AuthCheck::only_cookie().check(&req, conn)?;
+        let auth = AuthCheck::only_cookie().check(&req, &app.config, conn)?;
 
         let filter = if let Some(crate_name) = req.query().get("crate_name") {
             ListFilter::CrateName(crate_name.clone())
@@ -267,7 +267,7 @@ pub async fn handle_invite(state: AppState, req: BytesRequest) -> AppResult<Json
 
         let conn = &mut state.db_write()?;
 
-        let auth = AuthCheck::default().check(&req, conn)?;
+        let auth = AuthCheck::default().check(&req, &state.config, conn)?;
         let user_id = auth.user_id();
 
         let config = &state.config;

src/controllers/krate/follow.rs

@@ -20,7 +20,9 @@ pub async fn follow(
 ) -> AppResult<Response> {
     conduit_compat(move || {
         let conn = &mut *app.db_write()?;
-        let user_id = AuthCheck::default().check(&req, conn)?.user_id();
+        let user_id = AuthCheck::default()
+            .check(&req, &app.config, conn)?
+            .user_id();
         let follow = follow_target(&crate_name, conn, user_id)?;
         diesel::insert_into(follows::table)
             .values(&follow)
@@ -40,7 +42,9 @@ pub async fn unfollow(
 ) -> AppResult<Response> {
     conduit_compat(move || {
         let conn = &mut *app.db_write()?;
-        let user_id = AuthCheck::default().check(&req, conn)?.user_id();
+        let user_id = AuthCheck::default()
+            .check(&req, &app.config, conn)?
+            .user_id();
         let follow = follow_target(&crate_name, conn, user_id)?;
         diesel::delete(&follow).execute(conn)?;
 
@@ -59,7 +63,9 @@ pub async fn following(
         use diesel::dsl::exists;
 
         let conn = &mut *app.db_read_prefer_primary()?;
-        let user_id = AuthCheck::only_cookie().check(&req, conn)?.user_id();
+        let user_id = AuthCheck::only_cookie()
+            .check(&req, &app.config, conn)?
+            .user_id();
         let follow = follow_target(&crate_name, conn, user_id)?;
         let following =
             diesel::select(exists(follows::table.find(follow.id()))).get_result::<bool>(conn)?;

src/controllers/krate/owners.rs

@@ -106,7 +106,7 @@ fn modify_owners(
     let auth = AuthCheck::default()
         .with_endpoint_scope(EndpointScope::ChangeOwners)
         .for_crate(crate_name)
-        .check(req, conn)?;
+        .check(req, &app.config, conn)?;
 
     let user = auth.user();
 

src/controllers/krate/publish.rs

@@ -94,7 +94,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
         let auth = AuthCheck::default()
             .with_endpoint_scope(endpoint_scope)
             .for_crate(&new_crate.name)
-            .check(&req, conn)?;
+            .check(&req,&app.config, conn)?;
 
         let api_token_id = auth.api_token_id();
         let user = auth.user();

src/controllers/krate/search.rs

@@ -190,7 +190,9 @@ pub async fn search(app: AppState, req: Parts) -> AppResult<Json<Value>> {
             // Calculating the total number of results with filters is not supported yet.
             supports_seek = false;
 
-            let user_id = AuthCheck::default().check(&req, conn)?.user_id();
+            let user_id = AuthCheck::default()
+                .check(&req, &app.config, conn)?
+                .user_id();
 
             query = query.filter(
                 crates::id.eq_any(

src/controllers/token.rs

@@ -13,7 +13,7 @@ use serde_json as json;
 pub async fn list(app: AppState, req: Parts) -> AppResult<Json<Value>> {
     conduit_compat(move || {
         let conn = &mut *app.db_read_prefer_primary()?;
-        let auth = AuthCheck::only_cookie().check(&req, conn)?;
+        let auth = AuthCheck::only_cookie().check(&req, &app.config, conn)?;
         let user = auth.user();
 
         let tokens: Vec<ApiToken> = ApiToken::belonging_to(user)
@@ -53,7 +53,7 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult<Json<Value>> {
 
         let conn = &mut *app.db_write()?;
 
-        let auth = AuthCheck::default().check(&req, conn)?;
+        let auth = AuthCheck::default().check(&req, &app.config, conn)?;
         if auth.api_token_id().is_some() {
             return Err(bad_request(
                 "cannot use an API token to create a new API token",
@@ -107,7 +107,7 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult<Json<Value>> {
 pub async fn revoke(app: AppState, Path(id): Path<i32>, req: Parts) -> AppResult<Json<Value>> {
     conduit_compat(move || {
         let conn = &mut *app.db_write()?;
-        let auth = AuthCheck::default().check(&req, conn)?;
+        let auth = AuthCheck::default().check(&req, &app.config, conn)?;
         let user = auth.user();
         diesel::update(ApiToken::belonging_to(user).find(id))
             .set(api_tokens::revoked.eq(true))
@@ -122,7 +122,7 @@ pub async fn revoke(app: AppState, Path(id): Path<i32>, req: Parts) -> AppResult
 pub async fn revoke_current(app: AppState, req: Parts) -> AppResult<Response> {
     conduit_compat(move || {
         let conn = &mut *app.db_write()?;
-        let auth = AuthCheck::default().check(&req, conn)?;
+        let auth = AuthCheck::default().check(&req, &app.config, conn)?;
         let api_token_id = auth
             .api_token_id()
             .ok_or_else(|| bad_request("token not provided"))?;