From ed10bcb70e90dad7394ca90283165e8770fcc960 Mon Sep 17 00:00:00 2001 From: vohoanglong0107 Date: Fri, 29 Mar 2024 00:10:08 +0000 Subject: [PATCH 1/2] feat: use user id instead of username --- src/bors/handlers/trybuild.rs | 2 +- src/github/mod.rs | 2 ++ src/github/webhook.rs | 10 ++++++++++ src/permissions.rs | 23 ++++++++++++----------- src/tests/event.rs | 3 ++- src/tests/permissions.rs | 7 ++++--- src/tests/state.rs | 4 +++- 7 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index 08866e57..0cc8c0b1 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -262,7 +262,7 @@ async fn check_try_permissions( ) -> anyhow::Result { let result = if !repo .permissions_resolver - .has_permission(&author.username, PermissionType::Try) + .has_permission(&author.id, PermissionType::Try) .await { tracing::info!("Permission denied"); diff --git a/src/github/mod.rs b/src/github/mod.rs index 10d6f957..68497609 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -2,6 +2,7 @@ //! for working with (GitHub) repositories. use std::fmt::{Debug, Display, Formatter}; +use octocrab::models::UserId; use url::Url; pub mod api; @@ -46,6 +47,7 @@ impl Display for GithubRepoName { #[derive(Debug, PartialEq)] pub struct GithubUser { + pub id: UserId, pub username: String, pub html_url: Url, } diff --git a/src/github/webhook.rs b/src/github/webhook.rs index fc02dff2..60dae176 100644 --- a/src/github/webhook.rs +++ b/src/github/webhook.rs @@ -282,6 +282,7 @@ fn parse_comment_from_pr_review( fn parse_user(user: Author) -> GithubUser { GithubUser { + id: user.id, username: user.login, html_url: user.html_url, } @@ -381,6 +382,9 @@ mod tests { name: "bors-kindergarten", }, author: GithubUser { + id: UserId( + 4539057, + ), username: "Kobzol", html_url: Url { scheme: "https", @@ -424,6 +428,9 @@ mod tests { name: "bors-kindergarten", }, author: GithubUser { + id: UserId( + 4539057, + ), username: "Kobzol", html_url: Url { scheme: "https", @@ -467,6 +474,9 @@ mod tests { name: "bors-kindergarten", }, author: GithubUser { + id: UserId( + 4539057, + ), username: "Kobzol", html_url: Url { scheme: "https", diff --git a/src/permissions.rs b/src/permissions.rs index f53d062f..28db6331 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -1,4 +1,5 @@ use axum::async_trait; +use octocrab::models::UserId; use std::collections::HashSet; use tokio::sync::Mutex; @@ -14,7 +15,7 @@ pub enum PermissionType { /// Decides if a GitHub user can perform various actions using the bot. #[async_trait] pub trait PermissionResolver { - async fn has_permission(&self, username: &str, permission: PermissionType) -> bool; + async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool; async fn reload(&self); } @@ -46,11 +47,11 @@ impl TeamApiPermissionResolver { #[async_trait] impl PermissionResolver for TeamApiPermissionResolver { - async fn has_permission(&self, username: &str, permission: PermissionType) -> bool { + async fn has_permission(&self, user_id: &UserId, permission: PermissionType) -> bool { self.permissions .lock() .await - .has_permission(username, permission) + .has_permission(user_id, permission) } async fn reload(&self) { @@ -59,15 +60,15 @@ impl PermissionResolver for TeamApiPermissionResolver { } pub struct UserPermissions { - review_users: HashSet, - try_users: HashSet, + review_users: HashSet, + try_users: HashSet, } impl UserPermissions { - fn has_permission(&self, username: &str, permission: PermissionType) -> bool { + fn has_permission(&self, user_id: &UserId, permission: PermissionType) -> bool { match permission { - PermissionType::Review => self.review_users.contains(username), - PermissionType::Try => self.try_users.contains(username), + PermissionType::Review => self.review_users.contains(user_id), + PermissionType::Try => self.try_users.contains(user_id), } } } @@ -90,14 +91,14 @@ async fn load_permissions(repo: &GithubRepoName) -> anyhow::Result, + github_ids: HashSet, } /// Loads users that are allowed to perform try/review from the Rust Team API. async fn load_users_from_team_api( repository_name: &str, permission: PermissionType, -) -> anyhow::Result> { +) -> anyhow::Result> { let permission = match permission { PermissionType::Review => "review", PermissionType::Try => "try", @@ -112,5 +113,5 @@ async fn load_users_from_team_api( .json::() .await .map_err(|error| anyhow::anyhow!("Cannot deserialize users from team API: {error:?}"))?; - Ok(users.github_users) + Ok(users.github_ids) } diff --git a/src/tests/event.rs b/src/tests/event.rs index c11b3fa7..0cd2595a 100644 --- a/src/tests/event.rs +++ b/src/tests/event.rs @@ -1,5 +1,5 @@ use derive_builder::Builder; -use octocrab::models::RunId; +use octocrab::models::{RunId, UserId}; use crate::bors::event::PullRequestComment; use crate::bors::{event, CheckSuite, CheckSuiteStatus}; @@ -9,6 +9,7 @@ use crate::tests::state::{default_merge_sha, default_repo_name}; fn default_user() -> GithubUser { GithubUser { + id: UserId(1), username: "".to_string(), html_url: "https://user.com".parse().unwrap(), } diff --git a/src/tests/permissions.rs b/src/tests/permissions.rs index cf94c34c..1e33a1f4 100644 --- a/src/tests/permissions.rs +++ b/src/tests/permissions.rs @@ -2,12 +2,13 @@ use std::sync::{Arc, Mutex}; use crate::permissions::{PermissionResolver, PermissionType}; use axum::async_trait; +use octocrab::models::UserId; pub struct NoPermissions; #[async_trait] impl PermissionResolver for NoPermissions { - async fn has_permission(&self, _username: &str, _permission: PermissionType) -> bool { + async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool { false } async fn reload(&self) {} @@ -17,7 +18,7 @@ pub struct AllPermissions; #[async_trait] impl PermissionResolver for AllPermissions { - async fn has_permission(&self, _username: &str, _permission: PermissionType) -> bool { + async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool { true } async fn reload(&self) {} @@ -37,7 +38,7 @@ impl Default for MockPermissions { #[async_trait] impl PermissionResolver for Arc> { - async fn has_permission(&self, _username: &str, _permission: PermissionType) -> bool { + async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool { false } async fn reload(&self) { diff --git a/src/tests/state.rs b/src/tests/state.rs index 24bf3a79..c94109f3 100644 --- a/src/tests/state.rs +++ b/src/tests/state.rs @@ -7,7 +7,7 @@ use std::time::Duration; use crate::config::RepositoryConfig; use axum::async_trait; use derive_builder::Builder; -use octocrab::models::RunId; +use octocrab::models::{RunId, UserId}; use super::permissions::AllPermissions; use crate::bors::event::{ @@ -29,6 +29,8 @@ use crate::tests::github::{default_base_branch, PRBuilder}; pub fn test_bot_user() -> GithubUser { GithubUser { + // just a random one, to reduce the chance of duplicate id + id: UserId(517237103), username: "".to_string(), html_url: "https://test-bors.bot.com".parse().unwrap(), } From 00ecce7c9aafaf68737d6cc6c3dba082475f76ac Mon Sep 17 00:00:00 2001 From: vohoanglong0107 Date: Sat, 30 Mar 2024 15:59:30 +0000 Subject: [PATCH 2/2] fix: rename username to user id --- src/permissions.rs | 2 +- src/tests/permissions.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/permissions.rs b/src/permissions.rs index 28db6331..760b4370 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -15,7 +15,7 @@ pub enum PermissionType { /// Decides if a GitHub user can perform various actions using the bot. #[async_trait] pub trait PermissionResolver { - async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool; + async fn has_permission(&self, user_id: &UserId, permission: PermissionType) -> bool; async fn reload(&self); } diff --git a/src/tests/permissions.rs b/src/tests/permissions.rs index 1e33a1f4..ba13d212 100644 --- a/src/tests/permissions.rs +++ b/src/tests/permissions.rs @@ -8,7 +8,7 @@ pub struct NoPermissions; #[async_trait] impl PermissionResolver for NoPermissions { - async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool { + async fn has_permission(&self, _user_id: &UserId, _permission: PermissionType) -> bool { false } async fn reload(&self) {} @@ -18,7 +18,7 @@ pub struct AllPermissions; #[async_trait] impl PermissionResolver for AllPermissions { - async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool { + async fn has_permission(&self, _user_id: &UserId, _permission: PermissionType) -> bool { true } async fn reload(&self) {} @@ -38,7 +38,7 @@ impl Default for MockPermissions { #[async_trait] impl PermissionResolver for Arc> { - async fn has_permission(&self, _username: &UserId, _permission: PermissionType) -> bool { + async fn has_permission(&self, _user_id: &UserId, _permission: PermissionType) -> bool { false } async fn reload(&self) {