diff --git a/data/permissions/people.json.example b/data/permissions/people.json.example new file mode 100644 index 00000000..4dd10e23 --- /dev/null +++ b/data/permissions/people.json.example @@ -0,0 +1,3 @@ +{ + "people": {} +} diff --git a/data/permissions/teams.json.example b/data/permissions/teams.json.example new file mode 100644 index 00000000..6f2ae93d --- /dev/null +++ b/data/permissions/teams.json.example @@ -0,0 +1,4 @@ +{ + "all" : {}, + "infra" : {} +} diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index f3bec9fd..b2e7427c 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -117,11 +117,11 @@ async fn check_unknown_reviewers( repo_state: &RepositoryState, reviewers: &[String], ) -> Vec { - 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| { diff --git a/src/permissions.rs b/src/permissions.rs index 3e7f7ab1..d7d1b912 100644 --- a/src/permissions.rs +++ b/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, + teams: HashSet, +} + +impl TeamDirectory { + pub fn new(usernames: HashSet, teams: HashSet) -> 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, - review_usernames: HashSet, try_users: HashSet, + directory: TeamDirectory, } impl UserPermissions { pub fn new( review_users: HashSet, - review_usernames: HashSet, try_users: HashSet, + directory: TeamDirectory, ) -> Self { + let team_names: HashSet = directory + .teams + .iter() + .map(|name| name.to_lowercase()) + .collect(); + let people_names: HashSet = 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, - github_users: HashSet, } +type PeopleResponse = HashMap; + +type TeamResponse = HashMap; + enum TeamSource { Url(String), Directory(String), @@ -90,20 +125,26 @@ impl TeamApiClient { ) -> anyhow::Result { 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, HashSet)> { + ) -> anyhow::Result> { 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> { + 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::() + .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::(&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> { + 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::() + .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::(&data) + .map_err(|error| { + anyhow::anyhow!("Cannot deserialize teams from a file '{path}': {error:?}") + })? + .into_keys() + .collect(); + Ok(response) } } } diff --git a/src/tests/github.rs b/src/tests/github.rs index 9c337aba..1186af6b 100644 --- a/src/tests/github.rs +++ b/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>>, comments: HashMap, users: HashMap, + teams: HashSet, pub oauth_config: OAuthConfig, workflow_run_id_counter: u64, } @@ -67,6 +68,18 @@ impl GitHub { self.users.get(name) } + pub fn users(&self) -> Vec { + self.users.values().cloned().collect() + } + + pub fn add_team(&mut self, name: &str) { + self.teams.insert(name.to_string()); + } + + pub fn teams(&self) -> Vec { + self.teams.iter().cloned().collect() + } + pub fn default_repo(&self) -> Arc> { 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, }; diff --git a/src/tests/mock/permissions.rs b/src/tests/mock/permissions.rs index 55140258..034455f9 100644 --- a/src/tests/mock/permissions.rs +++ b/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::>(), - "github_users": users.iter().map(|(user, _)| user.name.clone()).collect::>() }); Mock::given(method("GET")) @@ -49,6 +49,34 @@ impl TeamApiMockServer { .await; } + let people: HashMap = { + 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 = { + 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 } }