diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index a124dad513937..0e77bacd3b216 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3787,6 +3787,10 @@ func (q *FakeQuerier) GetUserLinkByLinkedID(_ context.Context, id string) (datab defer q.mutex.RUnlock() for _, link := range q.userLinks { + user, err := q.getUserByIDNoLock(link.UserID) + if err == nil && user.Deleted { + continue + } if link.LinkedID == id { return link, nil } diff --git a/coderd/database/migrations/testdata/fixtures/000048_userdelete.up.sql b/coderd/database/migrations/testdata/fixtures/000048_userdelete.up.sql new file mode 100644 index 0000000000000..0fb1d0efd4aca --- /dev/null +++ b/coderd/database/migrations/testdata/fixtures/000048_userdelete.up.sql @@ -0,0 +1,19 @@ +-- This is a deleted user that shares the same username and linked_id as the existing user below. +-- Any future migrations need to handle this case. +INSERT INTO public.users(id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, deleted) + VALUES ('a0061a8e-7db7-4585-838c-3116a003dd21', 'githubuser@coder.com', 'githubuser', '\x', '2022-11-02 13:05:21.445455+02', '2022-11-02 13:05:21.445455+02', 'active', '{}', true) ON CONFLICT DO NOTHING; +INSERT INTO public.organization_members VALUES ('a0061a8e-7db7-4585-838c-3116a003dd21', 'bb640d07-ca8a-4869-b6bc-ae61ebb2fda1', '2022-11-02 13:05:21.447595+02', '2022-11-02 13:05:21.447595+02', '{}') ON CONFLICT DO NOTHING; +INSERT INTO public.user_links(user_id, login_type, linked_id, oauth_access_token) + VALUES('a0061a8e-7db7-4585-838c-3116a003dd21', 'github', '100', ''); + + +INSERT INTO public.users(id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, deleted) + VALUES ('fc1511ef-4fcf-4a3b-98a1-8df64160e35a', 'githubuser@coder.com', 'githubuser', '\x', '2022-11-02 13:05:21.445455+02', '2022-11-02 13:05:21.445455+02', 'active', '{}', false) ON CONFLICT DO NOTHING; +INSERT INTO public.organization_members VALUES ('fc1511ef-4fcf-4a3b-98a1-8df64160e35a', 'bb640d07-ca8a-4869-b6bc-ae61ebb2fda1', '2022-11-02 13:05:21.447595+02', '2022-11-02 13:05:21.447595+02', '{}') ON CONFLICT DO NOTHING; +INSERT INTO public.user_links(user_id, login_type, linked_id, oauth_access_token) + VALUES('fc1511ef-4fcf-4a3b-98a1-8df64160e35a', 'github', '100', ''); + +-- Additionally, there is no unique constraint on user_id. So also add another user_link for the same user. +-- This has happened on a production database. +INSERT INTO public.user_links(user_id, login_type, linked_id, oauth_access_token) +VALUES('fc1511ef-4fcf-4a3b-98a1-8df64160e35a', 'oidc', 'foo', ''); diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c4ba338c7fe52..f2ef7b697ed00 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -7088,11 +7088,15 @@ func (q *sqlQuerier) InsertTemplateVersionVariable(ctx context.Context, arg Inse const getUserLinkByLinkedID = `-- name: GetUserLinkByLinkedID :one SELECT - user_id, login_type, linked_id, oauth_access_token, oauth_refresh_token, oauth_expiry, oauth_access_token_key_id, oauth_refresh_token_key_id, debug_context + user_links.user_id, user_links.login_type, user_links.linked_id, user_links.oauth_access_token, user_links.oauth_refresh_token, user_links.oauth_expiry, user_links.oauth_access_token_key_id, user_links.oauth_refresh_token_key_id, user_links.debug_context FROM user_links +INNER JOIN + users ON user_links.user_id = users.id WHERE linked_id = $1 + AND + deleted = false ` func (q *sqlQuerier) GetUserLinkByLinkedID(ctx context.Context, linkedID string) (UserLink, error) { diff --git a/coderd/database/queries/user_links.sql b/coderd/database/queries/user_links.sql index b2bfc7d593cbc..9fc0e6f9d7598 100644 --- a/coderd/database/queries/user_links.sql +++ b/coderd/database/queries/user_links.sql @@ -1,10 +1,14 @@ -- name: GetUserLinkByLinkedID :one SELECT - * + user_links.* FROM user_links +INNER JOIN + users ON user_links.user_id = users.id WHERE - linked_id = $1; + linked_id = $1 + AND + deleted = false; -- name: GetUserLinkByUserIDLoginType :one SELECT diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index e502b7b4cc780..f4e12cec8b2cb 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -603,6 +603,11 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, http.StatusUnauthorized, resp.StatusCode) }) + + // The bug only is exercised when a deleted user with the same linked_id exists. + // Still related open issues: + // - https://github.com/coder/coder/issues/12116 + // - https://github.com/coder/coder/issues/12115 t.Run("ChangedEmail", func(t *testing.T) { t.Parallel() @@ -627,7 +632,7 @@ func TestUserOAuth2Github(t *testing.T) { coderEmail, } - client := coderdtest.New(t, &coderdtest.Options{ + owner := coderdtest.New(t, &coderdtest.Options{ Auditor: auditor, GithubOAuth2Config: &coderd.GithubOAuth2Config{ AllowSignups: true, @@ -650,9 +655,21 @@ func TestUserOAuth2Github(t *testing.T) { }, }, }) + coderdtest.CreateFirstUser(t, owner) + + ctx := testutil.Context(t, testutil.WaitLong) + // Create the user, then delete the user, then create again. + // This causes the email change to fail. + client := codersdk.New(owner.URL) - ctx := testutil.Context(t, testutil.WaitMedium) - // This should register the user + client, _ = fake.Login(t, client, jwt.MapClaims{}) + deleted, err := client.User(ctx, "me") + require.NoError(t, err) + + err = owner.DeleteUser(ctx, deleted.ID) + require.NoError(t, err) + + // Create the user again. client, _ = fake.Login(t, client, jwt.MapClaims{}) user, err := client.User(ctx, "me") require.NoError(t, err) @@ -666,7 +683,8 @@ func TestUserOAuth2Github(t *testing.T) { client, _ = fake.Login(t, client, jwt.MapClaims{}) user, err = client.User(ctx, "me") require.NoError(t, err) - require.Equal(t, user.ID, userID) + + require.Equal(t, user.ID, userID, "user_id is different, a new user was likely created") require.Equal(t, user.Email, *gmailEmail.Email) // Entirely change emails. @@ -681,7 +699,8 @@ func TestUserOAuth2Github(t *testing.T) { client, _ = fake.Login(t, client, jwt.MapClaims{}) user, err = client.User(ctx, "me") require.NoError(t, err) - require.Equal(t, user.ID, userID) + + require.Equal(t, user.ID, userID, "user_id is different, a new user was likely created") require.Equal(t, user.Email, newEmail) }) }