Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not query user_link for deleted accounts #12112

Merged
merged 15 commits into from
Feb 13, 2024
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Feb 12, 2024

Closes #10972

What this does

This solution is a patch, I think we need to create better unique constraints on the database to prevent other types of bugs in the future.

If you change your github email and log in, we link based on your Github UID. So your email is updated on Coder. This was failing if you previously deleted an account that was linked to the same github. The previous GetUserLinkByLinkedID would fetch a deleted user, and hit this line:

coder/coderd/userauth.go

Lines 1728 to 1729 in 4f0203d

// If the user was deleted, act as if no account link exists.
user = database.User{}

This would then instead make a new account. A simple fix is to ignore deleted acounts.

Future work

  • It is possible to have more than 1 user_link per account. This does exist on prod, and is a mistake imo.
  • It is possible to have multiple user_links with the same linked_id because of deleted users. I am not sure what the solution is here 馃. When we delete an account, should we do something to the user_link?

@Emyrk Emyrk changed the title chore: creaet unit test to exercise failed email change bug chore: create unit test to exercise failed email change bug Feb 12, 2024
Copy link
Member Author

Emyrk commented Feb 12, 2024

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I think this patch is OK for now, but we should also:

  1. Delete user_links when a user is deleted
  2. Update the schema to ensure that this situation cannot occur in future.

We should create issues for the above change(s) and reference them in the unit test so we do not forget to do them.

Also: the PR title is incorrect as this is no longer just a chore but actually changes functionality.

@Emyrk Emyrk changed the title chore: create unit test to exercise failed email change bug fix: do not query user_link for deleted accounts Feb 13, 2024
@Emyrk Emyrk changed the base branch from stevenmasley/linked_id_bug to main February 13, 2024 14:03
@Emyrk Emyrk changed the base branch from main to stevenmasley/linked_id_bug February 13, 2024 14:07
Added to the migration that added the delete field to users
Base automatically changed from stevenmasley/linked_id_bug to main February 13, 2024 18:06
@Emyrk Emyrk merged commit 5d483a7 into main Feb 13, 2024
24 checks passed
@Emyrk Emyrk deleted the stevenmasley/linked_id_fix branch February 13, 2024 19:02
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants