Merge pull request #9930 from Turbo87/issue-2736

Fix owner removal for duplicate logins

Author: Turbo87

src/models/krate.rs

@@ -2,7 +2,7 @@ use chrono::NaiveDateTime;
 use diesel::associations::Identifiable;
 use diesel::dsl;
 use diesel::pg::Pg;
-use diesel::sql_types::{Bool, Text};
+use diesel::sql_types::{Bool, Integer, Text};
 use diesel_async::AsyncPgConnection;
 use secrecy::SecretString;
 use thiserror::Error;
@@ -18,7 +18,7 @@ use crate::schema::*;
 use crate::sql::canon_crate_name;
 use crate::util::diesel::prelude::*;
 use crate::util::diesel::Conn;
-use crate::util::errors::{version_not_found, AppResult};
+use crate::util::errors::{bad_request, version_not_found, AppResult};
 use crate::{app::App, util::errors::BoxedAppError};
 
 use super::Team;
@@ -457,12 +457,44 @@ impl Crate {
     pub fn owner_remove(&self, conn: &mut impl Conn, login: &str) -> AppResult<()> {
         use diesel::RunQueryDsl;
 
-        let owner = Owner::find_by_login(conn, login)?;
+        let query = diesel::sql_query(
+            r#"WITH crate_owners_with_login AS (
+                SELECT
+                    crate_owners.*,
+                    CASE WHEN crate_owners.owner_kind = 1 THEN
+                         teams.login
+                    ELSE
+                         users.gh_login
+                    END AS login
+                FROM crate_owners
+                LEFT JOIN teams
+                    ON crate_owners.owner_id = teams.id
+                    AND crate_owners.owner_kind = 1
+                LEFT JOIN users
+                    ON crate_owners.owner_id = users.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_login
+            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);"#,
+        );
 
-        let target = crate_owners::table.find((self.id(), owner.id(), owner.kind()));
-        diesel::update(target)
-            .set(crate_owners::deleted.eq(true))
+        let num_updated_rows = query
+            .bind::<Integer, _>(self.id)
+            .bind::<Text, _>(login)
             .execute(conn)?;
+
+        if num_updated_rows == 0 {
+            let error = format!("could not find owner with login `{login}`");
+            return Err(bad_request(error));
+        }
+
         Ok(())
     }
 

src/models/owner.rs

@@ -79,26 +79,6 @@ impl Owner {
         }
     }
 
