Skip to content

feat: added check for invalid username and if it is invalid find related names #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ chrono = "0.4"

itertools = "0.14.0"

strsim = "0.10.0"

[dev-dependencies]
insta = "1.26"
derive_builder = "0.20.0"
Expand Down
26 changes: 25 additions & 1 deletion src/bors/handlers/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,16 @@ pub(super) async fn command_approve(
};
let approver = match approver {
Approver::Myself => author.username.clone(),
Approver::Specified(approver) => approver.clone(),
Approver::Specified(approver) => {
if let Some(error_comment) = repo_state.client.validate_reviewers(approver).await? {
repo_state
.client
.post_comment(pr.number, error_comment)
.await?;
return Ok(());
}
approver.clone()
}
};
db.approve(repo_state.repository(), pr.number, approver.as_str())
.await?;
Expand Down Expand Up @@ -274,6 +283,21 @@ mod tests {
.await;
}

#[sqlx::test]
async fn approve_empty_reviewer_in_list(pool: sqlx::PgPool) {
BorsBuilder::new(pool)
.world(create_world_with_approve_config())
.run_test(|mut tester| async {
tester.post_comment("@bors r=user1,,user2").await?;
assert_eq!(
tester.get_comment().await?,
"Error: Empty reviewer name provided. Use r=username to specify a reviewer."
);
Ok(tester)
})
.await;
}

#[sqlx::test]
async fn insufficient_permission_approve(pool: sqlx::PgPool) {
let world = World::default();
Expand Down
67 changes: 67 additions & 0 deletions src/github/api/client.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use anyhow::Context;
use octocrab::models::{App, Repository};
use octocrab::{Error, Octocrab};
use strsim::levenshtein;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strtim dependency is missing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh shit, forgotten to push it, done now !

use tracing::log;

use crate::bors::event::PullRequestComment;
Expand Down Expand Up @@ -287,6 +288,72 @@ impl GithubRepositoryClient {
run_ids.map(|workflow_id| self.get_workflow_url(workflow_id))
}

/// Validates a reviewer's GitHub username and returns validation results if the username is invalid.
/// If multiple reviewers are provided (comma-separated), validates each one.
/// Returns a validation result for the first invalid username encountered, or None if all usernames are valid.
pub async fn validate_reviewers(&self, reviewer_list: &str) -> anyhow::Result<Option<Comment>> {
if reviewer_list.trim().is_empty() {
return Ok(Some(Comment::new(
"Error: No reviewer specified. Use r=username to specify a reviewer.".to_string(),
)));
}

for username in reviewer_list.split(',').map(|s| s.trim()) {
if username.is_empty() {
return Ok(Some(Comment::new(
"Error: Empty reviewer name provided. Use r=username to specify a reviewer."
.to_string(),
)));
}

match self.client.users(username).profile().await {
Ok(_) => continue, // user exist continue
Err(octocrab::Error::GitHub { source, .. }) => {
if source.message.contains("Not Found") {
let similar_usernames = self.find_similar_usernames(username).await?;

let mut message = format!("Invalid reviewer username: `{}`", username);
if !similar_usernames.is_empty() {
message.push_str("\nDid you mean one of these users?\n");
for similar in similar_usernames {
message.push_str(&format!("- {}\n", similar));
}
}

return Ok(Some(Comment::new(message)));
}
}
Err(_) => continue,
}
}
Ok(None)
}

/// Searches for GitHub usernames similar to the provided username using Levenshtein distance.
async fn find_similar_usernames(&self, username: &str) -> anyhow::Result<Vec<String>> {
const MAX_DISTANCE: usize = 2;

let search_results = self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use the GitHub search API for this, that could be slow, and unknown GH users are not relevant anyway. Instead, we should try to search for known users in the team database. It will also be easier to test it like this.

But we can also skip the "find similar" functionality in this PR, if you want.

Copy link
Author

@Gmin2 Gmin2 Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use the GitHub search API for this, that could be slow, and unknown GH users are not relevant anyway. Instead, we should try to search for known users in the team database. It will also be easier to test it like this.

so we should use this right ?

pub(crate) async fn load_permissions(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the reviewer list from the team API.

.client
.search()
.users(username)
.send()
.await
.context("Failed to search for similar usernames")?;

let similar_usernames = search_results
.items
.into_iter()
.filter(|user| {
let distance = levenshtein(username, &user.login);
distance > 0 && distance <= MAX_DISTANCE
})
.map(|user| user.login)
.collect();

Ok(similar_usernames)
}

fn format_pr(&self, pr: PullRequestNumber) -> String {
format!("{}/{}", self.repository(), pr)
}
Expand Down