diff --git a/.sqlx/query-71942069bc62e1dbedca845779de5f3d0a6878caf78ee0d600bb2c8fe30036fd.json b/.sqlx/query-71942069bc62e1dbedca845779de5f3d0a6878caf78ee0d600bb2c8fe30036fd.json new file mode 100644 index 00000000..36046c3d --- /dev/null +++ b/.sqlx/query-71942069bc62e1dbedca845779de5f3d0a6878caf78ee0d600bb2c8fe30036fd.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET delegated = TRUE WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [] + }, + "hash": "71942069bc62e1dbedca845779de5f3d0a6878caf78ee0d600bb2c8fe30036fd" +} diff --git a/.sqlx/query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json b/.sqlx/query-c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7.json similarity index 58% rename from .sqlx/query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json rename to .sqlx/query-c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7.json index 86c22dfb..c326d5e0 100644 --- a/.sqlx/query-491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1.json +++ b/.sqlx/query-c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE pr.repository = $1\n AND pr.number = $2\n", + "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\n pr.delegated,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE pr.repository = $1\n AND pr.number = $2\n", "describe": { "columns": [ { @@ -30,11 +30,16 @@ }, { "ordinal": 5, + "name": "delegated", + "type_info": "Bool" + }, + { + "ordinal": 6, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 6, + "ordinal": 7, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -51,9 +56,10 @@ false, true, true, + false, null, false ] }, - "hash": "491dd6278535e1015a8071f416a56112a153713a71af8d62917e2ff9873ecec1" + "hash": "c4b0d4ed216c6b8efceb1b65ae0ad9d830e8870cfdf37542a408b99c961318f7" } diff --git a/.sqlx/query-c9d86eebfcadcae17f8a5e7e31f0e7c14ea0eabe866a27fbdedbddcc0166af43.json b/.sqlx/query-c9d86eebfcadcae17f8a5e7e31f0e7c14ea0eabe866a27fbdedbddcc0166af43.json new file mode 100644 index 00000000..b9a767d2 --- /dev/null +++ b/.sqlx/query-c9d86eebfcadcae17f8a5e7e31f0e7c14ea0eabe866a27fbdedbddcc0166af43.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE pull_request SET delegated = FALSE WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4" + ] + }, + "nullable": [] + }, + "hash": "c9d86eebfcadcae17f8a5e7e31f0e7c14ea0eabe866a27fbdedbddcc0166af43" +} diff --git a/.sqlx/query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json b/.sqlx/query-f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4.json similarity index 58% rename from .sqlx/query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json rename to .sqlx/query-f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4.json index 5cbf4626..2e7de668 100644 --- a/.sqlx/query-67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a.json +++ b/.sqlx/query-f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE build.id = $1\n", + "query": "\nSELECT\n pr.id,\n pr.repository,\n pr.number,\n pr.approved_by,\n pr.priority,\n pr.delegated,\n CASE WHEN pr.build_id IS NULL\n THEN NULL\n ELSE (\n build.id,\n build.repository,\n build.branch,\n build.commit_sha,\n build.status,\n build.parent,\n build.created_at\n )\n END AS \"try_build: BuildModel\",\n pr.created_at as \"created_at: DateTime\"\nFROM pull_request as pr\n LEFT JOIN build ON pr.build_id = build.id\nWHERE build.id = $1\n", "describe": { "columns": [ { @@ -30,11 +30,16 @@ }, { "ordinal": 5, + "name": "delegated", + "type_info": "Bool" + }, + { + "ordinal": 6, "name": "try_build: BuildModel", "type_info": "Record" }, { - "ordinal": 6, + "ordinal": 7, "name": "created_at: DateTime", "type_info": "Timestamptz" } @@ -50,9 +55,10 @@ false, true, true, + false, null, false ] }, - "hash": "67511f5f15c07537d3d4f2d011abedd2173ab301e8a06c740b587738b140795a" + "hash": "f223617621c400f216ecfe415f0bf54a0f8cfb8aecace69b5d53cb888e5c9fa4" } diff --git a/docs/commands.md b/docs/commands.md index a9987ed7..40e505ed 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -8,4 +8,6 @@ which is by default set to `@bors`. | `try` | `try` | Start a try build based on the most recent commit from the main branch. | | `try parent=` | `try` | Start a try build based on the specified parent commit `sha`. | | `try cancel` | `try` | Cancel a running try build. | -| `p=` | `try` | Set the priority of a PR. | +| `p=` | `review` | Set the priority of a PR. | +| `delegate+` | `review` | Delegate approval authority to the PR author. | +| `delegate-` | `review` | Remove any previously granted delegation. | diff --git a/migrations/20250304150656_add_delegated_to_pr.down.sql b/migrations/20250304150656_add_delegated_to_pr.down.sql new file mode 100644 index 00000000..1289d6ca --- /dev/null +++ b/migrations/20250304150656_add_delegated_to_pr.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +ALTER TABLE pull_request DROP COLUMN delegated; \ No newline at end of file diff --git a/migrations/20250304150656_add_delegated_to_pr.up.sql b/migrations/20250304150656_add_delegated_to_pr.up.sql new file mode 100644 index 00000000..44147ca1 --- /dev/null +++ b/migrations/20250304150656_add_delegated_to_pr.up.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE pull_request ADD COLUMN delegated BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index 1155125e..f134f520 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -49,4 +49,8 @@ pub enum BorsCommand { TryCancel, /// Set the priority of a commit. SetPriority(Priority), + /// Delegate approval authority to the pull request author. + Delegate, + /// Revoke any previously granted delegation. + Undelegate, } diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index 235ffe51..32b48a74 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -51,6 +51,8 @@ impl CommandParser { parser_ping, parser_try_cancel, parser_try, + parse_delegate_author, + parse_undelegate, ]; text.lines() @@ -152,6 +154,7 @@ fn parse_approve_on_behalf<'a>(parts: &[CommandPart<'a>]) -> ParseResult<'a> { if value.is_empty() { return Some(Err(CommandParseError::MissingArgValue { arg: "r" })); } + match parse_priority_arg(&parts[1..]) { Ok(priority) => Some(Ok(BorsCommand::Approve { approver: Approver::Specified(value.to_string()), @@ -270,6 +273,24 @@ fn parser_try_cancel<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseRe } } +/// Parses "@bors delegate+" +fn parse_delegate_author<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if command == "delegate+" { + Some(Ok(BorsCommand::Delegate)) + } else { + None + } +} + +/// Parses "@bors delegate-" +fn parse_undelegate<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { + if command == "delegate-" { + Some(Ok(BorsCommand::Undelegate)) + } else { + None + } +} + /// Parses "@bors p=" fn parse_priority<'a>(parts: &[CommandPart<'a>]) -> ParseResult<'a> { // If we have more than one part, check for duplicate priority arguments @@ -821,6 +842,27 @@ line two assert!(matches!(cmds[0], Ok(BorsCommand::TryCancel))); } + #[test] + fn parse_delegate_author() { + let cmds = parse_commands("@bors delegate+"); + assert_eq!(cmds.len(), 1); + assert!(matches!(cmds[0], Ok(BorsCommand::Delegate))); + } + + #[test] + fn parse_undelegate() { + let cmds = parse_commands("@bors delegate-"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::Undelegate)); + } + + #[test] + fn parse_delegate_author_unknown_arg() { + let cmds = parse_commands("@bors delegate+ a"); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0], Ok(BorsCommand::Delegate)); + } + fn parse_commands(text: &str) -> Vec> { CommandParser::new("@bors".to_string()).parse_commands(text) } diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index 302f8d25..20f3496d 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -19,6 +19,8 @@ pub(super) async fn command_help( }, BorsCommand::Unapprove, BorsCommand::SetPriority(0), + BorsCommand::Delegate, + BorsCommand::Undelegate, BorsCommand::Try { parent: None, jobs: vec![], @@ -53,6 +55,12 @@ fn get_command_help(command: BorsCommand) -> String { BorsCommand::SetPriority(_) => { "`p=`: Set the priority of this PR" } + BorsCommand::Delegate => { + "`delegate+`: Delegate approval authority to the PR author" + } + BorsCommand::Undelegate => { + "`delegate-`: Remove any previously granted delegation" + } BorsCommand::Help => { "`help`: Print this help message" } @@ -82,6 +90,8 @@ mod tests { - `r= [p=]`: Approve this PR on behalf of ``. Optionally, you can specify a ``. - `r-`: Unapprove this PR - `p=`: Set the priority of this PR + - `delegate+`: Delegate approval authority to the PR author + - `delegate-`: Remove any previously granted delegation - `try [parent=] [jobs=]`: Start a try build. Optionally, you can specify a `` SHA or a list of `` to run - `try cancel`: Cancel a running try build - `ping`: Check if the bot is alive diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 045ddea8..bdb0a231 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use anyhow::Context; use octocrab::Octocrab; -use review::command_set_priority; +use review::{command_delegate, command_set_priority, command_undelegate}; use tracing::Instrument; use crate::bors::command::{BorsCommand, CommandParseError}; @@ -18,6 +18,8 @@ use crate::bors::handlers::workflow::{ handle_check_suite_completed, handle_workflow_completed, handle_workflow_started, }; use crate::bors::{BorsContext, Comment, RepositoryState}; +use crate::github::{GithubUser, PullRequest}; +use crate::permissions::PermissionType; use crate::{load_repositories, PgDbClient, TeamApiClient}; #[cfg(test)] @@ -237,6 +239,18 @@ async fn handle_comment( .instrument(span) .await } + BorsCommand::Delegate => { + let span = tracing::info_span!("Delegate"); + command_delegate(repo, database, &pull_request, &comment.author) + .instrument(span) + .await + } + BorsCommand::Undelegate => { + let span = tracing::info_span!("Undelegate"); + command_undelegate(repo, database, &pull_request, &comment.author) + .instrument(span) + .await + } BorsCommand::Help => { let span = tracing::info_span!("Help"); command_help(repo, &pull_request).instrument(span).await @@ -334,6 +348,52 @@ fn is_bors_observed_branch(branch: &str) -> bool { branch == TRY_BRANCH_NAME } +/// Deny permission for a request. +async fn deny_request( + repo: &RepositoryState, + pr: &PullRequest, + author: &GithubUser, + permission_type: PermissionType, +) -> anyhow::Result<()> { + tracing::warn!( + "Permission denied for request command by {}", + author.username + ); + repo.client + .post_comment( + pr.number, + Comment::new(format!( + "@{}: :key: Insufficient privileges: not in {} users", + author.username, permission_type + )), + ) + .await +} + +/// Check if a user has specified permission or has been delegated. +async fn has_permission( + repo_state: &RepositoryState, + author: &GithubUser, + pr: &PullRequest, + db: &PgDbClient, + permission: PermissionType, +) -> anyhow::Result { + if repo_state + .permissions + .load() + .has_permission(author.id, permission.clone()) + { + return Ok(true); + } + + let pr_model = db + .get_or_create_pull_request(repo_state.repository(), pr.number) + .await?; + let is_delegated = pr_model.delegated && author.id == pr.author.id; + + Ok(is_delegated) +} + #[cfg(test)] mod tests { use crate::tests::mocks::{run_test, Comment, User}; diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 0b705c1a..66edc1fc 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -3,6 +3,8 @@ use std::sync::Arc; use crate::bors::command::Approver; use crate::bors::event::PullRequestEdited; use crate::bors::event::PullRequestPushed; +use crate::bors::handlers::deny_request; +use crate::bors::handlers::has_permission; use crate::bors::handlers::labels::handle_label_trigger; use crate::bors::Comment; use crate::bors::RepositoryState; @@ -25,7 +27,7 @@ pub(super) async fn command_approve( priority: Option, ) -> anyhow::Result<()> { tracing::info!("Approving PR {}", pr.number); - if !sufficient_approve_permission(repo_state.clone(), author) { + if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { deny_request(&repo_state, pr, author, PermissionType::Review).await?; return Ok(()); }; @@ -53,7 +55,7 @@ pub(super) async fn command_unapprove( author: &GithubUser, ) -> anyhow::Result<()> { tracing::info!("Unapproving PR {}", pr.number); - if !sufficient_unapprove_permission(repo_state.clone(), pr, author) { + if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { deny_request(&repo_state, pr, author, PermissionType::Review).await?; return Ok(()); }; @@ -71,7 +73,7 @@ pub(super) async fn command_set_priority( author: &GithubUser, priority: u32, ) -> anyhow::Result<()> { - if !sufficient_priority_permission(repo_state.clone(), author) { + if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { deny_request(&repo_state, pr, author, PermissionType::Review).await?; return Ok(()); }; @@ -79,6 +81,39 @@ pub(super) async fn command_set_priority( .await } +/// Delegate approval authority of a pull request to its author. +pub(super) async fn command_delegate( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, +) -> anyhow::Result<()> { + tracing::info!("Delegating PR {} approval", pr.number); + if !sufficient_delegate_permission(repo_state.clone(), author) { + deny_request(&repo_state, pr, author, PermissionType::Review).await?; + return Ok(()); + } + + let delegatee = pr.author.username.clone(); + db.delegate(repo_state.repository(), pr.number).await?; + notify_of_delegation(&repo_state, pr, &delegatee).await +} + +/// Revoke any previously granted delegation. +pub(super) async fn command_undelegate( + repo_state: Arc, + db: Arc, + pr: &PullRequest, + author: &GithubUser, +) -> anyhow::Result<()> { + tracing::info!("Undelegating PR {} approval", pr.number); + if !has_permission(&repo_state, author, pr, &db, PermissionType::Review).await? { + deny_request(&repo_state, pr, author, PermissionType::Review).await?; + return Ok(()); + } + db.undelegate(repo_state.repository(), pr.number).await +} + pub(super) async fn handle_pull_request_edited( repo_state: Arc, db: Arc, @@ -122,13 +157,7 @@ pub(super) async fn handle_push_to_pull_request( notify_of_pushed_pr(&repo_state, pr_number, pr.head.sha.clone()).await } -fn sufficient_approve_permission(repo: Arc, author: &GithubUser) -> bool { - repo.permissions - .load() - .has_permission(author.id, PermissionType::Review) -} - -fn sufficient_priority_permission(repo: Arc, author: &GithubUser) -> bool { +fn sufficient_delegate_permission(repo: Arc, author: &GithubUser) -> bool { repo.permissions .load() .has_permission(author.id, PermissionType::Review) @@ -150,39 +179,6 @@ async fn notify_of_approval( .await } -fn sufficient_unapprove_permission( - repo: Arc, - pr: &PullRequest, - author: &GithubUser, -) -> bool { - author.id == pr.author.id - || repo - .permissions - .load() - .has_permission(author.id, PermissionType::Review) -} - -async fn deny_request( - repo: &RepositoryState, - pr: &PullRequest, - author: &GithubUser, - permission_type: PermissionType, -) -> anyhow::Result<()> { - tracing::warn!( - "Permission denied for request command by {}", - author.username - ); - repo.client - .post_comment( - pr.number, - Comment::new(format!( - "@{}: :key: Insufficient privileges: not in {} users", - author.username, permission_type - )), - ) - .await -} - async fn notify_of_unapproval(repo: &RepositoryState, pr: &PullRequest) -> anyhow::Result<()> { repo.client .post_comment( @@ -225,13 +221,28 @@ PR will need to be re-approved."#, .await } +async fn notify_of_delegation( + repo: &RepositoryState, + pr: &PullRequest, + delegatee: &str, +) -> anyhow::Result<()> { + repo.client + .post_comment( + pr.number, + Comment::new(format!("@{} can now approve this pull request", delegatee)), + ) + .await +} + #[cfg(test)] mod tests { use crate::{ + bors::handlers::{trybuild::TRY_MERGE_BRANCH_NAME, TRY_BRANCH_NAME}, github::PullRequestNumber, + permissions::PermissionType, tests::mocks::{ - default_pr_number, default_repo_name, run_test, BorsBuilder, BorsTester, Permissions, - PullRequestChangeEvent, User, World, + default_pr_number, default_repo_name, run_test, BorsBuilder, BorsTester, Comment, + Permissions, PullRequestChangeEvent, User, World, }, }; @@ -603,4 +614,284 @@ approve = ["+approved"] }) .await; } + + fn reviewer() -> User { + User::new(10, "reviewer") + } + + fn as_reviewer(text: &str) -> Comment { + Comment::from(text).with_author(reviewer()) + } + + fn create_world_with_delegate_config() -> World { + let world = World::default(); + world.default_repo().lock().set_config( + r#" +[labels] +approve = ["+approved"] +"#, + ); + world.default_repo().lock().permissions = Permissions::default(); + world + .default_repo() + .lock() + .permissions + .users + .insert(reviewer(), vec![PermissionType::Review]); + + world + } + + #[sqlx::test] + async fn delegate_author(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + assert_eq!( + tester.get_comment().await?, + format!( + "@{} can now approve this pull request", + User::default().name + ) + ); + + let pr = tester.get_default_pr().await?; + assert!(pr.delegated); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn delegatee_can_approve(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + tester.post_comment("@bors r+").await?; + tester.expect_comments(1).await; + + check_pr_approved_by(&tester, default_pr_number().into(), &User::default().name) + .await; + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn delegatee_can_try(pool: sqlx::PgPool) { + let world = BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + tester.post_comment("@bors try").await?; + tester.expect_comments(1).await; + + Ok(tester) + }) + .await; + world.check_sha_history( + default_repo_name(), + TRY_MERGE_BRANCH_NAME, + &["main-sha1", "merge-main-sha1-pr-1-sha-0"], + ); + world.check_sha_history( + default_repo_name(), + TRY_BRANCH_NAME, + &["merge-main-sha1-pr-1-sha-0"], + ); + } + + #[sqlx::test] + async fn delegatee_can_set_priority(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + tester.post_comment("@bors p=7").await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(pr.priority == Some(7)) + }) + .await?; + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn delegate_insufficient_permission(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment("@bors delegate+").await?; + assert_eq!( + tester.get_comment().await?, + format!( + "@{}: :key: Insufficient privileges: not in review users", + User::default().name + ) + ); + + let pr = tester.get_default_pr().await?; + assert!(!pr.delegated); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn undelegate_by_reviewer(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert!(pr.delegated); + + tester.post_comment(as_reviewer("@bors delegate-")).await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(!pr.delegated) + }) + .await?; + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn undelegate_by_delegatee(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + tester.post_comment("@bors delegate-").await?; + tester + .wait_for(|| async { + let pr = tester.get_default_pr().await?; + Ok(!pr.delegated) + }) + .await?; + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn undelegate_insufficient_permission(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment("@bors delegate-").await?; + assert_eq!( + tester.get_comment().await?, + format!( + "@{}: :key: Insufficient privileges: not in review users", + User::default().name + ) + .as_str() + ); + + let pr = tester.get_default_pr().await?; + assert!(!pr.delegated); + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn reviewer_unapprove_delegated_approval(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + tester + .post_comment(Comment::from("@bors r+").with_author(User::default())) + .await?; + tester.expect_comments(1).await; + check_pr_approved_by(&tester, default_pr_number().into(), &User::default().name) + .await; + + tester.post_comment(as_reviewer("@bors r-")).await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert!(!pr.is_approved()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn non_author_cannot_use_delegation(pool: sqlx::PgPool) { + BorsBuilder::new(pool) + .world(create_world_with_delegate_config()) + .run_test(|mut tester| async { + tester.post_comment(as_reviewer("@bors delegate+")).await?; + tester.expect_comments(1).await; + + tester + .post_comment( + Comment::from("@bors r+").with_author(User::new(999, "not-the-author")), + ) + .await?; + tester.expect_comments(1).await; + + let pr = tester.get_default_pr().await?; + assert!(!pr.is_approved()); + + Ok(tester) + }) + .await; + } + + #[sqlx::test] + async fn delegate_insufficient_permission_try_user(pool: sqlx::PgPool) { + let world = World::default(); + let try_user = User::new(200, "try-user"); + world.default_repo().lock().permissions = Permissions::default(); + world + .default_repo() + .lock() + .permissions + .users + .insert(try_user.clone(), vec![PermissionType::Try]); + + BorsBuilder::new(pool) + .world(world) + .run_test(|mut tester| async { + tester + .post_comment(Comment::from("@bors delegate+").with_author(try_user)) + .await?; + assert_eq!( + tester.get_comment().await?, + "@try-user: :key: Insufficient privileges: not in review users" + ); + + let pr = tester.get_default_pr().await?; + assert!(!pr.delegated); + Ok(tester) + }) + .await; + } } diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index fb15cd3a..7dbf35dc 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -21,11 +21,14 @@ use crate::github::{ use crate::permissions::PermissionType; use crate::PgDbClient; +use super::deny_request; +use super::has_permission; + // This branch serves for preparing the final commit. // It will be reset to master and merged with the branch that should be tested. // Because this action (reset + merge) is not atomic, this branch should not run CI checks to avoid // starting them twice. -const TRY_MERGE_BRANCH_NAME: &str = "automation/bors/try-merge"; +pub(super) const TRY_MERGE_BRANCH_NAME: &str = "automation/bors/try-merge"; // This branch should run CI checks. pub(super) const TRY_BRANCH_NAME: &str = "automation/bors/try"; @@ -44,7 +47,8 @@ pub(super) async fn command_try_build( jobs: Vec, ) -> anyhow::Result<()> { let repo = repo.as_ref(); - if !check_try_permissions(repo, pr, author).await? { + if !has_permission(repo, author, pr, &db, PermissionType::Try).await? { + deny_request(repo, pr, author, PermissionType::Try).await?; return Ok(()); } @@ -190,7 +194,8 @@ pub(super) async fn command_try_cancel( author: &GithubUser, ) -> anyhow::Result<()> { let repo = repo.as_ref(); - if !check_try_permissions(repo, pr, author).await? { + if !has_permission(repo, author, pr, &db, PermissionType::Try).await? { + deny_request(repo, pr, author, PermissionType::Try).await?; return Ok(()); } @@ -322,33 +327,6 @@ handled during merge and rebase. This is normal, and you should still perform st Comment::new(message) } -async fn check_try_permissions( - repo: &RepositoryState, - pr: &PullRequest, - author: &GithubUser, -) -> anyhow::Result { - let result = if !repo - .permissions - .load() - .has_permission(author.id, PermissionType::Try) - { - tracing::warn!("Try permission denied for {}", author.username); - repo.client - .post_comment( - pr.number, - Comment::new(format!( - "@{}: :key: Insufficient privileges: not in try users", - author.username - )), - ) - .await?; - false - } else { - true - }; - Ok(result) -} - #[cfg(test)] mod tests { use crate::bors::handlers::trybuild::{TRY_BRANCH_NAME, TRY_MERGE_BRANCH_NAME}; diff --git a/src/database/client.rs b/src/database/client.rs index 16691e1d..27618d97 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -7,10 +7,10 @@ use crate::github::PullRequestNumber; use crate::github::{CommitSha, GithubRepoName}; use super::operations::{ - approve_pull_request, create_build, create_pull_request, create_workflow, find_build, - find_pr_by_build, get_pull_request, get_running_builds, get_workflows_for_build, - set_pr_priority, unapprove_pull_request, update_build_status, update_pr_build_id, - update_workflow_status, + approve_pull_request, create_build, create_pull_request, create_workflow, + delegate_pull_request, find_build, find_pr_by_build, get_pull_request, get_running_builds, + get_workflows_for_build, set_pr_priority, unapprove_pull_request, undelegate_pull_request, + update_build_status, update_pr_build_id, update_workflow_status, }; use super::RunId; @@ -55,6 +55,24 @@ impl PgDbClient { set_pr_priority(&self.pool, pr.id, priority).await } + pub async fn delegate( + &self, + repo: &GithubRepoName, + pr_number: PullRequestNumber, + ) -> anyhow::Result<()> { + let pr = self.get_or_create_pull_request(repo, pr_number).await?; + delegate_pull_request(&self.pool, pr.id).await + } + + pub async fn undelegate( + &self, + repo: &GithubRepoName, + pr_number: PullRequestNumber, + ) -> anyhow::Result<()> { + let pr = self.get_or_create_pull_request(repo, pr_number).await?; + undelegate_pull_request(&self.pool, pr.id).await + } + pub async fn get_or_create_pull_request( &self, repo: &GithubRepoName, diff --git a/src/database/mod.rs b/src/database/mod.rs index a8810a84..7cdcff27 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -120,6 +120,7 @@ pub struct PullRequestModel { pub repository: GithubRepoName, pub number: PullRequestNumber, pub approved_by: Option, + pub delegated: bool, pub priority: Option, pub try_build: Option, pub created_at: DateTime, diff --git a/src/database/operations.rs b/src/database/operations.rs index 23c42599..54b76776 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -28,6 +28,7 @@ SELECT pr.number, pr.approved_by, pr.priority, + pr.delegated, CASE WHEN pr.build_id IS NULL THEN NULL ELSE ( @@ -101,6 +102,32 @@ pub(crate) async fn unapprove_pull_request( Ok(()) } +pub(crate) async fn delegate_pull_request( + executor: impl PgExecutor<'_>, + pr_id: i32, +) -> anyhow::Result<()> { + sqlx::query!( + "UPDATE pull_request SET delegated = TRUE WHERE id = $1", + pr_id, + ) + .execute(executor) + .await?; + Ok(()) +} + +pub(crate) async fn undelegate_pull_request( + executor: impl PgExecutor<'_>, + pr_id: i32, +) -> anyhow::Result<()> { + sqlx::query!( + "UPDATE pull_request SET delegated = FALSE WHERE id = $1", + pr_id + ) + .execute(executor) + .await?; + Ok(()) +} + pub(crate) async fn find_pr_by_build( executor: impl PgExecutor<'_>, build_id: i32, @@ -114,6 +141,7 @@ SELECT pr.number, pr.approved_by, pr.priority, + pr.delegated, CASE WHEN pr.build_id IS NULL THEN NULL ELSE (