From 6dabebc4e88844b1c91fb3ea3042ea7153be219d Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 12:21:14 -0400 Subject: [PATCH 01/32] disambiguate username --- crates/crates_io_database/src/models/mod.rs | 4 +- .../src/models/oauth_github.rs | 98 +++++++++++++++++++ crates/crates_io_database/src/models/owner.rs | 2 +- crates/crates_io_database/src/models/user.rs | 4 +- src/bin/crates-io/admin/delete_crate.rs | 2 +- src/controllers/krate/owners.rs | 53 +++++++++- 6 files changed, 153 insertions(+), 10 deletions(-) create mode 100644 crates/crates_io_database/src/models/oauth_github.rs diff --git a/crates/crates_io_database/src/models/mod.rs b/crates/crates_io_database/src/models/mod.rs index 49f322858b7..d38ad5f2388 100644 --- a/crates/crates_io_database/src/models/mod.rs +++ b/crates/crates_io_database/src/models/mod.rs @@ -14,11 +14,12 @@ pub use self::email::{Email, NewEmail}; pub use self::follow::Follow; pub use self::keyword::{CrateKeyword, Keyword}; pub use self::krate::{Crate, CrateName, NewCrate}; +pub use self::oauth_github::{NewOauthGithub, OauthGithub}; pub use self::owner::{CrateOwner, Owner, OwnerKind}; pub use self::team::{NewTeam, Team}; pub use self::token::ApiToken; pub use self::trustpub::TrustpubData; -pub use self::user::{NewOauthGithub, NewUser, OauthGithub, PublicUser, User}; +pub use self::user::{NewUser, PublicUser, User}; pub use self::version::{NewVersion, TopVersions, Version}; pub mod helpers; @@ -42,3 +43,4 @@ pub mod trustpub; pub mod user; pub mod version; pub mod versions_published_by; +pub mod oauth_github; diff --git a/crates/crates_io_database/src/models/oauth_github.rs b/crates/crates_io_database/src/models/oauth_github.rs new file mode 100644 index 00000000000..86176636373 --- /dev/null +++ b/crates/crates_io_database/src/models/oauth_github.rs @@ -0,0 +1,98 @@ +use bon::Builder; +use chrono::{DateTime, Utc}; +use diesel::prelude::*; +use diesel::upsert::excluded; +use diesel_async::{AsyncPgConnection, RunQueryDsl}; + +use crate::fns::lower; +use crate::models::User; +use crate::schema::oauth_github; + +/// The model representing a row in the `oauth_github` database table, linked to a user record. +#[derive(Associations, Identifiable, Selectable, Queryable, Debug, Clone)] +#[diesel( + table_name = oauth_github, + check_for_backend(diesel::pg::Pg), + primary_key(account_id), + belongs_to(User), +)] +pub struct OauthGithub { + /// In the process of being migrated from `users.gh_id`. + /// GitHub API docs describe this type as int64. + pub account_id: i64, + /// In the process of being migrated from `users.gh_avatar`. + pub avatar: Option, + /// In the process of being migrated from `users.gh_encrypted_token`. + pub encrypted_token: Vec, + /// The last time we verified with GitHub what the GitHub username for this user was, and + /// whether the account was valid. + pub last_sync: DateTime, + /// In the process of being migrated from `users.gh_login`. + pub login: String, + /// Foreign key to the `users` table. + pub user_id: i32, +} + +impl OauthGithub { + pub async fn find_by_username( + mut conn: &AsyncPgConnection, + login: &str, + ) -> QueryResult { + User::query() + .filter(lower(oauth_github::login).eq(login.to_lowercase())) + .first(&mut conn) + .await + } +} + +/// Represents a new crates.io user to GitHub user OAuth link to be inserted into the +/// `oauth_github` table. +#[derive(Insertable, Debug, Builder)] +#[diesel( + table_name = oauth_github, + check_for_backend(diesel::pg::Pg), + primary_key(account_id), + belongs_to(User), +)] +pub struct NewOauthGithub<'a> { + pub account_id: i64, // corresponds to users.gh_id + pub avatar: Option<&'a str>, // corresponds to users.gh_avatar + pub encrypted_token: &'a [u8], // corresponds to users.gh_encrypted_token + pub login: &'a str, // corresponds to users.gh_login + pub user_id: i32, +} + +impl NewOauthGithub<'_> { + /// Inserts the associated GitHub account info into the database, or updates an existing record. + /// + /// GitHub `account_id` is the primary key of the `oauth_github` table, and comes from GitHub. + /// + /// Each GitHub account ID can only be associated with one crates.io account, so that we know + /// who to log in when we get a GitHub oAuth response. + /// + /// If this function gets an `account_id` conflict, it does not and should not update the + /// `user_id` to that of the currently-logged-in crates.io user's ID because that would mean + /// that GitHub account has already been associated with a different crates.io account. In that + /// case, the currently-logged-in crates.io user should be logged out and the crates.io user + /// already associated with this GitHub user should be logged in. + /// + /// We may eventually implement the ability to associate multiple GitHub accounts with one + /// crates.io account. + /// + /// This function should be called if there is no current user and should update the encrypted + /// token, login, or avatar if those have changed. + pub async fn insert_or_update(&self, mut conn: &AsyncPgConnection) -> QueryResult { + diesel::insert_into(oauth_github::table) + .values(self) + .on_conflict(oauth_github::account_id) + .do_update() + .set(( + oauth_github::encrypted_token.eq(excluded(oauth_github::encrypted_token)), + oauth_github::login.eq(excluded(oauth_github::login)), + oauth_github::avatar.eq(excluded(oauth_github::avatar)), + oauth_github::last_sync.eq(Utc::now()), + )) + .get_result(&mut conn) + .await + } +} diff --git a/crates/crates_io_database/src/models/owner.rs b/crates/crates_io_database/src/models/owner.rs index c95de392df9..3850bcb23b7 100644 --- a/crates/crates_io_database/src/models/owner.rs +++ b/crates/crates_io_database/src/models/owner.rs @@ -104,7 +104,7 @@ impl Owner { pub fn login(&self) -> &str { match self { - Owner::User(user) => &user.gh_login, + Owner::User(user) => &user.username, Owner::Team(team) => &team.login, } } diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 96ef89e447b..eb2e8d3b08e 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -70,9 +70,9 @@ impl User { .await } - pub async fn find_by_login(mut conn: &AsyncPgConnection, login: &str) -> QueryResult { + pub async fn find_by_username(mut conn: &AsyncPgConnection, login: &str) -> QueryResult { User::query() - .filter(lower(users::gh_login).eq(login.to_lowercase())) + .filter(lower(users::username).eq(login.to_lowercase())) .filter(users::gh_id.ne(-1)) .order(users::gh_id.desc()) .first(&mut conn) diff --git a/src/bin/crates-io/admin/delete_crate.rs b/src/bin/crates-io/admin/delete_crate.rs index 6764ad2ef40..f28c26f6427 100644 --- a/src/bin/crates-io/admin/delete_crate.rs +++ b/src/bin/crates-io/admin/delete_crate.rs @@ -59,7 +59,7 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { .await .context("Failed to look up crate name from the database")?; - let deleted_by = User::find_by_login(&conn, &opts.deleted_by) + let deleted_by = User::find_by_username(&conn, &opts.deleted_by) .await .context("Failed to look up `--deleted-by` user from the database")?; diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 5bb999ce115..93113277c9b 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -14,6 +14,7 @@ use crate::{App, app::AppState}; use crate::{auth::AuthCheck, email::EmailMessage}; use axum::Json; use chrono::Utc; +use crates_io_database::models::OauthGithub; use crates_io_encryption::TokenEncryption; use crates_io_github::{GitHubAuth, GitHubClient, GitHubError}; use diesel::prelude::*; @@ -21,6 +22,7 @@ use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl}; use http::StatusCode; use http::request::Parts; use minijinja::context; +use regex::Regex; use secrecy::ExposeSecret; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -333,7 +335,8 @@ async fn add_owner( krate: &Crate, login: &str, ) -> Result { - if login.contains(':') { + // Check if this is a team login (e.g github:org:team). Regex matches strings with exactly two colons. + if Regex::new(r"^[^:]+:[^:]+:[^:]+$").is_ok_and(|r| r.is_match(login)) { let encryption = &app.config.token_encryption; add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await } else { @@ -348,11 +351,51 @@ async fn invite_user_owner( krate: &Crate, login: &str, ) -> Result { - let user = User::find_by_login(conn, login) - .await - .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; + // disambiguated username + if login.contains(':') { + let mut chunks = login.split(':'); + let prefix = chunks.next().unwrap(); + if !vec!["github", "cratesio"].contains(&prefix) { + let error = + "unsupported username prefix, only github and cratesio prefixes are supported"; + return Err(bad_request(error).into()); + } + + let user = if prefix == "github" { + OauthGithub::find_by_username(conn, login) + .await + .optional()? + } else { + User::find_by_username(conn, login).await.optional()? + }; + + let Some(user) = user else { + return Err(bad_request("could not find user with {prefix} username {login}").into()); + }; + send_user_invite(app, conn, req_user, user, krate).await + } else { + let user = User::find_by_username(conn, login) + .await + .optional()? + .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; + + if user.gh_login != user.username { + let error = "error: username {user.username} is possibly ambiguous.\n\nCaused by: \n The crates.io account `{user.username}` is associated with GitHub user `{user.gh_login}`.\n To confirm this is the account you want to add, please run one of the following:\n\n $ cargo owner --add cratesio:{user.username}\n $ cargo owner --add github:{user.gh_login}\n\n If this is not the account you want to add, verify the crates.io username of the account you want."; + return Err(bad_request(error).into()); + } + + send_user_invite(app, conn, req_user, user, krate).await + } +} + +async fn send_user_invite( + app: &App, + conn: &mut AsyncPgConnection, + req_user: &User, + user: User, + krate: &Crate, +) -> Result { // Users are invited and must accept before being added let expires_at = Utc::now() + app.config.ownership_invitations_expiration; let invite = NewCrateOwnerInvitation { From f3ea0da79d6950f19077d19fa397ff543862abbd Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 14:01:56 -0400 Subject: [PATCH 02/32] owner add tests --- crates/crates_io_database/src/models/krate.rs | 2 +- .../crates_io_test_utils/src/builders/user.rs | 19 ++- src/controllers/krate/owners.rs | 48 ++++--- src/tests/routes/crates/owners/add.rs | 132 +++++++++++++++++- src/tests/team.rs | 4 +- src/tests/util/test_app.rs | 36 +++++ 6 files changed, 218 insertions(+), 23 deletions(-) diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index f18cf47a1e1..ef97bc6ce7d 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -265,7 +265,7 @@ impl Crate { pub enum NewOwnerInvite { /// The invitee was a [`User`], and they must accept the invite through the /// UI or via the provided invite token. - User(User, SecretString), + User(User, SecretString, String), /// The invitee was a [`Team`], and they were immediately added as an owner. Team(Team), diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index c24f5550802..d5c6709b75e 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -23,6 +23,7 @@ static ENCRYPTED_TOKEN: LazyLock> = LazyLock::new(|| { pub struct UserBuilder<'a> { username: &'a str, display_name: Option<&'a str>, + gh_login: &'a str, } impl<'a> UserBuilder<'a> { @@ -32,11 +33,12 @@ impl<'a> UserBuilder<'a> { Self { username: "octocat", display_name: None, + gh_login: "octocat", } } pub fn with_username(self, username: &'a str) -> Self { - Self { username, ..self } + Self { username, gh_login: username, ..self } } pub fn with_display_name(self, display_name: &'a str) -> Self { @@ -46,10 +48,16 @@ impl<'a> UserBuilder<'a> { } } + pub fn with_gh_username(self, gh_login: &'a str) -> Self { + Self { + gh_login: gh_login, + ..self + } + } + pub fn build(self) -> User { User { id: 1, - gh_login: self.username.into(), name: self.display_name.map(ToString::to_string), gh_id: 123, gh_avatar: None, @@ -59,6 +67,7 @@ impl<'a> UserBuilder<'a> { is_admin: false, publish_notifications: true, username: self.username.into(), + gh_login: self.gh_login.into(), created_at: None, } } @@ -66,7 +75,7 @@ impl<'a> UserBuilder<'a> { pub fn new_user(self) -> NewUser<'a> { NewUser::builder() .gh_id(next_gh_id()) - .gh_login(self.username) + .gh_login(self.gh_login) .username(self.username) .maybe_name(self.display_name) .build() @@ -106,6 +115,10 @@ impl<'a> OauthGithubBuilder<'a> { } } + pub fn with_login(self, login: &'a str) -> Self { + Self { login, ..self } + } + pub async fn insert(self, mut conn: &AsyncPgConnection) { diesel::insert_into(oauth_github::table) .values(( diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 93113277c9b..49889829618 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -250,10 +250,10 @@ async fn modify_owners( // the invite, and a best-effort attempt should be made // to email them the invite token for one-click // acceptance. - Ok(NewOwnerInvite::User(invitee, token)) => { + Ok(NewOwnerInvite::User(invitee, token, username)) => { msgs.push(format!( "user {} has been invited to be an owner of crate {}", - invitee.gh_login, krate.name, + username, krate.name, )); if let Some(recipient) = @@ -262,7 +262,7 @@ async fn modify_owners( let email = EmailMessage::from_template( "owner_invite", context! { - inviter => user.gh_login, + inviter => user.username, domain => app.emails.domain, crate_name => krate.name, token => token.expose_secret() @@ -288,7 +288,7 @@ async fn modify_owners( // This user has a pending invite. Err(OwnerAddError::AlreadyInvited(user)) => msgs.push(format!( "user {} already has a pending invitation to be an owner of crate {}", - user.gh_login, krate.name + user.username, krate.name )), // An opaque error occurred. @@ -361,19 +361,25 @@ async fn invite_user_owner( return Err(bad_request(error).into()); } + let username = chunks.next().unwrap(); + let user = if prefix == "github" { - OauthGithub::find_by_username(conn, login) + OauthGithub::find_by_username(conn, username) .await .optional()? + .ok_or_else(|| bad_request(format_args!("could not find user with github username {username}. If you meant to add a github team, format is github:org:team")))? } else { - User::find_by_username(conn, login).await.optional()? - }; - - let Some(user) = user else { - return Err(bad_request("could not find user with {prefix} username {login}").into()); + User::find_by_username(conn, username) + .await + .optional()? + .ok_or_else(|| { + bad_request(format_args!( + "could not find user with cratesio username {username}." + )) + })? }; - send_user_invite(app, conn, req_user, user, krate).await + send_user_invite(app, conn, req_user, user, username, krate).await } else { let user = User::find_by_username(conn, login) .await @@ -381,11 +387,20 @@ async fn invite_user_owner( .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; if user.gh_login != user.username { - let error = "error: username {user.username} is possibly ambiguous.\n\nCaused by: \n The crates.io account `{user.username}` is associated with GitHub user `{user.gh_login}`.\n To confirm this is the account you want to add, please run one of the following:\n\n $ cargo owner --add cratesio:{user.username}\n $ cargo owner --add github:{user.gh_login}\n\n If this is not the account you want to add, verify the crates.io username of the account you want."; + let error = format!( + "error: username {} is possibly ambiguous.\n\n\ + Caused by: \n \ + The crates.io account `{}` is associated with GitHub user `{}`.\n \ + To confirm this is the account you want to add, please run one of the following:\n\n \ + $ cargo owner --add cratesio:{}\n \ + $ cargo owner --add github:{}\n\n \ + If this is not the account you want to add, verify the crates.io username of the account you want.", + user.username, user.username, user.gh_login, user.username, user.gh_login + ); return Err(bad_request(error).into()); } - send_user_invite(app, conn, req_user, user, krate).await + send_user_invite(app, conn, req_user, user, login, krate).await } } @@ -394,6 +409,7 @@ async fn send_user_invite( conn: &mut AsyncPgConnection, req_user: &User, user: User, + username: &str, krate: &Crate, ) -> Result { // Users are invited and must accept before being added @@ -406,9 +422,9 @@ async fn send_user_invite( }; match invite.create(conn).await? { - NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => { - Ok(NewOwnerInvite::User(user, plaintext_token)) - } + NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => Ok( + NewOwnerInvite::User(user, plaintext_token, username.to_owned()), + ), NewCrateOwnerInvitationOutcome::AlreadyExists => { Err(OwnerAddError::AlreadyInvited(Box::new(user))) } diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 5905825a2e5..0403247bb51 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -1,4 +1,4 @@ -use crate::builders::CrateBuilder; +use crate::builders::{CrateBuilder, OauthGithubBuilder}; use crate::owners::expire_invitation; use crate::util::{RequestHelper, TestApp}; use crates_io::models::token::{CrateScope, EndpointScope}; @@ -383,3 +383,133 @@ async fn no_invite_emails_for_txn_rollback() { // 9 emails to the good invitees should have been sent. assert_eq!(app.emails().await.len(), 9); } + +#[tokio::test(flavor = "multi_thread")] +async fn test_unsupported_disambiguation_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + app.db_new_user("cilantro").await; + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .add_named_owner("guacamole", "gitlab:cilantro") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unsupported username prefix, only github and cratesio prefixes are supported"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguated_github_username_not_found() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .add_named_owner("guacamole", "github:nonexistent") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username nonexistent"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguated_cratesio_username_not_found() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .add_named_owner("guacamole", "cratesio:nonexistent") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with cratesio username nonexistent"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_ambiguous_username_error() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + app.db_new_user_with_gh_login("cilantro", "cilantro-gh") + .await; + + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("guacamole", "cilantro").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"error: username cilantro is possibly ambiguous.\n\nCaused by: \n The crates.io account `cilantro` is associated with GitHub user `cilantro-gh`.\n To confirm this is the account you want to add, please run one of the following:\n\n $ cargo owner --add cratesio:cilantro\n $ cargo owner --add github:cilantro-gh\n\n If this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguate_with_github_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let new_user = app + .db_new_user_with_gh_login("cilantro", "cilantro-gh") + .await; + + // Create oauth_github entry with the GitHub login + OauthGithubBuilder::for_user(new_user.as_model()) + .with_login("cilantro-gh") + .insert(&mut conn) + .await; + + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // Using github: prefix should resolve the ambiguity and invite the user + let response = cookie + .add_named_owner("guacamole", "github:cilantro-gh") + .await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user cilantro-gh has been invited to be an owner of crate guacamole","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguate_with_cratesio_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + app.db_new_user_with_gh_login("cilantro", "cilantro-gh") + .await; + + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // Using cratesio: prefix should resolve the ambiguity and invite the user + let response = cookie + .add_named_owner("guacamole", "cratesio:cilantro") + .await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user cilantro has been invited to be an owner of crate guacamole","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_already_owner_error() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("guacamole", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // The cookie user is already the owner of the crate + let response = cookie + .add_named_owner("guacamole", &cookie.as_model().gh_login) + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`foo` is already an owner"}]}"#); +} diff --git a/src/tests/team.rs b/src/tests/team.rs index 33fac9dd686..4c9403d2dd8 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -51,7 +51,7 @@ async fn weird_name() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like /"}]}"#); } -/// Tests adding team without second `:` +/// Tests that `github:foo` is treated as a disambiguated username lookup. #[tokio::test(flavor = "multi_thread")] async fn one_colon() { let (app, _, user, token) = TestApp::init().with_token().await; @@ -63,7 +63,7 @@ async fn one_colon() { let response = token.add_named_owner("foo_one_colon", "github:foo").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"missing github team argument; format is github:org:team"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username foo. If you meant to add a github team, format is github:org:team"}]}"#); } #[tokio::test(flavor = "multi_thread")] diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index ab0c147e71f..0db0209c1a3 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -175,6 +175,42 @@ impl TestApp { } } + /// Create a new user with a different GitHub login and a verified email + /// address in the database (`@example.com`) and return a mock + /// user session. + /// + /// This method updates the database directly. + pub async fn db_new_user_with_gh_login( + &self, + username: &str, + gh_login: &str, + ) -> MockCookieUser { + let conn = self.db_conn().await; + + let email = format!("{username}@example.com"); + + let new_user = crate::builders::UserBuilder::new() + .with_username(username) + .with_gh_username(gh_login) + .new_user(); + let id = new_user.insert(&conn).await.unwrap(); + + let new_email = NewEmail::builder() + .user_id(id) + .email(&email) + .verified(true) + .build(); + + new_email.insert(&conn).await.unwrap(); + + let user = User::find(&conn, id).await.unwrap(); + + MockCookieUser { + app: self.clone(), + user, + } + } + /// Obtains a reference to the upstream repository ("the index") pub fn upstream_index(&self) -> &UpstreamIndex { assert_some!(self.0.index.as_ref()) From 8be3e00e88c7e70f99582ba6d76b2d3f675cfd7b Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 15:20:09 -0400 Subject: [PATCH 03/32] remove owner disambiguation --- crates/crates_io_database/src/models/krate.rs | 44 +++++++++++++++- src/controllers/krate/owners.rs | 50 ++++++++++++++++++- 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index ef97bc6ce7d..344e5ca8cd0 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -213,7 +213,7 @@ impl Crate { Ok(users.chain(teams).collect()) } - pub async fn owner_remove( + pub async fn owner_remove_with_username( &self, mut conn: &AsyncPgConnection, login: &str, @@ -225,7 +225,7 @@ impl Crate { CASE WHEN crate_owners.owner_kind = 1 THEN teams.login ELSE - users.gh_login + users.username END AS login FROM crate_owners LEFT JOIN teams @@ -256,6 +256,44 @@ impl Crate { return Err(OwnerRemoveError::not_found(login)); } + Ok(()) + } + pub async fn owner_remove_with_gh_username( + &self, + mut conn: &AsyncPgConnection, + username: &str, + ) -> Result<(), OwnerRemoveError> { + let query = diesel::sql_query( + r#"WITH crate_owners_with_gh_login AS ( + SELECT + crate_owners.*, + login + FROM crate_owners + JOIN oauth_github + ON crate_owners.owner_id = oauth_github.user_id + AND crate_owners.owner_kind = 0 + WHERE crate_owners.crate_id = $1 + AND crate_owners.deleted = false + ) + UPDATE crate_owners + SET deleted = true + FROM crate_owners_with_gh_login + WHERE crate_owners.crate_id = crate_owners_with_gh_login.crate_id + AND crate_owners.owner_id = crate_owners_with_gh_login.owner_id + AND crate_owners.owner_kind = crate_owners_with_gh_login.owner_kind + AND lower(crate_owners_with_gh_login.login) = lower($2);"#, + ); + + let num_updated_rows = query + .bind::(self.id) + .bind::(username) + .execute(&mut conn) + .await?; + + if num_updated_rows == 0 { + return Err(OwnerRemoveError::not_found(username)); + } + Ok(()) } } @@ -277,6 +315,8 @@ pub enum OwnerRemoveError { Diesel(#[from] diesel::result::Error), #[error("Could not find owner with login `{login}`")] NotFound { login: String }, + #[error("{0}")] + AppError(String), } impl OwnerRemoveError { diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 49889829618..1d877ea4c9b 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -299,7 +299,7 @@ async fn modify_owners( msgs.join(",") } else { for login in &logins { - krate.owner_remove(conn, login).await?; + remove_owner(&krate, conn, login).await? } if User::owning(&krate, conn).await?.is_empty() { return Err(bad_request( @@ -344,6 +344,53 @@ async fn add_owner( } } +async fn remove_owner( + krate: &Crate, + conn: &mut AsyncPgConnection, + login: &str, +) -> Result<(), OwnerRemoveError> { + // Check if this is a team login (e.g github:org:team). Regex matches strings with exactly two colons. + if Regex::new(r"^[^:]+:[^:]+:[^:]+$").is_ok_and(|r| r.is_match(login)) { + krate.owner_remove_with_username(conn, login).await + } else if login.contains(':') { + let mut chunks = login.split(':'); + let prefix = chunks.next().unwrap(); + if !["github", "cratesio"].contains(&prefix) { + let error = + "unsupported username prefix, only github and cratesio prefixes are supported"; + return Err(OwnerRemoveError::AppError(error.to_string())); + } + + let username = chunks.next().unwrap(); + + if prefix == "github" { + krate.owner_remove_with_gh_username(conn, username).await + } else { + krate.owner_remove_with_username(conn, username).await + } + } else { + let user = User::find_by_username(conn, login) + .await + .optional()? + .ok_or_else(|| OwnerRemoveError::not_found(login))?; + + if user.gh_login != user.username { + let error = format!( + "error: username {} is possibly ambiguous.\n\n\ + Caused by: \n \ + The crates.io account `{}` is associated with GitHub user `{}`.\n \ + To confirm this is the account you want to remove, please run one of the following:\n\n \ + $ cargo owner --remove cratesio:{}\n \ + $ cargo owner --remove github:{}\n\n \ + If this is not the account you want to remove, verify the crates.io username of the account you want.", + user.username, user.username, user.gh_login, user.username, user.gh_login + ); + return Err(OwnerRemoveError::AppError(error)); + } + krate.owner_remove_with_username(conn, login).await + } +} + async fn invite_user_owner( app: &App, conn: &mut AsyncPgConnection, @@ -604,6 +651,7 @@ impl From for BoxedAppError { OwnerRemoveError::NotFound { login } => { bad_request(format!("could not find owner with login `{login}`")) } + OwnerRemoveError::AppError(error) => bad_request(error), } } } From d6a9e225f4c789e88d90aaed7cbe44865a384850 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 15:57:57 -0400 Subject: [PATCH 04/32] remove owner tests --- src/tests/owners.rs | 2 +- src/tests/routes/crates/list.rs | 2 +- src/tests/routes/crates/owners/remove.rs | 144 ++++++++++++++++++++++- src/tests/routes/me/get.rs | 2 +- src/tests/routes/users/stats.rs | 2 +- src/tests/team.rs | 2 +- 6 files changed, 148 insertions(+), 6 deletions(-) diff --git a/src/tests/owners.rs b/src/tests/owners.rs index b3f42d86c7e..e008dd4e2cd 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -393,7 +393,7 @@ async fn deleted_ownership_isnt_in_owner_user() { let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove(&conn, &user.gh_login).await.unwrap(); + krate.owner_remove_with_username(&conn, &user.gh_login).await.unwrap(); let json: UserResponse = anon .get("/api/v1/crates/foo_my_packages/owner_user") diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index cca52c9cb93..ea85c225b54 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -1370,7 +1370,7 @@ async fn crates_by_user_id_not_including_deleted_owners() -> anyhow::Result<()> let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove(&conn, "foo").await.unwrap(); + krate.owner_remove_with_username(&conn, "foo").await.unwrap(); for response in search_both_by_user_id(&anon, user.id).await { assert_eq!(response.crates.len(), 0); diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index 066a7bd7520..0dc3dd72805 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -1,5 +1,6 @@ -use crate::builders::CrateBuilder; +use crate::builders::{CrateBuilder, OauthGithubBuilder}; use crate::util::{RequestHelper, TestApp}; +use crate::{add_team_to_crate, new_team}; use crates_io::models::CrateOwner; use crates_io_github::{GitHubOrganization, GitHubTeam, GitHubTeamMembership, MockGitHubClient}; use insta::assert_snapshot; @@ -158,3 +159,144 @@ async fn test_remove_uppercase_team() { assert_snapshot!(response.status(), @"200 OK"); assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); } + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_ambiguous_user() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app + .db_new_user_with_gh_login("user2", "user2-gh") + .await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "user2").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"error: username user2 is possibly ambiguous.\n\nCaused by: \n The crates.io account `user2` is associated with GitHub user `user2-gh`.\n To confirm this is the account you want to remove, please run one of the following:\n\n $ cargo owner --remove cratesio:user2\n $ cargo owner --remove github:user2-gh\n\n If this is not the account you want to remove, verify the crates.io username of the account you want."}]}"#); +} + +/// Test that an unsupported prefix (e.g. gitlab:) returns an error. +#[tokio::test(flavor = "multi_thread")] +async fn test_unsupported_disambiguation_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .remove_named_owner("foo", "gitlab:user2") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unsupported username prefix, only github and cratesio prefixes are supported"}]}"#); +} + +/// Test that removing with nonexistent github username returns an error. +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguated_github_username_not_found() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .remove_named_owner("foo", "github:nonexistent") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `nonexistent`"}]}"#); +} + +/// Test that removing with nonexistent cratesio usrname returns an error. +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguated_cratesio_username_not_found() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .remove_named_owner("foo", "cratesio:nonexistent") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `nonexistent`"}]}"#); +} + + +/// Test that removing an ambiguous user with github: prefix works. +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguate_remove_with_github_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app + .db_new_user_with_gh_login("user2", "user2-gh") + .await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2-gh") + .insert(&mut conn) + .await; + + let response = cookie + .remove_named_owner("foo", "github:user2-gh") + .await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +/// Test that removing an ambiguous user with cratesio: prefix works . +#[tokio::test(flavor = "multi_thread")] +async fn test_disambiguate_remove_with_cratesio_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app + .db_new_user_with_gh_login("user2", "user2-gh") + .await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie + .remove_named_owner("foo", "cratesio:user2") + .await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} diff --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index 83cc6c3db9a..320656e3f69 100644 --- a/src/tests/routes/me/get.rs +++ b/src/tests/routes/me/get.rs @@ -53,7 +53,7 @@ async fn test_user_owned_crates_doesnt_include_deleted_ownership() { .expect_build(&mut conn) .await; krate - .owner_remove(&conn, &user_model.gh_login) + .owner_remove_with_username(&conn, &user_model.gh_login) .await .unwrap(); diff --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index d29bf6a4af0..6f633ec1183 100644 --- a/src/tests/routes/users/stats.rs +++ b/src/tests/routes/users/stats.rs @@ -53,7 +53,7 @@ async fn user_total_downloads() -> anyhow::Result<()> { .execute(&mut conn) .await?; no_longer_my_krate - .owner_remove(&conn, &user.gh_login) + .owner_remove_with_username(&conn, &user.gh_login) .await .unwrap(); diff --git a/src/tests/team.rs b/src/tests/team.rs index 4c9403d2dd8..6ca0692e8ca 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -493,7 +493,7 @@ async fn crates_by_team_id_not_including_deleted_owners() -> anyhow::Result<()> .expect_build(&mut conn) .await; add_team_to_crate(&t, &krate, user.id, &mut conn).await?; - krate.owner_remove(&conn, &t.login).await.unwrap(); + krate.owner_remove_with_username(&conn, &t.login).await.unwrap(); let json = anon.search(&format!("team_id={}", t.id)).await; assert_eq!(json.crates.len(), 0); From ce2762c07921f2da1adac8cf0dfb3d37e3e404ce Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 16:11:10 -0400 Subject: [PATCH 05/32] update all relevant uses of gh_login to username --- src/bin/crates-io/admin/verify_token.rs | 2 +- src/controllers/krate/delete.rs | 2 +- src/controllers/krate/update.rs | 4 ++-- src/controllers/token.rs | 4 ++-- src/controllers/trustpub/github_configs/create.rs | 2 +- src/controllers/user/update.rs | 4 ++-- src/controllers/version/update.rs | 2 +- src/tests/owners.rs | 2 +- src/tests/routes/crates/admin.rs | 2 +- src/tests/routes/users/stats.rs | 2 +- src/worker/jobs/expiry_notification.rs | 2 +- 11 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/bin/crates-io/admin/verify_token.rs b/src/bin/crates-io/admin/verify_token.rs index 1daee68af16..43b4b027bee 100644 --- a/src/bin/crates-io/admin/verify_token.rs +++ b/src/bin/crates-io/admin/verify_token.rs @@ -23,6 +23,6 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { let token = HashedToken::parse(&opts.api_token)?; let token = ApiToken::find_by_api_token(&mut conn, &token).await?; let user = User::find(&conn, token.user_id).await?; - println!("The token belongs to user {}", user.gh_login); + println!("The token belongs to user {}", user.username); Ok(()) } diff --git a/src/controllers/krate/delete.rs b/src/controllers/krate/delete.rs index 4ba86087952..3839dae54ec 100644 --- a/src/controllers/krate/delete.rs +++ b/src/controllers/krate/delete.rs @@ -160,7 +160,7 @@ pub async fn delete_crate( let email = EmailMessage::from_template( "crate_deletion", context! { - user => user.gh_login, + user => user.username, krate => crate_name }, )?; diff --git a/src/controllers/krate/update.rs b/src/controllers/krate/update.rs index b59d2f1a317..09a7e7e8458 100644 --- a/src/controllers/krate/update.rs +++ b/src/controllers/krate/update.rs @@ -121,9 +121,9 @@ async fn update_inner( krate.name = %krate.name, network.client.ip = %**real_ip, usr.id = user.id, - usr.name = %user.gh_login, + usr.name = %user.username, "User {} set trustpub_only={trustpub_only} for crate {}", - user.gh_login, + user.username, krate.name ); diff --git a/src/controllers/token.rs b/src/controllers/token.rs index a7912d0bf92..61112690631 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -154,7 +154,7 @@ pub async fn create_api_token( network.client.ip = client_ip, http.headers = ?headers, "Blocked token creation for user `{}` (id: {}) due to disabled flag (token name: `{}`)", - user.gh_login, user.id, new.api_token.name + user.username, user.id, new.api_token.name ); let message = disable_message.clone(); @@ -212,7 +212,7 @@ pub async fn create_api_token( if let Some(recipient) = recipient { let context = context! { token_name => &new.api_token.name, - user_name => &user.gh_login, + user_name => &user.username, domain => app.emails.domain, }; diff --git a/src/controllers/trustpub/github_configs/create.rs b/src/controllers/trustpub/github_configs/create.rs index c8b7b081366..929a68d6274 100644 --- a/src/controllers/trustpub/github_configs/create.rs +++ b/src/controllers/trustpub/github_configs/create.rs @@ -96,7 +96,7 @@ pub async fn create_trustpub_github_config( )); }; let gh_auth = encryption.decrypt(gh_auth).map_err(|err| { - let login = &auth_user.gh_login; + let login = &auth_user.username; warn!("Failed to decrypt GitHub token for user {login}: {err}"); server_error("Internal server error") })?; diff --git a/src/controllers/user/update.rs b/src/controllers/user/update.rs index 9a197d14ce0..a150e0e955c 100644 --- a/src/controllers/user/update.rs +++ b/src/controllers/user/update.rs @@ -78,7 +78,7 @@ pub async fn update_user( let email = EmailMessage::from_template( "unsubscribe_notifications", context! { - user_name => user.gh_login, + user_name => user.username, domain => state.emails.domain }, ); @@ -123,7 +123,7 @@ pub async fn update_user( let email = EmailMessage::from_template( "user_confirm", context! { - user_name => user.gh_login, + user_name => user.username, domain => state.emails.domain, token => token.expose_secret() }, diff --git a/src/controllers/version/update.rs b/src/controllers/version/update.rs index 05c21321c2a..86c5121cf44 100644 --- a/src/controllers/version/update.rs +++ b/src/controllers/version/update.rs @@ -132,7 +132,7 @@ pub async fn perform_version_yank_update( let action = if yanked { "yanking" } else { "unyanking" }; warn!( "Admin {} is {action} {}@{}", - user.gh_login, krate.name, version.num + user.username, krate.name, version.num ); } else { return Err(custom( diff --git a/src/tests/owners.rs b/src/tests/owners.rs index e008dd4e2cd..866ac603ffa 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -393,7 +393,7 @@ async fn deleted_ownership_isnt_in_owner_user() { let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove_with_username(&conn, &user.gh_login).await.unwrap(); + krate.owner_remove_with_username(&conn, &user.username).await.unwrap(); let json: UserResponse = anon .get("/api/v1/crates/foo_my_packages/owner_user") diff --git a/src/tests/routes/crates/admin.rs b/src/tests/routes/crates/admin.rs index 8115c633d61..44f4d62f9ff 100644 --- a/src/tests/routes/crates/admin.rs +++ b/src/tests/routes/crates/admin.rs @@ -62,7 +62,7 @@ async fn index_include_yanked() -> anyhow::Result<()> { .await; // Include fully yanked (all versions were yanked) crates - let username = &user.gh_login; + let username = &user.username; let response = admin.admin_list::<()>(username).await; assert_json_snapshot!(response.json(), { diff --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index 6f633ec1183..c20a63f180a 100644 --- a/src/tests/routes/users/stats.rs +++ b/src/tests/routes/users/stats.rs @@ -53,7 +53,7 @@ async fn user_total_downloads() -> anyhow::Result<()> { .execute(&mut conn) .await?; no_longer_my_krate - .owner_remove_with_username(&conn, &user.gh_login) + .owner_remove_with_username(&conn, &user.username) .await .unwrap(); diff --git a/src/worker/jobs/expiry_notification.rs b/src/worker/jobs/expiry_notification.rs index d8d6f2eb6d4..623630dc497 100644 --- a/src/worker/jobs/expiry_notification.rs +++ b/src/worker/jobs/expiry_notification.rs @@ -89,7 +89,7 @@ async fn handle_expiring_token( let email = EmailMessage::from_template( "expiry_notification", context! { - name => user.gh_login, + name => user.username, token_id => token.id, token_name => token.name, expiry_date => token.expired_at.unwrap().to_rfc3339_opts(SecondsFormat::Secs, true) From 5928cdafa1c4af3c4290900977ab52e4616d76ee Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 16:14:24 -0400 Subject: [PATCH 06/32] snapshot fix --- src/controllers/krate/owners.rs | 2 +- src/tests/routes/crates/owners/add.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 1d877ea4c9b..68f42348804 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -421,7 +421,7 @@ async fn invite_user_owner( .optional()? .ok_or_else(|| { bad_request(format_args!( - "could not find user with cratesio username {username}." + "could not find user with cratesio username {username}" )) })? }; diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 0403247bb51..2ac02df1e76 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -414,7 +414,7 @@ async fn test_disambiguated_github_username_not_found() { .add_named_owner("guacamole", "github:nonexistent") .await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username nonexistent"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username nonexistent. If you meant to add a github team, format is github:org:team"}]}"#); } #[tokio::test(flavor = "multi_thread")] From 5870c0c4234455d0c1b01cc0b17162dbfc3c2e36 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 16:48:25 -0400 Subject: [PATCH 07/32] DRY disambiguate implementation --- src/controllers/krate/owners.rs | 155 +++++++++++------------ src/tests/routes/crates/owners/remove.rs | 3 +- 2 files changed, 76 insertions(+), 82 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 68f42348804..0569315f64f 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -326,128 +326,115 @@ async fn modify_owners( Ok(Json(ModifyResponse { msg, ok: true })) } -/// Invites `login` as an owner of this crate, returning the created -/// [`NewOwnerInvite`]. -async fn add_owner( - app: &App, - conn: &mut AsyncPgConnection, - req_user: &User, - krate: &Crate, - login: &str, -) -> Result { - // Check if this is a team login (e.g github:org:team). Regex matches strings with exactly two colons. - if Regex::new(r"^[^:]+:[^:]+:[^:]+$").is_ok_and(|r| r.is_match(login)) { - let encryption = &app.config.token_encryption; - add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await - } else { - invite_user_owner(app, conn, req_user, krate, login).await - } +enum Login<'a> { + /// A team login e.g `github:org:team` + Team, + /// A disambiguated `github:username` resolved via oauth_github. + GitHub(&'a str), + /// A disambiguated `cratesio:username` resolved via users table. + CratesIo(&'a str), + /// A username that was unambiguous (username == gh_login). + Username(User), } -async fn remove_owner( - krate: &Crate, +/// Disambiguate a login string +async fn disambiguate_login<'a>( conn: &mut AsyncPgConnection, - login: &str, -) -> Result<(), OwnerRemoveError> { - // Check if this is a team login (e.g github:org:team). Regex matches strings with exactly two colons. + login: &'a str, + add: bool, +) -> Result, BoxedAppError> { + // Team login: exactly two colons (e.g. github:org:team) if Regex::new(r"^[^:]+:[^:]+:[^:]+$").is_ok_and(|r| r.is_match(login)) { - krate.owner_remove_with_username(conn, login).await + return Ok(Login::Team); } else if login.contains(':') { + // disambiguate user login let mut chunks = login.split(':'); let prefix = chunks.next().unwrap(); if !["github", "cratesio"].contains(&prefix) { - let error = - "unsupported username prefix, only github and cratesio prefixes are supported"; - return Err(OwnerRemoveError::AppError(error.to_string())); + return Err(bad_request( + "unsupported username prefix, only github and cratesio prefixes are supported", + )); } let username = chunks.next().unwrap(); - - if prefix == "github" { - krate.owner_remove_with_gh_username(conn, username).await - } else { - krate.owner_remove_with_username(conn, username).await - } + return match prefix { + "github" => Ok(Login::GitHub(username)), + _ => Ok(Login::CratesIo(username)), + }; } else { + // check if login is ambiguous let user = User::find_by_username(conn, login) .await .optional()? - .ok_or_else(|| OwnerRemoveError::not_found(login))?; + .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; + let command = if add { "add" } else { "remove" }; if user.gh_login != user.username { let error = format!( "error: username {} is possibly ambiguous.\n\n\ - Caused by: \n \ - The crates.io account `{}` is associated with GitHub user `{}`.\n \ - To confirm this is the account you want to remove, please run one of the following:\n\n \ - $ cargo owner --remove cratesio:{}\n \ - $ cargo owner --remove github:{}\n\n \ - If this is not the account you want to remove, verify the crates.io username of the account you want.", + Caused by: \n \ + The crates.io account `{}` is associated with GitHub user `{}`.\n \ + To confirm this is the account you want to {command}, please run one of the following:\n\n \ + $ cargo owner --{command} cratesio:{}\n \ + $ cargo owner --{command} github:{}\n\n \ + If this is not the account you want to {command}, verify the crates.io username of the account you want.", user.username, user.username, user.gh_login, user.username, user.gh_login ); - return Err(OwnerRemoveError::AppError(error)); + return Err(bad_request(error)); } - krate.owner_remove_with_username(conn, login).await + Ok(Login::Username(user)) } } -async fn invite_user_owner( +/// Invites `login` as an owner of this crate, returning the created +/// [`NewOwnerInvite`]. +async fn add_owner( app: &App, conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, login: &str, ) -> Result { - // disambiguated username - if login.contains(':') { - let mut chunks = login.split(':'); - let prefix = chunks.next().unwrap(); - if !vec!["github", "cratesio"].contains(&prefix) { - let error = - "unsupported username prefix, only github and cratesio prefixes are supported"; - return Err(bad_request(error).into()); + match disambiguate_login(conn, login, true).await? { + Login::Team => { + let encryption = &app.config.token_encryption; + add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await } - - let username = chunks.next().unwrap(); - - let user = if prefix == "github" { - OauthGithub::find_by_username(conn, username) + Login::GitHub(username) => { + let user = OauthGithub::find_by_username(conn, username) .await .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with github username {username}. If you meant to add a github team, format is github:org:team")))? - } else { - User::find_by_username(conn, username) + .ok_or_else(|| bad_request(format_args!("could not find user with github username {username}. If you meant to add a github team, format is github:org:team")))?; + send_user_invite(app, conn, req_user, user, username, krate).await + } + Login::CratesIo(username) => { + let user = User::find_by_username(conn, username) .await .optional()? .ok_or_else(|| { bad_request(format_args!( "could not find user with cratesio username {username}" )) - })? - }; - - send_user_invite(app, conn, req_user, user, username, krate).await - } else { - let user = User::find_by_username(conn, login) - .await - .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; - - if user.gh_login != user.username { - let error = format!( - "error: username {} is possibly ambiguous.\n\n\ - Caused by: \n \ - The crates.io account `{}` is associated with GitHub user `{}`.\n \ - To confirm this is the account you want to add, please run one of the following:\n\n \ - $ cargo owner --add cratesio:{}\n \ - $ cargo owner --add github:{}\n\n \ - If this is not the account you want to add, verify the crates.io username of the account you want.", - user.username, user.username, user.gh_login, user.username, user.gh_login - ); - return Err(bad_request(error).into()); + })?; + send_user_invite(app, conn, req_user, user, username, krate).await } + Login::Username(user) => send_user_invite(app, conn, req_user, user, login, krate).await, + } +} - send_user_invite(app, conn, req_user, user, login, krate).await +async fn remove_owner( + krate: &Crate, + conn: &mut AsyncPgConnection, + login: &str, +) -> Result<(), OwnerRemoveError> { + match disambiguate_login(conn, login, false) + .await + .map_err(OwnerRemoveError::from)? + { + Login::Team => krate.owner_remove_with_username(conn, login).await, + Login::GitHub(username) => krate.owner_remove_with_gh_username(conn, username).await, + Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await, + Login::Username(_) => krate.owner_remove_with_username(conn, login).await, } } @@ -644,6 +631,14 @@ impl From for OwnerAddError { } } +/// A [`BoxedAppError`] does not impl [`std::error::Error`] so it needs a manual +/// [`From`] impl. +impl From for OwnerRemoveError { + fn from(value: BoxedAppError) -> Self { + Self::AppError(value.to_string()) + } +} + impl From for BoxedAppError { fn from(error: OwnerRemoveError) -> Self { match error { diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index 0dc3dd72805..9ee2ef79a2f 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -1,6 +1,5 @@ use crate::builders::{CrateBuilder, OauthGithubBuilder}; use crate::util::{RequestHelper, TestApp}; -use crate::{add_team_to_crate, new_team}; use crates_io::models::CrateOwner; use crates_io_github::{GitHubOrganization, GitHubTeam, GitHubTeamMembership, MockGitHubClient}; use insta::assert_snapshot; @@ -61,7 +60,7 @@ async fn test_unknown_user() { let response = cookie.remove_named_owner("foo", "unknown").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `unknown`"}]}"#); } #[tokio::test(flavor = "multi_thread")] From ede90c5f72279a0ddeb85dddad49ef976e09bc26 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 17:37:44 -0400 Subject: [PATCH 08/32] fix formatting, minor updates --- crates/crates_io_database/src/models/krate.rs | 3 + crates/crates_io_database/src/models/mod.rs | 2 +- .../src/models/oauth_github.rs | 5 +- .../crates_io_test_utils/src/builders/user.rs | 4 +- src/controllers/krate/owners.rs | 75 ++++++++++--------- src/tests/owners.rs | 5 +- src/tests/routes/crates/list.rs | 5 +- src/tests/routes/crates/owners/add.rs | 66 +++++++--------- src/tests/routes/crates/owners/remove.rs | 29 ++----- src/tests/team.rs | 5 +- 10 files changed, 90 insertions(+), 109 deletions(-) diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index 344e5ca8cd0..0efb4128a4b 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -213,6 +213,7 @@ impl Crate { Ok(users.chain(teams).collect()) } + /// Remove owner given a cratesio username pub async fn owner_remove_with_username( &self, mut conn: &AsyncPgConnection, @@ -258,6 +259,8 @@ impl Crate { Ok(()) } + + /// Remove owner given a github username pub async fn owner_remove_with_gh_username( &self, mut conn: &AsyncPgConnection, diff --git a/crates/crates_io_database/src/models/mod.rs b/crates/crates_io_database/src/models/mod.rs index d38ad5f2388..c64f5e0194e 100644 --- a/crates/crates_io_database/src/models/mod.rs +++ b/crates/crates_io_database/src/models/mod.rs @@ -36,6 +36,7 @@ mod email; mod follow; mod keyword; pub mod krate; +pub mod oauth_github; mod owner; pub mod team; pub mod token; @@ -43,4 +44,3 @@ pub mod trustpub; pub mod user; pub mod version; pub mod versions_published_by; -pub mod oauth_github; diff --git a/crates/crates_io_database/src/models/oauth_github.rs b/crates/crates_io_database/src/models/oauth_github.rs index 86176636373..e96e043c5f5 100644 --- a/crates/crates_io_database/src/models/oauth_github.rs +++ b/crates/crates_io_database/src/models/oauth_github.rs @@ -34,10 +34,7 @@ pub struct OauthGithub { } impl OauthGithub { - pub async fn find_by_username( - mut conn: &AsyncPgConnection, - login: &str, - ) -> QueryResult { + pub async fn find_by_username(mut conn: &AsyncPgConnection, login: &str) -> QueryResult { User::query() .filter(lower(oauth_github::login).eq(login.to_lowercase())) .first(&mut conn) diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index d5c6709b75e..acfe30364ab 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -50,7 +50,7 @@ impl<'a> UserBuilder<'a> { pub fn with_gh_username(self, gh_login: &'a str) -> Self { Self { - gh_login: gh_login, + gh_login, ..self } } @@ -59,6 +59,7 @@ impl<'a> UserBuilder<'a> { User { id: 1, name: self.display_name.map(ToString::to_string), + gh_login: self.gh_login.into(), gh_id: 123, gh_avatar: None, gh_encrypted_token: None, @@ -67,7 +68,6 @@ impl<'a> UserBuilder<'a> { is_admin: false, publish_notifications: true, username: self.username.into(), - gh_login: self.gh_login.into(), created_at: None, } } diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 0569315f64f..0142d168289 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -329,11 +329,11 @@ async fn modify_owners( enum Login<'a> { /// A team login e.g `github:org:team` Team, - /// A disambiguated `github:username` resolved via oauth_github. + /// A disambiguated `github:username` resolved via `oauth_github`. GitHub(&'a str), /// A disambiguated `cratesio:username` resolved via users table. CratesIo(&'a str), - /// A username that was unambiguous (username == gh_login). + /// A username that was unambiguous (username == `gh_login`). Username(User), } @@ -346,44 +346,47 @@ async fn disambiguate_login<'a>( // Team login: exactly two colons (e.g. github:org:team) if Regex::new(r"^[^:]+:[^:]+:[^:]+$").is_ok_and(|r| r.is_match(login)) { return Ok(Login::Team); - } else if login.contains(':') { - // disambiguate user login + } + + // disambiguate user login + if login.contains(':') { let mut chunks = login.split(':'); let prefix = chunks.next().unwrap(); - if !["github", "cratesio"].contains(&prefix) { - return Err(bad_request( - "unsupported username prefix, only github and cratesio prefixes are supported", - )); - } let username = chunks.next().unwrap(); return match prefix { "github" => Ok(Login::GitHub(username)), - _ => Ok(Login::CratesIo(username)), + "cratesio" => Ok(Login::CratesIo(username)), + _ => Err(bad_request( + "unsupported username prefix, only github and cratesio prefixes are supported", + )), }; - } else { - // check if login is ambiguous - let user = User::find_by_username(conn, login) - .await - .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; - - let command = if add { "add" } else { "remove" }; - if user.gh_login != user.username { - let error = format!( - "error: username {} is possibly ambiguous.\n\n\ - Caused by: \n \ - The crates.io account `{}` is associated with GitHub user `{}`.\n \ - To confirm this is the account you want to {command}, please run one of the following:\n\n \ - $ cargo owner --{command} cratesio:{}\n \ - $ cargo owner --{command} github:{}\n\n \ - If this is not the account you want to {command}, verify the crates.io username of the account you want.", - user.username, user.username, user.gh_login, user.username, user.gh_login - ); - return Err(bad_request(error)); - } - Ok(Login::Username(user)) } + + // check if login is ambiguous + let user = User::find_by_username(conn, login) + .await + .optional()? + .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; + + let command = if add { "add" } else { "remove" }; + let username = user.username.to_owned(); + let gh_login = user.gh_login.to_owned(); + let error = format_args!( + "error: username {username} is possibly ambiguous.\n\n\ + Caused by: \n \ + The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n \ + To confirm this is the account you want to {command}, please run one of the following:\n\n \ + $ cargo owner --{command} cratesio:{username}\n \ + $ cargo owner --{command} github:{gh_login}\n\n \ + If this is not the account you want to {command}, verify the crates.io username of the account you want.", + ); + + if user.username != user.gh_login { + return Err(bad_request(error)); + } + + Ok(Login::Username(user)) } /// Invites `login` as an owner of this crate, returning the created @@ -456,9 +459,9 @@ async fn send_user_invite( }; match invite.create(conn).await? { - NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => Ok( - NewOwnerInvite::User(user, plaintext_token, username.to_owned()), - ), + NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => { + Ok(NewOwnerInvite::User(user, plaintext_token, username.into())) + } NewCrateOwnerInvitationOutcome::AlreadyExists => { Err(OwnerAddError::AlreadyInvited(Box::new(user))) } @@ -631,8 +634,6 @@ impl From for OwnerAddError { } } -/// A [`BoxedAppError`] does not impl [`std::error::Error`] so it needs a manual -/// [`From`] impl. impl From for OwnerRemoveError { fn from(value: BoxedAppError) -> Self { Self::AppError(value.to_string()) diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 866ac603ffa..ce8563f3459 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -393,7 +393,10 @@ async fn deleted_ownership_isnt_in_owner_user() { let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove_with_username(&conn, &user.username).await.unwrap(); + krate + .owner_remove_with_username(&conn, &user.username) + .await + .unwrap(); let json: UserResponse = anon .get("/api/v1/crates/foo_my_packages/owner_user") diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index ea85c225b54..c79ccf7c484 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -1370,7 +1370,10 @@ async fn crates_by_user_id_not_including_deleted_owners() -> anyhow::Result<()> let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut conn) .await; - krate.owner_remove_with_username(&conn, "foo").await.unwrap(); + krate + .owner_remove_with_username(&conn, "foo") + .await + .unwrap(); for response in search_both_by_user_id(&anon, user.id).await { assert_eq!(response.crates.len(), 0); diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 2ac02df1e76..423dc96e388 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -12,13 +12,13 @@ async fn test_cargo_invite_owners() { let (app, _, owner) = TestApp::init().with_user().await; let mut conn = app.db_conn().await; - let new_user = app.db_new_user("cilantro").await; - CrateBuilder::new("guacamole", owner.as_model().id) + let new_user = app.db_new_user("user2").await; + CrateBuilder::new("foo", owner.as_model().id) .expect_build(&mut conn) .await; let json = owner - .add_named_owner("guacamole", &new_user.as_model().gh_login) + .add_named_owner("foo", &new_user.as_model().gh_login) .await .good(); @@ -30,7 +30,7 @@ async fn test_cargo_invite_owners() { // version of cargo assert_eq!( json.msg, - "user cilantro has been invited to be an owner of crate guacamole" + "user user2 has been invited to be an owner of crate foo" ) } @@ -389,14 +389,12 @@ async fn test_unsupported_disambiguation_prefix() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - app.db_new_user("cilantro").await; - CrateBuilder::new("guacamole", cookie.as_model().id) + app.db_new_user("user2").await; + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; - let response = cookie - .add_named_owner("guacamole", "gitlab:cilantro") - .await; + let response = cookie.add_named_owner("foo", "gitlab:user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unsupported username prefix, only github and cratesio prefixes are supported"}]}"#); } @@ -406,13 +404,11 @@ async fn test_disambiguated_github_username_not_found() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - CrateBuilder::new("guacamole", cookie.as_model().id) + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; - let response = cookie - .add_named_owner("guacamole", "github:nonexistent") - .await; + let response = cookie.add_named_owner("foo", "github:nonexistent").await; assert_snapshot!(response.status(), @"400 Bad Request"); assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username nonexistent. If you meant to add a github team, format is github:org:team"}]}"#); } @@ -422,13 +418,11 @@ async fn test_disambiguated_cratesio_username_not_found() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - CrateBuilder::new("guacamole", cookie.as_model().id) + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; - let response = cookie - .add_named_owner("guacamole", "cratesio:nonexistent") - .await; + let response = cookie.add_named_owner("foo", "cratesio:nonexistent").await; assert_snapshot!(response.status(), @"400 Bad Request"); assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with cratesio username nonexistent"}]}"#); } @@ -438,16 +432,15 @@ async fn test_ambiguous_username_error() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - app.db_new_user_with_gh_login("cilantro", "cilantro-gh") - .await; + app.db_new_user_with_gh_login("user2", "user2-gh").await; - CrateBuilder::new("guacamole", cookie.as_model().id) + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; - let response = cookie.add_named_owner("guacamole", "cilantro").await; + let response = cookie.add_named_owner("foo", "user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"error: username cilantro is possibly ambiguous.\n\nCaused by: \n The crates.io account `cilantro` is associated with GitHub user `cilantro-gh`.\n To confirm this is the account you want to add, please run one of the following:\n\n $ cargo owner --add cratesio:cilantro\n $ cargo owner --add github:cilantro-gh\n\n If this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"error: username user2 is possibly ambiguous.\n\nCaused by: \n The crates.io account `user2` is associated with GitHub user `user2-gh`.\n To confirm this is the account you want to add, please run one of the following:\n\n $ cargo owner --add cratesio:user2\n $ cargo owner --add github:user2-gh\n\n If this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -455,26 +448,22 @@ async fn test_disambiguate_with_github_prefix() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - let new_user = app - .db_new_user_with_gh_login("cilantro", "cilantro-gh") - .await; + let new_user = app.db_new_user_with_gh_login("user2", "user2-gh").await; // Create oauth_github entry with the GitHub login OauthGithubBuilder::for_user(new_user.as_model()) - .with_login("cilantro-gh") + .with_login("user2-gh") .insert(&mut conn) .await; - CrateBuilder::new("guacamole", cookie.as_model().id) + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; // Using github: prefix should resolve the ambiguity and invite the user - let response = cookie - .add_named_owner("guacamole", "github:cilantro-gh") - .await; + let response = cookie.add_named_owner("foo", "github:user2-gh").await; assert_snapshot!(response.status(), @"200 OK"); - assert_snapshot!(response.text(), @r#"{"msg":"user cilantro-gh has been invited to be an owner of crate guacamole","ok":true}"#); + assert_snapshot!(response.text(), @r#"{"msg":"user user2-gh has been invited to be an owner of crate foo","ok":true}"#); } #[tokio::test(flavor = "multi_thread")] @@ -482,19 +471,16 @@ async fn test_disambiguate_with_cratesio_prefix() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - app.db_new_user_with_gh_login("cilantro", "cilantro-gh") - .await; + app.db_new_user_with_gh_login("user2", "user2-gh").await; - CrateBuilder::new("guacamole", cookie.as_model().id) + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; // Using cratesio: prefix should resolve the ambiguity and invite the user - let response = cookie - .add_named_owner("guacamole", "cratesio:cilantro") - .await; + let response = cookie.add_named_owner("foo", "cratesio:user2").await; assert_snapshot!(response.status(), @"200 OK"); - assert_snapshot!(response.text(), @r#"{"msg":"user cilantro has been invited to be an owner of crate guacamole","ok":true}"#); + assert_snapshot!(response.text(), @r#"{"msg":"user user2 has been invited to be an owner of crate foo","ok":true}"#); } #[tokio::test(flavor = "multi_thread")] @@ -502,13 +488,13 @@ async fn test_already_owner_error() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - CrateBuilder::new("guacamole", cookie.as_model().id) + CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; // The cookie user is already the owner of the crate let response = cookie - .add_named_owner("guacamole", &cookie.as_model().gh_login) + .add_named_owner("foo", &cookie.as_model().gh_login) .await; assert_snapshot!(response.status(), @"400 Bad Request"); assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`foo` is already an owner"}]}"#); diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index 9ee2ef79a2f..c5dc4941954 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -162,9 +162,7 @@ async fn test_remove_uppercase_team() { #[tokio::test(flavor = "multi_thread")] async fn test_remove_ambiguous_user() { let (app, _, cookie) = TestApp::full().with_user().await; - let user2 = app - .db_new_user_with_gh_login("user2", "user2-gh") - .await; + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; let mut conn = app.db_conn().await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -195,9 +193,7 @@ async fn test_unsupported_disambiguation_prefix() { .expect_build(&mut conn) .await; - let response = cookie - .remove_named_owner("foo", "gitlab:user2") - .await; + let response = cookie.remove_named_owner("foo", "gitlab:user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unsupported username prefix, only github and cratesio prefixes are supported"}]}"#); } @@ -212,9 +208,7 @@ async fn test_disambiguated_github_username_not_found() { .expect_build(&mut conn) .await; - let response = cookie - .remove_named_owner("foo", "github:nonexistent") - .await; + let response = cookie.remove_named_owner("foo", "github:nonexistent").await; assert_snapshot!(response.status(), @"400 Bad Request"); assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `nonexistent`"}]}"#); } @@ -236,14 +230,11 @@ async fn test_disambiguated_cratesio_username_not_found() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `nonexistent`"}]}"#); } - /// Test that removing an ambiguous user with github: prefix works. #[tokio::test(flavor = "multi_thread")] async fn test_disambiguate_remove_with_github_prefix() { let (app, _, cookie) = TestApp::full().with_user().await; - let user2 = app - .db_new_user_with_gh_login("user2", "user2-gh") - .await; + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; let mut conn = app.db_conn().await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -264,9 +255,7 @@ async fn test_disambiguate_remove_with_github_prefix() { .insert(&mut conn) .await; - let response = cookie - .remove_named_owner("foo", "github:user2-gh") - .await; + let response = cookie.remove_named_owner("foo", "github:user2-gh").await; assert_snapshot!(response.status(), @"200 OK"); assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); } @@ -275,9 +264,7 @@ async fn test_disambiguate_remove_with_github_prefix() { #[tokio::test(flavor = "multi_thread")] async fn test_disambiguate_remove_with_cratesio_prefix() { let (app, _, cookie) = TestApp::full().with_user().await; - let user2 = app - .db_new_user_with_gh_login("user2", "user2-gh") - .await; + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; let mut conn = app.db_conn().await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -293,9 +280,7 @@ async fn test_disambiguate_remove_with_cratesio_prefix() { .await .unwrap(); - let response = cookie - .remove_named_owner("foo", "cratesio:user2") - .await; + let response = cookie.remove_named_owner("foo", "cratesio:user2").await; assert_snapshot!(response.status(), @"200 OK"); assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); } diff --git a/src/tests/team.rs b/src/tests/team.rs index 6ca0692e8ca..dd18cc5557c 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -493,7 +493,10 @@ async fn crates_by_team_id_not_including_deleted_owners() -> anyhow::Result<()> .expect_build(&mut conn) .await; add_team_to_crate(&t, &krate, user.id, &mut conn).await?; - krate.owner_remove_with_username(&conn, &t.login).await.unwrap(); + krate + .owner_remove_with_username(&conn, &t.login) + .await + .unwrap(); let json = anon.search(&format!("team_id={}", t.id)).await; assert_eq!(json.crates.len(), 0); From e5a6ffecca05866e335148f49cbbeb645023a234 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 17:41:43 -0400 Subject: [PATCH 09/32] fix formatting --- crates/crates_io_test_utils/src/builders/user.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index acfe30364ab..acba2e2f0ca 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -49,10 +49,7 @@ impl<'a> UserBuilder<'a> { } pub fn with_gh_username(self, gh_login: &'a str) -> Self { - Self { - gh_login, - ..self - } + Self { gh_login, ..self } } pub fn build(self) -> User { From d4f4876f7d6e72a5774ec0300d9a186664982767 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 12 Jul 2026 19:47:39 -0400 Subject: [PATCH 10/32] disambiguate using oauth_github.login rather than users.gh_login --- crates/crates_io_database/src/models/krate.rs | 8 ++-- .../src/models/oauth_github.rs | 8 +++- crates/crates_io_database/src/models/user.rs | 7 ++- .../crates_io_test_utils/src/builders/user.rs | 2 +- src/controllers/krate/owners.rs | 46 +++++++++++-------- src/tests/routes/crates/owners/add.rs | 7 ++- src/tests/routes/crates/owners/remove.rs | 7 ++- src/tests/routes/me/get.rs | 2 +- src/tests/util/test_app.rs | 2 +- 9 files changed, 58 insertions(+), 31 deletions(-) diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index 0efb4128a4b..39b95c9bf7b 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -261,10 +261,10 @@ impl Crate { } /// Remove owner given a github username - pub async fn owner_remove_with_gh_username( + pub async fn owner_remove_with_gh_login( &self, mut conn: &AsyncPgConnection, - username: &str, + login: &str, ) -> Result<(), OwnerRemoveError> { let query = diesel::sql_query( r#"WITH crate_owners_with_gh_login AS ( @@ -289,12 +289,12 @@ impl Crate { let num_updated_rows = query .bind::(self.id) - .bind::(username) + .bind::(login) .execute(&mut conn) .await?; if num_updated_rows == 0 { - return Err(OwnerRemoveError::not_found(username)); + return Err(OwnerRemoveError::not_found(login)); } Ok(()) diff --git a/crates/crates_io_database/src/models/oauth_github.rs b/crates/crates_io_database/src/models/oauth_github.rs index e96e043c5f5..f92e30f805a 100644 --- a/crates/crates_io_database/src/models/oauth_github.rs +++ b/crates/crates_io_database/src/models/oauth_github.rs @@ -34,9 +34,13 @@ pub struct OauthGithub { } impl OauthGithub { - pub async fn find_by_username(mut conn: &AsyncPgConnection, login: &str) -> QueryResult { - User::query() + pub async fn find_by_login( + mut conn: &AsyncPgConnection, + login: &str, + ) -> QueryResult { + oauth_github::table .filter(lower(oauth_github::login).eq(login.to_lowercase())) + .select(OauthGithub::as_select()) .first(&mut conn) .await } diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index eb2e8d3b08e..7a54a581369 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -70,9 +70,12 @@ impl User { .await } - pub async fn find_by_username(mut conn: &AsyncPgConnection, login: &str) -> QueryResult { + pub async fn find_by_username( + mut conn: &AsyncPgConnection, + username: &str, + ) -> QueryResult { User::query() - .filter(lower(users::username).eq(login.to_lowercase())) + .filter(lower(users::username).eq(username.to_lowercase())) .filter(users::gh_id.ne(-1)) .order(users::gh_id.desc()) .first(&mut conn) diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index acba2e2f0ca..fa08f6d4ff9 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -48,7 +48,7 @@ impl<'a> UserBuilder<'a> { } } - pub fn with_gh_username(self, gh_login: &'a str) -> Self { + pub fn with_gh_login(self, gh_login: &'a str) -> Self { Self { gh_login, ..self } } diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 0142d168289..ae74ff2766f 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -333,7 +333,7 @@ enum Login<'a> { GitHub(&'a str), /// A disambiguated `cratesio:username` resolved via users table. CratesIo(&'a str), - /// A username that was unambiguous (username == `gh_login`). + /// An unambigous username (`users.username` == `oauth_github.login`) Username(User), } @@ -369,20 +369,29 @@ async fn disambiguate_login<'a>( .optional()? .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; + let oauth_github = OauthGithub::belonging_to(&user) + .select(OauthGithub::as_select()) + .first(conn) + .await + .optional()?; + let command = if add { "add" } else { "remove" }; let username = user.username.to_owned(); - let gh_login = user.gh_login.to_owned(); - let error = format_args!( - "error: username {username} is possibly ambiguous.\n\n\ - Caused by: \n \ - The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n \ - To confirm this is the account you want to {command}, please run one of the following:\n\n \ - $ cargo owner --{command} cratesio:{username}\n \ - $ cargo owner --{command} github:{gh_login}\n\n \ - If this is not the account you want to {command}, verify the crates.io username of the account you want.", - ); - - if user.username != user.gh_login { + + if let Some(oauth_github) = oauth_github + && oauth_github.login != user.username + { + let gh_login = &oauth_github.login; + let error = format_args!( + "error: username {username} is possibly ambiguous.\n\n\ + Caused by: \n \ + The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n \ + To confirm this is the account you want to {command}, please run one of the following:\n\n \ + $ cargo owner --{command} cratesio:{username}\n \ + $ cargo owner --{command} github:{gh_login}\n\n \ + If this is not the account you want to {command}, verify the crates.io username of the account you want.", + ); + return Err(bad_request(error)); } @@ -403,12 +412,13 @@ async fn add_owner( let encryption = &app.config.token_encryption; add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await } - Login::GitHub(username) => { - let user = OauthGithub::find_by_username(conn, username) + Login::GitHub(login) => { + let oauth = OauthGithub::find_by_login(conn, login) .await .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with github username {username}. If you meant to add a github team, format is github:org:team")))?; - send_user_invite(app, conn, req_user, user, username, krate).await + .ok_or_else(|| bad_request(format_args!("could not find user with github username {login}. If you meant to add a github team, format is github:org:team")))?; + let user = User::find(conn, oauth.user_id).await?; + send_user_invite(app, conn, req_user, user, login, krate).await } Login::CratesIo(username) => { let user = User::find_by_username(conn, username) @@ -435,7 +445,7 @@ async fn remove_owner( .map_err(OwnerRemoveError::from)? { Login::Team => krate.owner_remove_with_username(conn, login).await, - Login::GitHub(username) => krate.owner_remove_with_gh_username(conn, username).await, + Login::GitHub(login) => krate.owner_remove_with_gh_login(conn, login).await, Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await, Login::Username(_) => krate.owner_remove_with_username(conn, login).await, } diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 423dc96e388..131b0291401 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -432,7 +432,12 @@ async fn test_ambiguous_username_error() { let (app, _, cookie) = TestApp::full().with_user().await; let mut conn = app.db_conn().await; - app.db_new_user_with_gh_login("user2", "user2-gh").await; + let new_user = app.db_new_user_with_gh_login("user2", "user2-gh").await; + + OauthGithubBuilder::for_user(new_user.as_model()) + .with_login("user2-gh") + .insert(&mut conn) + .await; CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index c5dc4941954..a82ab83a0e9 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -165,6 +165,11 @@ async fn test_remove_ambiguous_user() { let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; let mut conn = app.db_conn().await; + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2-gh") + .insert(&mut conn) + .await; + let krate = CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; @@ -213,7 +218,7 @@ async fn test_disambiguated_github_username_not_found() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `nonexistent`"}]}"#); } -/// Test that removing with nonexistent cratesio usrname returns an error. +/// Test that removing with nonexistent cratesio username returns an error. #[tokio::test(flavor = "multi_thread")] async fn test_disambiguated_cratesio_username_not_found() { let (app, _, cookie) = TestApp::full().with_user().await; diff --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index 320656e3f69..c231b9ae6af 100644 --- a/src/tests/routes/me/get.rs +++ b/src/tests/routes/me/get.rs @@ -53,7 +53,7 @@ async fn test_user_owned_crates_doesnt_include_deleted_ownership() { .expect_build(&mut conn) .await; krate - .owner_remove_with_username(&conn, &user_model.gh_login) + .owner_remove_with_username(&conn, &user_model.username) .await .unwrap(); diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index 0db0209c1a3..559fa415445 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -191,7 +191,7 @@ impl TestApp { let new_user = crate::builders::UserBuilder::new() .with_username(username) - .with_gh_username(gh_login) + .with_gh_login(gh_login) .new_user(); let id = new_user.insert(&conn).await.unwrap(); From ff2f94a0f17607018e82705c69b9e1ff505f6a05 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 13 Jul 2026 10:31:20 -0400 Subject: [PATCH 11/32] split out refactoring changes --- crates/crates_io_database/src/models/mod.rs | 3 +- .../src/models/oauth_github.rs | 99 --------------- crates/crates_io_database/src/models/owner.rs | 2 +- crates/crates_io_database/src/models/user.rs | 13 ++ src/bin/crates-io/admin/verify_token.rs | 2 +- src/controllers/krate/delete.rs | 2 +- src/controllers/krate/owners.rs | 113 +++++++++--------- src/controllers/krate/update.rs | 4 +- src/controllers/session.rs | 45 +++++++ src/controllers/token.rs | 4 +- .../trustpub/github_configs/create.rs | 2 +- src/controllers/user/update.rs | 4 +- src/controllers/version/update.rs | 2 +- src/tests/routes/crates/admin.rs | 2 +- src/tests/routes/crates/owners/add.rs | 12 +- src/tests/routes/crates/owners/remove.rs | 2 +- src/tests/routes/me/get.rs | 2 +- src/tests/routes/users/stats.rs | 2 +- src/worker/jobs/expiry_notification.rs | 2 +- 19 files changed, 138 insertions(+), 179 deletions(-) delete mode 100644 crates/crates_io_database/src/models/oauth_github.rs diff --git a/crates/crates_io_database/src/models/mod.rs b/crates/crates_io_database/src/models/mod.rs index c64f5e0194e..58d90952ebb 100644 --- a/crates/crates_io_database/src/models/mod.rs +++ b/crates/crates_io_database/src/models/mod.rs @@ -14,11 +14,11 @@ pub use self::email::{Email, NewEmail}; pub use self::follow::Follow; pub use self::keyword::{CrateKeyword, Keyword}; pub use self::krate::{Crate, CrateName, NewCrate}; -pub use self::oauth_github::{NewOauthGithub, OauthGithub}; pub use self::owner::{CrateOwner, Owner, OwnerKind}; pub use self::team::{NewTeam, Team}; pub use self::token::ApiToken; pub use self::trustpub::TrustpubData; +pub use self::user::{NewOauthGithub, OauthGithub}; pub use self::user::{NewUser, PublicUser, User}; pub use self::version::{NewVersion, TopVersions, Version}; @@ -36,7 +36,6 @@ mod email; mod follow; mod keyword; pub mod krate; -pub mod oauth_github; mod owner; pub mod team; pub mod token; diff --git a/crates/crates_io_database/src/models/oauth_github.rs b/crates/crates_io_database/src/models/oauth_github.rs deleted file mode 100644 index f92e30f805a..00000000000 --- a/crates/crates_io_database/src/models/oauth_github.rs +++ /dev/null @@ -1,99 +0,0 @@ -use bon::Builder; -use chrono::{DateTime, Utc}; -use diesel::prelude::*; -use diesel::upsert::excluded; -use diesel_async::{AsyncPgConnection, RunQueryDsl}; - -use crate::fns::lower; -use crate::models::User; -use crate::schema::oauth_github; - -/// The model representing a row in the `oauth_github` database table, linked to a user record. -#[derive(Associations, Identifiable, Selectable, Queryable, Debug, Clone)] -#[diesel( - table_name = oauth_github, - check_for_backend(diesel::pg::Pg), - primary_key(account_id), - belongs_to(User), -)] -pub struct OauthGithub { - /// In the process of being migrated from `users.gh_id`. - /// GitHub API docs describe this type as int64. - pub account_id: i64, - /// In the process of being migrated from `users.gh_avatar`. - pub avatar: Option, - /// In the process of being migrated from `users.gh_encrypted_token`. - pub encrypted_token: Vec, - /// The last time we verified with GitHub what the GitHub username for this user was, and - /// whether the account was valid. - pub last_sync: DateTime, - /// In the process of being migrated from `users.gh_login`. - pub login: String, - /// Foreign key to the `users` table. - pub user_id: i32, -} - -impl OauthGithub { - pub async fn find_by_login( - mut conn: &AsyncPgConnection, - login: &str, - ) -> QueryResult { - oauth_github::table - .filter(lower(oauth_github::login).eq(login.to_lowercase())) - .select(OauthGithub::as_select()) - .first(&mut conn) - .await - } -} - -/// Represents a new crates.io user to GitHub user OAuth link to be inserted into the -/// `oauth_github` table. -#[derive(Insertable, Debug, Builder)] -#[diesel( - table_name = oauth_github, - check_for_backend(diesel::pg::Pg), - primary_key(account_id), - belongs_to(User), -)] -pub struct NewOauthGithub<'a> { - pub account_id: i64, // corresponds to users.gh_id - pub avatar: Option<&'a str>, // corresponds to users.gh_avatar - pub encrypted_token: &'a [u8], // corresponds to users.gh_encrypted_token - pub login: &'a str, // corresponds to users.gh_login - pub user_id: i32, -} - -impl NewOauthGithub<'_> { - /// Inserts the associated GitHub account info into the database, or updates an existing record. - /// - /// GitHub `account_id` is the primary key of the `oauth_github` table, and comes from GitHub. - /// - /// Each GitHub account ID can only be associated with one crates.io account, so that we know - /// who to log in when we get a GitHub oAuth response. - /// - /// If this function gets an `account_id` conflict, it does not and should not update the - /// `user_id` to that of the currently-logged-in crates.io user's ID because that would mean - /// that GitHub account has already been associated with a different crates.io account. In that - /// case, the currently-logged-in crates.io user should be logged out and the crates.io user - /// already associated with this GitHub user should be logged in. - /// - /// We may eventually implement the ability to associate multiple GitHub accounts with one - /// crates.io account. - /// - /// This function should be called if there is no current user and should update the encrypted - /// token, login, or avatar if those have changed. - pub async fn insert_or_update(&self, mut conn: &AsyncPgConnection) -> QueryResult { - diesel::insert_into(oauth_github::table) - .values(self) - .on_conflict(oauth_github::account_id) - .do_update() - .set(( - oauth_github::encrypted_token.eq(excluded(oauth_github::encrypted_token)), - oauth_github::login.eq(excluded(oauth_github::login)), - oauth_github::avatar.eq(excluded(oauth_github::avatar)), - oauth_github::last_sync.eq(Utc::now()), - )) - .get_result(&mut conn) - .await - } -} diff --git a/crates/crates_io_database/src/models/owner.rs b/crates/crates_io_database/src/models/owner.rs index 3850bcb23b7..c95de392df9 100644 --- a/crates/crates_io_database/src/models/owner.rs +++ b/crates/crates_io_database/src/models/owner.rs @@ -104,7 +104,7 @@ impl Owner { pub fn login(&self) -> &str { match self { - Owner::User(user) => &user.username, + Owner::User(user) => &user.gh_login, Owner::Team(team) => &team.login, } } diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 7a54a581369..d34cdd8a14b 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -191,6 +191,19 @@ pub struct OauthGithub { pub user_id: i32, } +impl OauthGithub { + pub async fn find_by_login( + mut conn: &AsyncPgConnection, + login: &str, + ) -> QueryResult { + oauth_github::table + .filter(lower(oauth_github::login).eq(login.to_lowercase())) + .select(OauthGithub::as_select()) + .first(&mut conn) + .await + } +} + /// Represents a new crates.io user to GitHub user OAuth link to be inserted into the /// `oauth_github` table. #[derive(Insertable, Debug, Builder)] diff --git a/src/bin/crates-io/admin/verify_token.rs b/src/bin/crates-io/admin/verify_token.rs index 43b4b027bee..1daee68af16 100644 --- a/src/bin/crates-io/admin/verify_token.rs +++ b/src/bin/crates-io/admin/verify_token.rs @@ -23,6 +23,6 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { let token = HashedToken::parse(&opts.api_token)?; let token = ApiToken::find_by_api_token(&mut conn, &token).await?; let user = User::find(&conn, token.user_id).await?; - println!("The token belongs to user {}", user.username); + println!("The token belongs to user {}", user.gh_login); Ok(()) } diff --git a/src/controllers/krate/delete.rs b/src/controllers/krate/delete.rs index 3839dae54ec..4ba86087952 100644 --- a/src/controllers/krate/delete.rs +++ b/src/controllers/krate/delete.rs @@ -160,7 +160,7 @@ pub async fn delete_crate( let email = EmailMessage::from_template( "crate_deletion", context! { - user => user.username, + user => user.gh_login, krate => crate_name }, )?; diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index ae74ff2766f..9232a8d1345 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -262,7 +262,7 @@ async fn modify_owners( let email = EmailMessage::from_template( "owner_invite", context! { - inviter => user.username, + inviter => user.gh_login, domain => app.emails.domain, crate_name => krate.name, token => token.expose_secret() @@ -288,7 +288,7 @@ async fn modify_owners( // This user has a pending invite. Err(OwnerAddError::AlreadyInvited(user)) => msgs.push(format!( "user {} already has a pending invitation to be an owner of crate {}", - user.username, krate.name + user.gh_login, krate.name )), // An opaque error occurred. @@ -326,6 +326,60 @@ async fn modify_owners( Ok(Json(ModifyResponse { msg, ok: true })) } + +/// Invites `login` as an owner of this crate, returning the created +/// [`NewOwnerInvite`]. +async fn add_owner( + app: &App, + conn: &mut AsyncPgConnection, + req_user: &User, + krate: &Crate, + login: &str, +) -> Result { + match disambiguate_login(conn, login, true).await? { + Login::Team => { + let encryption = &app.config.token_encryption; + add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await + } + Login::GitHub(login) => { + let oauth = OauthGithub::find_by_login(conn, login) + .await + .optional()? + .ok_or_else(|| bad_request(format_args!("could not find user with github username {login}. If you meant to add a github team, format is github:org:team")))?; + let user = User::find(conn, oauth.user_id).await?; + send_user_invite(app, conn, req_user, user, login, krate).await + } + Login::CratesIo(username) => { + let user = User::find_by_username(conn, username) + .await + .optional()? + .ok_or_else(|| { + bad_request(format_args!( + "could not find user with cratesio username {username}" + )) + })?; + send_user_invite(app, conn, req_user, user, username, krate).await + } + Login::Username(user) => send_user_invite(app, conn, req_user, user, login, krate).await, + } +} + +async fn remove_owner( + krate: &Crate, + conn: &mut AsyncPgConnection, + login: &str, +) -> Result<(), OwnerRemoveError> { + match disambiguate_login(conn, login, false) + .await + .map_err(OwnerRemoveError::from)? + { + Login::Team => krate.owner_remove_with_username(conn, login).await, + Login::GitHub(login) => krate.owner_remove_with_gh_login(conn, login).await, + Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await, + Login::Username(_) => krate.owner_remove_with_username(conn, login).await, + } +} + enum Login<'a> { /// A team login e.g `github:org:team` Team, @@ -367,7 +421,7 @@ async fn disambiguate_login<'a>( let user = User::find_by_username(conn, login) .await .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; + .ok_or_else(|| bad_request(format_args!("could not find owner with login `{login}`")))?; let oauth_github = OauthGithub::belonging_to(&user) .select(OauthGithub::as_select()) @@ -398,59 +452,6 @@ async fn disambiguate_login<'a>( Ok(Login::Username(user)) } -/// Invites `login` as an owner of this crate, returning the created -/// [`NewOwnerInvite`]. -async fn add_owner( - app: &App, - conn: &mut AsyncPgConnection, - req_user: &User, - krate: &Crate, - login: &str, -) -> Result { - match disambiguate_login(conn, login, true).await? { - Login::Team => { - let encryption = &app.config.token_encryption; - add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await - } - Login::GitHub(login) => { - let oauth = OauthGithub::find_by_login(conn, login) - .await - .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with github username {login}. If you meant to add a github team, format is github:org:team")))?; - let user = User::find(conn, oauth.user_id).await?; - send_user_invite(app, conn, req_user, user, login, krate).await - } - Login::CratesIo(username) => { - let user = User::find_by_username(conn, username) - .await - .optional()? - .ok_or_else(|| { - bad_request(format_args!( - "could not find user with cratesio username {username}" - )) - })?; - send_user_invite(app, conn, req_user, user, username, krate).await - } - Login::Username(user) => send_user_invite(app, conn, req_user, user, login, krate).await, - } -} - -async fn remove_owner( - krate: &Crate, - conn: &mut AsyncPgConnection, - login: &str, -) -> Result<(), OwnerRemoveError> { - match disambiguate_login(conn, login, false) - .await - .map_err(OwnerRemoveError::from)? - { - Login::Team => krate.owner_remove_with_username(conn, login).await, - Login::GitHub(login) => krate.owner_remove_with_gh_login(conn, login).await, - Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await, - Login::Username(_) => krate.owner_remove_with_username(conn, login).await, - } -} - async fn send_user_invite( app: &App, conn: &mut AsyncPgConnection, diff --git a/src/controllers/krate/update.rs b/src/controllers/krate/update.rs index 09a7e7e8458..b59d2f1a317 100644 --- a/src/controllers/krate/update.rs +++ b/src/controllers/krate/update.rs @@ -121,9 +121,9 @@ async fn update_inner( krate.name = %krate.name, network.client.ip = %**real_ip, usr.id = user.id, - usr.name = %user.username, + usr.name = %user.gh_login, "User {} set trustpub_only={trustpub_only} for crate {}", - user.username, + user.gh_login, krate.name ); diff --git a/src/controllers/session.rs b/src/controllers/session.rs index e6749e375ac..fa101efbe34 100644 --- a/src/controllers/session.rs +++ b/src/controllers/session.rs @@ -170,6 +170,7 @@ async fn create_or_update_user( conn.transaction(async |conn| { let update_result = update_user(gh_user, encrypted_token, conn).await; +<<<<<<< HEAD match update_result { Ok(user_id) => Ok(user_id), Err(diesel::result::Error::NotFound) => { @@ -179,6 +180,50 @@ async fn create_or_update_user( // a one-to-one relationship; this will need to be changed if/when we allow // crates.io users to link more than one GitHub account to their crates.io account. create_user(gh_user, encrypted_token, emails, conn).await +======= + let user_id = new_user.insert_or_update(conn).await?; + + // To assist in eventually someday allowing OAuth with more than GitHub, also + // write the GitHub info to the `oauth_github` table. This table is read when + // loading user details (e.g. the avatar), so a failure to write must fail the + // request just like a failure to write to the `users` table. + let new_oauth_github = NewOauthGithub::builder() + .user_id(user_id) + .account_id(new_user.gh_id as i64) + .encrypted_token(new_user.gh_encrypted_token) + .login(new_user.gh_login) + .maybe_avatar(user.avatar_url.as_deref()) + .build(); + + new_oauth_github.insert_or_update(conn).await?; + + // To send the user an account verification email + if let Some(user_email) = user.email.as_deref() { + let new_email = NewEmail::builder() + .user_id(user_id) + .email(user_email) + .build(); + + if let Some(token) = new_email.insert_if_missing(conn).await? { + let email = EmailMessage::from_template( + "user_confirm", + context! { + user_name => new_user.gh_login, + domain => emails.domain, + token => token.expose_secret() + }, + ); + + match email { + Ok(email) => { + // Swallows any error. Some users might insert an invalid email address here. + let _ = emails.send(user_email, email).await; + } + Err(error) => { + warn!("Failed to render user confirmation email template: {error}"); + } + }; +>>>>>>> d2608c9e2 (split out refactoring changes) } Err(error) => Err(error), } diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 61112690631..a7912d0bf92 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -154,7 +154,7 @@ pub async fn create_api_token( network.client.ip = client_ip, http.headers = ?headers, "Blocked token creation for user `{}` (id: {}) due to disabled flag (token name: `{}`)", - user.username, user.id, new.api_token.name + user.gh_login, user.id, new.api_token.name ); let message = disable_message.clone(); @@ -212,7 +212,7 @@ pub async fn create_api_token( if let Some(recipient) = recipient { let context = context! { token_name => &new.api_token.name, - user_name => &user.username, + user_name => &user.gh_login, domain => app.emails.domain, }; diff --git a/src/controllers/trustpub/github_configs/create.rs b/src/controllers/trustpub/github_configs/create.rs index 929a68d6274..c8b7b081366 100644 --- a/src/controllers/trustpub/github_configs/create.rs +++ b/src/controllers/trustpub/github_configs/create.rs @@ -96,7 +96,7 @@ pub async fn create_trustpub_github_config( )); }; let gh_auth = encryption.decrypt(gh_auth).map_err(|err| { - let login = &auth_user.username; + let login = &auth_user.gh_login; warn!("Failed to decrypt GitHub token for user {login}: {err}"); server_error("Internal server error") })?; diff --git a/src/controllers/user/update.rs b/src/controllers/user/update.rs index a150e0e955c..9a197d14ce0 100644 --- a/src/controllers/user/update.rs +++ b/src/controllers/user/update.rs @@ -78,7 +78,7 @@ pub async fn update_user( let email = EmailMessage::from_template( "unsubscribe_notifications", context! { - user_name => user.username, + user_name => user.gh_login, domain => state.emails.domain }, ); @@ -123,7 +123,7 @@ pub async fn update_user( let email = EmailMessage::from_template( "user_confirm", context! { - user_name => user.username, + user_name => user.gh_login, domain => state.emails.domain, token => token.expose_secret() }, diff --git a/src/controllers/version/update.rs b/src/controllers/version/update.rs index 86c5121cf44..05c21321c2a 100644 --- a/src/controllers/version/update.rs +++ b/src/controllers/version/update.rs @@ -132,7 +132,7 @@ pub async fn perform_version_yank_update( let action = if yanked { "yanking" } else { "unyanking" }; warn!( "Admin {} is {action} {}@{}", - user.username, krate.name, version.num + user.gh_login, krate.name, version.num ); } else { return Err(custom( diff --git a/src/tests/routes/crates/admin.rs b/src/tests/routes/crates/admin.rs index 44f4d62f9ff..8115c633d61 100644 --- a/src/tests/routes/crates/admin.rs +++ b/src/tests/routes/crates/admin.rs @@ -62,7 +62,7 @@ async fn index_include_yanked() -> anyhow::Result<()> { .await; // Include fully yanked (all versions were yanked) crates - let username = &user.username; + let username = &user.gh_login; let response = admin.admin_list::<()>(username).await; assert_json_snapshot!(response.json(), { diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 131b0291401..0a722c71b46 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -12,13 +12,13 @@ async fn test_cargo_invite_owners() { let (app, _, owner) = TestApp::init().with_user().await; let mut conn = app.db_conn().await; - let new_user = app.db_new_user("user2").await; - CrateBuilder::new("foo", owner.as_model().id) + let new_user = app.db_new_user("cilantro").await; + CrateBuilder::new("guacamole", owner.as_model().id) .expect_build(&mut conn) .await; let json = owner - .add_named_owner("foo", &new_user.as_model().gh_login) + .add_named_owner("guacamole", &new_user.as_model().gh_login) .await .good(); @@ -30,7 +30,7 @@ async fn test_cargo_invite_owners() { // version of cargo assert_eq!( json.msg, - "user user2 has been invited to be an owner of crate foo" + "user cilantro has been invited to be an owner of crate guacamole" ) } @@ -304,7 +304,7 @@ async fn test_unknown_user() { let response = cookie.add_named_owner("foo", "unknown").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `unknown`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -369,7 +369,7 @@ async fn no_invite_emails_for_txn_rollback() { let response = token.add_named_owners("crate_name", &usernames).await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `bananas`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `bananas`"}]}"#); // No emails should have been sent. assert_eq!(app.emails().await.len(), 0); diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index a82ab83a0e9..ab98b0c5522 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -60,7 +60,7 @@ async fn test_unknown_user() { let response = cookie.remove_named_owner("foo", "unknown").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `unknown`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); } #[tokio::test(flavor = "multi_thread")] diff --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index c231b9ae6af..320656e3f69 100644 --- a/src/tests/routes/me/get.rs +++ b/src/tests/routes/me/get.rs @@ -53,7 +53,7 @@ async fn test_user_owned_crates_doesnt_include_deleted_ownership() { .expect_build(&mut conn) .await; krate - .owner_remove_with_username(&conn, &user_model.username) + .owner_remove_with_username(&conn, &user_model.gh_login) .await .unwrap(); diff --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index c20a63f180a..6f633ec1183 100644 --- a/src/tests/routes/users/stats.rs +++ b/src/tests/routes/users/stats.rs @@ -53,7 +53,7 @@ async fn user_total_downloads() -> anyhow::Result<()> { .execute(&mut conn) .await?; no_longer_my_krate - .owner_remove_with_username(&conn, &user.username) + .owner_remove_with_username(&conn, &user.gh_login) .await .unwrap(); diff --git a/src/worker/jobs/expiry_notification.rs b/src/worker/jobs/expiry_notification.rs index 623630dc497..d8d6f2eb6d4 100644 --- a/src/worker/jobs/expiry_notification.rs +++ b/src/worker/jobs/expiry_notification.rs @@ -89,7 +89,7 @@ async fn handle_expiring_token( let email = EmailMessage::from_template( "expiry_notification", context! { - name => user.username, + name => user.gh_login, token_id => token.id, token_name => token.name, expiry_date => token.expired_at.unwrap().to_rfc3339_opts(SecondsFormat::Secs, true) From b1a679f8b1bc4319ae594eb6ac2df081c7f4735a Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 13 Jul 2026 11:10:34 -0400 Subject: [PATCH 12/32] minor updates to split out refactoring from behavioural changes --- src/controllers/krate/owners.rs | 11 +++++------ src/tests/routes/crates/owners/add.rs | 4 ++-- src/tests/routes/crates/owners/remove.rs | 2 +- src/tests/routes/me/get.rs | 2 +- src/tests/routes/users/stats.rs | 2 +- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 9232a8d1345..ff69d0f4a06 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -326,7 +326,6 @@ async fn modify_owners( Ok(Json(ModifyResponse { msg, ok: true })) } - /// Invites `login` as an owner of this crate, returning the created /// [`NewOwnerInvite`]. async fn add_owner( @@ -347,7 +346,7 @@ async fn add_owner( .optional()? .ok_or_else(|| bad_request(format_args!("could not find user with github username {login}. If you meant to add a github team, format is github:org:team")))?; let user = User::find(conn, oauth.user_id).await?; - send_user_invite(app, conn, req_user, user, login, krate).await + invite_user_owner(app, conn, req_user, user, login, krate).await } Login::CratesIo(username) => { let user = User::find_by_username(conn, username) @@ -358,9 +357,9 @@ async fn add_owner( "could not find user with cratesio username {username}" )) })?; - send_user_invite(app, conn, req_user, user, username, krate).await + invite_user_owner(app, conn, req_user, user, username, krate).await } - Login::Username(user) => send_user_invite(app, conn, req_user, user, login, krate).await, + Login::Username(user) => invite_user_owner(app, conn, req_user, user, login, krate).await, } } @@ -421,7 +420,7 @@ async fn disambiguate_login<'a>( let user = User::find_by_username(conn, login) .await .optional()? - .ok_or_else(|| bad_request(format_args!("could not find owner with login `{login}`")))?; + .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; let oauth_github = OauthGithub::belonging_to(&user) .select(OauthGithub::as_select()) @@ -452,7 +451,7 @@ async fn disambiguate_login<'a>( Ok(Login::Username(user)) } -async fn send_user_invite( +async fn invite_user_owner( app: &App, conn: &mut AsyncPgConnection, req_user: &User, diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 0a722c71b46..6b8b1f14dbe 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -304,7 +304,7 @@ async fn test_unknown_user() { let response = cookie.add_named_owner("foo", "unknown").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `unknown`"}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -369,7 +369,7 @@ async fn no_invite_emails_for_txn_rollback() { let response = token.add_named_owners("crate_name", &usernames).await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `bananas`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `bananas`"}]}"#); // No emails should have been sent. assert_eq!(app.emails().await.len(), 0); diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index ab98b0c5522..a82ab83a0e9 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -60,7 +60,7 @@ async fn test_unknown_user() { let response = cookie.remove_named_owner("foo", "unknown").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `unknown`"}]}"#); } #[tokio::test(flavor = "multi_thread")] diff --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index 320656e3f69..c231b9ae6af 100644 --- a/src/tests/routes/me/get.rs +++ b/src/tests/routes/me/get.rs @@ -53,7 +53,7 @@ async fn test_user_owned_crates_doesnt_include_deleted_ownership() { .expect_build(&mut conn) .await; krate - .owner_remove_with_username(&conn, &user_model.gh_login) + .owner_remove_with_username(&conn, &user_model.username) .await .unwrap(); diff --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index 6f633ec1183..c20a63f180a 100644 --- a/src/tests/routes/users/stats.rs +++ b/src/tests/routes/users/stats.rs @@ -53,7 +53,7 @@ async fn user_total_downloads() -> anyhow::Result<()> { .execute(&mut conn) .await?; no_longer_my_krate - .owner_remove_with_username(&conn, &user.gh_login) + .owner_remove_with_username(&conn, &user.username) .await .unwrap(); From 4d9231b791bd5612e96bce052cfd1394b5b0d532 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 19 Jul 2026 10:27:13 -0400 Subject: [PATCH 13/32] update comments; parse login before owner check; update parse login function --- crates/crates_io_database/src/models/krate.rs | 2 - crates/crates_io_database/src/models/owner.rs | 9 +- crates/crates_io_database/src/models/user.rs | 3 +- src/bin/crates-io/admin/delete_crate.rs | 2 +- src/controllers/krate/owners.rs | 290 ++++++++++-------- 5 files changed, 166 insertions(+), 140 deletions(-) diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index 39b95c9bf7b..2219d97060c 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -318,8 +318,6 @@ pub enum OwnerRemoveError { Diesel(#[from] diesel::result::Error), #[error("Could not find owner with login `{login}`")] NotFound { login: String }, - #[error("{0}")] - AppError(String), } impl OwnerRemoveError { diff --git a/crates/crates_io_database/src/models/owner.rs b/crates/crates_io_database/src/models/owner.rs index c95de392df9..2e09aa73a7c 100644 --- a/crates/crates_io_database/src/models/owner.rs +++ b/crates/crates_io_database/src/models/owner.rs @@ -102,7 +102,14 @@ impl Owner { } } - pub fn login(&self) -> &str { + pub fn username(&self) -> &str { + match self { + Owner::User(user) => &user.username, + Owner::Team(team) => &team.login, + } + } + + pub fn gh_login(&self) -> &str { match self { Owner::User(user) => &user.gh_login, Owner::Team(team) => &team.login, diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index d34cdd8a14b..1d7d478a259 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -198,7 +198,8 @@ impl OauthGithub { ) -> QueryResult { oauth_github::table .filter(lower(oauth_github::login).eq(login.to_lowercase())) - .select(OauthGithub::as_select()) + .filter(oauth_github::account_id.ne(-1)) + .order(oauth_github::account_id.desc()) .first(&mut conn) .await } diff --git a/src/bin/crates-io/admin/delete_crate.rs b/src/bin/crates-io/admin/delete_crate.rs index f28c26f6427..0865d97e03d 100644 --- a/src/bin/crates-io/admin/delete_crate.rs +++ b/src/bin/crates-io/admin/delete_crate.rs @@ -31,7 +31,7 @@ pub struct Opts { #[arg(short, long)] yes: bool, - /// Your GitHub username. + /// Your crates.io username #[arg(long)] deleted_by: String, diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index ff69d0f4a06..7c8e76169c5 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -22,7 +22,6 @@ use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl}; use http::StatusCode; use http::request::Parts; use minijinja::context; -use regex::Regex; use secrecy::ExposeSecret; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -174,7 +173,10 @@ pub struct ChangeOwnersRequest { /// /// For users, use just the username (e.g., `"octocat"`). /// For GitHub teams, use the format `github:org:team` (e.g., `"github:rust-lang:owners"`). - #[schema(example = json!(["octocat", "github:rust-lang:owners"]))] + /// + /// To disambiguate between crates.io and GitHub usernames, use + /// the `cratesio:username` or `github:username` prefix. + #[schema(example = json!(["octocat", "github:rust-lang:owners", "cratesio:some_user"]))] #[serde(alias = "users")] owners: Vec, } @@ -239,13 +241,20 @@ async fn modify_owners( let comma_sep_msg = if add { let mut msgs = Vec::with_capacity(logins.len()); for login in &logins { - let login_test = - |owner: &Owner| owner.login().to_lowercase() == *login.to_lowercase(); + let parsed_login = parse_login(login)?; + let login_test = |owner: &Owner| -> bool { + let username = match parsed_login { + Login::GitHubTeam { .. } => login, + Login::GitHub(u) | Login::CratesIo(u) | Login::Unprefixed(u) => u, + }; + + owner.username().eq_ignore_ascii_case(username) + }; if owners.iter().any(login_test) { return Err(bad_request(format_args!("`{login}` is already an owner"))); } - match add_owner(&app, conn, user, &krate, login).await { + match add_owner(&app, conn, user, &krate, parsed_login).await { // A user was successfully invited, and they must accept // the invite, and a best-effort attempt should be made // to email them the invite token for one-click @@ -299,7 +308,8 @@ async fn modify_owners( msgs.join(",") } else { for login in &logins { - remove_owner(&krate, conn, login).await? + let parsed_login = parse_login(login)?; + remove_owner(&krate, conn, parsed_login, &owners).await? } if User::owning(&krate, conn).await?.is_empty() { return Err(bad_request( @@ -333,20 +343,34 @@ async fn add_owner( conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, - login: &str, + login: Login<'_>, ) -> Result { - match disambiguate_login(conn, login, true).await? { - Login::Team => { + match login { + Login::GitHubTeam { login, org, team } => { let encryption = &app.config.token_encryption; - add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await + add_github_team_owner( + &*app.github, + conn, + req_user, + krate, + login, + org, + team, + encryption, + ) + .await } - Login::GitHub(login) => { - let oauth = OauthGithub::find_by_login(conn, login) + Login::GitHub(username) => { + let oauth = OauthGithub::find_by_login(conn, username) .await .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with github username {login}. If you meant to add a github team, format is github:org:team")))?; + .ok_or_else(|| { + bad_request(format_args!( + "could not find user with github username {username}" + )) + })?; let user = User::find(conn, oauth.user_id).await?; - invite_user_owner(app, conn, req_user, user, login, krate).await + invite_user_owner(app, conn, req_user, user, username, krate).await } Login::CratesIo(username) => { let user = User::find_by_username(conn, username) @@ -359,96 +383,132 @@ async fn add_owner( })?; invite_user_owner(app, conn, req_user, user, username, krate).await } - Login::Username(user) => invite_user_owner(app, conn, req_user, user, login, krate).await, + Login::Unprefixed(username) => { + // check if login is ambiguous + let user = User::find_by_username(conn, username) + .await + .optional()? + .ok_or_else(|| { + bad_request(format_args!("could not find user with login `{username}`")) + })?; + + let oauth_github = OauthGithub::belonging_to(&user) + .select(OauthGithub::as_select()) + .first(conn) + .await + .optional()?; + + if let Some(oauth_github) = oauth_github + && oauth_github.login.to_lowercase() != user.username.to_lowercase() + { + let gh_login = &oauth_github.login; + let error = format_args!( + "username {username} is possibly ambiguous.\n\n\ + Caused by: \n \ + The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n \ + To confirm this is the account you want to add, please run one of the following:\n\n \ + $ cargo owner --add cratesio:{username}\n \ + $ cargo owner --add github:{gh_login}\n\n \ + If this is not the account you want to add, verify the crates.io username of the account you want.", + ); + + return Err(OwnerAddError::AppError(bad_request(error))); + } + invite_user_owner(app, conn, req_user, user, username, krate).await + } } } async fn remove_owner( krate: &Crate, conn: &mut AsyncPgConnection, - login: &str, + login: Login<'_>, + owners: &Vec, ) -> Result<(), OwnerRemoveError> { - match disambiguate_login(conn, login, false) - .await - .map_err(OwnerRemoveError::from)? - { - Login::Team => krate.owner_remove_with_username(conn, login).await, - Login::GitHub(login) => krate.owner_remove_with_gh_login(conn, login).await, + match login { + Login::GitHubTeam { login, .. } => krate.owner_remove_with_username(conn, login).await, + Login::GitHub(username) => krate.owner_remove_with_gh_login(conn, username).await, Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await, - Login::Username(_) => krate.owner_remove_with_username(conn, login).await, + Login::Unprefixed(username) => { + let cratesio_owner_to_remove = owners + .iter() + .find(|o| o.username().to_lowercase() == username.to_lowercase()); + let github_owner_to_remove = owners + .iter() + .find(|o| o.gh_login().to_lowercase() == username.to_lowercase()); + + // check if ambiguous. assumes usernames are unique on separate services. + if let Some(cratesio_owner) = cratesio_owner_to_remove + && let Some(github_owner) = github_owner_to_remove + && cratesio_owner.id() != github_owner.id() + { + let error = format_args!( + "username {username} is ambiguous.\n\n\ + Caused by: \n \ + There are two owners of this crate with the username `{username}` on different services.\n \ + To confirm which owner you want to remove, please run one of the following:\n\n \ + $ cargo owner --remove cratesio:{username}\n \ + $ cargo owner --remove github:{username}\n\n \ + If this is not the account you want to remove, verify the crates.io username of the account you want.", + ); + + return Err(OwnerRemoveError::AppError(bad_request(error))); + } + + if cratesio_owner_to_remove.is_some() { + krate.owner_remove_with_username(conn, username).await + } else if github_owner_to_remove.is_some() { + krate.owner_remove_with_gh_login(conn, username).await + } else { + Err(OwnerRemoveError::NotFound { + login: username.into(), + }) + } + } } } +/// Parsed login string representation enum Login<'a> { - /// A team login e.g `github:org:team` - Team, - /// A disambiguated `github:username` resolved via `oauth_github`. + /// GitHub organization team (e.g `github:org:team`). the original login is preserved as a convenience to avoid rebuilding it. + GitHubTeam { + login: &'a str, + org: &'a str, + team: &'a str, + }, + /// GitHub user (e.g. `github:username`). GitHub(&'a str), - /// A disambiguated `cratesio:username` resolved via users table. + /// crates.io user (`crates.io:username`). CratesIo(&'a str), - /// An unambigous username (`users.username` == `oauth_github.login`) - Username(User), + /// Unprefixed username (`username` without any prefix) + Unprefixed(&'a str), } -/// Disambiguate a login string -async fn disambiguate_login<'a>( - conn: &mut AsyncPgConnection, - login: &'a str, - add: bool, -) -> Result, BoxedAppError> { - // Team login: exactly two colons (e.g. github:org:team) - if Regex::new(r"^[^:]+:[^:]+:[^:]+$").is_ok_and(|r| r.is_match(login)) { - return Ok(Login::Team); - } - - // disambiguate user login - if login.contains(':') { - let mut chunks = login.split(':'); - let prefix = chunks.next().unwrap(); - - let username = chunks.next().unwrap(); - return match prefix { - "github" => Ok(Login::GitHub(username)), - "cratesio" => Ok(Login::CratesIo(username)), - _ => Err(bad_request( - "unsupported username prefix, only github and cratesio prefixes are supported", - )), - }; +fn parse_login<'a>(login: &'a str) -> Result, BoxedAppError> { + // sanitization + fn is_valid(s: &str, label: &str) -> Result { + if let Some(c) = s + .chars() + .find(|c| !matches!(c, 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_')) + { + return Err(bad_request(format_args!( + "{label} cannot contain special characters like {c}" + ))); + } + Ok(true) } - // check if login is ambiguous - let user = User::find_by_username(conn, login) - .await - .optional()? - .ok_or_else(|| bad_request(format_args!("could not find user with login `{login}`")))?; - - let oauth_github = OauthGithub::belonging_to(&user) - .select(OauthGithub::as_select()) - .first(conn) - .await - .optional()?; - - let command = if add { "add" } else { "remove" }; - let username = user.username.to_owned(); - - if let Some(oauth_github) = oauth_github - && oauth_github.login != user.username - { - let gh_login = &oauth_github.login; - let error = format_args!( - "error: username {username} is possibly ambiguous.\n\n\ - Caused by: \n \ - The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n \ - To confirm this is the account you want to {command}, please run one of the following:\n\n \ - $ cargo owner --{command} cratesio:{username}\n \ - $ cargo owner --{command} github:{gh_login}\n\n \ - If this is not the account you want to {command}, verify the crates.io username of the account you want.", - ); - - return Err(bad_request(error)); + match login.split(':').collect::>().as_slice() { + ["github", org, team] if is_valid(org, "organization")? && is_valid(team, "team")? => { + Ok(Login::GitHubTeam { login, org, team }) + } + ["github", username] if is_valid(username, "username")? => Ok(Login::GitHub(username)), + ["cratesio", username] if is_valid(username, "username")? => Ok(Login::CratesIo(username)), + [username] if is_valid(username, "username")? => Ok(Login::Unprefixed(username)), + _ => Err(bad_request( + "invalid username format. only github:org:team, github:username, cratesio:username and username are supported.", + )), } - - Ok(Login::Username(user)) } async fn invite_user_owner( @@ -478,41 +538,23 @@ async fn invite_user_owner( } } -async fn add_team_owner( +/// Tries to add a github team owner. Assumes `org` and `team` are +/// correctly parsed out of the full `login`. `login` is passed as a +/// convenience to avoid rebuilding it. +async fn add_github_team_owner( gh_client: &dyn GitHubClient, conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, login: &str, + org: &str, + team: &str, encryption: &TokenEncryption, ) -> Result { - // github:rust-lang:owners - let mut chunks = login.split(':'); - - let team_system = chunks.next().unwrap(); - if team_system != "github" { - let error = "unknown organization handler, only 'github:org:team' is supported"; - return Err(bad_request(error).into()); - } - - // unwrap is documented above as part of the calling contract - let org = chunks.next().unwrap(); - let team = chunks.next().ok_or_else(|| { - let error = "missing github team argument; format is github:org:team"; - bad_request(error) - })?; - // Always recreate teams to get the most up-to-date GitHub ID - let team = create_or_update_github_team( - gh_client, - conn, - &login.to_lowercase(), - org, - team, - req_user, - encryption, - ) - .await?; + let team = + create_or_update_github_team(gh_client, conn, login, org, team, req_user, encryption) + .await?; // Teams are added as owners immediately, since the above call ensures // the user is a team member. @@ -528,7 +570,7 @@ async fn add_team_owner( } /// Tries to create or update a GitHub Team. Assumes `org` and `team` are -/// correctly parsed out of the full `name`. `name` is passed as a +/// correctly parsed out of the full `login`. `login` is passed as a /// convenience to avoid rebuilding it. pub async fn create_or_update_github_team( gh_client: &dyn GitHubClient, @@ -539,21 +581,6 @@ pub async fn create_or_update_github_team( req_user: &User, encryption: &TokenEncryption, ) -> AppResult { - // GET orgs/:org/teams - // check that `team` is the `slug` in results, and grab its data - - // "sanitization" - fn is_allowed_char(c: char) -> bool { - matches!(c, 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_') - } - - if let Some(c) = org_name.chars().find(|c| !is_allowed_char(*c)) { - return Err(bad_request(format_args!( - "organization cannot contain special \ - characters like {c}" - ))); - } - let Some(token) = req_user.gh_encrypted_token.as_ref() else { return Err(bad_request( "Cannot add a GitHub team as an owner without a connected GitHub account", @@ -644,12 +671,6 @@ impl From for OwnerAddError { } } -impl From for OwnerRemoveError { - fn from(value: BoxedAppError) -> Self { - Self::AppError(value.to_string()) - } -} - impl From for BoxedAppError { fn from(error: OwnerRemoveError) -> Self { match error { @@ -657,7 +678,6 @@ impl From for BoxedAppError { OwnerRemoveError::NotFound { login } => { bad_request(format!("could not find owner with login `{login}`")) } - OwnerRemoveError::AppError(error) => bad_request(error), } } } From 584bc468bb26a13dff6646b303e8d40b39998cda Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 19 Jul 2026 19:44:58 -0400 Subject: [PATCH 14/32] owner remove error conversion --- src/controllers/krate/owners.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 7c8e76169c5..bdd8cc742d1 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -424,11 +424,11 @@ async fn remove_owner( conn: &mut AsyncPgConnection, login: Login<'_>, owners: &Vec, -) -> Result<(), OwnerRemoveError> { +) -> Result<(), BoxedAppError> { match login { - Login::GitHubTeam { login, .. } => krate.owner_remove_with_username(conn, login).await, - Login::GitHub(username) => krate.owner_remove_with_gh_login(conn, username).await, - Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await, + Login::GitHubTeam { login, .. } => krate.owner_remove_with_username(conn, login).await?, + Login::GitHub(username) => krate.owner_remove_with_gh_login(conn, username).await?, + Login::CratesIo(username) => krate.owner_remove_with_username(conn, username).await?, Login::Unprefixed(username) => { let cratesio_owner_to_remove = owners .iter() @@ -452,20 +452,19 @@ async fn remove_owner( If this is not the account you want to remove, verify the crates.io username of the account you want.", ); - return Err(OwnerRemoveError::AppError(bad_request(error))); + return Err(bad_request(error)); } if cratesio_owner_to_remove.is_some() { - krate.owner_remove_with_username(conn, username).await + krate.owner_remove_with_username(conn, username).await? } else if github_owner_to_remove.is_some() { - krate.owner_remove_with_gh_login(conn, username).await + krate.owner_remove_with_gh_login(conn, username).await? } else { - Err(OwnerRemoveError::NotFound { - login: username.into(), - }) + return Err(OwnerRemoveError::not_found(username).into()); } } - } + }; + Ok(()) } /// Parsed login string representation From 7b4abe0a1dfc701edba5a425fdb26fd37bf78fc8 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 19 Jul 2026 21:11:03 -0400 Subject: [PATCH 15/32] index migrations for username and gh login fields --- .../2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql | 2 ++ .../2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql | 2 ++ 2 files changed, 4 insertions(+) create mode 100644 migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql create mode 100644 migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql new file mode 100644 index 00000000000..76d2f28117e --- /dev/null +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS index_users_username; +DROP INDEX IF EXISTS index_oauth_github_login; diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql new file mode 100644 index 00000000000..604cb58e69a --- /dev/null +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX index_users_username ON users (canon_username(username)); +CREATE INDEX index_oauth_github_login ON oauth_github (lower(login)); From 2050824f08e81b942c3df432964f6ce12c27f728 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sun, 19 Jul 2026 21:25:34 -0400 Subject: [PATCH 16/32] update error message --- src/controllers/krate/owners.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index bdd8cc742d1..86183de126d 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -403,12 +403,10 @@ async fn add_owner( { let gh_login = &oauth_github.login; let error = format_args!( - "username {username} is possibly ambiguous.\n\n\ - Caused by: \n \ - The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n \ - To confirm this is the account you want to add, please run one of the following:\n\n \ - $ cargo owner --add cratesio:{username}\n \ - $ cargo owner --add github:{gh_login}\n\n \ + "username {username} is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n + To confirm this is the account you want to add, please run one of the following:\n\n + $ cargo owner --add cratesio:{username}\n + $ cargo owner --add github:{gh_login}\n\n If this is not the account you want to add, verify the crates.io username of the account you want.", ); @@ -443,12 +441,10 @@ async fn remove_owner( && cratesio_owner.id() != github_owner.id() { let error = format_args!( - "username {username} is ambiguous.\n\n\ - Caused by: \n \ - There are two owners of this crate with the username `{username}` on different services.\n \ - To confirm which owner you want to remove, please run one of the following:\n\n \ - $ cargo owner --remove cratesio:{username}\n \ - $ cargo owner --remove github:{username}\n\n \ + "username {username} is ambiguous. There are two owners of this crate with the username `{username}` on different services.\n + To confirm which owner you want to remove, please run one of the following:\n\n + $ cargo owner --remove cratesio:{username}\n + $ cargo owner --remove github:{username}\n\n If this is not the account you want to remove, verify the crates.io username of the account you want.", ); From 3dd13a2ca163fd91c1fae65bb02e7d317edaa89e Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 20 Jul 2026 09:37:03 -0400 Subject: [PATCH 17/32] minor updates --- .../up.sql | 4 ++-- src/bin/crates-io/admin/delete_crate.rs | 2 +- src/controllers/krate/owners.rs | 12 +++++++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql index 604cb58e69a..d3a607e9c62 100644 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql @@ -1,2 +1,2 @@ -CREATE INDEX index_users_username ON users (canon_username(username)); -CREATE INDEX index_oauth_github_login ON oauth_github (lower(login)); +CREATE INDEX IF NOT EXISTS index_users_username ON users (canon_username(username)); +CREATE INDEX IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); diff --git a/src/bin/crates-io/admin/delete_crate.rs b/src/bin/crates-io/admin/delete_crate.rs index 0865d97e03d..a7be8556d8b 100644 --- a/src/bin/crates-io/admin/delete_crate.rs +++ b/src/bin/crates-io/admin/delete_crate.rs @@ -31,7 +31,7 @@ pub struct Opts { #[arg(short, long)] yes: bool, - /// Your crates.io username + /// Your crates.io username. #[arg(long)] deleted_by: String, diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 86183de126d..770297105b0 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -176,7 +176,7 @@ pub struct ChangeOwnersRequest { /// /// To disambiguate between crates.io and GitHub usernames, use /// the `cratesio:username` or `github:username` prefix. - #[schema(example = json!(["octocat", "github:rust-lang:owners", "cratesio:some_user"]))] + #[schema(example = json!(["octocat", "github:rust-lang:owners", "cratesio:some_user", "github:other_user"]))] #[serde(alias = "users")] owners: Vec, } @@ -248,7 +248,7 @@ async fn modify_owners( Login::GitHub(u) | Login::CratesIo(u) | Login::Unprefixed(u) => u, }; - owner.username().eq_ignore_ascii_case(username) + owner.username().to_lowercase() == username.to_lowercase() }; if owners.iter().any(login_test) { return Err(bad_request(format_args!("`{login}` is already an owner"))); @@ -412,6 +412,7 @@ async fn add_owner( return Err(OwnerAddError::AppError(bad_request(error))); } + invite_user_owner(app, conn, req_user, user, username, krate).await } } @@ -482,6 +483,10 @@ enum Login<'a> { fn parse_login<'a>(login: &'a str) -> Result, BoxedAppError> { // sanitization fn is_valid(s: &str, label: &str) -> Result { + if s.is_empty() { + return Err(bad_request(format_args!("{label} cannot be empty"))); + } + if let Some(c) = s .chars() .find(|c| !matches!(c, 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_')) @@ -490,6 +495,7 @@ fn parse_login<'a>(login: &'a str) -> Result, BoxedAppError> { "{label} cannot contain special characters like {c}" ))); } + Ok(true) } @@ -501,7 +507,7 @@ fn parse_login<'a>(login: &'a str) -> Result, BoxedAppError> { ["cratesio", username] if is_valid(username, "username")? => Ok(Login::CratesIo(username)), [username] if is_valid(username, "username")? => Ok(Login::Unprefixed(username)), _ => Err(bad_request( - "invalid username format. only github:org:team, github:username, cratesio:username and username are supported.", + "invalid argument. only github:org:team, github:username, cratesio:username and username are supported.", )), } } From 6eb1a81cc41a5c4493ffead79a6040adb89e22f9 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 20 Jul 2026 10:05:40 -0400 Subject: [PATCH 18/32] test updates --- src/tests/issues/issue1205.rs | 8 ++++---- src/tests/routes/crates/owners/add.rs | 10 +++++++--- src/tests/routes/crates/owners/remove.rs | 10 +++++++--- ...ation__openapi__openapi_internal_snapshot-2.snap | 13 +++++++++---- .../integration__openapi__openapi_snapshot-2.snap | 13 +++++++++---- src/tests/team.rs | 11 +++++------ 6 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/tests/issues/issue1205.rs b/src/tests/issues/issue1205.rs index 5d2cdc5d815..937aeda22d8 100644 --- a/src/tests/issues/issue1205.rs +++ b/src/tests/issues/issue1205.rs @@ -27,8 +27,8 @@ async fn test_issue_1205() -> anyhow::Result<()> { let owners = krate.owners(&conn).await?; assert_eq!(owners.len(), 2); - assert_eq!(owners[0].login(), "foo"); - assert_eq!(owners[1].login(), "github:rustaudio:owners"); + assert_eq!(owners[0].username(), "foo"); + assert_eq!(owners[1].username(), "github:rustaudio:owners"); let response = user .add_named_owner(CRATE_NAME, "github:rustaudio:cratesio-push") @@ -38,8 +38,8 @@ async fn test_issue_1205() -> anyhow::Result<()> { let owners = krate.owners(&conn).await?; assert_eq!(owners.len(), 2); - assert_eq!(owners[0].login(), "foo"); - assert_eq!(owners[1].login(), "github:rustaudio:cratesio-push"); + assert_eq!(owners[0].username(), "foo"); + assert_eq!(owners[1].username(), "github:rustaudio:cratesio-push"); let response = user .remove_named_owner(CRATE_NAME, "github:rustaudio:owners") diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 6b8b1f14dbe..2cf9f1315b3 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -396,7 +396,7 @@ async fn test_unsupported_disambiguation_prefix() { let response = cookie.add_named_owner("foo", "gitlab:user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unsupported username prefix, only github and cratesio prefixes are supported"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -410,7 +410,7 @@ async fn test_disambiguated_github_username_not_found() { let response = cookie.add_named_owner("foo", "github:nonexistent").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username nonexistent. If you meant to add a github team, format is github:org:team"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username nonexistent"}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -445,7 +445,11 @@ async fn test_ambiguous_username_error() { let response = cookie.add_named_owner("foo", "user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"error: username user2 is possibly ambiguous.\n\nCaused by: \n The crates.io account `user2` is associated with GitHub user `user2-gh`.\n To confirm this is the account you want to add, please run one of the following:\n\n $ cargo owner --add cratesio:user2\n $ cargo owner --add github:user2-gh\n\n If this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username user2 is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n + To confirm this is the account you want to add, please run one of the following:\n\n + $ cargo owner --add cratesio:{username}\n + $ cargo owner --add github:{gh_login}\n\n + If this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); } #[tokio::test(flavor = "multi_thread")] diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index a82ab83a0e9..62d18e426f1 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -60,7 +60,7 @@ async fn test_unknown_user() { let response = cookie.remove_named_owner("foo", "unknown").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `unknown`"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `unknown`"}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -185,7 +185,11 @@ async fn test_remove_ambiguous_user() { let response = cookie.remove_named_owner("foo", "user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"error: username user2 is possibly ambiguous.\n\nCaused by: \n The crates.io account `user2` is associated with GitHub user `user2-gh`.\n To confirm this is the account you want to remove, please run one of the following:\n\n $ cargo owner --remove cratesio:user2\n $ cargo owner --remove github:user2-gh\n\n If this is not the account you want to remove, verify the crates.io username of the account you want."}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username {username} is ambiguous. There are two owners of this crate with the username `{username}` on different services.\n + To confirm which owner you want to remove, please run one of the following:\n\n + $ cargo owner --remove cratesio:{username}\n + $ cargo owner --remove github:{username}\n\n + If this is not the account you want to remove, verify the crates.io username of the account you want."}]}"#); } /// Test that an unsupported prefix (e.g. gitlab:) returns an error. @@ -200,7 +204,7 @@ async fn test_unsupported_disambiguation_prefix() { let response = cookie.remove_named_owner("foo", "gitlab:user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unsupported username prefix, only github and cratesio prefixes are supported"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); } /// Test that removing with nonexistent github username returns an error. diff --git a/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap b/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap index 2d5092c19a6..6d4d77d7784 100644 --- a/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap +++ b/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap @@ -1,5 +1,6 @@ --- source: src/tests/openapi.rs +assertion_line: 19 expression: response.json() --- { @@ -2787,10 +2788,12 @@ expression: response.json() "schema": { "properties": { "owners": { - "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).", + "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).\n\nTo disambiguate between crates.io and GitHub usernames, use\nthe `cratesio:username` or `github:username` prefix.", "example": [ "octocat", - "github:rust-lang:owners" + "github:rust-lang:owners", + "cratesio:some_user", + "github:other_user" ], "items": { "type": "string" @@ -2907,10 +2910,12 @@ expression: response.json() "schema": { "properties": { "owners": { - "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).", + "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).\n\nTo disambiguate between crates.io and GitHub usernames, use\nthe `cratesio:username` or `github:username` prefix.", "example": [ "octocat", - "github:rust-lang:owners" + "github:rust-lang:owners", + "cratesio:some_user", + "github:other_user" ], "items": { "type": "string" diff --git a/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap b/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap index e3c4afce4e4..ea451c5b060 100644 --- a/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap +++ b/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap @@ -1,5 +1,6 @@ --- source: src/tests/openapi.rs +assertion_line: 10 expression: response.json() --- { @@ -2434,10 +2435,12 @@ expression: response.json() "schema": { "properties": { "owners": { - "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).", + "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).\n\nTo disambiguate between crates.io and GitHub usernames, use\nthe `cratesio:username` or `github:username` prefix.", "example": [ "octocat", - "github:rust-lang:owners" + "github:rust-lang:owners", + "cratesio:some_user", + "github:other_user" ], "items": { "type": "string" @@ -2554,10 +2557,12 @@ expression: response.json() "schema": { "properties": { "owners": { - "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).", + "description": "List of owner login names to add or remove.\n\nFor users, use just the username (e.g., `\"octocat\"`).\nFor GitHub teams, use the format `github:org:team` (e.g., `\"github:rust-lang:owners\"`).\n\nTo disambiguate between crates.io and GitHub usernames, use\nthe `cratesio:username` or `github:username` prefix.", "example": [ "octocat", - "github:rust-lang:owners" + "github:rust-lang:owners", + "cratesio:some_user", + "github:other_user" ], "items": { "type": "string" diff --git a/src/tests/team.rs b/src/tests/team.rs index dd18cc5557c..641eca57377 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -32,7 +32,7 @@ async fn not_github() { .add_named_owner("foo_not_github", "dropbox:foo:foo") .await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"unknown organization handler, only 'github:org:team' is supported"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -51,19 +51,18 @@ async fn weird_name() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like /"}]}"#); } -/// Tests that `github:foo` is treated as a disambiguated username lookup. +/// Tests adding team without second `:` #[tokio::test(flavor = "multi_thread")] async fn one_colon() { let (app, _, user, token) = TestApp::init().with_token().await; let mut conn = app.db_conn().await; - CrateBuilder::new("foo_one_colon", user.as_model().id) .expect_build(&mut conn) .await; let response = token.add_named_owner("foo_one_colon", "github:foo").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username foo. If you meant to add a github team, format is github:org:team"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"missing github team argument; format is github:org:team"}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -145,7 +144,7 @@ async fn add_team_mixed_case() -> anyhow::Result<()> { let owners = krate.owners(&conn).await?; assert_eq!(owners.len(), 2); let owner = &owners[1]; - assert_eq!(owner.login(), owner.login().to_lowercase()); + assert_eq!(owner.username(), owner.username().to_lowercase()); let json = anon.crate_owner_teams("foo_mixed_case").await.good(); assert_eq!(json.teams.len(), 1); @@ -174,7 +173,7 @@ async fn add_team_as_org_owner() -> anyhow::Result<()> { let owners = krate.owners(&conn).await?; assert_eq!(owners.len(), 2); let owner = &owners[1]; - assert_eq!(owner.login(), owner.login().to_lowercase()); + assert_eq!(owner.username(), owner.username().to_lowercase()); let json = anon.crate_owner_teams("foo_org_owner").await.good(); assert_eq!(json.teams.len(), 1); From feff92477de0da7328456c7411a3a473c8cfae41 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 20 Jul 2026 13:27:28 -0400 Subject: [PATCH 19/32] tests --- crates/crates_io_database/src/models/owner.rs | 8 +- crates/crates_io_database/src/models/user.rs | 3 + .../crates_io_test_utils/src/builders/user.rs | 2 + src/controllers/krate/owners.rs | 72 ++-- src/tests/owners.rs | 4 +- src/tests/routes/crates/owners/add.rs | 358 +++++++++++++++++- src/tests/routes/crates/owners/remove.rs | 265 ++++++++++++- src/tests/team.rs | 97 ++++- 8 files changed, 752 insertions(+), 57 deletions(-) diff --git a/crates/crates_io_database/src/models/owner.rs b/crates/crates_io_database/src/models/owner.rs index 2e09aa73a7c..44fb4abbd0c 100644 --- a/crates/crates_io_database/src/models/owner.rs +++ b/crates/crates_io_database/src/models/owner.rs @@ -108,11 +108,11 @@ impl Owner { Owner::Team(team) => &team.login, } } - - pub fn gh_login(&self) -> &str { + + pub fn gh_login(&self) -> Option<&str> { match self { - Owner::User(user) => &user.gh_login, - Owner::Team(team) => &team.login, + Owner::User(user) => user.gh_username.as_deref(), + Owner::Team(team) => Some(&team.login), } } diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 1d7d478a259..bd4162b9044 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -49,6 +49,9 @@ pub struct User { pub name: Option, pub gh_id: i32, pub gh_login: String, + // Rename this field to gh_login when gh_login is removed from the user table. + #[diesel(select_expression = oauth_github::login.nullable())] + pub gh_username: Option, #[diesel(select_expression = oauth_github::avatar.nullable())] pub gh_avatar: Option, #[diesel(select_expression = oauth_github::encrypted_token.nullable())] diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index fa08f6d4ff9..ec3ef121716 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -57,6 +57,8 @@ impl<'a> UserBuilder<'a> { id: 1, name: self.display_name.map(ToString::to_string), gh_login: self.gh_login.into(), + // rename to gh_login once gh_login is removed from the User struct + gh_username: Some(self.gh_login.into()), gh_id: 123, gh_avatar: None, gh_encrypted_token: None, diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 770297105b0..fd817ae36a0 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -243,15 +243,23 @@ async fn modify_owners( for login in &logins { let parsed_login = parse_login(login)?; let login_test = |owner: &Owner| -> bool { - let username = match parsed_login { - Login::GitHubTeam { .. } => login, - Login::GitHub(u) | Login::CratesIo(u) | Login::Unprefixed(u) => u, - }; - - owner.username().to_lowercase() == username.to_lowercase() + match parsed_login { + // match against the team's github:org:team username + Login::GitHubTeam { .. } => { + owner.username().to_lowercase() == login.to_lowercase() + } + // match against the owner's github username + Login::GitHub(username) => owner + .gh_login() + .is_some_and(|u| u.to_lowercase() == username.to_lowercase()), + // match against the owner's cratesio username + Login::CratesIo(u) | Login::Unprefixed(u) => { + owner.username().to_lowercase() == u.to_lowercase() + } + } }; if owners.iter().any(login_test) { - return Err(bad_request(format_args!("`{login}` is already an owner"))); + return Err(bad_request(format_args!("{login} is already an owner"))); } match add_owner(&app, conn, user, &krate, parsed_login).await { @@ -347,18 +355,7 @@ async fn add_owner( ) -> Result { match login { Login::GitHubTeam { login, org, team } => { - let encryption = &app.config.token_encryption; - add_github_team_owner( - &*app.github, - conn, - req_user, - krate, - login, - org, - team, - encryption, - ) - .await + add_github_team_owner(app, conn, req_user, krate, login, org, team).await } Login::GitHub(username) => { let oauth = OauthGithub::find_by_login(conn, username) @@ -403,16 +400,16 @@ async fn add_owner( { let gh_login = &oauth_github.login; let error = format_args!( - "username {username} is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n - To confirm this is the account you want to add, please run one of the following:\n\n - $ cargo owner --add cratesio:{username}\n - $ cargo owner --add github:{gh_login}\n\n - If this is not the account you want to add, verify the crates.io username of the account you want.", + "username `{username}` is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n\n\ + To confirm this is the account you want to add, please run one of the following:\n\n\ + $ cargo owner --add cratesio:{username}\n\ + $ cargo owner --add github:{gh_login}\n\n\ + If this is not the account you want to add, verify the crates.io username of the account you want.", ); return Err(OwnerAddError::AppError(bad_request(error))); } - + invite_user_owner(app, conn, req_user, user, username, krate).await } } @@ -422,7 +419,7 @@ async fn remove_owner( krate: &Crate, conn: &mut AsyncPgConnection, login: Login<'_>, - owners: &Vec, + owners: &[Owner], ) -> Result<(), BoxedAppError> { match login { Login::GitHubTeam { login, .. } => krate.owner_remove_with_username(conn, login).await?, @@ -432,9 +429,10 @@ async fn remove_owner( let cratesio_owner_to_remove = owners .iter() .find(|o| o.username().to_lowercase() == username.to_lowercase()); - let github_owner_to_remove = owners - .iter() - .find(|o| o.gh_login().to_lowercase() == username.to_lowercase()); + let github_owner_to_remove = owners.iter().find(|o| { + o.gh_login() + .is_some_and(|u| u.to_lowercase() == username.to_lowercase()) + }); // check if ambiguous. assumes usernames are unique on separate services. if let Some(cratesio_owner) = cratesio_owner_to_remove @@ -442,11 +440,11 @@ async fn remove_owner( && cratesio_owner.id() != github_owner.id() { let error = format_args!( - "username {username} is ambiguous. There are two owners of this crate with the username `{username}` on different services.\n - To confirm which owner you want to remove, please run one of the following:\n\n - $ cargo owner --remove cratesio:{username}\n - $ cargo owner --remove github:{username}\n\n - If this is not the account you want to remove, verify the crates.io username of the account you want.", + "username `{username}` is ambiguous. There are two owners of this crate with the username `{username}` on different services.\n\n\ + To confirm which owner you want to remove, please run one of the following:\n\n\ + $ cargo owner --remove cratesio:{username}\n\ + $ cargo owner --remove github:{username}\n\n\ + If this is not the account you want to remove, verify the crates.io username of the account you want.", ); return Err(bad_request(error)); @@ -543,15 +541,17 @@ async fn invite_user_owner( /// correctly parsed out of the full `login`. `login` is passed as a /// convenience to avoid rebuilding it. async fn add_github_team_owner( - gh_client: &dyn GitHubClient, + app: &App, conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, login: &str, org: &str, team: &str, - encryption: &TokenEncryption, ) -> Result { + let gh_client = &*app.github; + let encryption = &app.config.token_encryption; + // Always recreate teams to get the most up-to-date GitHub ID let team = create_or_update_github_team(gh_client, conn, login, org, team, req_user, encryption) diff --git a/src/tests/owners.rs b/src/tests/owners.rs index ce8563f3459..e5a963ffaa3 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -242,7 +242,7 @@ async fn modify_multiple_owners() -> anyhow::Result<()> { .add_named_owners("owners_multiple", &["user2", username]) .await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`foo` is already an owner"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"foo is already an owner"}]}"#); assert_eq!(krate.owners(&conn).await?.len(), 1); // Adding multiple users at once succeeds. @@ -380,7 +380,7 @@ async fn add_existing_team() { assert_eq!(ret.status(), StatusCode::BAD_REQUEST); assert_eq!( ret.text(), - r#"{"errors":[{"detail":"`github:test_org:bananas` is already an owner"}]}"# + r#"{"errors":[{"detail":"github:test_org:bananas is already an owner"}]}"# ); } diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 2cf9f1315b3..1f25edb8b95 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -1,6 +1,7 @@ use crate::builders::{CrateBuilder, OauthGithubBuilder}; use crate::owners::expire_invitation; use crate::util::{RequestHelper, TestApp}; +use crates_io::models::CrateOwner; use crates_io::models::token::{CrateScope, EndpointScope}; use insta::assert_snapshot; @@ -445,11 +446,7 @@ async fn test_ambiguous_username_error() { let response = cookie.add_named_owner("foo", "user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username user2 is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n - To confirm this is the account you want to add, please run one of the following:\n\n - $ cargo owner --add cratesio:{username}\n - $ cargo owner --add github:{gh_login}\n\n - If this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username `user2` is possibly ambiguous. The crates.io account `user2` is associated with GitHub user `user2-gh`.\n\nTo confirm this is the account you want to add, please run one of the following:\n\n$ cargo owner --add cratesio:user2\n$ cargo owner --add github:user2-gh\n\nIf this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); } #[tokio::test(flavor = "multi_thread")] @@ -506,5 +503,354 @@ async fn test_already_owner_error() { .add_named_owner("foo", &cookie.as_model().gh_login) .await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`foo` is already an owner"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"foo is already an owner"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_add_mixed_case_unprefixed_login() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + app.db_new_user("user2").await; + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "USer2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user USer2 has been invited to be an owner of crate foo","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_add_mixed_case_cratesio_login() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + app.db_new_user("user2").await; + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "cratesio:USeR2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user USeR2 has been invited to be an owner of crate foo","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_add_mixed_case_github_login() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2-gh") + .insert(&mut conn) + .await; + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github:UseR2-gh").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user UseR2-gh has been invited to be an owner of crate foo","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_already_owner_unprefixed() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let user2 = app.db_new_user("user2").await; + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // add existing owner + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.add_named_owner("foo", "user2").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"user2 is already an owner"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_already_owner_cratesio() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let user2 = app.db_new_user("user2").await; + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // add existing owner + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.add_named_owner("foo", "cratesio:user2").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"cratesio:user2 is already an owner"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_already_owner_github() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + // `user2` has matching crates.io username and GitHub login. + let user2 = app.db_new_user("user2").await; + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2") + .insert(&mut conn) + .await; + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // add existing owner + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.add_named_owner("foo", "github:user2").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"github:user2 is already an owner"}]}"#); +} + +/// An existing owner whose crates.io username differs from their GitHub login +/// is still detected as "already an owner" when re-added via the `github:` +/// prefix, because the duplicate check matches `github:` logins against the +/// owner's GitHub login (not their crates.io username). +#[tokio::test(flavor = "multi_thread")] +async fn test_already_owner_github_mismatched_username() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2-gh") + .insert(&mut conn) + .await; + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // add existing owner + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.add_named_owner("foo", "github:user2-gh").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"github:user2-gh is already an owner"}]}"#); +} + +// A login is rejected before any database lookup when it has the wrong number +// of colons, empty components, or invalid characters. + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_team_with_extra_component() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .add_named_owner("foo", "github:alice:team:extra") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_org_with_extra_component() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github::team:extra").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_org() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github::team").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_team() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github:org:").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"team cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_github_username() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github:").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_cratesio_username() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "cratesio:").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_login() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_single_colon() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", ":").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_double_colon() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "::").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_github_username_with_invalid_char() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github:a&lice*").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot contain special characters like &"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_org_with_invalid_char() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github:or&g:team").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like &"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_team_with_invalid_char() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "github:org:te@m").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"team cannot contain special characters like @"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_unprefixed_login_with_invalid_char() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "a&lice").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot contain special characters like &"}]}"#); } diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index 62d18e426f1..b23ae0ae40e 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -162,18 +162,136 @@ async fn test_remove_uppercase_team() { #[tokio::test(flavor = "multi_thread")] async fn test_remove_ambiguous_user() { let (app, _, cookie) = TestApp::full().with_user().await; - let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice", "alice-gh").await; + let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice") + .insert(&mut conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + for user in [&cratesio_alice, &github_alice] { + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + } + + let response = cookie.remove_named_owner("foo", "alice").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username `alice` is ambiguous. There are two owners of this crate with the username `alice` on different services.\n\nTo confirm which owner you want to remove, please run one of the following:\n\n$ cargo owner --remove cratesio:alice\n$ cargo owner --remove github:alice\n\nIf this is not the account you want to remove, verify the crates.io username of the account you want."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_ambiguous_user_with_cratesio_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice", "alice-gh").await; + let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice") + .insert(&mut conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + for user in [&cratesio_alice, &github_alice] { + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + } + + let response = cookie.remove_named_owner("foo", "cratesio:alice").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_ambiguous_user_with_github_prefix() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice", "alice-gh").await; + let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice") + .insert(&mut conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + for user in [&cratesio_alice, &github_alice] { + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + } + + let response = cookie.remove_named_owner("foo", "github:alice").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_unprefixed_non_ambiguous() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user("user2").await; let mut conn = app.db_conn().await; OauthGithubBuilder::for_user(user2.as_model()) - .with_login("user2-gh") + .with_login("user2") .insert(&mut conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) .await; + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + let response = cookie.remove_named_owner("foo", "user2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_unprefixed_username_only() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; CrateOwner::builder() .crate_id(krate.id) .user_id(user2.as_model().id) @@ -184,12 +302,145 @@ async fn test_remove_ambiguous_user() { .unwrap(); let response = cookie.remove_named_owner("foo", "user2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_mixed_case_cratesio() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user("user2").await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "cratesio:USer2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_mixed_case_github() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; + let mut conn = app.db_conn().await; + + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2-gh") + .insert(&mut conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "github:useR2-gH").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_team_with_extra_component() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie + .remove_named_owner("foo", "github:alice:team:extra") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_org() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.remove_named_owner("foo", "github::team").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_team() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.remove_named_owner("foo", "github:org:").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"team cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_cratesio_username() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.remove_named_owner("foo", "cratesio:").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_empty_login() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.remove_named_owner("foo", "").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_reject_github_username_with_invalid_char() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.remove_named_owner("foo", "github:a&lice*").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username {username} is ambiguous. There are two owners of this crate with the username `{username}` on different services.\n - To confirm which owner you want to remove, please run one of the following:\n\n - $ cargo owner --remove cratesio:{username}\n - $ cargo owner --remove github:{username}\n\n - If this is not the account you want to remove, verify the crates.io username of the account you want."}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username cannot contain special characters like &"}]}"#); } /// Test that an unsupported prefix (e.g. gitlab:) returns an error. diff --git a/src/tests/team.rs b/src/tests/team.rs index 641eca57377..05be27d37b3 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -51,7 +51,6 @@ async fn weird_name() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like /"}]}"#); } -/// Tests adding team without second `:` #[tokio::test(flavor = "multi_thread")] async fn one_colon() { let (app, _, user, token) = TestApp::init().with_token().await; @@ -62,7 +61,101 @@ async fn one_colon() { let response = token.add_named_owner("foo_one_colon", "github:foo").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"missing github team argument; format is github:org:team"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username foo"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn too_many_colons() { + let (app, _, user, token) = TestApp::init().with_token().await; + let mut conn = app.db_conn().await; + CrateBuilder::new("foo_too_many_colons", user.as_model().id) + .expect_build(&mut conn) + .await; + + let response = token + .add_named_owner("foo_too_many_colons", "github:test:core:extra") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid argument. only github:org:team, github:username, cratesio:username and username are supported."}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn empty_org() { + let (app, _, user, token) = TestApp::init().with_token().await; + let mut conn = app.db_conn().await; + CrateBuilder::new("foo_empty_org", user.as_model().id) + .expect_build(&mut conn) + .await; + + let response = token.add_named_owner("foo_empty_org", "github::core").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot be empty"}]}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn empty_team() { + let (app, _, user, token) = TestApp::init().with_token().await; + let mut conn = app.db_conn().await; + CrateBuilder::new("foo_empty_team", user.as_model().id) + .expect_build(&mut conn) + .await; + + let response = token + .add_named_owner("foo_empty_team", "github:test-org:") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"team cannot be empty"}]}"#); +} + +/// Re-adding a team that is already an owner is rejected. +#[tokio::test(flavor = "multi_thread")] +async fn already_owner_team() { + let (app, _) = TestApp::init().empty().await; + let mut conn = app.db_conn().await; + let user = app.db_new_user("user-all-teams").await; + let token = user.db_new_token("arbitrary token name").await; + + CrateBuilder::new("foo_already_team", user.as_model().id) + .expect_build(&mut conn) + .await; + + token + .add_named_owner("foo_already_team", "github:test-org:core") + .await + .good(); + + let response = token + .add_named_owner("foo_already_team", "github:test-org:core") + .await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"github:test-org:core is already an owner"}]}"#); +} + +/// Removing a team owner works and is case-insensitive in the team name. +#[tokio::test(flavor = "multi_thread")] +async fn remove_team_case_insensitive() { + let (app, anon) = TestApp::init().empty().await; + let mut conn = app.db_conn().await; + let user = app.db_new_user("user-all-teams").await; + let token = user.db_new_token("arbitrary token name").await; + + CrateBuilder::new("foo_remove_team_case", user.as_model().id) + .expect_build(&mut conn) + .await; + + token + .add_named_owner("foo_remove_team_case", "github:test-org:core") + .await + .good(); + + let response = token + .remove_named_owner("foo_remove_team_case", "github:test-ORG:COre") + .await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); + + let json = anon.crate_owner_teams("foo_remove_team_case").await.good(); + assert_eq!(json.teams.len(), 0); } #[tokio::test(flavor = "multi_thread")] From ce90cd21ae9942e31daa6b360d35f765e5209d96 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 20 Jul 2026 13:52:11 -0400 Subject: [PATCH 20/32] rebase and resolve merge conflicts --- src/controllers/session.rs | 45 --------------------------- src/tests/routes/crates/owners/add.rs | 2 -- src/tests/team.rs | 5 +-- 3 files changed, 3 insertions(+), 49 deletions(-) diff --git a/src/controllers/session.rs b/src/controllers/session.rs index fa101efbe34..e6749e375ac 100644 --- a/src/controllers/session.rs +++ b/src/controllers/session.rs @@ -170,7 +170,6 @@ async fn create_or_update_user( conn.transaction(async |conn| { let update_result = update_user(gh_user, encrypted_token, conn).await; -<<<<<<< HEAD match update_result { Ok(user_id) => Ok(user_id), Err(diesel::result::Error::NotFound) => { @@ -180,50 +179,6 @@ async fn create_or_update_user( // a one-to-one relationship; this will need to be changed if/when we allow // crates.io users to link more than one GitHub account to their crates.io account. create_user(gh_user, encrypted_token, emails, conn).await -======= - let user_id = new_user.insert_or_update(conn).await?; - - // To assist in eventually someday allowing OAuth with more than GitHub, also - // write the GitHub info to the `oauth_github` table. This table is read when - // loading user details (e.g. the avatar), so a failure to write must fail the - // request just like a failure to write to the `users` table. - let new_oauth_github = NewOauthGithub::builder() - .user_id(user_id) - .account_id(new_user.gh_id as i64) - .encrypted_token(new_user.gh_encrypted_token) - .login(new_user.gh_login) - .maybe_avatar(user.avatar_url.as_deref()) - .build(); - - new_oauth_github.insert_or_update(conn).await?; - - // To send the user an account verification email - if let Some(user_email) = user.email.as_deref() { - let new_email = NewEmail::builder() - .user_id(user_id) - .email(user_email) - .build(); - - if let Some(token) = new_email.insert_if_missing(conn).await? { - let email = EmailMessage::from_template( - "user_confirm", - context! { - user_name => new_user.gh_login, - domain => emails.domain, - token => token.expose_secret() - }, - ); - - match email { - Ok(email) => { - // Swallows any error. Some users might insert an invalid email address here. - let _ = emails.send(user_email, email).await; - } - Err(error) => { - warn!("Failed to render user confirmation email template: {error}"); - } - }; ->>>>>>> d2608c9e2 (split out refactoring changes) } Err(error) => Err(error), } diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 1f25edb8b95..85f8a9475ec 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -668,8 +668,6 @@ async fn test_already_owner_github_mismatched_username() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"github:user2-gh is already an owner"}]}"#); } -// A login is rejected before any database lookup when it has the wrong number -// of colons, empty components, or invalid characters. #[tokio::test(flavor = "multi_thread")] async fn test_reject_team_with_extra_component() { diff --git a/src/tests/team.rs b/src/tests/team.rs index 05be27d37b3..d5ac8e8ff87 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -51,6 +51,7 @@ async fn weird_name() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like /"}]}"#); } +/// Resolved as a disambiguated username #[tokio::test(flavor = "multi_thread")] async fn one_colon() { let (app, _, user, token) = TestApp::init().with_token().await; @@ -59,9 +60,9 @@ async fn one_colon() { .expect_build(&mut conn) .await; - let response = token.add_named_owner("foo_one_colon", "github:foo").await; + let response = token.add_named_owner("foo_one_colon", "github:user2").await; assert_snapshot!(response.status(), @"400 Bad Request"); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username foo"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with github username user2"}]}"#); } #[tokio::test(flavor = "multi_thread")] From b9d834804012f570d65e4e7b175c8c635435b75d Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 20 Jul 2026 13:57:29 -0400 Subject: [PATCH 21/32] minor formatting updates --- crates/crates_io_database/src/models/user.rs | 2 +- crates/crates_io_test_utils/src/builders/user.rs | 6 +++++- src/tests/routes/crates/owners/add.rs | 11 +++++------ src/tests/routes/crates/owners/remove.rs | 12 ++++++------ ...ration__openapi__openapi_internal_snapshot-2.snap | 1 - .../integration__openapi__openapi_snapshot-2.snap | 1 - 6 files changed, 17 insertions(+), 16 deletions(-) diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index bd4162b9044..0d0055f4ffd 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -49,7 +49,7 @@ pub struct User { pub name: Option, pub gh_id: i32, pub gh_login: String, - // Rename this field to gh_login when gh_login is removed from the user table. + // Rename this field to gh_login when gh_login is removed from this struct. #[diesel(select_expression = oauth_github::login.nullable())] pub gh_username: Option, #[diesel(select_expression = oauth_github::avatar.nullable())] diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index ec3ef121716..e01e44e6157 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -38,7 +38,11 @@ impl<'a> UserBuilder<'a> { } pub fn with_username(self, username: &'a str) -> Self { - Self { username, gh_login: username, ..self } + Self { + username, + gh_login: username, + ..self + } } pub fn with_display_name(self, display_name: &'a str) -> Self { diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 85f8a9475ec..96589a3ee13 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -437,7 +437,7 @@ async fn test_ambiguous_username_error() { OauthGithubBuilder::for_user(new_user.as_model()) .with_login("user2-gh") - .insert(&mut conn) + .insert(&conn) .await; CrateBuilder::new("foo", cookie.as_model().id) @@ -459,7 +459,7 @@ async fn test_disambiguate_with_github_prefix() { // Create oauth_github entry with the GitHub login OauthGithubBuilder::for_user(new_user.as_model()) .with_login("user2-gh") - .insert(&mut conn) + .insert(&conn) .await; CrateBuilder::new("foo", cookie.as_model().id) @@ -544,7 +544,7 @@ async fn test_add_mixed_case_github_login() { let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; OauthGithubBuilder::for_user(user2.as_model()) .with_login("user2-gh") - .insert(&mut conn) + .insert(&conn) .await; CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) @@ -614,7 +614,7 @@ async fn test_already_owner_github() { let user2 = app.db_new_user("user2").await; OauthGithubBuilder::for_user(user2.as_model()) .with_login("user2") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) @@ -647,7 +647,7 @@ async fn test_already_owner_github_mismatched_username() { let user2 = app.db_new_user_with_gh_login("user2", "user2-gh").await; OauthGithubBuilder::for_user(user2.as_model()) .with_login("user2-gh") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) .expect_build(&mut conn) @@ -668,7 +668,6 @@ async fn test_already_owner_github_mismatched_username() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"github:user2-gh is already an owner"}]}"#); } - #[tokio::test(flavor = "multi_thread")] async fn test_reject_team_with_extra_component() { let (app, _, cookie) = TestApp::full().with_user().await; diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index b23ae0ae40e..90428d1a896 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -168,7 +168,7 @@ async fn test_remove_ambiguous_user() { let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; OauthGithubBuilder::for_user(github_alice.as_model()) .with_login("alice") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -200,7 +200,7 @@ async fn test_remove_ambiguous_user_with_cratesio_prefix() { let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; OauthGithubBuilder::for_user(github_alice.as_model()) .with_login("alice") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -232,7 +232,7 @@ async fn test_remove_ambiguous_user_with_github_prefix() { let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; OauthGithubBuilder::for_user(github_alice.as_model()) .with_login("alice") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -263,7 +263,7 @@ async fn test_remove_unprefixed_non_ambiguous() { OauthGithubBuilder::for_user(user2.as_model()) .with_login("user2") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -337,7 +337,7 @@ async fn test_remove_mixed_case_github() { OauthGithubBuilder::for_user(user2.as_model()) .with_login("user2-gh") - .insert(&mut conn) + .insert(&conn) .await; let krate = CrateBuilder::new("foo", cookie.as_model().id) @@ -512,7 +512,7 @@ async fn test_disambiguate_remove_with_github_prefix() { OauthGithubBuilder::for_user(user2.as_model()) .with_login("user2-gh") - .insert(&mut conn) + .insert(&conn) .await; let response = cookie.remove_named_owner("foo", "github:user2-gh").await; diff --git a/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap b/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap index 6d4d77d7784..32abf5006a7 100644 --- a/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap +++ b/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap @@ -1,6 +1,5 @@ --- source: src/tests/openapi.rs -assertion_line: 19 expression: response.json() --- { diff --git a/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap b/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap index ea451c5b060..7a56ccec460 100644 --- a/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap +++ b/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap @@ -1,6 +1,5 @@ --- source: src/tests/openapi.rs -assertion_line: 10 expression: response.json() --- { From 2afe4974d84520e4508dd7c500b3cb9768606c48 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Mon, 20 Jul 2026 14:10:46 -0400 Subject: [PATCH 22/32] regenerate schema, fix migration to run concurrently --- .../down.sql | 4 ++-- .../metadata.toml | 1 + .../up.sql | 4 ++-- packages/crates-io-api-client/schema.ts | 14 ++++++++++++-- src/tests/team.rs | 2 +- 5 files changed, 18 insertions(+), 7 deletions(-) create mode 100644 migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/metadata.toml diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql index 76d2f28117e..7fe26e8d694 100644 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql @@ -1,2 +1,2 @@ -DROP INDEX IF EXISTS index_users_username; -DROP INDEX IF EXISTS index_oauth_github_login; +DROP INDEX CONCURRENTLY IF EXISTS index_users_username; +DROP INDEX CONCURRENTLY IF EXISTS index_oauth_github_login; diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/metadata.toml b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/metadata.toml new file mode 100644 index 00000000000..79e9221c1f2 --- /dev/null +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/metadata.toml @@ -0,0 +1 @@ +run_in_transaction = false diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql index d3a607e9c62..55ef7874679 100644 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql @@ -1,2 +1,2 @@ -CREATE INDEX IF NOT EXISTS index_users_username ON users (canon_username(username)); -CREATE INDEX IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); +CREATE INDEX CONCURRENTLY IF NOT EXISTS index_users_username ON users (canon_username(username)); +CREATE INDEX CONCURRENTLY IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); diff --git a/packages/crates-io-api-client/schema.ts b/packages/crates-io-api-client/schema.ts index ec2f411dda2..e6118dc0f1f 100644 --- a/packages/crates-io-api-client/schema.ts +++ b/packages/crates-io-api-client/schema.ts @@ -2673,9 +2673,14 @@ export interface operations { * * For users, use just the username (e.g., `"octocat"`). * For GitHub teams, use the format `github:org:team` (e.g., `"github:rust-lang:owners"`). + * + * To disambiguate between crates.io and GitHub usernames, use + * the `cratesio:username` or `github:username` prefix. * @example [ * "octocat", - * "github:rust-lang:owners" + * "github:rust-lang:owners", + * "cratesio:some_user", + * "github:other_user" * ] */ owners: string[]; @@ -2720,9 +2725,14 @@ export interface operations { * * For users, use just the username (e.g., `"octocat"`). * For GitHub teams, use the format `github:org:team` (e.g., `"github:rust-lang:owners"`). + * + * To disambiguate between crates.io and GitHub usernames, use + * the `cratesio:username` or `github:username` prefix. * @example [ * "octocat", - * "github:rust-lang:owners" + * "github:rust-lang:owners", + * "cratesio:some_user", + * "github:other_user" * ] */ owners: string[]; diff --git a/src/tests/team.rs b/src/tests/team.rs index d5ac8e8ff87..c33acbc0d99 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -51,7 +51,7 @@ async fn weird_name() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like /"}]}"#); } -/// Resolved as a disambiguated username +/// Resolved as a disambiguated username. #[tokio::test(flavor = "multi_thread")] async fn one_colon() { let (app, _, user, token) = TestApp::init().with_token().await; From eb3855922f2e60845e6ca825729100733a6ecb63 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Tue, 21 Jul 2026 01:31:40 -0400 Subject: [PATCH 23/32] fix migration --- .../2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql | 4 ++-- .../2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql index 7fe26e8d694..76d2f28117e 100644 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql @@ -1,2 +1,2 @@ -DROP INDEX CONCURRENTLY IF EXISTS index_users_username; -DROP INDEX CONCURRENTLY IF EXISTS index_oauth_github_login; +DROP INDEX IF EXISTS index_users_username; +DROP INDEX IF EXISTS index_oauth_github_login; diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql index 55ef7874679..d3a607e9c62 100644 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql +++ b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql @@ -1,2 +1,2 @@ -CREATE INDEX CONCURRENTLY IF NOT EXISTS index_users_username ON users (canon_username(username)); -CREATE INDEX CONCURRENTLY IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); +CREATE INDEX IF NOT EXISTS index_users_username ON users (canon_username(username)); +CREATE INDEX IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); From 3a924cb3a2153b5166def3e4a39b342422cb5708 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Tue, 21 Jul 2026 01:44:35 -0400 Subject: [PATCH 24/32] fix migrations --- .../2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql | 2 -- .../2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql | 2 -- .../2026-07-18-120000-0000_add_users_username_index/down.sql | 1 + .../metadata.toml | 0 .../2026-07-18-120000-0000_add_users_username_index/up.sql | 1 + .../down.sql | 1 + .../metadata.toml | 1 + .../2026-07-18-120001-0000_add_oauth_github_login_index/up.sql | 1 + 8 files changed, 5 insertions(+), 4 deletions(-) delete mode 100644 migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql delete mode 100644 migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql create mode 100644 migrations/2026-07-18-120000-0000_add_users_username_index/down.sql rename migrations/{2026-07-18-120000-0000_add_owner_lookup_indexes => 2026-07-18-120000-0000_add_users_username_index}/metadata.toml (100%) create mode 100644 migrations/2026-07-18-120000-0000_add_users_username_index/up.sql create mode 100644 migrations/2026-07-18-120001-0000_add_oauth_github_login_index/down.sql create mode 100644 migrations/2026-07-18-120001-0000_add_oauth_github_login_index/metadata.toml create mode 100644 migrations/2026-07-18-120001-0000_add_oauth_github_login_index/up.sql diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql deleted file mode 100644 index 76d2f28117e..00000000000 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/down.sql +++ /dev/null @@ -1,2 +0,0 @@ -DROP INDEX IF EXISTS index_users_username; -DROP INDEX IF EXISTS index_oauth_github_login; diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql b/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql deleted file mode 100644 index d3a607e9c62..00000000000 --- a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/up.sql +++ /dev/null @@ -1,2 +0,0 @@ -CREATE INDEX IF NOT EXISTS index_users_username ON users (canon_username(username)); -CREATE INDEX IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); diff --git a/migrations/2026-07-18-120000-0000_add_users_username_index/down.sql b/migrations/2026-07-18-120000-0000_add_users_username_index/down.sql new file mode 100644 index 00000000000..c3c61cb76c0 --- /dev/null +++ b/migrations/2026-07-18-120000-0000_add_users_username_index/down.sql @@ -0,0 +1 @@ +DROP INDEX CONCURRENTLY IF EXISTS index_users_username; diff --git a/migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/metadata.toml b/migrations/2026-07-18-120000-0000_add_users_username_index/metadata.toml similarity index 100% rename from migrations/2026-07-18-120000-0000_add_owner_lookup_indexes/metadata.toml rename to migrations/2026-07-18-120000-0000_add_users_username_index/metadata.toml diff --git a/migrations/2026-07-18-120000-0000_add_users_username_index/up.sql b/migrations/2026-07-18-120000-0000_add_users_username_index/up.sql new file mode 100644 index 00000000000..a0f484a238b --- /dev/null +++ b/migrations/2026-07-18-120000-0000_add_users_username_index/up.sql @@ -0,0 +1 @@ +CREATE INDEX CONCURRENTLY IF NOT EXISTS index_users_username ON users (canon_username(username)); diff --git a/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/down.sql b/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/down.sql new file mode 100644 index 00000000000..53a2c516a45 --- /dev/null +++ b/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/down.sql @@ -0,0 +1 @@ +DROP INDEX CONCURRENTLY IF EXISTS index_oauth_github_login; diff --git a/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/metadata.toml b/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/metadata.toml new file mode 100644 index 00000000000..79e9221c1f2 --- /dev/null +++ b/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/metadata.toml @@ -0,0 +1 @@ +run_in_transaction = false diff --git a/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/up.sql b/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/up.sql new file mode 100644 index 00000000000..17f7debfb0d --- /dev/null +++ b/migrations/2026-07-18-120001-0000_add_oauth_github_login_index/up.sql @@ -0,0 +1 @@ +CREATE INDEX CONCURRENTLY IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); From a827d1ab0af2eb91efd3cb1f08ae1bb81da6f703 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Thu, 23 Jul 2026 10:46:36 -0400 Subject: [PATCH 25/32] remove unecessary oauth_github lookup since its included in left join --- src/controllers/krate/owners.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index fd817ae36a0..74874ad7a16 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -389,16 +389,10 @@ async fn add_owner( bad_request(format_args!("could not find user with login `{username}`")) })?; - let oauth_github = OauthGithub::belonging_to(&user) - .select(OauthGithub::as_select()) - .first(conn) - .await - .optional()?; - if let Some(oauth_github) = oauth_github - && oauth_github.login.to_lowercase() != user.username.to_lowercase() + if let Some(gh_login) = user.gh_username.to_owned() + && gh_login.to_lowercase() != user.username.to_lowercase() { - let gh_login = &oauth_github.login; let error = format_args!( "username `{username}` is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n\n\ To confirm this is the account you want to add, please run one of the following:\n\n\ From 5eed9fa8bccfcdac0c6d2b4b03117a93f973cfef Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sat, 25 Jul 2026 10:37:52 -0400 Subject: [PATCH 26/32] minor comment update --- crates/crates_io_database/src/models/user.rs | 3 ++- crates/crates_io_test_utils/src/builders/user.rs | 1 - src/controllers/krate/owners.rs | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 0d0055f4ffd..430e2dcf65a 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -49,7 +49,8 @@ pub struct User { pub name: Option, pub gh_id: i32, pub gh_login: String, - // Rename this field to gh_login when gh_login is removed from this struct. + // This is the same as gh_login, but reads from oauth_github instead. + // Can rename to `gh_login` or something more appropriate when gh_login is removed from this struct. #[diesel(select_expression = oauth_github::login.nullable())] pub gh_username: Option, #[diesel(select_expression = oauth_github::avatar.nullable())] diff --git a/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index e01e44e6157..6ac9ee6bc21 100644 --- a/crates/crates_io_test_utils/src/builders/user.rs +++ b/crates/crates_io_test_utils/src/builders/user.rs @@ -61,7 +61,6 @@ impl<'a> UserBuilder<'a> { id: 1, name: self.display_name.map(ToString::to_string), gh_login: self.gh_login.into(), - // rename to gh_login once gh_login is removed from the User struct gh_username: Some(self.gh_login.into()), gh_id: 123, gh_avatar: None, diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 74874ad7a16..2129ea11973 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -389,7 +389,6 @@ async fn add_owner( bad_request(format_args!("could not find user with login `{username}`")) })?; - if let Some(gh_login) = user.gh_username.to_owned() && gh_login.to_lowercase() != user.username.to_lowercase() { From 644e9e03e2b425b736da55fb8a47d632d1e636b0 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Tue, 4 Aug 2026 18:01:01 -0400 Subject: [PATCH 27/32] canonicalization fixes and tests, resolve ambigous login first before ownership check in case of shared logins --- crates/crates_io_database/src/models/krate.rs | 8 +- crates/crates_io_database/src/models/user.rs | 56 +++++- crates/crates_io_database/src/schema.rs | 36 ++-- src/controllers/krate/owners.rs | 81 +++++--- src/tests/routes/crates/owners/add.rs | 82 ++++++++ src/tests/routes/crates/owners/remove.rs | 188 ++++++++++++++++++ src/util.rs | 1 + src/util/canon_username.rs | 65 ++++++ 8 files changed, 463 insertions(+), 54 deletions(-) create mode 100644 src/util/canon_username.rs diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index 2219d97060c..79f9072189d 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -213,7 +213,7 @@ impl Crate { Ok(users.chain(teams).collect()) } - /// Remove owner given a cratesio username + /// Remove owner given a cratesio username. pub async fn owner_remove_with_username( &self, mut conn: &AsyncPgConnection, @@ -244,7 +244,7 @@ impl Crate { WHERE crate_owners.crate_id = crate_owners_with_login.crate_id AND crate_owners.owner_id = crate_owners_with_login.owner_id AND crate_owners.owner_kind = crate_owners_with_login.owner_kind - AND lower(crate_owners_with_login.login) = lower($2);"#, + AND canon_username(crate_owners_with_login.login) = canon_username($2);"#, ); let num_updated_rows = query @@ -260,7 +260,7 @@ impl Crate { Ok(()) } - /// Remove owner given a github username + /// Remove owner given a github username. pub async fn owner_remove_with_gh_login( &self, mut conn: &AsyncPgConnection, @@ -284,7 +284,7 @@ impl Crate { WHERE crate_owners.crate_id = crate_owners_with_gh_login.crate_id AND crate_owners.owner_id = crate_owners_with_gh_login.owner_id AND crate_owners.owner_kind = crate_owners_with_gh_login.owner_kind - AND lower(crate_owners_with_gh_login.login) = lower($2);"#, + AND canon_username(crate_owners_with_gh_login.login) = canon_username($2);"#, ); let num_updated_rows = query diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 430e2dcf65a..07738313deb 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -7,8 +7,8 @@ use diesel::upsert::excluded; use diesel_async::{AsyncPgConnection, RunQueryDsl}; use serde::Serialize; -use crate::fns::lower; use crate::models::{Crate, CrateOwner, Email, OwnerKind}; +use crate::fns::canon_username; use crate::schema::{crate_owners, emails, oauth_github, users}; /// Public data for a crates.io user. @@ -79,7 +79,7 @@ impl User { username: &str, ) -> QueryResult { User::query() - .filter(lower(users::username).eq(username.to_lowercase())) + .filter(canon_username(users::username).eq(canon_username(username))) .filter(users::gh_id.ne(-1)) .order(users::gh_id.desc()) .first(&mut conn) @@ -201,7 +201,7 @@ impl OauthGithub { login: &str, ) -> QueryResult { oauth_github::table - .filter(lower(oauth_github::login).eq(login.to_lowercase())) + .filter(canon_username(oauth_github::login).eq(canon_username(login))) .filter(oauth_github::account_id.ne(-1)) .order(oauth_github::account_id.desc()) .first(&mut conn) @@ -238,3 +238,53 @@ impl NewOauthGithub<'_> { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use crates_io_test_db::TestDatabase; + + async fn insert_user( + conn: &AsyncPgConnection, + username: &str, + gh_login: &str, + account_id: i32, + ) -> QueryResult { + let user_id = NewUser::builder() + .gh_id(account_id) + .gh_login(gh_login) + .username(username) + .gh_encrypted_token(&[]) + .build() + .insert(conn) + .await?; + + NewOauthGithub::builder() + .account_id(account_id as i64) + .encrypted_token(&[]) + .login(gh_login) + .user_id(user_id) + .build() + .insert(conn) + .await?; + + Ok(user_id) + } + + #[tokio::test] + async fn find_by_login_returns_highest_account_id_among_duplicate_case_insensitive_logins() { + let test_db = TestDatabase::new(); + let conn = test_db.async_connect().await; + + insert_user(&conn, "alice", "alice", 100).await.unwrap(); + let user_id = insert_user(&conn, "alice", "ALICE", 200).await.unwrap(); + + for login in ["alice", "ALICE", "Alice"] { + let found = OauthGithub::find_by_login(&conn, login).await.unwrap(); + + assert_eq!(found.account_id, 200, "find_by_login({login:?})"); + assert_eq!(found.user_id, user_id, "find_by_login({login:?})"); + assert_eq!(found.login, "ALICE", "find_by_login({login:?})"); + } + } +} diff --git a/crates/crates_io_database/src/schema.rs b/crates/crates_io_database/src/schema.rs index e0886ce0616..1d07af8f258 100644 --- a/crates/crates_io_database/src/schema.rs +++ b/crates/crates_io_database/src/schema.rs @@ -768,6 +768,24 @@ diesel::table! { } } +diesel::table! { + /// Representation of the `recent_crate_downloads` view. + /// + /// This data represents the downloads in the last 90 days. + /// This view does not contain realtime data. + /// It is refreshed by the `update-downloads` script. + recent_crate_downloads (crate_id) { + /// The `crate_id` column of the `recent_crate_downloads` view. + /// + /// Its SQL type is `Integer`. + crate_id -> Integer, + /// The `downloads` column of the `recent_crate_downloads` table. + /// + /// Its SQL type is `BigInt`. + downloads -> BigInt, + } +} + diesel::table! { use diesel::sql_types::*; use diesel_full_text_search::Tsvector; @@ -791,24 +809,6 @@ diesel::table! { } } -diesel::table! { - /// Representation of the `recent_crate_downloads` view. - /// - /// This data represents the downloads in the last 90 days. - /// This view does not contain realtime data. - /// It is refreshed by the `update-downloads` script. - recent_crate_downloads (crate_id) { - /// The `crate_id` column of the `recent_crate_downloads` view. - /// - /// Its SQL type is `Integer`. - crate_id -> Integer, - /// The `downloads` column of the `recent_crate_downloads` table. - /// - /// Its SQL type is `BigInt`. - downloads -> BigInt, - } -} - diesel::table! { use diesel::sql_types::*; use diesel_full_text_search::Tsvector; diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 2129ea11973..0f0403955e3 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -8,6 +8,7 @@ use crate::models::{ CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam, krate::NewOwnerInvite, token::EndpointScope, }; +use crate::util::canon_username::canon_username; use crate::util::errors::{AppResult, BoxedAppError, bad_request, crate_not_found, custom}; use crate::views::EncodableOwner; use crate::{App, app::AppState}; @@ -242,27 +243,31 @@ async fn modify_owners( let mut msgs = Vec::with_capacity(logins.len()); for login in &logins { let parsed_login = parse_login(login)?; + let owner = resolve_unprefixed_login(conn, &parsed_login).await?; + let login_test = |owner: &Owner| -> bool { + // assume a case where there is a user with username alice, and a different username with the github username alice. does this currently work? match parsed_login { // match against the team's github:org:team username Login::GitHubTeam { .. } => { - owner.username().to_lowercase() == login.to_lowercase() + canon_username(owner.username()) == canon_username(login) } // match against the owner's github username Login::GitHub(username) => owner .gh_login() - .is_some_and(|u| u.to_lowercase() == username.to_lowercase()), + .is_some_and(|u| canon_username(u) == canon_username(username)), // match against the owner's cratesio username Login::CratesIo(u) | Login::Unprefixed(u) => { - owner.username().to_lowercase() == u.to_lowercase() + canon_username(owner.username()) == canon_username(u) } } }; + if owners.iter().any(login_test) { return Err(bad_request(format_args!("{login} is already an owner"))); } - match add_owner(&app, conn, user, &krate, parsed_login).await { + match add_owner(&app, conn, user, &krate, parsed_login, owner).await { // A user was successfully invited, and they must accept // the invite, and a best-effort attempt should be made // to email them the invite token for one-click @@ -344,14 +349,51 @@ async fn modify_owners( Ok(Json(ModifyResponse { msg, ok: true })) } +/// Check if an unprefixed login is ambiguous. +/// +/// Returns `Ok(None)` for prefixed logins, and Ok(user) for a resolved unprefixed login. +async fn resolve_unprefixed_login( + conn: &mut AsyncPgConnection, + login: &Login<'_>, +) -> Result, BoxedAppError> { + let Login::Unprefixed(username) = login else { + return Ok(None); + }; + + let Some(user) = User::find_by_username(conn, username).await.optional()? else { + return Err(bad_request(format_args!( + "could not find user with login `{username}`" + ))); + }; + + if let Some(gh_login) = &user.gh_username + && canon_username(gh_login) != canon_username(&user.username) + { + let error = format_args!( + "username `{username}` is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n\n\ + To confirm this is the account you want to add, please run one of the following:\n\n\ + $ cargo owner --add cratesio:{username}\n\ + $ cargo owner --add github:{gh_login}\n\n\ + If this is not the account you want to add, verify the crates.io username of the account you want.", + ); + + return Err(bad_request(error)); + } + + Ok(Some(user)) +} + /// Invites `login` as an owner of this crate, returning the created /// [`NewOwnerInvite`]. +/// +/// `owner` is the resolved login if the supplied login was unprefixed. passing it here to avoid a duplicate `find_by_username()` request to the database. async fn add_owner( app: &App, conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, login: Login<'_>, + owner: Option, ) -> Result { match login { Login::GitHubTeam { login, org, team } => { @@ -381,28 +423,9 @@ async fn add_owner( invite_user_owner(app, conn, req_user, user, username, krate).await } Login::Unprefixed(username) => { - // check if login is ambiguous - let user = User::find_by_username(conn, username) - .await - .optional()? - .ok_or_else(|| { - bad_request(format_args!("could not find user with login `{username}`")) - })?; - - if let Some(gh_login) = user.gh_username.to_owned() - && gh_login.to_lowercase() != user.username.to_lowercase() - { - let error = format_args!( - "username `{username}` is possibly ambiguous. The crates.io account `{username}` is associated with GitHub user `{gh_login}`.\n\n\ - To confirm this is the account you want to add, please run one of the following:\n\n\ - $ cargo owner --add cratesio:{username}\n\ - $ cargo owner --add github:{gh_login}\n\n\ - If this is not the account you want to add, verify the crates.io username of the account you want.", - ); - - return Err(OwnerAddError::AppError(bad_request(error))); - } - + let user = owner.ok_or_else(|| { + bad_request(format_args!("could not find user with login `{username}`")) + })?; invite_user_owner(app, conn, req_user, user, username, krate).await } } @@ -421,10 +444,10 @@ async fn remove_owner( Login::Unprefixed(username) => { let cratesio_owner_to_remove = owners .iter() - .find(|o| o.username().to_lowercase() == username.to_lowercase()); + .find(|o| canon_username(o.username()) == canon_username(username)); let github_owner_to_remove = owners.iter().find(|o| { o.gh_login() - .is_some_and(|u| u.to_lowercase() == username.to_lowercase()) + .is_some_and(|u| canon_username(u) == canon_username(username)) }); // check if ambiguous. assumes usernames are unique on separate services. @@ -465,7 +488,7 @@ enum Login<'a> { }, /// GitHub user (e.g. `github:username`). GitHub(&'a str), - /// crates.io user (`crates.io:username`). + /// crates.io user (`cratesio:username`). CratesIo(&'a str), /// Unprefixed username (`username` without any prefix) Unprefixed(&'a str), diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index 96589a3ee13..b2e051ca99c 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -449,6 +449,88 @@ async fn test_ambiguous_username_error() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username `user2` is possibly ambiguous. The crates.io account `user2` is associated with GitHub user `user2-gh`.\n\nTo confirm this is the account you want to add, please run one of the following:\n\n$ cargo owner --add cratesio:user2\n$ cargo owner --add github:user2-gh\n\nIf this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); } +#[tokio::test(flavor = "multi_thread")] +async fn test_separator_variant_gh_login_is_not_ambiguous() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let new_user = app.db_new_user_with_gh_login("user-2", "user_2").await; + + OauthGithubBuilder::for_user(new_user.as_model()) + .with_login("user_2") + .insert(&conn) + .await; + + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "user-2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user user-2 has been invited to be an owner of crate foo","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_add_separator_variant_unprefixed_login() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + app.db_new_user("user-2").await; + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + let response = cookie.add_named_owner("foo", "user_2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user user_2 has been invited to be an owner of crate foo","ok":true}"#); +} + +/// Test that ambiguity is resolved before comparing against existing owners +#[tokio::test(flavor = "multi_thread")] +async fn test_shared_login_is_ambiguous_even_when_one_account_is_already_an_owner() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice", "alice-gh").await; + OauthGithubBuilder::for_user(cratesio_alice.as_model()) + .with_login("alice-gh") + .insert(&conn) + .await; + + let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice") + .insert(&conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + // The crates.io `alice` is already an owner, the GitHub `alice` is not. + CrateOwner::builder() + .crate_id(krate.id) + .user_id(cratesio_alice.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.add_named_owner("foo", "alice").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username `alice` is possibly ambiguous. The crates.io account `alice` is associated with GitHub user `alice-gh`.\n\nTo confirm this is the account you want to add, please run one of the following:\n\n$ cargo owner --add cratesio:alice\n$ cargo owner --add github:alice-gh\n\nIf this is not the account you want to add, verify the crates.io username of the account you want."}]}"#); + + let response = cookie.add_named_owner("foo", "cratesio:alice").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"cratesio:alice is already an owner"}]}"#); + + // …while disambiguating to the GitHub `alice` invites the other account. + let response = cookie.add_named_owner("foo", "github:alice").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"user alice has been invited to be an owner of crate foo","ok":true}"#); +} + #[tokio::test(flavor = "multi_thread")] async fn test_disambiguate_with_github_prefix() { let (app, _, cookie) = TestApp::full().with_user().await; diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index 90428d1a896..fb67290a9db 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -255,6 +255,120 @@ async fn test_remove_ambiguous_user_with_github_prefix() { assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); } +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_ambiguous_user_differing_only_by_separator() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice-2", "alice-2-gh").await; + OauthGithubBuilder::for_user(cratesio_alice.as_model()) + .with_login("alice-2-gh") + .insert(&conn) + .await; + + let github_alice = app.db_new_user_with_gh_login("bob", "alice_2").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice_2") + .insert(&conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + for user in [&cratesio_alice, &github_alice] { + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + } + + let response = cookie.remove_named_owner("foo", "alice-2").await; + assert_snapshot!(response.status(), @"400 Bad Request"); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"username `alice-2` is ambiguous. There are two owners of this crate with the username `alice-2` on different services.\n\nTo confirm which owner you want to remove, please run one of the following:\n\n$ cargo owner --remove cratesio:alice-2\n$ cargo owner --remove github:alice-2\n\nIf this is not the account you want to remove, verify the crates.io username of the account you want."}]}"#); + + // The suggested commands name the owners as they are stored, so both work. + let response = cookie.remove_named_owner("foo", "github:alice_2").await; + assert_snapshot!(response.status(), @"200 OK"); + + let response = cookie.remove_named_owner("foo", "cratesio:alice-2").await; + assert_snapshot!(response.status(), @"200 OK"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_shared_login_when_only_cratesio_user_is_owner() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice", "alice-gh").await; + OauthGithubBuilder::for_user(cratesio_alice.as_model()) + .with_login("alice-gh") + .insert(&conn) + .await; + + let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice") + .insert(&conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + CrateOwner::builder() + .crate_id(krate.id) + .user_id(cratesio_alice.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "alice").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_shared_login_when_only_github_user_is_owner() { + let (app, _, cookie) = TestApp::full().with_user().await; + let mut conn = app.db_conn().await; + + let cratesio_alice = app.db_new_user_with_gh_login("alice", "alice-gh").await; + OauthGithubBuilder::for_user(cratesio_alice.as_model()) + .with_login("alice-gh") + .insert(&conn) + .await; + + let github_alice = app.db_new_user_with_gh_login("bob", "alice").await; + OauthGithubBuilder::for_user(github_alice.as_model()) + .with_login("alice") + .insert(&conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + + CrateOwner::builder() + .crate_id(krate.id) + .user_id(github_alice.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "alice").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + #[tokio::test(flavor = "multi_thread")] async fn test_remove_unprefixed_non_ambiguous() { let (app, _, cookie) = TestApp::full().with_user().await; @@ -357,6 +471,80 @@ async fn test_remove_mixed_case_github() { assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); } +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_separator_variant_unprefixed() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user("user-2").await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "user_2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_separator_variant_cratesio() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user("user-2").await; + let mut conn = app.db_conn().await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "cratesio:user_2").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_remove_separator_variant_github() { + let (app, _, cookie) = TestApp::full().with_user().await; + let user2 = app.db_new_user_with_gh_login("user2", "user-2-gh").await; + let mut conn = app.db_conn().await; + + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user-2-gh") + .insert(&conn) + .await; + + let krate = CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .await; + CrateOwner::builder() + .crate_id(krate.id) + .user_id(user2.as_model().id) + .created_by(cookie.as_model().id) + .build() + .insert(&conn) + .await + .unwrap(); + + let response = cookie.remove_named_owner("foo", "github:user_2_gh").await; + assert_snapshot!(response.status(), @"200 OK"); + assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#); +} + #[tokio::test(flavor = "multi_thread")] async fn test_reject_team_with_extra_component() { let (app, _, cookie) = TestApp::full().with_user().await; diff --git a/src/util.rs b/src/util.rs index 7ef863d203f..9661cc551a6 100644 --- a/src/util.rs +++ b/src/util.rs @@ -2,6 +2,7 @@ pub use self::io_util::{read_fill, read_le_u32}; pub use self::request_helpers::*; pub use crates_io_database::utils::token; +pub mod canon_username; pub mod diesel; pub mod errors; mod io_util; diff --git a/src/util/canon_username.rs b/src/util/canon_username.rs new file mode 100644 index 00000000000..6386d668a60 --- /dev/null +++ b/src/util/canon_username.rs @@ -0,0 +1,65 @@ +/// Replaces all instances of `-` with `_` in the given username +pub fn canon_username(username: &str) -> String { + username.replace("-", "_").to_lowercase() +} + +#[cfg(test)] +mod tests { + use super::*; + use crates_io_database::fns::canon_username as canon_username_sql; + use crates_io_test_db::TestDatabase; + use diesel_async::RunQueryDsl; + + const USERNAMES: &[(&str, &str)] = &[ + ("foo", "foo"), + ("Foo", "foo"), + ("FOO", "foo"), + ("foo-bar", "foo_bar"), + ("foo_bar", "foo_bar"), + ("Foo-Bar", "foo_bar"), + ("FOO-BAR", "foo_bar"), + ("foo-biz-bar", "foo_biz_bar"), + ("foo--bar", "foo__bar"), + ("-foo-", "_foo_"), + ("-", "_"), + ("user-2", "user_2"), + ("github:User-2", "github:user_2"), + ("", ""), + ]; + + #[test] + fn normalizes_case_and_separators() { + for &(input, expected) in USERNAMES { + assert_eq!(canon_username(input), expected, "canon_username({input:?})"); + } + } + + #[test] + fn usernames_differing_only_by_case_or_separator_match() { + assert_eq!(canon_username("foo-bar"), canon_username("foo_bar")); + assert_eq!(canon_username("Foo-Bar"), canon_username("fOO_bAR")); + assert_eq!(canon_username("user-2"), canon_username("USER_2")); + } + + #[test] + fn distinct_usernames_do_not_match() { + assert_ne!(canon_username("foobar"), canon_username("foo_bar")); + assert_ne!(canon_username("foo-bar"), canon_username("foo--bar")); + assert_ne!(canon_username("alice"), canon_username("alice2")); + } + + #[tokio::test] + async fn matches_the_canon_username_sql_implementation() { + let test_db = TestDatabase::new(); + let mut conn = test_db.async_connect().await; + + for &(input, _) in USERNAMES { + let from_sql: String = diesel::select(canon_username_sql(input)) + .get_result(&mut conn) + .await + .unwrap(); + + assert_eq!(canon_username(input), from_sql, "canon_username({input:?})"); + } + } +} From b09e6a39f6119fb39e82fc9ff9ecc1ca69973d2c Mon Sep 17 00:00:00 2001 From: moskirathe Date: Tue, 4 Aug 2026 18:11:20 -0400 Subject: [PATCH 28/32] minor update --- crates/crates_io_database/src/models/user.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 07738313deb..995ec475abd 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -254,7 +254,6 @@ mod tests { .gh_id(account_id) .gh_login(gh_login) .username(username) - .gh_encrypted_token(&[]) .build() .insert(conn) .await?; From 33d0069469e353ad86d13c01ef2ffac646024016 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Tue, 4 Aug 2026 18:17:40 -0400 Subject: [PATCH 29/32] fix lint --- crates/crates_io_database/src/models/user.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 995ec475abd..d6eb065e0fc 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -7,8 +7,8 @@ use diesel::upsert::excluded; use diesel_async::{AsyncPgConnection, RunQueryDsl}; use serde::Serialize; -use crate::models::{Crate, CrateOwner, Email, OwnerKind}; use crate::fns::canon_username; +use crate::models::{Crate, CrateOwner, Email, OwnerKind}; use crate::schema::{crate_owners, emails, oauth_github, users}; /// Public data for a crates.io user. From d42af5a576d6e6b1f65ef560d5444f26cdc08578 Mon Sep 17 00:00:00 2001 From: moskirathe Date: Tue, 4 Aug 2026 23:55:09 -0400 Subject: [PATCH 30/32] minor update --- crates/crates_io_database/src/models/user.rs | 21 ++++++++++---------- src/controllers/krate/owners.rs | 4 ---- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index d6eb065e0fc..026fb8bed57 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -248,10 +248,10 @@ mod tests { conn: &AsyncPgConnection, username: &str, gh_login: &str, - account_id: i32, + gh_id: i32, ) -> QueryResult { let user_id = NewUser::builder() - .gh_id(account_id) + .gh_id(gh_id) .gh_login(gh_login) .username(username) .build() @@ -259,7 +259,7 @@ mod tests { .await?; NewOauthGithub::builder() - .account_id(account_id as i64) + .account_id(gh_id as i64) .encrypted_token(&[]) .login(gh_login) .user_id(user_id) @@ -271,19 +271,20 @@ mod tests { } #[tokio::test] - async fn find_by_login_returns_highest_account_id_among_duplicate_case_insensitive_logins() { + async fn test_find_by_login_returns_highest_account_id_account() { let test_db = TestDatabase::new(); let conn = test_db.async_connect().await; insert_user(&conn, "alice", "alice", 100).await.unwrap(); - let user_id = insert_user(&conn, "alice", "ALICE", 200).await.unwrap(); + let user_id = insert_user(&conn, "alice", "Alice", 200).await.unwrap(); - for login in ["alice", "ALICE", "Alice"] { - let found = OauthGithub::find_by_login(&conn, login).await.unwrap(); + // case-insensitive checks + for login in ["alice", "Alice", "ALICE"] { + let user = OauthGithub::find_by_login(&conn, login).await.unwrap(); - assert_eq!(found.account_id, 200, "find_by_login({login:?})"); - assert_eq!(found.user_id, user_id, "find_by_login({login:?})"); - assert_eq!(found.login, "ALICE", "find_by_login({login:?})"); + assert_eq!(user.account_id, 200); + assert_eq!(user.user_id, user_id); + assert_eq!(user.login, "Alice"); } } } diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 0f0403955e3..06f8e899e3c 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -246,17 +246,13 @@ async fn modify_owners( let owner = resolve_unprefixed_login(conn, &parsed_login).await?; let login_test = |owner: &Owner| -> bool { - // assume a case where there is a user with username alice, and a different username with the github username alice. does this currently work? match parsed_login { - // match against the team's github:org:team username Login::GitHubTeam { .. } => { canon_username(owner.username()) == canon_username(login) } - // match against the owner's github username Login::GitHub(username) => owner .gh_login() .is_some_and(|u| canon_username(u) == canon_username(username)), - // match against the owner's cratesio username Login::CratesIo(u) | Login::Unprefixed(u) => { canon_username(owner.username()) == canon_username(u) } From c1091af697afd17d29c913707891aca43bca939a Mon Sep 17 00:00:00 2001 From: moskirathe Date: Wed, 5 Aug 2026 00:02:33 -0400 Subject: [PATCH 31/32] minor update --- src/util/canon_username.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/canon_username.rs b/src/util/canon_username.rs index 6386d668a60..d44b6def705 100644 --- a/src/util/canon_username.rs +++ b/src/util/canon_username.rs @@ -30,7 +30,7 @@ mod tests { #[test] fn normalizes_case_and_separators() { for &(input, expected) in USERNAMES { - assert_eq!(canon_username(input), expected, "canon_username({input:?})"); + assert_eq!(canon_username(input), expected); } } @@ -59,7 +59,7 @@ mod tests { .await .unwrap(); - assert_eq!(canon_username(input), from_sql, "canon_username({input:?})"); + assert_eq!(canon_username(input), from_sql); } } } From 35131d3cdaf54bedeb119232683ece870e76ccdf Mon Sep 17 00:00:00 2001 From: moskirathe Date: Sat, 8 Aug 2026 09:52:16 -0400 Subject: [PATCH 32/32] remove unused migration added in a separate pr --- .../2026-07-18-120000-0000_add_users_username_index/down.sql | 1 - .../metadata.toml | 1 - .../2026-07-18-120000-0000_add_users_username_index/up.sql | 1 - 3 files changed, 3 deletions(-) delete mode 100644 migrations/2026-07-18-120000-0000_add_users_username_index/down.sql delete mode 100644 migrations/2026-07-18-120000-0000_add_users_username_index/metadata.toml delete mode 100644 migrations/2026-07-18-120000-0000_add_users_username_index/up.sql diff --git a/migrations/2026-07-18-120000-0000_add_users_username_index/down.sql b/migrations/2026-07-18-120000-0000_add_users_username_index/down.sql deleted file mode 100644 index c3c61cb76c0..00000000000 --- a/migrations/2026-07-18-120000-0000_add_users_username_index/down.sql +++ /dev/null @@ -1 +0,0 @@ -DROP INDEX CONCURRENTLY IF EXISTS index_users_username; diff --git a/migrations/2026-07-18-120000-0000_add_users_username_index/metadata.toml b/migrations/2026-07-18-120000-0000_add_users_username_index/metadata.toml deleted file mode 100644 index 79e9221c1f2..00000000000 --- a/migrations/2026-07-18-120000-0000_add_users_username_index/metadata.toml +++ /dev/null @@ -1 +0,0 @@ -run_in_transaction = false diff --git a/migrations/2026-07-18-120000-0000_add_users_username_index/up.sql b/migrations/2026-07-18-120000-0000_add_users_username_index/up.sql deleted file mode 100644 index a0f484a238b..00000000000 --- a/migrations/2026-07-18-120000-0000_add_users_username_index/up.sql +++ /dev/null @@ -1 +0,0 @@ -CREATE INDEX CONCURRENTLY IF NOT EXISTS index_users_username ON users (canon_username(username));