Skip to content

Commit 188f03b

Browse files
EmyrkEmyrk
committed
Add migrations to prevent deleted users with links
1 parent c426259 commit 188f03b

File tree

4 files changed

+52
-16
lines changed

4 files changed

+52
-16
lines changed

‎coderd/database/dbmem/dbmem.go

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5573,10 +5573,26 @@ func (q *FakeQuerier) InsertUserGroupsByName(_ context.Context, arg database.Ins
55735573
return nil
55745574
}
55755575

5576+
// Took the error from the real database.
5577+
var deletedUserLinkError = &pq.Error{
5578+
Severity: "ERROR",
5579+
// "raise_exception" error
5580+
Code: "P0001",
5581+
Message: "Cannot create user_link for deleted user",
5582+
Where: "PL/pgSQL function insert_user_links_fail_if_user_deleted() line 7 at RAISE",
5583+
File: "pl_exec.c",
5584+
Line: "3864",
5585+
Routine: "exec_stmt_raise",
5586+
}
5587+
55765588
func (q *FakeQuerier) InsertUserLink(_ context.Context, args database.InsertUserLinkParams) (database.UserLink, error) {
55775589
q.mutex.Lock()
55785590
defer q.mutex.Unlock()
55795591

5592+
if u, err := q.getUserByIDNoLock(args.UserID); err == nil && u.Deleted {
5593+
return database.UserLink{}, deletedUserLinkError
5594+
}
5595+
55805596
//nolint:gosimple
55815597
link := database.UserLink{
55825598
UserID: args.UserID,
@@ -6736,20 +6752,15 @@ func (q *FakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.U
67366752
if u.ID == params.ID {
67376753
u.Deleted = params.Deleted
67386754
q.users[i] = u
6739-
// NOTE: In the real world, this is done by a trigger.
6740-
i := 0
6741-
for {
6742-
if i >= len(q.apiKeys) {
6743-
break
6744-
}
6745-
k := q.apiKeys[i]
6746-
if k.UserID == u.ID {
6747-
q.apiKeys[i] = q.apiKeys[len(q.apiKeys)-1]
6748-
q.apiKeys = q.apiKeys[:len(q.apiKeys)-1]
6749-
// We removed an element, so decrement
6750-
i--
6751-
}
6752-
i++
6755+
if params.Deleted {
6756+
// NOTE: In the real world, this is done by a trigger.
6757+
q.apiKeys = slices.DeleteFunc(q.apiKeys, func(u database.APIKey) bool {
6758+
return params.ID == u.UserID
6759+
})
6760+
6761+
q.userLinks = slices.DeleteFunc(q.userLinks, func(u database.UserLink) bool {
6762+
return params.ID == u.UserID
6763+
})
67536764
}
67546765
return nil
67556766
}
@@ -6804,6 +6815,10 @@ func (q *FakeQuerier) UpdateUserLink(_ context.Context, params database.UpdateUs
68046815
q.mutex.Lock()
68056816
defer q.mutex.Unlock()
68066817

6818+
if u, err := q.getUserByIDNoLock(params.UserID); err == nil && u.Deleted {
6819+
return database.UserLink{}, deletedUserLinkError
6820+
}
6821+
68076822
for i, link := range q.userLinks {
68086823
if link.UserID == params.UserID && link.LoginType == params.LoginType {
68096824
link.OAuthAccessToken = params.OAuthAccessToken

‎coderd/database/migrations/000190_trigger_delete_user_user_link.down.sql renamed to ‎coderd/database/migrations/000191_trigger_delete_user_user_link.down.sql

File renamed without changes.

‎coderd/database/migrations/000190_trigger_delete_user_user_link.up.sql renamed to ‎coderd/database/migrations/000191_trigger_delete_user_user_link.up.sql

File renamed without changes.

‎coderd/userauth_test.go

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"net/url"
1010
"strings"
1111
"testing"
12+
"time"
1213

1314
"github.com/coreos/go-oidc/v3/oidc"
1415
"github.com/golang-jwt/jwt/v4"
@@ -24,6 +25,7 @@ import (
2425
"github.com/coder/coder/v2/coderd/coderdtest"
2526
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
2627
"github.com/coder/coder/v2/coderd/database"
28+
"github.com/coder/coder/v2/coderd/database/dbauthz"
2729
"github.com/coder/coder/v2/coderd/database/dbgen"
2830
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2931
"github.com/coder/coder/v2/coderd/promoauth"
@@ -632,7 +634,7 @@ func TestUserOAuth2Github(t *testing.T) {
632634
coderEmail,
633635
}
634636

635-
owner := coderdtest.New(t, &coderdtest.Options{
637+
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
636638
Auditor: auditor,
637639
GithubOAuth2Config: &coderd.GithubOAuth2Config{
638640
AllowSignups: true,
@@ -655,9 +657,12 @@ func TestUserOAuth2Github(t *testing.T) {
655657
},
656658
},
657659
})
658-
coderdtest.CreateFirstUser(t, owner)
660+
first := coderdtest.CreateFirstUser(t, owner)
659661

660662
ctx := testutil.Context(t, testutil.WaitLong)
663+
ownerUser, err := owner.User(context.Background(), "me")
664+
require.NoError(t, err)
665+
661666
// Create the user, then delete the user, then create again.
662667
// This causes the email change to fail.
663668
client := codersdk.New(owner.URL)
@@ -668,6 +673,22 @@ func TestUserOAuth2Github(t *testing.T) {
668673

669674
err = owner.DeleteUser(ctx, deleted.ID)
670675
require.NoError(t, err)
676+
// Check no user links for the user
677+
links, err := db.GetUserLinksByUserID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(ownerUser, first.OrganizationID)), deleted.ID)
678+
require.NoError(t, err)
679+
require.Empty(t, links)
680+
681+
// Make sure a user_link cannot be created with a deleted user.
682+
_, err = db.InsertUserLink(dbauthz.AsSystemRestricted(ctx), database.InsertUserLinkParams{
683+
UserID: deleted.ID,
684+
LoginType: "github",
685+
LinkedID: "100",
686+
OAuthAccessToken: "random",
687+
OAuthRefreshToken: "random",
688+
OAuthExpiry: time.Now(),
689+
DebugContext: []byte(`{}`),
690+
})
691+
require.ErrorContains(t, err, "Cannot create user_link for deleted user")
671692

672693
// Create the user again.
673694
client, _ = fake.Login(t, client, jwt.MapClaims{})

0 commit comments

Comments
 (0)