tests pass

Author: carols10cents

crates/crates_io_database/src/models/user.rs

@@ -42,6 +42,9 @@ impl User {
     pub async fn find_by_login(mut conn: &AsyncPgConnection, login: &str) -> QueryResult<User> {
         User::query()
             .filter(lower(users::gh_login).eq(login.to_lowercase()))
+            // This ordering will be unnecessary when we switch to crates.io usernames that
+            // are unique.
+            .order(users::id.desc())
             .first(&mut conn)
             .await
     }
@@ -185,6 +188,7 @@ impl OauthGithubUpdate<'_> {
         diesel::insert_into(oauth_github::table)
             .values((
                 oauth_github::user_id.eq(user_id),
+                oauth_github::account_id.eq(self.account_id),
                 oauth_github::encrypted_token.eq(self.encrypted_token),
                 oauth_github::login.eq(self.login),
                 oauth_github::avatar.eq(self.avatar),

crates/crates_io_database_dump/src/dump-db.toml

@@ -254,15 +254,10 @@ id in (
 id = "public"
 gh_login = "public"
 name = "public"
-gh_avatar = "public"
-gh_id = "public"
 account_lock_reason = "private"
 account_lock_until = "private"
 is_admin = "private"
 publish_notifications = "private"
-gh_encrypted_token = "private"
-[users.column_defaults]
-gh_encrypted_token = "''"
 
 [version_downloads]
 dependencies = ["versions"]

src/controllers/session.rs

@@ -156,44 +156,53 @@ pub async fn save_user_to_database(
         .maybe_avatar(gh_user.avatar_url.as_deref())
         .build();
 
-    // should start a transaction here
-
-    match oauth_github_update.update(conn).await {
-        // oauth github exists, which means user must too
-        Ok(oauth_github) => {
-            let user = User::find(conn, oauth_github.user_id).await?;
-
-            // update user display name; eventually we should stop syncing this with github
-            diesel::update(users::table)
-                .filter(users::id.eq(user.id))
-                .set(users::name.eq(gh_user.name.as_ref()))
-                .execute(conn)
-                .await?;
-
-            Ok(user.id)
-        }
-        // oauth github does not exist, which means user must not either and we need to create both
-        Err(diesel::result::Error::NotFound) => {
-            let new_user = NewUser::builder()
-                .gh_login(&gh_user.login)
-                .maybe_name(gh_user.name.as_deref())
-                .build();
-
-            let user_id =
-                create_or_update_user(&new_user, gh_user.email.as_deref(), emails, conn).await?;
-
-            oauth_github_update.insert(conn, user_id).await?;
-
-            Ok(user_id)
-        }
-        Err(error) if is_read_only_error(&error) => {
-            // If we're in read only mode, we can't update their details
-            // just look for an existing user
-            find_user_by_gh_id(conn, gh_user.id).await?.ok_or(error)
+    conn.transaction(async |conn| {
+        match dbg!(oauth_github_update.update(conn).await) {
+            // oauth github exists, which means user must too
+            Ok(oauth_github) => {
+                let user = User::find(conn, oauth_github.user_id).await?;
+                dbg!(&user);
+
+                // update user display name and gh_login; eventually we should stop syncing these
+                // with github
+                diesel::update(users::table)
+                    .filter(users::id.eq(user.id))
+                    .set((
+                        users::name.eq(gh_user.name.as_ref()),
+                        users::gh_login.eq(&gh_user.login),
+                    ))
+                    .execute(conn)
+                    .await?;
+
+                Ok(user.id)
+            }
+            // oauth github does not exist, which means user must not either and we need to create both
+            Err(diesel::result::Error::NotFound) => {
+                let new_user = NewUser::builder()
+                    .gh_login(&gh_user.login)
+                    .maybe_name(gh_user.name.as_deref())
+                    .build();
+                dbg!(&new_user);
+
+                let user_id =
+                    create_or_update_user(&new_user, gh_user.email.as_deref(), emails, conn)
+                        .await?;
+                dbg!(&user_id);
+
+                dbg!(oauth_github_update.insert(conn, user_id).await)?;
+
+                Ok(user_id)
+            }
+            Err(error) if is_read_only_error(&error) => {
+                // If we're in read only mode, we can't update their details
+                // just look for an existing user
+                find_user_by_gh_id(conn, gh_user.id).await?.ok_or(error)
+            }
+            // other error
+            Err(e) => Err(dbg!(e)),
         }
-        // other error
-        Err(e) => Err(e),
-    }
+    })
+    .await
 }
 
 /// Inserts the user into the database, or updates an existing one.
@@ -206,41 +215,38 @@ async fn create_or_update_user(
     emails: &Emails,
     conn: &mut AsyncPgConnection,
 ) -> QueryResult<i32> {
-    conn.transaction(async |conn| {
-        let user_id = new_user.insert(conn).await?;
-
-        // To send the user an account verification email
-        if let Some(user_email) = email {
-            let new_email = NewEmail::builder()
-                .user_id(user_id)
-                .email(user_email)
-                .build();
-
-            if let Some(token) = new_email.insert_if_missing(conn).await? {
-                let email = EmailMessage::from_template(
-                    "user_confirm",
-                    context! {
-                        user_name => new_user.gh_login,
-                        domain => emails.domain,
-                        token => token.expose_secret()
-                    },
-                );
-
-                match email {
-                    Ok(email) => {
-                        // Swallows any error. Some users might insert an invalid email address here.
-                        let _ = emails.send(user_email, email).await;
-                    }
-                    Err(error) => {
-                        warn!("Failed to render user confirmation email template: {error}");
-                    }
-                };
-            }
+    let user_id = new_user.insert(conn).await?;
+
+    // To send the user an account verification email
+    if let Some(user_email) = email {
+        let new_email = NewEmail::builder()
+            .user_id(user_id)
+            .email(user_email)
+            .build();
+
+        if let Some(token) = new_email.insert_if_missing(conn).await? {
+            let email = EmailMessage::from_template(
+                "user_confirm",
+                context! {
+                    user_name => new_user.gh_login,
+                    domain => emails.domain,
+                    token => token.expose_secret()
+                },
+            );
+
+            match email {
+                Ok(email) => {
+                    // Swallows any error. Some users might insert an invalid email address here.
+                    let _ = emails.send(user_email, email).await;
+                }
+                Err(error) => {
+                    warn!("Failed to render user confirmation email template: {error}");
+                }
+            };
         }
+    }
 
-        Ok(user_id)
-    })
-    .await
+    Ok(user_id)
 }
 
 async fn find_user_by_gh_id(mut conn: &AsyncPgConnection, gh_id: i32) -> QueryResult<Option<i32>> {

src/tests/mod.rs

@@ -1,12 +1,14 @@
 use crate::util::{RequestHelper, TestApp};
-use crates_io::models::{NewCategory, NewTeam, NewUser};
+use crates_io::models::{NewCategory, NewTeam, NewUser, OauthGithubUpdate};
 use crates_io::views::{
     EncodableCategory, EncodableCrate, EncodableKeyword, EncodableOwner, EncodableVersion,
     GoodCrate,
 };
 
 use crate::util::github::next_gh_id;
+use crates_io::util::gh_token_encryption::GitHubTokenEncryption;
 use serde::{Deserialize, Serialize};
+use std::sync::LazyLock;
 
 mod account_lock;
 mod authentication;
@@ -94,6 +96,20 @@ fn new_user(login: &str) -> NewUser<'_> {
     NewUser::builder().gh_login(login).build()
 }
 
+fn new_oauth_github(login: &str) -> OauthGithubUpdate<'_> {
+    static ENCRYPTED_TOKEN: LazyLock<Vec<u8>> = LazyLock::new(|| {
+        GitHubTokenEncryption::for_testing()
+            .encrypt("some random token")
+            .unwrap()
+    });
+
+    OauthGithubUpdate::builder()
+        .account_id(next_gh_id() as i64)
+        .encrypted_token(&ENCRYPTED_TOKEN)
+        .login(login)
+        .build()
+}
+
 fn new_team(login: &str) -> NewTeam<'_> {
     NewTeam::builder()
         .login(login)

src/tests/user.rs

@@ -47,9 +47,19 @@ async fn updating_existing_user_doesnt_change_api_token() -> anyhow::Result<()>
     // Use the original API token to find the now updated user
     let hashed_token = assert_ok!(HashedToken::parse(token));
     let api_token = assert_ok!(ApiToken::find_by_api_token(&mut conn, &hashed_token).await);
-    let user = assert_ok!(User::find(&conn, api_token.user_id).await);
 
+    let user = assert_ok!(User::find(&conn, api_token.user_id).await);
+    // For now, the user's `gh_login` should be kept in sync; when we stop updating usernames
+    // from GitHub this test should be changed to assert that the user record remains `foo`
     assert_eq!(user.gh_login, "bar");
+    // The associated `oauth_github` record's username should be updated to `bar`
+    let oauth_github_login: String = oauth_github::table
+        .filter(oauth_github::user_id.eq(api_token.user_id))
+        .select(oauth_github::login)
+        .first(&mut conn)
+        .await?;
+    assert_eq!(oauth_github_login, "bar");
+
     let decrypted_token = encryption.decrypt(&user.gh_encrypted_token.unwrap())?;
     assert_eq!(decrypted_token.secret(), "bar_token");
 

src/tests/util/test_app.rs

@@ -132,30 +132,35 @@ impl TestApp {
         self.0.test_database.async_connect().await
     }
 
-    /// Create a new user with a verified email address in the database
-    /// (`<username>@example.com`) and return a mock user session.
+    /// Create a new user with:
+    ///
+    /// - a verified email address in the database (`<username>@example.com`)
+    /// - an associated `oauth_github` record
+    ///
+    /// and return a mock user session.
     ///
     /// This method updates the database directly
     pub async fn db_new_user(&self, username: &str) -> MockCookieUser {
         let conn = self.db_conn().await;
 
-        let email = format!("{username}@example.com");
-
         let new_user = crate::new_user(username);
         let id = new_user.insert(&conn).await.unwrap();
 
+        let email = format!("{username}@example.com");
         let new_email = NewEmail::builder()
             .user_id(id)
             .email(&email)
             .verified(true)
             .build();
-
         new_email.insert(&conn).await.unwrap();
 
+        let oauth_github = crate::new_oauth_github(username);
+        oauth_github.insert(&conn, id).await.unwrap();
+
         let user = User {
             id,
             name: new_user.name.map(str::to_string),
-            gh_id: None,
+            gh_id: Some(oauth_github.account_id),
             gh_login: new_user.gh_login.to_string(),
             gh_avatar: None,
             gh_encrypted_token: None,

src/tests/worker/sync_admins.rs

@@ -89,7 +89,11 @@ async fn create_user(
     conn: &mut AsyncPgConnection,
 ) -> QueryResult<()> {
     let user_id = diesel::insert_into(users::table)
-        .values((users::name.eq(name), users::is_admin.eq(is_admin)))
+        .values((
+            users::name.eq(name),
+            users::gh_login.eq(name),
+            users::is_admin.eq(is_admin),
+        ))
         .returning(users::id)
         .get_result::<i32>(conn)
         .await?;