Merge pull request #531 from Kobzol/rollup-auth

Require review permissions for creating rollups

Author: Kobzol

src/github/oauth.rs

@@ -1,6 +1,6 @@
-use crate::github::GithubRepoName;
 use crate::github::api::DEFAULT_REQUEST_TIMEOUT;
 use crate::github::api::client::GithubRepositoryClient;
+use crate::github::{GithubRepoName, GithubUser};
 use anyhow::Context;
 use octocrab::OctocrabBuilder;
 use octocrab::service::middleware::retry::RetryConfig;
@@ -76,17 +76,17 @@ impl OAuthClient {
             .context("Cannot get user authenticated with OAuth")?;
 
         let client_repo = GithubRepoName::new(&user.login, repo.name());
-        let client = GithubRepositoryClient::new(user.html_url, user_client, client_repo);
+        let client = GithubRepositoryClient::new(user.html_url.clone(), user_client, client_repo);
         Ok(UserGitHubClient {
-            username: user.login,
+            user: user.into(),
             client,
         })
     }
 }
 
 /// GitHub client authenticated to work with a user's fork of a repository managed by bors.
 pub struct UserGitHubClient {
-    pub username: String,
+    pub user: GithubUser,
     pub client: GithubRepositoryClient,
 }
 

src/github/rollup.rs

@@ -4,6 +4,7 @@ use crate::bors::make_text_ignored_by_bors;
 use crate::github::api::client::GithubRepositoryClient;
 use crate::github::api::operations::{ForcePush, MergeError};
 use crate::github::oauth::{OAuthClient, UserGitHubClient};
+use crate::permissions::PermissionType;
 use crate::server::ServerStateRef;
 use anyhow::Context;
 use axum::extract::{Query, State};
@@ -47,6 +48,7 @@ pub enum RollupError {
     PullRequestNotFound { pr: PullRequestNumber },
     NoPullRequestsSelected,
     TooManyPullRequests,
+    NotAuthenticated,
 }
 
 impl IntoResponse for RollupError {
@@ -77,6 +79,11 @@ impl IntoResponse for RollupError {
                 StatusCode::BAD_REQUEST,
                 format!("Rolling up too many pull requests, at most {ROLLUP_PR_LIMIT} is allowed"),
             ),
+            RollupError::NotAuthenticated => (
+                StatusCode::FORBIDDEN,
+                "You are not allowed to create rollups. Review permissions are required."
+                    .to_string(),
+            ),
         }
         .into_response()
     }
@@ -116,6 +123,16 @@ pub async fn oauth_callback_handler(
         .get_authenticated_client(repo_name.clone(), &callback.code)
         .await?;
 
+    // The rollup author is expected to r+ the rollup, so they must have review permissions to
+    // create it in the first place.
+    if !repo_state
+        .permissions
+        .load()
+        .has_permission(user_client.user.id, PermissionType::Review)
+    {
+        return Err(RollupError::NotAuthenticated);
+    }
+
     let span = tracing::info_span!(
         "create_rollup",
         repo = %repo_name,
@@ -165,7 +182,7 @@ async fn create_rollup(
         mut pr_nums,
     } = rollup_state;
 
-    let username = user_client.username;
+    let username = user_client.user.username;
 
     tracing::info!("User {username} is creating a rollup with PRs: {pr_nums:?}");
 
@@ -360,6 +377,7 @@ async fn create_rollup(
 mod tests {
     use crate::github::rollup::OAuthRollupState;
     use crate::github::{GithubRepoName, PullRequestNumber};
+    use crate::permissions::PermissionType;
     use crate::tests::{
         ApiRequest, ApiResponse, BorsTester, Comment, GitHub, MergeBehavior, PullRequest, Repo,
         User, default_repo_name, run_test,
@@ -368,7 +386,15 @@ mod tests {
 
     #[sqlx::test]
     async fn rollup_missing_fork(pool: sqlx::PgPool) {
-        run_test(pool, async |ctx: &mut BorsTester| {
+        let mut gh = GitHub::default();
+        gh.add_user(rollup_user());
+        gh.get_repo(())
+            .lock()
+            .permissions
+            .users
+            .insert(rollup_user(), vec![PermissionType::Review]);
+
+        run_test((pool, gh), async |ctx: &mut BorsTester| {
             let pr1 = ctx.open_pr((), |_| {}).await?;
             let pr2 = ctx.open_pr((), |_| {}).await?;
 
@@ -435,6 +461,24 @@ mod tests {
         .await;
     }
 
+    #[sqlx::test]
+    async fn rollup_missing_review_permissions(pool: sqlx::PgPool) {
+        let mut gh = GitHub::default();
+        gh.add_user(rollup_user());
+        run_test((pool, gh), async |ctx: &mut BorsTester| {
+            let pr1 = ctx.open_pr((), |_| {}).await?;
+
+            make_rollup(ctx, &[&pr1])
+                .await?
+                .assert_status(StatusCode::FORBIDDEN)
+                .assert_body(
+                    "You are not allowed to create rollups. Review permissions are required.",
+                );
+            Ok(())
+        })
+        .await;
+    }
+
     #[sqlx::test]
     async fn rollup_merge_conflict(pool: sqlx::PgPool) {
         let gh = run_test((pool, rollup_state()), async |ctx: &mut BorsTester| {
@@ -640,6 +684,12 @@ mod tests {
         let mut gh = GitHub::default();
         let rolluper = rollup_user();
         gh.add_user(rolluper.clone());
+        gh.get_repo(())
+            .lock()
+            .permissions
+            .users
+            .insert(rolluper.clone(), vec![PermissionType::Review]);
+
         // Create fork
         let mut repo = Repo::new(rolluper, fork_repo().name());
         repo.fork = true;

src/tests/mock/mod.rs

@@ -354,16 +354,17 @@ EjQJOzV2OIk4waurl+BsbOHP7C0Zhp7rpyWx4fUCgYEAp/4UceUfbJZGa8CcWZ8F
 34PVnCZP7HB3k2eBSpDp4vk=
 -----END PRIVATE KEY-----"###;
 
-fn oauth_user_from_request(request: &Request) -> User {
+fn oauth_user_from_request(github: Arc<Mutex<GitHub>>, request: &Request) -> User {
     let auth: &HeaderValue = request
         .headers
         .get(AUTHORIZATION)
         .expect("Authorization header not found");
     let (_bearer, token) = auth.to_str().unwrap().split_once(" ").unwrap();
     let (user, rest) = token.split_once("-").unwrap();
     assert_eq!(rest, "access-token");
-    // TODO: load this from GitHub state
-    User::new(10000, user)
+
+    let gh = github.lock();
+    gh.get_user(user).expect("User not found").clone()
 }
 
 /// Create a mock that dynamically responds to its requests using the given function `f`.

src/tests/mock/oauth.rs

@@ -42,7 +42,7 @@ pub async fn mock_oauth(github: Arc<Mutex<GitHub>>, mock_server: &MockServer) {
     Mock::given(method("GET"))
         .and(path("/user"))
         .respond_with(move |req: &Request| {
-            let user = oauth_user_from_request(req);
+            let user = oauth_user_from_request(github.clone(), req);
             ResponseTemplate::new(200).set_body_json(GitHubUser::from(user))
         })
         .mount(mock_server)