-    /// Finds the owner by name. Never recreates a team, to ensure that
-    /// organizations that were deleted after they were added can still be
-    /// removed.
-    ///
-    /// May be a user's GH login or a full team name. This is case
-    /// sensitive.
-    pub fn find_by_login(conn: &mut impl Conn, name: &str) -> AppResult<Owner> {
-        if name.contains(':') {
-            Team::find_by_login(conn, name)
-                .optional()?
-                .map(Owner::Team)
-                .ok_or_else(|| bad_request(format_args!("could not find team with login `{name}`")))
-        } else {
-            User::find_by_login(conn, name)
-                .optional()?
-                .map(Owner::User)
-                .ok_or_else(|| bad_request(format_args!("could not find user with login `{name}`")))
-        }
-    }
-
     pub fn kind(&self) -> i32 {
         match self {
             Owner::User(_) => OwnerKind::User as i32,

src/tests/issues/issue1205.rs

@@ -41,7 +41,7 @@ async fn test_issue_1205() -> anyhow::Result<()> {
         .remove_named_owner(CRATE_NAME, "github:rustaudio:owners")
         .await;
     assert_eq!(response.status(), StatusCode::BAD_REQUEST);
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find team with login `github:rustaudio:owners`"}]}"#);
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `github:rustaudio:owners`"}]}"#);
 
     Ok(())
 }

src/tests/issues/issue2736.rs

@@ -0,0 +1,66 @@
+use crate::models::{CrateOwner, OwnerKind};
+use crate::tests::builders::CrateBuilder;
+use crate::tests::util::{RequestHelper, TestApp};
+use crates_io_database::schema::{crate_owners, users};
+use diesel::prelude::*;
+use http::StatusCode;
+use insta::assert_snapshot;
+
+/// See <https://github.com/rust-lang/crates.io/issues/2736>.
+#[tokio::test(flavor = "multi_thread")]
+async fn test_issue_2736() -> anyhow::Result<()> {
+    let (app, _) = TestApp::full().empty();
+    let mut conn = app.db_conn();
+
+    // - A user had a GitHub account named, let's say, `foo`
+    let foo1 = app.db_new_user("foo");
+
+    // - Another user `someone_else` added them as an owner of a crate
+    let someone_else = app.db_new_user("someone_else");
+
+    let krate = CrateBuilder::new("crate1", someone_else.as_model().id).expect_build(&mut conn);
+
+    diesel::insert_into(crate_owners::table)
+        .values(CrateOwner {
+            crate_id: krate.id,
+            owner_id: foo1.as_model().id,
+            created_by: someone_else.as_model().id,
+            owner_kind: OwnerKind::User,
+            email_notifications: true,
+        })
+        .execute(&mut conn)?;
+
+    // - `foo` deleted their GitHub account (but crates.io has no real knowledge of this)
+    // - `foo` recreated their GitHub account with the same username (because it was still available), but in this situation GitHub assigns them a new ID
+    // - When `foo` now logs in to crates.io, it's a different account than their old `foo` crates.io account because of the new GitHub ID (and if it wasn't, this would be a security problem)
+    let foo2 = app.db_new_user("foo");
+
+    let github_ids = users::table
+        .filter(users::gh_login.eq("foo"))
+        .select(users::gh_id)
+        .load::<i32>(&mut conn)?;
+
+    assert_eq!(github_ids.len(), 2);
+    assert_ne!(github_ids[0], github_ids[1]);
+
+    // - The new `foo` account is NOT an owner of the crate
+    let owners = krate.owners(&mut conn)?;
+    assert_eq!(owners.len(), 2);
+    assert_none!(owners.iter().find(|o| o.id() == foo2.as_model().id));
+
+    // Removing an owner, whether it's valid/current or not, should always work (if performed by another valid owner, etc)
+    let response = someone_else.remove_named_owner("crate1", "foo").await;
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_snapshot!(response.text(), @r#"{"msg":"owners successfully removed","ok":true}"#);
+
+    let owners = krate.owners(&mut conn)?;
+    assert_eq!(owners.len(), 1);
+    assert_eq!(owners[0].id(), someone_else.as_model().id);
+
+    // Once that removal works, it should be possible to add the new account as an owner
+    let response = someone_else.add_named_owner("crate1", "foo").await;
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_snapshot!(response.text(), @r#"{"msg":"user foo has been invited to be an owner of crate crate1","ok":true}"#);
+
+    Ok(())
+}

src/tests/issues/mod.rs

@@ -1 +1,2 @@
 mod issue1205;
+mod issue2736;

src/tests/routes/crates/owners/remove.rs

@@ -58,7 +58,7 @@ async fn test_unknown_user() {
 
     let response = cookie.remove_named_owner("foo", "unknown").await;
     assert_eq!(response.status(), StatusCode::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")]
@@ -72,7 +72,7 @@ async fn test_unknown_team() {
         .remove_named_owner("foo", "github:unknown:unknown")
         .await;
     assert_eq!(response.status(), StatusCode::BAD_REQUEST);
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find team with login `github:unknown:unknown`"}]}"#);
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `github:unknown:unknown`"}]}"#);
 }
 
 #[tokio::test(flavor = "multi_thread")]

src/tests/team.rs

@@ -270,13 +270,14 @@ async fn remove_nonexistent_team() {
         .execute(&mut conn)
         .expect("couldn't insert nonexistent team");
 
-    token
+    let response = token
         .remove_named_owner(
             "foo_remove_nonexistent",
             "github:test-org:this-does-not-exist",
         )
-        .await
-        .good();
+        .await;
+    assert_eq!(response.status(), StatusCode::BAD_REQUEST);
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find owner with login `github:test-org:this-does-not-exist`"}]}"#);
 }
 
 /// Test trying to publish a crate we don't own