diff --git a/crates/crates_io_database/src/models/krate.rs b/crates/crates_io_database/src/models/krate.rs index f18cf47a1e1..79f9072189d 100644 --- a/crates/crates_io_database/src/models/krate.rs +++ b/crates/crates_io_database/src/models/krate.rs @@ -213,7 +213,8 @@ impl Crate { Ok(users.chain(teams).collect()) } - pub async fn owner_remove( + /// Remove owner given a cratesio username. + pub async fn owner_remove_with_username( &self, mut conn: &AsyncPgConnection, login: &str, @@ -225,7 +226,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 @@ -243,7 +244,47 @@ 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 + .bind::(self.id) + .bind::(login) + .execute(&mut conn) + .await?; + + if num_updated_rows == 0 { + return Err(OwnerRemoveError::not_found(login)); + } + + Ok(()) + } + + /// Remove owner given a github username. + pub async fn owner_remove_with_gh_login( + &self, + mut conn: &AsyncPgConnection, + login: &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 canon_username(crate_owners_with_gh_login.login) = canon_username($2);"#, ); let num_updated_rows = query @@ -265,7 +306,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_database/src/models/mod.rs b/crates/crates_io_database/src/models/mod.rs index 49f322858b7..58d90952ebb 100644 --- a/crates/crates_io_database/src/models/mod.rs +++ b/crates/crates_io_database/src/models/mod.rs @@ -18,7 +18,8 @@ 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::{NewOauthGithub, OauthGithub}; +pub use self::user::{NewUser, PublicUser, User}; pub use self::version::{NewVersion, TopVersions, Version}; pub mod helpers; diff --git a/crates/crates_io_database/src/models/owner.rs b/crates/crates_io_database/src/models/owner.rs index c95de392df9..44fb4abbd0c 100644 --- a/crates/crates_io_database/src/models/owner.rs +++ b/crates/crates_io_database/src/models/owner.rs @@ -102,13 +102,20 @@ impl Owner { } } - pub fn login(&self) -> &str { + pub fn username(&self) -> &str { match self { - Owner::User(user) => &user.gh_login, + Owner::User(user) => &user.username, Owner::Team(team) => &team.login, } } + pub fn gh_login(&self) -> Option<&str> { + match self { + Owner::User(user) => user.gh_username.as_deref(), + Owner::Team(team) => Some(&team.login), + } + } + pub fn id(&self) -> i32 { match self { Owner::User(user) => user.id, diff --git a/crates/crates_io_database/src/models/user.rs b/crates/crates_io_database/src/models/user.rs index 96ef89e447b..026fb8bed57 100644 --- a/crates/crates_io_database/src/models/user.rs +++ b/crates/crates_io_database/src/models/user.rs @@ -7,7 +7,7 @@ use diesel::upsert::excluded; use diesel_async::{AsyncPgConnection, RunQueryDsl}; use serde::Serialize; -use crate::fns::lower; +use crate::fns::canon_username; use crate::models::{Crate, CrateOwner, Email, OwnerKind}; use crate::schema::{crate_owners, emails, oauth_github, users}; @@ -49,6 +49,10 @@ pub struct User { pub name: Option, pub gh_id: i32, pub gh_login: String, + // 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())] pub gh_avatar: Option, #[diesel(select_expression = oauth_github::encrypted_token.nullable())] @@ -70,9 +74,12 @@ impl User { .await } - pub async fn find_by_login(mut conn: &AsyncPgConnection, login: &str) -> QueryResult { + pub async fn find_by_username( + mut conn: &AsyncPgConnection, + username: &str, + ) -> QueryResult { User::query() - .filter(lower(users::gh_login).eq(login.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) @@ -188,6 +195,20 @@ 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(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) + .await + } +} + /// Represents a new crates.io user to GitHub user OAuth link to be inserted into the /// `oauth_github` table. #[derive(Insertable, Debug, Builder)] @@ -217,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, + gh_id: i32, + ) -> QueryResult { + let user_id = NewUser::builder() + .gh_id(gh_id) + .gh_login(gh_login) + .username(username) + .build() + .insert(conn) + .await?; + + NewOauthGithub::builder() + .account_id(gh_id as i64) + .encrypted_token(&[]) + .login(gh_login) + .user_id(user_id) + .build() + .insert(conn) + .await?; + + Ok(user_id) + } + + #[tokio::test] + 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(); + + // case-insensitive checks + for login in ["alice", "Alice", "ALICE"] { + let user = OauthGithub::find_by_login(&conn, login).await.unwrap(); + + assert_eq!(user.account_id, 200); + assert_eq!(user.user_id, user_id); + assert_eq!(user.login, "Alice"); + } + } +} 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/crates/crates_io_test_utils/src/builders/user.rs b/crates/crates_io_test_utils/src/builders/user.rs index c24f5550802..6ac9ee6bc21 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,16 @@ 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,11 +52,16 @@ impl<'a> UserBuilder<'a> { } } + pub fn with_gh_login(self, gh_login: &'a str) -> Self { + Self { 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_login: self.gh_login.into(), + gh_username: Some(self.gh_login.into()), gh_id: 123, gh_avatar: None, gh_encrypted_token: None, @@ -66,7 +77,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 +117,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/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)); 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/bin/crates-io/admin/delete_crate.rs b/src/bin/crates-io/admin/delete_crate.rs index 6764ad2ef40..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 GitHub username. + /// Your crates.io username. #[arg(long)] deleted_by: String, @@ -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..06f8e899e3c 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -8,12 +8,14 @@ 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}; 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::*; @@ -172,7 +174,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", "github:other_user"]))] #[serde(alias = "users")] owners: Vec, } @@ -237,21 +242,36 @@ 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 owner = resolve_unprefixed_login(conn, &parsed_login).await?; + + let login_test = |owner: &Owner| -> bool { + match parsed_login { + Login::GitHubTeam { .. } => { + canon_username(owner.username()) == canon_username(login) + } + Login::GitHub(username) => owner + .gh_login() + .is_some_and(|u| canon_username(u) == canon_username(username)), + Login::CratesIo(u) | Login::Unprefixed(u) => { + canon_username(owner.username()) == canon_username(u) + } + } + }; + 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, 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 // 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) = @@ -297,7 +317,8 @@ async fn modify_owners( msgs.join(",") } else { for login in &logins { - krate.owner_remove(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( @@ -324,20 +345,180 @@ 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: &str, + login: Login<'_>, + owner: Option, ) -> Result { - if login.contains(':') { - 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 + match login { + Login::GitHubTeam { login, org, team } => { + add_github_team_owner(app, conn, req_user, krate, login, org, team).await + } + 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 {username}" + )) + })?; + let user = User::find(conn, oauth.user_id).await?; + invite_user_owner(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}" + )) + })?; + invite_user_owner(app, conn, req_user, user, username, krate).await + } + Login::Unprefixed(username) => { + 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 + } + } +} + +async fn remove_owner( + krate: &Crate, + conn: &mut AsyncPgConnection, + login: Login<'_>, + owners: &[Owner], +) -> 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::Unprefixed(username) => { + let cratesio_owner_to_remove = owners + .iter() + .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| canon_username(u) == canon_username(username)) + }); + + // 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. 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)); + } + + 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 { + return Err(OwnerRemoveError::not_found(username).into()); + } + } + }; + Ok(()) +} + +/// Parsed login string representation +enum Login<'a> { + /// 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), + /// crates.io user (`cratesio:username`). + CratesIo(&'a str), + /// Unprefixed username (`username` without any prefix) + Unprefixed(&'a str), +} + +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' | '-' | '_')) + { + return Err(bad_request(format_args!( + "{label} cannot contain special characters like {c}" + ))); + } + + Ok(true) + } + + 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 argument. only github:org:team, github:username, cratesio:username and username are supported.", + )), } } @@ -345,14 +526,10 @@ async fn invite_user_owner( app: &App, conn: &mut AsyncPgConnection, req_user: &User, + user: User, + username: &str, 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}`")))?; - // Users are invited and must accept before being added let expires_at = Utc::now() + app.config.ownership_invitations_expiration; let invite = NewCrateOwnerInvitation { @@ -364,7 +541,7 @@ async fn invite_user_owner( match invite.create(conn).await? { NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => { - Ok(NewOwnerInvite::User(user, plaintext_token)) + Ok(NewOwnerInvite::User(user, plaintext_token, username.into())) } NewCrateOwnerInvitationOutcome::AlreadyExists => { Err(OwnerAddError::AlreadyInvited(Box::new(user))) @@ -372,41 +549,25 @@ async fn invite_user_owner( } } -async fn add_team_owner( - gh_client: &dyn GitHubClient, +/// 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( + app: &App, conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, login: &str, - encryption: &TokenEncryption, + org: &str, + team: &str, ) -> 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) - })?; + 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.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. @@ -422,7 +583,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, @@ -433,21 +594,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", 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/owners.rs b/src/tests/owners.rs index b3f42d86c7e..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"}]}"# ); } @@ -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(&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/list.rs b/src/tests/routes/crates/list.rs index cca52c9cb93..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(&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 5905825a2e5..b2e051ca99c 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; +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; @@ -383,3 +384,552 @@ 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("user2").await; + CrateBuilder::new("foo", cookie.as_model().id) + .expect_build(&mut conn) + .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":"invalid argument. only github:org:team, github:username, cratesio:username and username 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("foo", cookie.as_model().id) + .expect_build(&mut conn) + .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"}]}"#); +} + +#[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.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"}]}"#); +} + +#[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; + + 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(&conn) + .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(), @"400 Bad Request"); + 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; + let mut conn = app.db_conn().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("user2-gh") + .insert(&conn) + .await; + + 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("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_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("user2", "user2-gh").await; + + 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("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_already_owner_error() { + 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; + + // The cookie user is already the owner of the crate + let response = cookie + .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"}]}"#); +} + +#[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(&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(&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(&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"}]}"#); +} + +#[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 066a7bd7520..fb67290a9db 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -1,4 +1,4 @@ -use crate::builders::CrateBuilder; +use crate::builders::{CrateBuilder, OauthGithubBuilder}; use crate::util::{RequestHelper, TestApp}; use crates_io::models::CrateOwner; use crates_io_github::{GitHubOrganization, GitHubTeam, GitHubTeamMembership, MockGitHubClient}; @@ -158,3 +158,577 @@ 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 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(&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(&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(&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_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; + let user2 = app.db_new_user("user2").await; + let mut conn = app.db_conn().await; + + OauthGithubBuilder::for_user(user2.as_model()) + .with_login("user2") + .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", "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) + .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_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(&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_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; + 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 cannot contain special characters like &"}]}"#); +} + +/// 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":"invalid argument. only github:org:team, github:username, cratesio:username and username 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 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; + 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(&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..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(&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 d29bf6a4af0..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(&conn, &user.gh_login) + .owner_remove_with_username(&conn, &user.username) .await .unwrap(); 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..32abf5006a7 100644 --- a/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap +++ b/src/tests/snapshots/integration__openapi__openapi_internal_snapshot-2.snap @@ -2787,10 +2787,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 +2909,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..7a56ccec460 100644 --- a/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap +++ b/src/tests/snapshots/integration__openapi__openapi_snapshot-2.snap @@ -2434,10 +2434,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 +2556,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 33fac9dd686..c33acbc0d99 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,112 @@ async fn weird_name() { assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"organization cannot contain special characters like /"}]}"#); } -/// Tests adding team without second `:` +/// Resolved as a disambiguated username. #[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; + 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 user2"}]}"#); +} + +#[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":"missing github team argument; format is github:org:team"}]}"#); + 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")] @@ -145,7 +238,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 +267,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); @@ -493,7 +586,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(&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); diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index ab0c147e71f..559fa415445 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_login(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()) 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..d44b6def705 --- /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); + } + } + + #[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); + } + } +}