Merge pull request #2050 from Kobzol/impersonation-check

Do not notify users when a non-sensitive command is executed on their behalf

Author: Urgau

src/github.rs

@@ -267,16 +267,17 @@ impl User {
 }
 
 // Returns the ID of the given user, if the user is in the `all` team.
-pub async fn get_id_for_username<'a>(
-    client: &'a GithubClient,
+pub async fn get_id_for_username(
+    client: &GithubClient,
     login: &str,
 ) -> anyhow::Result<Option<u64>> {
     let permission = crate::team_data::teams(client).await?;
     let map = permission.teams;
+    let login = login.to_lowercase();
     Ok(map["all"]
         .members
         .iter()
-        .find(|g| g.github == login)
+        .find(|g| g.github.to_lowercase() == login)
         .map(|u| u.github_id))
 }
 

src/zulip.rs

@@ -172,103 +172,162 @@ async fn process_zulip_request(ctx: &Context, req: Request) -> anyhow::Result<Op
     handle_command(ctx, gh_id, &req.data, &req.message).await
 }
 
-fn handle_command<'a>(
+async fn handle_command<'a>(
     ctx: &'a Context,
-    gh_id: u64,
+    mut gh_id: u64,
     command: &'a str,
     message_data: &'a Message,
-) -> std::pin::Pin<Box<dyn std::future::Future<Output = anyhow::Result<Option<String>>> + Send + 'a>>
-{
-    Box::pin(async move {
-        log::trace!("handling zulip command {:?}", command);
-        let words: Vec<&str> = command.split_whitespace().collect();
+) -> anyhow::Result<Option<String>> {
+    log::trace!("handling zulip command {:?}", command);
+    let words: Vec<&str> = command.split_whitespace().collect();
 
+    // Missing stream means that this is a direct message
+    if message_data.stream_id.is_none() {
+        // Handle impersonation
+        let mut impersonated = false;
         if let Some(&"as") = words.get(0) {
-            return execute_for_other_user(&ctx, words.iter().skip(1).copied(), message_data)
-                .await
-                .map_err(|e| {
-                    format_err!("Failed to parse; expected `as <username> <command...>`: {e:?}.")
-                });
+            if let Some(username) = words.get(1) {
+                let impersonated_gh_id = match get_id_for_username(&ctx.github, username)
+                    .await
+                    .context("getting ID of github user")?
+                {
+                    Some(id) => id.try_into()?,
+                    None => anyhow::bail!("Can only authorize for other GitHub users."),
+                };
+
+                // Impersonate => change actual gh_id
+                if impersonated_gh_id != gh_id {
+                    impersonated = true;
+                    gh_id = impersonated_gh_id;
+                }
+            } else {
+                return Err(anyhow::anyhow!(
+                    "Failed to parse command; expected `as <username> <command...>`."
+                ));
+            }
         }
 
-        // Missing stream means that this is a direct message
-        if message_data.stream_id.is_none() {
-            let cmd = parse_cli::<ChatCommand, _>(words.into_iter())?;
-            match cmd {
-                ChatCommand::Acknowledge { identifier } => {
-                    acknowledge(&ctx, gh_id, (&identifier).into()).await
-                }
-                ChatCommand::Add { url, description } => {
-                    add_notification(&ctx, gh_id, &url, &description.join(" ")).await
-                }
-                ChatCommand::Move { from, to } => move_notification(&ctx, gh_id, from, to).await,
-                ChatCommand::Meta { index, description } => {
-                    add_meta_notification(&ctx, gh_id, index, &description.join(" ")).await
-                }
-                ChatCommand::Whoami => whoami_cmd(&ctx, gh_id).await,
-                ChatCommand::Lookup(cmd) => lookup_cmd(&ctx, cmd).await,
-                ChatCommand::Work(cmd) => workqueue_commands(ctx, gh_id, cmd).await,
+        let cmd = parse_cli::<ChatCommand, _>(words.into_iter())?;
+        let output = match &cmd {
+            ChatCommand::Acknowledge { identifier } => {
+                acknowledge(&ctx, gh_id, identifier.into()).await
+            }
+            ChatCommand::Add { url, description } => {
+                add_notification(&ctx, gh_id, &url, &description.join(" ")).await
             }
-        } else {
-            // We are in a stream, where someone wrote `@**triagebot** <command(s)>`
-            let cmd_index = words
+            ChatCommand::Move { from, to } => move_notification(&ctx, gh_id, *from, *to).await,
+            ChatCommand::Meta { index, description } => {
+                add_meta_notification(&ctx, gh_id, *index, &description.join(" ")).await
+            }
+            ChatCommand::Whoami => whoami_cmd(&ctx, gh_id).await,
+            ChatCommand::Lookup(cmd) => lookup_cmd(&ctx, cmd).await,
+            ChatCommand::Work(cmd) => workqueue_commands(ctx, gh_id, cmd).await,
+        };
+
+        let output = output?;
+
+        // Let the impersonated person know about the impersonation if the command was sensitive
+        if impersonated && is_sensitive_command(&cmd) {
+            let impersonated_zulip_id = to_zulip_id(&ctx.github, gh_id)
+                .await?
+                .ok_or_else(|| anyhow::anyhow!("Zulip user for GitHub ID {gh_id} was not found"))?;
+            let users = ctx.zulip.get_zulip_users().await?;
+            let user = users
                 .iter()
-                .position(|w| *w == "@**triagebot**")
-                .unwrap_or(words.len());
-            let cmd_index = cmd_index + 1;
-            if cmd_index >= words.len() {
-                return Ok(Some("Unknown command".to_string()));
+                .find(|m| m.user_id == impersonated_zulip_id)
+                .ok_or_else(|| format_err!("Could not find Zulip user email."))?;
+
+            let sender = &message_data.sender_full_name;
+            let message = format!(
+                "{sender} ran `{command}` on your behalf. Output:\n{}",
+                output.as_deref().unwrap_or("<empty>")
+            );
+
+            MessageApiRequest {
+                recipient: Recipient::Private {
+                    id: user.user_id,
+                    email: &user.email,
+                },
+                content: &message,
             }
-            let cmd = parse_cli::<StreamCommand, _>(words[cmd_index..].into_iter().copied())?;
-            match cmd {
-                StreamCommand::EndTopic => {
-                    post_waiter(&ctx, message_data, WaitingMessage::end_topic())
-                        .await
-                        .map_err(|e| format_err!("Failed to await at this time: {e:?}"))
-                }
-                StreamCommand::EndMeeting => {
-                    post_waiter(&ctx, message_data, WaitingMessage::end_meeting())
-                        .await
-                        .map_err(|e| format_err!("Failed to await at this time: {e:?}"))
-                }
-                StreamCommand::Read => {
-                    post_waiter(&ctx, message_data, WaitingMessage::start_reading())
-                        .await
-                        .map_err(|e| format_err!("Failed to await at this time: {e:?}"))
-                }
-                StreamCommand::PingGoals {
-                    threshold,
-                    next_update,
-                } => {
-                    if project_goals::check_project_goal_acl(&ctx.github, gh_id).await? {
-                        ping_project_goals_owners(
-                            &ctx.github,
-                            &ctx.zulip,
-                            false,
-                            threshold as i64,
-                            &format!("on {next_update}"),
-                        )
-                        .await
-                        .map_err(|e| format_err!("Failed to await at this time: {e:?}"))?;
-                        Ok(None)
-                    } else {
-                        Err(format_err!(
+            .send(&ctx.zulip)
+            .await?;
+        }
+
+        Ok(output)
+    } else {
+        // We are in a stream, where someone wrote `@**triagebot** <command(s)>`
+        let cmd_index = words
+            .iter()
+            .position(|w| *w == "@**triagebot**")
+            .unwrap_or(words.len());
+        let cmd_index = cmd_index + 1;
+        if cmd_index >= words.len() {
+            return Ok(Some("Unknown command".to_string()));
+        }
+        let cmd = parse_cli::<StreamCommand, _>(words[cmd_index..].into_iter().copied())?;
+        match cmd {
+            StreamCommand::EndTopic => post_waiter(&ctx, message_data, WaitingMessage::end_topic())
+                .await
+                .map_err(|e| format_err!("Failed to await at this time: {e:?}")),
+            StreamCommand::EndMeeting => {
+                post_waiter(&ctx, message_data, WaitingMessage::end_meeting())
+                    .await
+                    .map_err(|e| format_err!("Failed to await at this time: {e:?}"))
+            }
+            StreamCommand::Read => post_waiter(&ctx, message_data, WaitingMessage::start_reading())
+                .await
+                .map_err(|e| format_err!("Failed to await at this time: {e:?}")),
+            StreamCommand::PingGoals {
+                threshold,
+                next_update,
+            } => {
+                if project_goals::check_project_goal_acl(&ctx.github, gh_id).await? {
+                    ping_project_goals_owners(
+                        &ctx.github,
+                        &ctx.zulip,
+                        false,
+                        threshold as i64,
+                        &format!("on {next_update}"),
+                    )
+                    .await
+                    .map_err(|e| format_err!("Failed to await at this time: {e:?}"))?;
+                    Ok(None)
+                } else {
+                    Err(format_err!(
                             "That command is only permitted for those running the project-goal program.",
                         ))
-                    }
                 }
-                StreamCommand::DocsUpdate => trigger_docs_update(message_data, &ctx.zulip),
             }
+            StreamCommand::DocsUpdate => trigger_docs_update(message_data, &ctx.zulip),
         }
-    })
+    }
+}
+
+/// Returns true if we should notify user who was impersonated by someone who executed this command.
+/// More or less, the following holds: `sensitive` == `not read-only`.
+fn is_sensitive_command(cmd: &ChatCommand) -> bool {
+    match cmd {
+        ChatCommand::Acknowledge { .. }
+        | ChatCommand::Add { .. }
+        | ChatCommand::Move { .. }
+        | ChatCommand::Meta { .. } => true,
+        ChatCommand::Whoami => false,
+        ChatCommand::Lookup(_) => false,
+        ChatCommand::Work(cmd) => match cmd {
+            WorkqueueCmd::Show => false,
+            WorkqueueCmd::SetPrLimit { .. } => true,
+            WorkqueueCmd::SetRotationMode { .. } => true,
+        },
+    }
 }
 
 /// Commands for working with the workqueue, e.g. showing how many PRs are assigned
 /// or modifying the PR review assignment limit.
 async fn workqueue_commands(
     ctx: &Context,
     gh_id: u64,
-    cmd: WorkqueueCmd,
+    cmd: &WorkqueueCmd,
 ) -> anyhow::Result<Option<String>> {
     let db_client = ctx.db.get().await;
 
@@ -326,7 +385,7 @@ async fn workqueue_commands(
         WorkqueueCmd::SetPrLimit { limit } => {
             let max_assigned_prs = match limit {
                 WorkqueueLimit::Unlimited => None,
-                WorkqueueLimit::Limit(limit) => Some(limit),
+                WorkqueueLimit::Limit(limit) => Some(*limit),
             };
             upsert_review_prefs(
                 &db_client,
@@ -419,7 +478,7 @@ async fn whoami_cmd(ctx: &Context, gh_id: u64) -> anyhow::Result<Option<String>>
     Ok(Some(output))
 }
 
-async fn lookup_cmd(ctx: &Context, cmd: LookupCmd) -> anyhow::Result<Option<String>> {
+async fn lookup_cmd(ctx: &Context, cmd: &LookupCmd) -> anyhow::Result<Option<String>> {
     let username = match &cmd {
         LookupCmd::Zulip { github_username } => github_username.clone(),
         // Usernames could contain spaces, so rejoin everything to serve as the username.
@@ -545,82 +604,6 @@ async fn lookup_zulip_username(ctx: &Context, gh_username: &str) -> anyhow::Resu
     ))
 }
 
-// This does two things:
-//  * execute the command for the other user
-//  * tell the user executed for that a command was run as them by the user
-//    given.
-async fn execute_for_other_user(
-    ctx: &Context,
-    mut words: impl Iterator<Item = &str>,
-    message_data: &Message,
-) -> anyhow::Result<Option<String>> {
-    // username is a GitHub username, not a Zulip username
-    let username = match words.next() {
-        Some(username) => username,
-        None => anyhow::bail!("no username provided"),
-    };
-    let user_id = match get_id_for_username(&ctx.github, username)
-        .await
-        .context("getting ID of github user")?
-    {
-        Some(id) => id.try_into().unwrap(),
-        None => anyhow::bail!("Can only authorize for other GitHub users."),
-    };
-    let mut command = words.fold(String::new(), |mut acc, piece| {
-        acc.push_str(piece);
-        acc.push(' ');
-        acc
-    });
-    let command = if command.is_empty() {
-        anyhow::bail!("no command provided")
-    } else {
-        assert_eq!(command.pop(), Some(' ')); // pop trailing space
-        command
-    };
-
-    let members = ctx
-        .zulip
-        .get_zulip_users()
-        .await
-        .map_err(|e| format_err!("Failed to get list of zulip users: {e:?}."))?;
-
-    // Map GitHub `user_id` to `zulip_user_id`.
-    let zulip_user_id = match to_zulip_id(&ctx.github, user_id).await {
-        Ok(Some(id)) => id as u64,
-        Ok(None) => anyhow::bail!("Could not find Zulip ID for GitHub ID: {user_id}"),
-        Err(e) => anyhow::bail!("Could not find Zulip ID for GitHub id {user_id}: {e:?}"),
-    };
-
-    let user = members
-        .iter()
-        .find(|m| m.user_id == zulip_user_id)
-        .ok_or_else(|| format_err!("Could not find Zulip user email."))?;
-
-    let output = handle_command(ctx, user_id, &command, message_data)
-        .await?
-        .unwrap_or_default();
-
-    // At this point, the command has been run.
-    let sender = &message_data.sender_full_name;
-    let message = format!("{sender} ran `{command}` with output `{output}` as you.");
-
-    let res = MessageApiRequest {
-        recipient: Recipient::Private {
-            id: user.user_id,
-            email: &user.email,
-        },
-        content: &message,
-    }
-    .send(&ctx.zulip)
-    .await;
-
-    if let Err(err) = res {
-        log::error!("Failed to notify real user about command: {:?}", err);
-    }
-
-    Ok(Some(output))
-}
-
 #[derive(serde::Serialize)]
 pub(crate) struct MessageApiRequest<'a> {
     pub(crate) recipient: Recipient<'a>,