Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/crates_io_database/src/models/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub use self::krate::{Crate, CrateName, NewCrate, RecentCrateDownloads};
pub use self::owner::{CrateOwner, Owner, OwnerKind};
pub use self::team::{NewTeam, Team};
pub use self::token::ApiToken;
pub use self::user::{NewUser, User};
pub use self::user::{AccountProvider, LinkedAccount, NewLinkedAccount, NewUser, User};
pub use self::version::{NewVersion, TopVersions, Version};

pub mod helpers;
Expand Down
79 changes: 77 additions & 2 deletions crates/crates_io_database/src/models/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use diesel_async::{AsyncPgConnection, RunQueryDsl};
use secrecy::SecretString;

use crate::models::{Crate, CrateOwner, Email, Owner, OwnerKind};
use crate::schema::{crate_owners, emails, users};
use crates_io_diesel_helpers::lower;
use crate::schema::{crate_owners, emails, linked_accounts, users};
use crates_io_diesel_helpers::{lower, pg_enum};

/// The model representing a row in the `users` database table.
#[derive(Clone, Debug, Queryable, Identifiable, Selectable)]
Expand Down Expand Up @@ -122,3 +122,78 @@ impl NewUser<'_> {
.await
}
}

// Supported OAuth providers. Currently only GitHub.
pg_enum! {
pub enum AccountProvider {
Github = 0,
}
}

/// Represents an OAuth account record linked to a user record.
#[derive(Associations, Identifiable, Selectable, Queryable, Debug, Clone)]
#[diesel(
table_name = linked_accounts,
check_for_backend(diesel::pg::Pg),
primary_key(provider, account_id),
belongs_to(User),
)]
pub struct LinkedAccount {
pub user_id: i32,
pub provider: AccountProvider,
pub account_id: i32, // corresponds to user.gh_id
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could very well be a problem to deal with later, but I could imagine having a provider that represents its IDs some other way, e.g. strings or UUIDs. We probably don't want to commit to such a design yet, but perhaps we'll eventually need some kind of "table-per-provider" approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a really good point. Hmm. I'm trying to think through whether I'd rather start with an oauth_github table and then add, say, oauth_gitlab, etc tables, or if i'd rather start with what's here and then need to migrate to a table-per-provider later....

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it'll probably be hard to know what we need until we need it. The good news is that we shouldn't be boxing ourselves into a hole that's impossible to escape regardless of which approach we choose.

#[diesel(deserialize_as = String)]
pub access_token: SecretString, // corresponds to user.gh_access_token
pub login: String, // corresponds to user.gh_login
pub avatar: Option<String>, // corresponds to user.gh_avatar
}

/// Represents a new linked account record insertable to the `linked_accounts` table
#[derive(Insertable, Debug, Builder)]
#[diesel(
table_name = linked_accounts,
check_for_backend(diesel::pg::Pg),
primary_key(provider, account_id),
belongs_to(User),
)]
pub struct NewLinkedAccount<'a> {
pub user_id: i32,
pub provider: AccountProvider,
pub account_id: i32, // corresponds to user.gh_id
pub access_token: &'a str, // corresponds to user.gh_access_token
pub login: &'a str, // corresponds to user.gh_login
pub avatar: Option<&'a str>, // corresponds to user.gh_avatar
}

impl NewLinkedAccount<'_> {
/// Inserts the linked account into the database, or updates an existing one.
///
/// This is to be used for logging in when there is no currently logged-in user, as opposed to
/// adding another linked account to a currently-logged-in user. The logic for adding another
/// linked account (when that ability gets added) will need to ensure that a particular
/// (provider, account_id) combo (ex: GitHub account with GitHub ID 1234) is only associated
/// with one crates.io account, so that we know what crates.io account to log in when we get an
/// oAuth request from GitHub ID 1234. In other words, we should NOT be updating the user_id on
/// an existing (provider, account_id) row when starting from a currently-logged-in crates.io \
/// user because that would mean that oAuth account has already been associated with a
/// different crates.io account.
///
/// This function should be called if there is no current user and should update, say, the
/// access token, login, or avatar if those have changed.
pub async fn insert_or_update(
&self,
conn: &mut AsyncPgConnection,
) -> QueryResult<LinkedAccount> {
diesel::insert_into(linked_accounts::table)
.values(self)
.on_conflict((linked_accounts::provider, linked_accounts::account_id))
.do_update()
.set((
linked_accounts::access_token.eq(excluded(linked_accounts::access_token)),
linked_accounts::login.eq(excluded(linked_accounts::login)),
linked_accounts::avatar.eq(excluded(linked_accounts::avatar)),
))
.get_result(conn)
.await
}
}
46 changes: 46 additions & 0 deletions crates/crates_io_database/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,50 @@ diesel::table! {
}
}

