Add TeamDirectory and load people/teams names

data/permissions/people.json.example

@@ -0,0 +1,3 @@
+{
+    "people": {}
+}

data/permissions/teams.json.example

@@ -0,0 +1,4 @@
+{
+    "all" : {},
+    "infra" : {}
+}

src/bors/handlers/review.rs

@@ -117,11 +117,11 @@ async fn check_unknown_reviewers(
     repo_state: &RepositoryState,
     reviewers: &[String],
 ) -> Vec<String> {
-    let permission = repo_state.permissions.load();
+    let directory = repo_state.permissions.load();
 
     reviewers
         .iter()
-        .filter(|reviewer| !permission.has_reviewer(reviewer))
+        .filter(|reviewer| !directory.user_exists(reviewer) && !directory.team_exists(reviewer))
         .cloned()
         .collect()
 }
@@ -782,6 +782,49 @@ approved = ["+approved"]
         .await;
     }
 
+    #[sqlx::test]
+    async fn approve_with_unknown_team(pool: sqlx::PgPool) {
+        run_test(pool, async |ctx: &mut BorsTester| {
+            ctx.post_comment("@bors r=nonexistent-team").await?;
+            insta::assert_snapshot!(
+                ctx.get_next_comment_text(()).await?,
+                @r###"
+            :pushpin: Commit pr-1-sha has been approved by `nonexistent-team`
+
+            It is now in the [queue](https://bors-test.com/queue/borstest) for this repository.
+
+            :warning: The following reviewer(s) could not be found: `nonexistent-team`
+            "###
+            );
+
+            ctx.pr(()).await.expect_approved_by("nonexistent-team");
+            Ok(())
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn approve_with_known_team(pool: sqlx::PgPool) {
+        let mut gh = GitHub::default();
+        gh.add_team("team1");
+
+        run_test((pool, gh), async |ctx: &mut BorsTester| {
+            ctx.post_comment("@bors r=team1").await?;
+            insta::assert_snapshot!(
+                ctx.get_next_comment_text(()).await?,
+                @"
+            :pushpin: Commit pr-1-sha has been approved by `team1`
+
+            It is now in the [queue](https://bors-test.com/queue/borstest) for this repository.
+            "
+            );
+
+            ctx.pr(()).await.expect_approved_by("team1");
+            Ok(())
+        })
+        .await;
+    }
+
     #[sqlx::test]
     async fn approve_with_known_reviewer_different_case(pool: sqlx::PgPool) {
         run_test(pool, async |ctx: &mut BorsTester| {

src/permissions.rs

@@ -1,6 +1,6 @@
 use octocrab::models::UserId;
 use serde::{Deserialize, Serialize};
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
 use std::fmt;
 
 use crate::github::GithubRepoName;
@@ -22,25 +22,55 @@ impl fmt::Display for PermissionType {
     }
 }
 
+pub struct TeamDirectory {
+    usernames: HashSet<String>,
+    teams: HashSet<String>,
+}
+
+impl TeamDirectory {
+    pub fn new(usernames: HashSet<String>, teams: HashSet<String>) -> Self {
+        Self { usernames, teams }
+    }
+
+    fn user_exists(&self, username: &str) -> bool {
+        self.usernames.contains(&username.to_lowercase())
+    }
+
+    fn team_exists(&self, team: &str) -> bool {
+        self.teams.contains(&team.to_lowercase())
+    }
+}
+
 pub struct UserPermissions {
     review_users: HashSet<UserId>,
-    review_usernames: HashSet<String>,
     try_users: HashSet<UserId>,
+    directory: TeamDirectory,
 }
 
 impl UserPermissions {
     pub fn new(
         review_users: HashSet<UserId>,
-        review_usernames: HashSet<String>,
         try_users: HashSet<UserId>,
+        directory: TeamDirectory,
     ) -> Self {
+        let team_names: HashSet<String> = directory
+            .teams
+            .iter()
+            .map(|name| name.to_lowercase())
+            .collect();
+        let people_names: HashSet<String> = directory
+            .usernames
+            .iter()
+            .map(|name| name.to_lowercase())
+            .collect();
+        let directory = TeamDirectory {
+            teams: team_names,
+            usernames: people_names,
+        };
         Self {
             review_users,
-            review_usernames: review_usernames
-                .into_iter()
-                .map(|username| username.to_lowercase())
-                .collect(),
             try_users,
+            directory,
         }
     }
 
@@ -51,19 +81,24 @@ impl UserPermissions {
         }
     }
 
-    /// Checks if the given username is a valid reviewer.
-    /// The usernames are checked in a case-insensitive manner.
-    pub fn has_reviewer(&self, username: &str) -> bool {
-        self.review_usernames.contains(&username.to_lowercase())
+    pub fn user_exists(&self, username: &str) -> bool {
+        self.directory.user_exists(username)
+    }
+
+    pub fn team_exists(&self, team: &str) -> bool {
+        self.directory.team_exists(team)
     }
 }
 
 #[derive(Deserialize, Serialize)]
 pub(crate) struct UserPermissionsResponse {
     github_ids: HashSet<UserId>,
-    github_users: HashSet<String>,
 }
 
+type PeopleResponse = HashMap<String, serde_json::Value>;
+
+type TeamResponse = HashMap<String, serde_json::Value>;
+
 enum TeamSource {
     Url(String),
     Directory(String),
@@ -90,20 +125,26 @@ impl TeamApiClient {
     ) -> anyhow::Result<UserPermissions> {
         tracing::info!("Reloading permissions for repository {repo}");
 
-        let (review_users, review_usernames) = self
-            .load_users(repo.name(), PermissionType::Review)
-            .await
-            .map_err(|error| anyhow::anyhow!("Cannot load review users: {error:?}"))?;
-
-        let (try_users, _try_usernames) =
-            self.load_users(repo.name(), PermissionType::Try)
-                .await
-                .map_err(|error| anyhow::anyhow!("Cannot load try users: {error:?}"))?;
+        let (review_users, try_users, known_usernames, known_teams) = tokio::try_join!(
+            async {
+                self.load_users(repo.name(), PermissionType::Review)
+                    .await
+                    .map_err(|error| anyhow::anyhow!("Cannot load review users: {error:?}"))
+            },
+            async {
+                self.load_users(repo.name(), PermissionType::Try)
+                    .await
+                    .map_err(|error| anyhow::anyhow!("Cannot load try users: {error:?}"))
+            },
+            self.load_people_names(),
+            self.load_team_names(),
+        )?;
+        let directory = TeamDirectory::new(known_usernames, known_teams);
 
         Ok(UserPermissions {
             review_users,
-            review_usernames,
             try_users,
+            directory,
         })
     }
 
@@ -113,7 +154,7 @@ impl TeamApiClient {
         &self,
         repository_name: &str,
         permission: PermissionType,
-    ) -> anyhow::Result<(HashSet<UserId>, HashSet<String>)> {
+    ) -> anyhow::Result<HashSet<UserId>> {
         let permission = match permission {
             PermissionType::Review => "review",
             PermissionType::Try => "try",
@@ -133,7 +174,7 @@ impl TeamApiClient {
                     .map_err(|error| {
                         anyhow::anyhow!("Cannot deserialize users from team API: {error:?}")
                     })?;
-                Ok((users.github_ids, users.github_users))
+                Ok(users.github_ids)
             }
             TeamSource::Directory(base_path) => {
                 let path = format!("{base_path}/bors.{permission}.json");
@@ -144,7 +185,75 @@ impl TeamApiClient {
                     serde_json::from_str(&data).map_err(|error| {
                         anyhow::anyhow!("Cannot deserialize users from a file '{path}': {error:?}")
                     })?;
-                Ok((users.github_ids, users.github_users))
+                Ok(users.github_ids)
+            }
+        }
+    }
+
+    async fn load_people_names(&self) -> anyhow::Result<HashSet<String>> {
+        match &self.team_source {
+            TeamSource::Url(base_url) => {
+                let url = format!("{base_url}/v1/people.json");
+                let response = reqwest::get(url)
+                    .await
+                    .and_then(|res| res.error_for_status())
+                    .map_err(|error| {
+                        anyhow::anyhow!("Cannot load people from team API: {error:?}")
+                    })?
+                    .json::<PeopleResponse>()
+                    .await
+                    .map_err(|error| {
+                        anyhow::anyhow!("Cannot deserialize people from team API: {error:?}")
+                    })?
+                    .into_keys()
+                    .collect();
+                Ok(response)
+            }
+            TeamSource::Directory(base_path) => {
+                let path = format!("{base_path}/people.json");
+                let data = std::fs::read_to_string(&path).map_err(|error| {
+                    anyhow::anyhow!("Could not read people from a file `{path}`: {error:?}")
+                })?;
+                let response = serde_json::from_str::<PeopleResponse>(&data)
+                    .map_err(|error| {
+                        anyhow::anyhow!("Cannot deserialize people from a file '{path}': {error:?}")
+                    })?
+                    .into_keys()
+                    .collect();
+                Ok(response)
+            }
+        }
+    }
+
+    async fn load_team_names(&self) -> anyhow::Result<HashSet<String>> {
+        match &self.team_source {
+            TeamSource::Url(base_url) => {
+                let url = format!("{base_url}/v1/teams.json");
+                let response = reqwest::get(url)
+                    .await
+                    .and_then(|res| res.error_for_status())
+                    .map_err(|error| anyhow::anyhow!("Cannot load teams from team API: {error:?}"))?
+                    .json::<TeamResponse>()
+                    .await
+                    .map_err(|error| {
+                        anyhow::anyhow!("Cannot deserialize teams from team API: {error:?}")
+                    })?
+                    .into_keys()
+                    .collect();
+                Ok(response)
+            }
+            TeamSource::Directory(base_path) => {
+                let path = format!("{base_path}/teams.json");
+                let data = std::fs::read_to_string(&path).map_err(|error| {
+                    anyhow::anyhow!("Could not read teams from a file '{path}': {error:?}")
+                })?;
+                let response = serde_json::from_str::<TeamResponse>(&data)
+                    .map_err(|error| {
+                        anyhow::anyhow!("Cannot deserialize teams from a file '{path}': {error:?}")
+                    })?
+                    .into_keys()
+                    .collect();
+                Ok(response)
             }
         }
     }

src/tests/github.rs

@@ -11,7 +11,7 @@ use octocrab::models::pulls::MergeableState;
 use octocrab::models::workflows::Conclusion;
 use octocrab::models::{CheckSuiteId, JobId, RunId};
 use parking_lot::Mutex;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::fmt::{Display, Formatter};
 use std::sync::Arc;
 use std::time::{Duration, SystemTime};
@@ -22,6 +22,7 @@ pub struct GitHub {
     pub(super) repos: HashMap<GithubRepoName, Arc<Mutex<Repo>>>,
     comments: HashMap<String, Comment>,
     users: HashMap<String, User>,
+    teams: HashSet<String>,
     pub oauth_config: OAuthConfig,
     workflow_run_id_counter: u64,
 }
@@ -67,6 +68,18 @@ impl GitHub {
         self.users.get(name)
     }
 
+    pub fn users(&self) -> Vec<User> {
+        self.users.values().cloned().collect()
+    }
+
+    pub fn add_team(&mut self, name: &str) {
+        self.teams.insert(name.to_string());
+    }
+
+    pub fn teams(&self) -> Vec<String> {
+        self.teams.iter().cloned().collect()
+    }
+
     pub fn default_repo(&self) -> Arc<Mutex<Repo>> {
         self.get_repo(default_repo_name())
     }
@@ -265,6 +278,7 @@ impl Default for GitHub {
             repos: HashMap::default(),
             comments: Default::default(),
             users: Default::default(),
+            teams: Default::default(),
             oauth_config: default_oauth_config(),
             workflow_run_id_counter: 0,
         };

src/tests/mock/permissions.rs

@@ -1,6 +1,7 @@
 use crate::permissions::PermissionType;
 use parking_lot::Mutex;
 use serde_json::json;
+use std::collections::HashMap;
 use std::sync::Arc;
 use wiremock::matchers::path;
 use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method};
@@ -24,7 +25,6 @@ impl TeamApiMockServer {
             users.retain(|p| p.1.contains(&kind));
             let permissions = json!({
                 "github_ids": users.iter().map(|(user, _)| user.github_id).collect::<Vec<_>>(),
-                "github_users": users.iter().map(|(user, _)| user.name.clone()).collect::<Vec<_>>()
             });
 
             Mock::given(method("GET"))
@@ -49,6 +49,34 @@ impl TeamApiMockServer {
                 .await;
         }
 
+        let people: HashMap<String, serde_json::Value> = {
+            let gh = github.lock();
+            gh.users()
+                .into_iter()
+                .map(|user| (user.name, json!({})))
+                .collect()
+        };
+
+        Mock::given(method("GET"))
+            .and(path("/v1/people.json"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(people))
+            .mount(&mock_server)
+            .await;
+
+        let teams: HashMap<String, serde_json::Value> = {
+            let gh = github.lock();
+            gh.teams()
+                .into_iter()
+                .map(|name| (name, json!({})))
+                .collect()
+        };
+
+        Mock::given(method("GET"))
+            .and(path("/v1/teams.json"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(teams))
+            .mount(&mock_server)
+            .await;
+
         Self { mock_server }
     }