fix: do not query user_link for deleted accounts #12112
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
johnstcn
reviewed
Feb 13, 2024
There was a problem hiding this comment.
I think this patch is OK for now, but we should also:
- Delete user_links when a user is deleted
- 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.
johnstcn
approved these changes
Feb 13, 2024
e681b22 to
2ca3ef9
Compare
2ca3ef9 to
65effdc
Compare
2883815 to
78e90d6
Compare
65effdc to
6f09151
Compare
78e90d6 to
90e7164
Compare
6f09151 to
24e767f
Compare
Added to the migration that added the delete field to users
Changing emails on github fails if another deleted user exists with the same link.
24e767f to
b0c3cbb
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GetUserLinkByLinkedIDwould fetch a deleted user, and hit this line:coder/coderd/userauth.go
Lines 1728 to 1729 in 4f0203d
This would then instead make a new account. A simple fix is to ignore deleted acounts.
Future work
linked_idbecause 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?