diesel::table! {
/// Representation of the `linked_accounts` table.
///
/// (Automatically generated by Diesel.)
linked_accounts (provider, account_id) {
/// The `user_id` column of the `linked_accounts` table.
///
/// Its SQL type is `Int4`.
///
/// (Automatically generated by Diesel.)
user_id -> Int4,
/// The `provider` column of the `linked_accounts` table.
///
/// Its SQL type is `Int4`.
///
/// (Automatically generated by Diesel.)
provider -> Int4,
/// The `account_id` column of the `linked_accounts` table.
///
/// Its SQL type is `Int4`.
///
/// (Automatically generated by Diesel.)
account_id -> Int4,
/// The `access_token` column of the `linked_accounts` table.
///
/// Its SQL type is `Varchar`.
///
/// (Automatically generated by Diesel.)
access_token -> Varchar,
/// The `login` column of the `linked_accounts` table.
///
/// Its SQL type is `Varchar`.
///
/// (Automatically generated by Diesel.)
login -> Varchar,
/// The `avatar` column of the `linked_accounts` table.
///
/// Its SQL type is `Nullable<Varchar>`.
///
/// (Automatically generated by Diesel.)
avatar -> Nullable<Varchar>,
}
}

diesel::table! {
/// Representation of the `metadata` table.
///
Expand Down Expand Up @@ -1066,6 +1110,7 @@ diesel::joinable!(dependencies -> versions (version_id));
diesel::joinable!(emails -> users (user_id));
diesel::joinable!(follows -> crates (crate_id));
diesel::joinable!(follows -> users (user_id));
diesel::joinable!(linked_accounts -> users (user_id));
diesel::joinable!(publish_limit_buckets -> users (user_id));
diesel::joinable!(publish_rate_overrides -> users (user_id));
diesel::joinable!(readme_renderings -> versions (version_id));
Expand Down Expand Up @@ -1094,6 +1139,7 @@ diesel::allow_tables_to_appear_in_same_query!(
emails,
follows,
keywords,
linked_accounts,
metadata,
processed_log_files,
publish_limit_buckets,
Expand Down
10 changes: 10 additions & 0 deletions crates/crates_io_database_dump/src/dump-db.toml
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,16 @@ keyword = "public"
crates_cnt = "public"
created_at = "public"

[linked_accounts.columns]
user_id = "public"
provider = "public"
account_id = "public"
access_token = "private"
login = "public"
avatar = "public"
[linked_accounts.column_defaults]
access_token = "''"

[metadata.columns]
total_downloads = "public"

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
---
source: crates/crates_io_database_dump/src/lib.rs
expression: content
snapshot_kind: text
---
BEGIN ISOLATION LEVEL REPEATABLE READ, READ ONLY;

\copy "categories" ("category", "crates_cnt", "created_at", "description", "id", "path", "slug") TO 'data/categories.csv' WITH CSV HEADER
\copy "crate_downloads" ("crate_id", "downloads") TO 'data/crate_downloads.csv' WITH CSV HEADER
\copy "crates" ("created_at", "description", "documentation", "homepage", "id", "max_features", "max_upload_size", "name", "readme", "repository", "updated_at") TO 'data/crates.csv' WITH CSV HEADER
\copy "keywords" ("crates_cnt", "created_at", "id", "keyword") TO 'data/keywords.csv' WITH CSV HEADER
\copy "linked_accounts" ("account_id", "avatar", "login", "provider", "user_id") TO 'data/linked_accounts.csv' WITH CSV HEADER
\copy "metadata" ("total_downloads") TO 'data/metadata.csv' WITH CSV HEADER
\copy "reserved_crate_names" ("name") TO 'data/reserved_crate_names.csv' WITH CSV HEADER
\copy "teams" ("avatar", "github_id", "id", "login", "name", "org_id") TO 'data/teams.csv' WITH CSV HEADER
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
---
source: crates/crates_io_database_dump/src/lib.rs
expression: content
snapshot_kind: text
---
BEGIN;
-- Disable triggers on each table.
Expand All @@ -9,6 +10,7 @@ BEGIN;
ALTER TABLE "crate_downloads" DISABLE TRIGGER ALL;
ALTER TABLE "crates" DISABLE TRIGGER ALL;
ALTER TABLE "keywords" DISABLE TRIGGER ALL;
ALTER TABLE "linked_accounts" DISABLE TRIGGER ALL;
ALTER TABLE "metadata" DISABLE TRIGGER ALL;
ALTER TABLE "reserved_crate_names" DISABLE TRIGGER ALL;
ALTER TABLE "teams" DISABLE TRIGGER ALL;
Expand All @@ -23,6 +25,7 @@ BEGIN;

-- Set defaults for non-nullable columns not included in the dump.

ALTER TABLE "linked_accounts" ALTER COLUMN "access_token" SET DEFAULT '';
ALTER TABLE "users" ALTER COLUMN "gh_access_token" SET DEFAULT '';

-- Truncate all tables.
Expand All @@ -31,6 +34,7 @@ BEGIN;
TRUNCATE "crate_downloads" RESTART IDENTITY CASCADE;
TRUNCATE "crates" RESTART IDENTITY CASCADE;
TRUNCATE "keywords" RESTART IDENTITY CASCADE;
TRUNCATE "linked_accounts" RESTART IDENTITY CASCADE;
TRUNCATE "metadata" RESTART IDENTITY CASCADE;
TRUNCATE "reserved_crate_names" RESTART IDENTITY CASCADE;
TRUNCATE "teams" RESTART IDENTITY CASCADE;
Expand All @@ -52,6 +56,7 @@ BEGIN;
\copy "crate_downloads" ("crate_id", "downloads") FROM 'data/crate_downloads.csv' WITH CSV HEADER
\copy "crates" ("created_at", "description", "documentation", "homepage", "id", "max_features", "max_upload_size", "name", "readme", "repository", "updated_at") FROM 'data/crates.csv' WITH CSV HEADER
\copy "keywords" ("crates_cnt", "created_at", "id", "keyword") FROM 'data/keywords.csv' WITH CSV HEADER
\copy "linked_accounts" ("account_id", "avatar", "login", "provider", "user_id") FROM 'data/linked_accounts.csv' WITH CSV HEADER
\copy "metadata" ("total_downloads") FROM 'data/metadata.csv' WITH CSV HEADER
\copy "reserved_crate_names" ("name") FROM 'data/reserved_crate_names.csv' WITH CSV HEADER
\copy "teams" ("avatar", "github_id", "id", "login", "name", "org_id") FROM 'data/teams.csv' WITH CSV HEADER
Expand All @@ -66,6 +71,7 @@ BEGIN;

-- Drop the defaults again.

ALTER TABLE "linked_accounts" ALTER COLUMN "access_token" DROP DEFAULT;
ALTER TABLE "users" ALTER COLUMN "gh_access_token" DROP DEFAULT;

-- Reenable triggers on each table.
Expand All @@ -74,6 +80,7 @@ BEGIN;
ALTER TABLE "crate_downloads" ENABLE TRIGGER ALL;
ALTER TABLE "crates" ENABLE TRIGGER ALL;
ALTER TABLE "keywords" ENABLE TRIGGER ALL;
ALTER TABLE "linked_accounts" ENABLE TRIGGER ALL;
ALTER TABLE "metadata" ENABLE TRIGGER ALL;
ALTER TABLE "reserved_crate_names" ENABLE TRIGGER ALL;
ALTER TABLE "teams" ENABLE TRIGGER ALL;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE linked_accounts;
9 changes: 9 additions & 0 deletions migrations/2025-01-29-205705_linked_accounts_table/up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CREATE TABLE linked_accounts (
user_id INTEGER NOT NULL REFERENCES users (id) ON DELETE CASCADE,
provider INTEGER NOT NULL,
account_id INTEGER NOT NULL,
access_token VARCHAR NOT NULL,
login VARCHAR NOT NULL,
avatar VARCHAR,
PRIMARY KEY (provider, account_id)
);
17 changes: 16 additions & 1 deletion src/controllers/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::app::AppState;
use crate::controllers::user::update::UserConfirmEmail;
use crate::email::Emails;
use crate::middleware::log_request::RequestLogExt;
use crate::models::{NewEmail, NewUser, User};
use crate::models::{AccountProvider, NewEmail, NewLinkedAccount, NewUser, User};
use crate::schema::users;
use crate::util::diesel::is_read_only_error;
use crate::util::errors::{AppResult, bad_request, server_error};
Expand Down Expand Up @@ -162,6 +162,21 @@ async fn create_or_update_user(
async move {
let user = new_user.insert_or_update(conn).await?;

// To assist in eventually someday allowing OAuth with more than GitHub, also
// write the GitHub info to the `linked_accounts` table. Nothing currently reads
// from this table. Only log errors but don't fail login if this writing fails.
let new_linked_account = NewLinkedAccount::builder()
.user_id(user.id)
.provider(AccountProvider::Github)
.account_id(user.gh_id)
.access_token(new_user.gh_access_token)
.login(&user.gh_login)
.maybe_avatar(user.gh_avatar.as_deref())
.build();
if let Err(e) = new_linked_account.insert_or_update(conn).await {
info!("Error inserting or updating linked_accounts record: {e}");
}

// To send the user an account verification email
if let Some(user_email) = email {
let new_email = NewEmail::builder()
Expand Down
2 changes: 2 additions & 0 deletions src/tests/dump_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ async fn test_dump_db_job() -> anyhow::Result<()> {
"YYYY-MM-DD-HHMMSS/data/crate_downloads.csv",
"YYYY-MM-DD-HHMMSS/data/crates.csv",
"YYYY-MM-DD-HHMMSS/data/keywords.csv",
"YYYY-MM-DD-HHMMSS/data/linked_accounts.csv",
"YYYY-MM-DD-HHMMSS/data/metadata.csv",
"YYYY-MM-DD-HHMMSS/data/reserved_crate_names.csv",
"YYYY-MM-DD-HHMMSS/data/teams.csv",
Expand Down Expand Up @@ -84,6 +85,7 @@ async fn test_dump_db_job() -> anyhow::Result<()> {
"data/crate_downloads.csv",
"data/crates.csv",
"data/keywords.csv",
"data/linked_accounts.csv",
"data/metadata.csv",
"data/reserved_crate_names.csv",
"data/teams.csv",
Expand Down
47 changes: 46 additions & 1 deletion src/tests/user.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::controllers::session;
use crate::models::{ApiToken, Email, User};
use crate::models::{AccountProvider, ApiToken, Email, LinkedAccount, User};
use crate::schema::linked_accounts;
use crate::tests::TestApp;
use crate::tests::util::github::next_gh_id;
use crate::tests::util::{MockCookieUser, RequestHelper};
Expand Down Expand Up @@ -265,3 +266,47 @@ async fn test_existing_user_email() -> anyhow::Result<()> {

Ok(())
}

// To assist in eventually someday allowing OAuth with more than GitHub, verify that we're starting
// to also write the GitHub info to the `linked_accounts` table. Nothing currently reads from this
// table other than this test.
#[tokio::test(flavor = "multi_thread")]
async fn also_write_to_linked_accounts() -> anyhow::Result<()> {
let (app, _) = TestApp::init().empty().await;
let mut conn = app.db_conn().await;

// Simulate logging in via GitHub. Don't use app.db_new_user because it inserts a user record
// directly into the database and we want to test the OAuth flow here.
let email = "potahto@example.com";

let emails = &app.as_inner().emails;

let gh_user = GithubUser {
id: next_gh_id(),
login: "arbitrary_username".to_string(),
name: None,
email: Some(email.to_string()),
avatar_url: None,
};

let u =
session::save_user_to_database(&gh_user, "some random token", emails, &mut conn).await?;

let linked_accounts = linked_accounts::table
.filter(linked_accounts::provider.eq(AccountProvider::Github))
.filter(linked_accounts::account_id.eq(u.gh_id))
.load::<LinkedAccount>(&mut conn)
.await
.unwrap();

assert_eq!(linked_accounts.len(), 1);
let linked_account = &linked_accounts[0];
assert_eq!(linked_account.user_id, u.id);
assert_eq!(linked_account.login, u.gh_login);
assert_eq!(
linked_account.access_token.expose_secret(),
u.gh_access_token.expose_secret()
);

Ok(())
}