Switch session create to check `oauth_github` first, then get user by carols10cents · Pull Request #13856 · rust-lang/crates.io

carols10cents · 2026-06-04T19:34:20Z

This is what I was talking about in this comment, that we can't use nice upserts but need to do multiple update/select/inserts in a transaction instead, so that we switch to using oauth_github as the source of truth of whether we're logging in an existing user or a new user.

I've extracted this from #13832 and I think making this change first is needed before we can stop using users.{gh_id, gh_avatar, gh_encrypted_token}.

carols10cents · 2026-06-06T17:21:20Z

@Turbo87 Ok, I rebased to pick up your new test and then thought a lot about the transactions to fix the problem. I think the transactions don't need to be nested, but as I've noted in the commit message of the last commit and in comments, we're going to have to rethink how the transactions and errors and inserts and updates work when the users table bases its uniqueness on users.username rather than users.gh_id.

Let me know if you/Claude think this is correct now!

carols10cents · 2026-06-09T13:43:32Z

@Turbo87 this is ready for re-review!

Turbo87

I'm fine with merging this once the issues are fixed, but I'm wondering whether we should prioritize splitting login from registration in the UI in some way. if we had dedicated endpoints for both operations then this probably would be less messy 😅

View changes since this review

We have `new_user`, which is a crates.io `NewUser`. I'm about to add usage of crates.io `User`s, which I think will be confusing unless this varibale name changes.

These actions don't all need to be in one transaction. We need: - The update to `oauth_github` and `users` for an existing user to happen in a transaction - The creation of `oauth_github` and `users` for a new user to happen in a transaction If updating a user fails because the connection is read-only, fall back to looking up the existing user without an update. In that case, we shouldn't even try to create a new user if the user doesn't exist, because that won't work either. Then, rather than nesting the transaction in `create_or_update_user`, move the creation of the `oauth_github` record into that transaction. == Race condition analysis == If the earlier "update" transaction fails because the record to update doesn't exist, then in some other request the records get created before this request's "insert" transaction runs, for now the "ON CONFLICT (gh_id) DO UPDATE" in `NewUser.insert_or_update` and the "ON CONFLICT (account_id) DO UPDATE" in the `oauth_github` insert that are in this request's "insert" transaction should work. When we switch the `users` table to be unique on `users.username` rather than `users.gh_id`, we need to change how this code works. If the username is already taken by a different crates.io account with a different associated `oauth_github`, we need to prompt the current user to choose a different username. If the current user is the same user whose account got created in between the statements of this request, we should log in this user.

And verify with a test.

This is Rust, not Ruby, also this comment won't get rendered anywhere so it doesn't need to be linked.

carols10cents · 2026-06-10T20:37:34Z

@Turbo87 Ready for re-re-review! I rebased and added new commits, but didn't change any of the commits you've already looked at.

I'm fine with merging this once the issues are fixed, but I'm wondering whether we should prioritize splitting login from registration in the UI in some way. if we had dedicated endpoints for both operations then this probably would be less messy 😅

omg yes 😭 filing a new issue for that now-- it's sort of related to some of the username issues but can be done first as a straight refactoring i think

carols10cents · 2026-07-06T17:25:04Z

@Turbo87 ready for re-review!

LawnGnome

I have a couple of comment nits, but otherwise this makes sense to me behaviourally. Approved, although I suspect @Turbo87 will want to take another look too.

I do kinda wonder if we should split create_or_update_user to have each path in a separate function for readability reasons, but I don't feel very strongly about it.

View changes since this review

Co-authored-by: Adam Harvey <adam@adamharvey.name>

carols10cents · 2026-07-08T17:07:12Z

I do kinda wonder if we should split create_or_update_user to have each path in a separate function for readability reasons, but I don't feel very strongly about it.

Yeah, I think that will be done soon as part of #13907.

Use a transaction inside update_user so it's impossible to call incorrectly, rather than relying on the caller to use a transaction.

But only restore an insert method, not insert_or_update. Also make last_sync have a default value

Let it be known that I think race conditions necessitating an update here will be possible and that the upsert would be friendlier, but that I suppose the user getting an error and reloading and having it work isn't the worst thing ever

…tcheroo

rustbot · 2026-07-10T19:54:25Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

When session::save_user_to_database was doing upserts for oauth_github records outside of a transaction with updates of user records, the test `user::github_with_email_does_not_overwrite_email` passed by coincidence. But now that we're switching the session update to try updating an oauth_github record first and assuming that if an oauth_github record doesn't exist then the user doesn't exist either, and then doing only an insert rather than an upsert if we're creating new records, the test started failing because TestApp with_user, which calls db_new_user, wasn't actually creating an oauth_github record for the user. Ensure all tests using db_new_user are realistic and valid by creating an oauth_github record for the user, much like we create a verified email address. This updated a lot of snapshots.

carols10cents · 2026-07-10T20:45:17Z

@Turbo87 @LawnGnome Ready for re-review! ❤️

LawnGnome

These changes address my concerns from the first review, and I think the code generally reads better now, so thanks for that!

View changes since this review

Explicitly set an avatar in the og image tests that need it.

carols10cents · 2026-07-14T18:41:36Z

@Turbo87 @LawnGnome Ready for re-review! ❤️

Turbo87

thank you! :)

View changes since this review

rustbot added the A-backend ⚙️ label Jun 4, 2026

carols10cents commented Jun 4, 2026

View reviewed changes

Comment thread src/controllers/session.rs Outdated

carols10cents commented Jun 4, 2026

View reviewed changes

Comment thread src/controllers/session.rs Outdated

carols10cents marked this pull request as ready for review June 4, 2026 19:45

carols10cents changed the title ~~WIP: Switch session create to check oauth_github first, then get user~~ Switch session create to check oauth_github first, then get user Jun 4, 2026

carols10cents requested review from LawnGnome, Turbo87 and eth3lbert June 4, 2026 19:45

Turbo87 mentioned this pull request Jun 5, 2026

tests: Cover existing-user login in read-only mode #13862

Merged

Turbo87 requested changes Jun 5, 2026

View reviewed changes

Comment thread src/controllers/session.rs Outdated

This comment has been minimized.

Sign in to view

carols10cents force-pushed the user-oauth-github-switcheroo branch from 97aa9a7 to 9eb725c Compare June 6, 2026 17:19

This comment has been minimized.

Sign in to view

carols10cents force-pushed the user-oauth-github-switcheroo branch from 9eb725c to 053f674 Compare June 9, 2026 13:26

This comment has been minimized.

Sign in to view

Turbo87 requested changes Jun 10, 2026

View reviewed changes

Comment thread src/controllers/session.rs Outdated

Comment thread src/controllers/session.rs

Comment thread src/tests/user.rs Outdated

Comment thread src/controllers/session.rs Outdated

carols10cents added 7 commits June 10, 2026 16:03

Rename variable from user to gh_user

f372178

We have `new_user`, which is a crates.io `NewUser`. I'm about to add usage of crates.io `User`s, which I think will be confusing unless this varibale name changes.

Switch signin from using users to using oauth_github

0841f08

Continue writing to users gh_ columns for now

910e2c0

And verify with a test.

Continue to write to gh_avatar even though it's never read

4084a85

Use a snapshot for a postgres error rather than static str

ef79682

Fix function reference in non-doc comment

cc4431e

This is Rust, not Ruby, also this comment won't get rendered anywhere so it doesn't need to be linked.

carols10cents force-pushed the user-oauth-github-switcheroo branch from 053f674 to cc4431e Compare June 10, 2026 20:32

This comment has been minimized.

Sign in to view

carols10cents mentioned this pull request Jun 10, 2026

Separate session controller authorize into "signup" and "login" #13907

Open

LawnGnome approved these changes Jul 7, 2026

View reviewed changes

Comment thread src/controllers/session.rs Outdated

Comment thread src/controllers/session.rs Outdated

carols10cents and others added 2 commits July 8, 2026 13:05

Clarify comment context

6ef39af

Co-authored-by: Adam Harvey <adam@adamharvey.name>

Move comment describing closure into said closure

6c8e2f3

Turbo87 reviewed Jul 9, 2026

View reviewed changes

carols10cents added 7 commits July 10, 2026 12:01

Extract an update_user function from create_or_update_user

392f929

Use a transaction inside update_user so it's impossible to call incorrectly, rather than relying on the caller to use a transaction.

Extract a create_user function from create_or_update_user

9d4214b

Put the body of create_user into a transaction

48a0adc

Clarify comment about why invalid email errors are ok

3934379

Restore the NewOauthGithub insertable/builder struct

53fc51a

But only restore an insert method, not insert_or_update. Also make last_sync have a default value

Only insert in create_user, not upsert

a8f269b

Let it be known that I think race conditions necessitating an update here will be possible and that the upsert would be friendlier, but that I suppose the user getting an error and reloading and having it work isn't the worst thing ever

Merge remote-tracking branch 'origin/main' into user-oauth-github-swi…

c6f8d7b

…tcheroo

carols10cents force-pushed the user-oauth-github-switcheroo branch from 7d622d7 to e3f4fe2 Compare July 10, 2026 19:59

carols10cents added 2 commits July 10, 2026 16:16

Express mutability of connection more consistently in test builder

a5bbce6

carols10cents force-pushed the user-oauth-github-switcheroo branch from e3f4fe2 to a5bbce6 Compare July 10, 2026 20:16

carols10cents requested a review from Turbo87 July 10, 2026 20:44

LawnGnome approved these changes Jul 13, 2026

View reviewed changes

Comment thread src/controllers/session.rs Outdated

Turbo87 reviewed Jul 14, 2026

View reviewed changes

Comment thread src/tests/util/test_app.rs

carols10cents added 2 commits July 14, 2026 14:20

Return test user to not having an avatar by default

71d891b

Explicitly set an avatar in the og image tests that need it.

Move transaction up to encompass user update or create

9dceaa1

Turbo87 approved these changes Jul 14, 2026

View reviewed changes

Turbo87 added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Jul 14, 2026

Turbo87 merged commit fdac0bf into rust-lang:main Jul 14, 2026
12 checks passed

rustbot mentioned this pull request Jul 14, 2026

Cargo owner username disambiguation #14213

Open

carols10cents deleted the user-oauth-github-switcheroo branch July 15, 2026 01:19

Uh oh!

Uh oh!

Conversation

carols10cents commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

carols10cents commented Jun 6, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

carols10cents commented Jun 9, 2026

Uh oh!

Turbo87 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

carols10cents commented Jun 10, 2026

Uh oh!

carols10cents commented Jul 6, 2026

Uh oh!

LawnGnome left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carols10cents commented Jul 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented Jul 10, 2026

Uh oh!

carols10cents commented Jul 10, 2026

Uh oh!

LawnGnome left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carols10cents commented Jul 14, 2026

Uh oh!

Turbo87 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carols10cents commented Jun 4, 2026 •

edited

Loading

Turbo87 left a comment •

edited by rustbot

Loading

LawnGnome left a comment •

edited by rustbot

Loading

LawnGnome left a comment •

edited by rustbot

Loading

Turbo87 left a comment •

edited by rustbot

Loading