Stop reading from users.gh_login

Author: carols10cents

crates/crates_io_api_types/src/lib.rs

@@ -527,6 +527,14 @@ pub struct EncodableCrateLinks {
     pub reverse_dependencies: String,
 }
 
+fn url_or_deprecation_message(gh_login: Option<&str>) -> String {
+    gh_login
+        .map(|gh_login| format!("https://github.com/{gh_login}"))
+        .unwrap_or(String::from(
+            "This field is deprecated. Use LinkedAccount instead.",
+        ))
+}
+
 /// A user or team that owns a crate.
 #[derive(Serialize, Deserialize, Debug, utoipa::ToSchema)]
 #[schema(as = Owner)]
@@ -592,7 +600,7 @@ impl EncodableOwner {
     pub fn from_user(user: PublicUser) -> Self {
         Self::User {
             github_username_matches: user.github_username_matches,
-            url: format!("https://github.com/{}", user.gh_login),
+            url: url_or_deprecation_message(user.gh_login.as_deref()),
             id: user.id,
             login: user.username,
             avatar: user.gh_avatar,
@@ -771,7 +779,8 @@ impl EncodablePrivateUser {
             created_at,
             ..
         } = user;
-        let url = format!("https://github.com/{gh_login}");
+
+        let url = url_or_deprecation_message(gh_login.as_deref());
 
         EncodablePrivateUser {
             id,
@@ -840,7 +849,9 @@ impl From<PublicUser> for EncodablePublicUser {
             created_at,
             ..
         } = user;
-        let url = format!("https://github.com/{gh_login}");
+
+        let url = url_or_deprecation_message(gh_login.as_deref());
+
         EncodablePublicUser {
             id,
             avatar: gh_avatar,

crates/crates_io_database/src/models/owner.rs

@@ -104,7 +104,7 @@ impl Owner {
 
     pub fn login(&self) -> &str {
         match self {
-            Owner::User(user) => &user.gh_login,
+            Owner::User(user) => &user.username,
             Owner::Team(team) => &team.login,
         }
     }

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::{canon_username, lower};
+use crate::fns::canon_username;
 use crate::models::{Crate, CrateOwner, Email, OwnerKind};
 use crate::schema::{crate_owners, emails, oauth_github, users};
 
@@ -46,7 +46,8 @@ pub fn users_by_username<'a>(username: &'a str) -> _ {
 pub struct PublicUser {
     pub id: i32,
     pub name: Option<String>,
-    pub gh_login: String,
+    #[diesel(select_expression = oauth_github::login.nullable())]
+    pub gh_login: Option<String>,
     #[diesel(select_expression = oauth_github::avatar.nullable())]
     pub gh_avatar: Option<String>,
     #[diesel(select_expression = github_username_matches())]
@@ -76,7 +77,8 @@ pub struct User {
     pub id: i32,
     pub name: Option<String>,
     pub gh_id: i32,
-    pub gh_login: String,
+    #[diesel(select_expression = oauth_github::login.nullable())]
+    pub gh_login: Option<String>,
     #[diesel(select_expression = oauth_github::avatar.nullable())]
     pub gh_avatar: Option<String>,
     #[diesel(select_expression = oauth_github::encrypted_token.nullable())]
@@ -98,15 +100,6 @@ impl User {
             .await
     }
 
-    pub async fn find_by_login(mut conn: &AsyncPgConnection, login: &str) -> QueryResult<User> {
-        User::query()
-            .filter(lower(users::gh_login).eq(login.to_lowercase()))
-            .filter(users::gh_id.ne(-1))
-            .order(users::gh_id.desc())
-            .first(&mut conn)
-            .await
-    }
-
     pub async fn owning(krate: &Crate, mut conn: &AsyncPgConnection) -> QueryResult<Vec<Self>> {
         CrateOwner::by_owner_kind(OwnerKind::User)
             .inner_join(users::table.left_join(oauth_github::table))

crates/crates_io_test_utils/src/builders/user.rs

@@ -70,7 +70,7 @@ impl<'a> UserBuilder<'a> {
     pub fn build(self) -> User {
         User {
             id: 1,
-            gh_login: self.gh_login.into(),
+            gh_login: Some(self.gh_login.into()),
             name: self.display_name.map(ToString::to_string),
             gh_id: self.gh_id.unwrap_or(123),
             gh_avatar: None,
@@ -115,7 +115,7 @@ impl<'a> OauthGithubBuilder<'a> {
             user_id: user.id,
             account_id: user.gh_id as i64,
             encrypted_token: &ENCRYPTED_TOKEN,
-            login: &user.gh_login,
+            login: user.gh_login.as_ref().unwrap_or(&user.username),
             avatar: None,
         }
     }

src/controllers/helpers/authorization.rs

@@ -48,13 +48,13 @@ impl Rights {
                     }
                 }
                 Owner::Team(ref team) => {
-                    if let Some(ref auth) = auth {
+                    if let (Some(auth), Some(gh_login)) = (&auth, &user.gh_login) {
                         // Phones home to GitHub to ask if this User is a member of the given team.
                         // Note that we're assuming that the given user is the one interested in
                         // the answer. If this is not the case, then we could accidentally leak
                         // private membership information here.
                         let is_team_member = match gh_client
-                            .team_membership(team.org_id, team.github_id, &user.gh_login, auth)
+                            .team_membership(team.org_id, team.github_id, gh_login, auth)
                             .await
                         {
                             Ok(membership) => membership.is_some_and(|m| m.is_active()),

src/controllers/krate/owners.rs

@@ -1,25 +1,24 @@
 //! All routes related to managing owners of a crate
 
-use crate::controllers::helpers::authorization::Rights;
-use crate::controllers::krate::CratePath;
-use crate::models::krate::OwnerRemoveError;
-use crate::models::{Crate, Owner, PublicUser, Team, User};
+use crate::controllers::{helpers::authorization::Rights, krate::CratePath};
 use crate::models::{
-    CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam,
-    krate::NewOwnerInvite, token::EndpointScope,
+    Crate, CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam, Owner,
+    PublicUser, Team, User,
+    krate::{NewOwnerInvite, OwnerRemoveError},
+    token::EndpointScope,
+    users_by_username,
 };
+use crate::schema::oauth_github;
 use crate::util::errors::{AppResult, BoxedAppError, bad_request, custom, forbidden};
 use crate::views::EncodableOwner;
-use crate::worker::jobs::SendEmail;
-use crate::{App, app::AppState};
-use crate::{auth::AuthCheck, email::EmailMessage};
+use crate::{App, app::AppState, auth::AuthCheck, email::EmailMessage, worker::jobs::SendEmail};
 use axum::Json;
 use chrono::Utc;
 use crates_io_encryption::TokenEncryption;
 use crates_io_github::{GitHubAuth, GitHubClient, GitHubError};
 use crates_io_worker::BackgroundJob;
 use diesel::prelude::*;
-use diesel_async::{AsyncConnection, AsyncPgConnection};
+use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
 use http::StatusCode;
 use http::request::Parts;
 use minijinja::context;
@@ -368,7 +367,12 @@ async fn add_owner(
             add_github_team_owner(github, conn, req_user, krate, team, encryption).await
         }
         Login::Unprefixed(login) => {
-            let user = User::find_by_login(conn, login).await.optional()?;
+            let user = users_by_username(login)
+                .left_join(oauth_github::table)
+                .select(User::as_select())
+                .first(conn)
+                .await
+                .optional()?;
             let user = user.ok_or_else(|| {
                 bad_request(format_args!("could not find user with login `{login}`"))
             })?;
@@ -543,13 +547,16 @@ pub async fn create_or_update_github_team(
     let org_id = team.organization.id;
     let gh_login = &req_user.gh_login;
 
-    let is_team_member = gh_client
-        .team_membership(org_id, team.id, gh_login, &auth)
-        .await?
-        .is_some_and(|m| m.is_active());
+    let can_add_team = if let Some(gh_login) = gh_login {
+        let is_team_member = gh_client
+            .team_membership(org_id, team.id, gh_login, &auth)
+            .await?
+            .is_some_and(|m| m.is_active());
 
-    let can_add_team =
-        is_team_member || is_gh_org_owner(gh_client, org_id, gh_login, &auth).await?;
+        is_team_member || is_gh_org_owner(gh_client, org_id, gh_login, &auth).await?
+    } else {
+        false
+    };
 
     if !can_add_team {
         return Err(custom(

src/controllers/trustpub/github_configs/create.rs

@@ -94,13 +94,16 @@ pub async fn create_trustpub_github_config(
     let owner = &json_config.repository_owner;
 
     let encryption = &state.config.token_encryption;
-    let Some(gh_auth) = auth_user.gh_encrypted_token.as_ref() else {
+    let (Some(gh_auth), Some(login)) = (
+        auth_user.gh_encrypted_token.as_ref(),
+        auth_user.gh_login.as_ref(),
+    ) else {
         return Err(bad_request(
             "Must have a linked GitHub account to create a Trusted Publishing config",
         ));
     };
+
     let gh_auth = encryption.decrypt(gh_auth).map_err(|err| {
-        let login = &auth_user.gh_login;
         warn!("Failed to decrypt GitHub token for user {login}: {err}");
         server_error("Internal server error")
     })?;

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

@@ -211,36 +211,36 @@ async fn invite_distinct_login_user(login: &str) -> Response<OwnerResp> {
 #[tokio::test(flavor = "multi_thread")]
 async fn unprefixed_crates_io_username_verbatim() {
     let response = invite_distinct_login_user("crates-user").await;
-    assert_snapshot!(response.status(), @"400 Bad Request");
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `crates-user`"}]}"#);
+    assert_snapshot!(response.status(), @"200 OK");
+    assert_snapshot!(response.text(), @r#"{"msg":"Crates.io user crates-user has been invited to be an owner of crate foo","ok":true}"#);
 }
 
 #[tokio::test(flavor = "multi_thread")]
 async fn unprefixed_crates_io_username_case_insensitive() {
     let response = invite_distinct_login_user("CRATES-USER").await;
-    assert_snapshot!(response.status(), @"400 Bad Request");
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `CRATES-USER`"}]}"#);
+    assert_snapshot!(response.status(), @"200 OK");
+    assert_snapshot!(response.text(), @r#"{"msg":"Crates.io user crates-user has been invited to be an owner of crate foo","ok":true}"#);
 }
 
 #[tokio::test(flavor = "multi_thread")]
 async fn unprefixed_crates_io_username_separator_variant() {
     let response = invite_distinct_login_user("crates_user").await;
-    assert_snapshot!(response.status(), @"400 Bad Request");
-    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `crates_user`"}]}"#);
+    assert_snapshot!(response.status(), @"200 OK");
+    assert_snapshot!(response.text(), @r#"{"msg":"Crates.io user crates-user has been invited to be an owner of crate foo","ok":true}"#);
 }
 
 #[tokio::test(flavor = "multi_thread")]
 async fn unprefixed_github_login_verbatim() {
     let response = invite_distinct_login_user("github-user").await;
-    assert_snapshot!(response.status(), @"200 OK");
-    assert_snapshot!(response.text(), @r#"{"msg":"Crates.io user crates-user has been invited to be an owner of crate foo","ok":true}"#);
+    assert_snapshot!(response.status(), @"400 Bad Request");
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `github-user`"}]}"#);
 }
 
 #[tokio::test(flavor = "multi_thread")]
 async fn unprefixed_github_login_case_insensitive() {
     let response = invite_distinct_login_user("GITHUB-USER").await;
-    assert_snapshot!(response.status(), @"200 OK");
-    assert_snapshot!(response.text(), @r#"{"msg":"Crates.io user crates-user has been invited to be an owner of crate foo","ok":true}"#);
+    assert_snapshot!(response.status(), @"400 Bad Request");
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"could not find user with login `GITHUB-USER`"}]}"#);
 }
 
 #[tokio::test(flavor = "multi_thread")]

src/tests/user.rs

@@ -377,7 +377,7 @@ async fn write_to_users_and_oauth_github() -> anyhow::Result<()> {
     assert_eq!(u.username, gh_login);
     assert_eq!(u.name.unwrap(), gh_display_name);
     assert_eq!(u.gh_id, gh_id);
-    assert_eq!(u.gh_login, gh_login);
+    assert_eq!(u.gh_login.unwrap(), gh_login);
     assert_eq!(u.gh_avatar.unwrap(), gh_avatar);
     let decrypted_token = encryption.decrypt(&u.gh_encrypted_token.unwrap())?;
     assert_eq!(decrypted_token.expose_secret(), gh_token);
@@ -413,7 +413,7 @@ async fn write_to_users_and_oauth_github() -> anyhow::Result<()> {
     assert_eq!(u.username, different_gh_login);
     assert_eq!(u.name.unwrap(), different_gh_display_name);
     assert_eq!(u.gh_id, gh_id);
-    assert_eq!(u.gh_login, different_gh_login);
+    assert_eq!(u.gh_login.unwrap(), different_gh_login);
     assert_eq!(u.gh_avatar.unwrap(), different_gh_avatar);
     let decrypted_token = encryption.decrypt(&u.gh_encrypted_token.unwrap())?;
     assert_eq!(decrypted_token.expose_secret(), different_gh_token);
@@ -445,7 +445,7 @@ async fn write_to_users_and_oauth_github() -> anyhow::Result<()> {
         .unwrap();
     let u = User::find(&conn, uid).await?;
 
-    assert_eq!(u.gh_login, gh_login);
+    assert_eq!(u.gh_login.unwrap(), gh_login);
     assert_eq!(u.gh_id, new_gh_id);
 
     let oauth_github_records: Vec<OauthGithub> = oauth_github::table.load(&mut conn).await.unwrap();

src/tests/util/test_app.rs

@@ -161,11 +161,15 @@ impl TestApp {
         let conn = self.db_conn().await;
 
         let new_user = builder.new_user();
+        let gh_login = new_user.gh_login;
         let email = format!("{}@example.com", new_user.username);
         let id = new_user.insert(&conn).await.unwrap();
         let user = User::find(&conn, id).await.unwrap();
 
-        OauthGithubBuilder::for_user(&user).insert(&conn).await;
+        OauthGithubBuilder::for_user(&user)
+            .with_login(gh_login)
+            .insert(&conn)
+            .await;
 
         let new_email = NewEmail::builder()
             .user_id(id)