Add migrations to prevent deleted users with links

coder · Emyrk · Feb 20, 2024 · Feb 12, 2024 · Feb 12, 2024 · Feb 12, 2024
commit 188f03ba50a1d0265b6198461fde655171a6f990
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -5573,10 +5573,26 @@ func (q *FakeQuerier) InsertUserGroupsByName(_ context.Context, arg database.Ins
 	return nil
 }
 
+// Took the error from the real database.
+var deletedUserLinkError = &pq.Error{
+	Severity: "ERROR",
+	// "raise_exception" error
+	Code:    "P0001",
+	Message: "Cannot create user_link for deleted user",
+	Where:   "PL/pgSQL function insert_user_links_fail_if_user_deleted() line 7 at RAISE",
+	File:    "pl_exec.c",
+	Line:    "3864",
+	Routine: "exec_stmt_raise",
+}
+
 func (q *FakeQuerier) InsertUserLink(_ context.Context, args database.InsertUserLinkParams) (database.UserLink, error) {
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
+	if u, err := q.getUserByIDNoLock(args.UserID); err == nil && u.Deleted {
+		return database.UserLink{}, deletedUserLinkError
+	}
+
 	//nolint:gosimple
 	link := database.UserLink{
 		UserID:                 args.UserID,
@@ -6736,20 +6752,15 @@ func (q *FakeQuerier) UpdateUserDeletedByID(_ context.Context, params database.U
 		if u.ID == params.ID {
 			u.Deleted = params.Deleted
 			q.users[i] = u
-			// NOTE: In the real world, this is done by a trigger.
-			i := 0
-			for {
-				if i >= len(q.apiKeys) {
-					break
-				}
-				k := q.apiKeys[i]
-				if k.UserID == u.ID {
-					q.apiKeys[i] = q.apiKeys[len(q.apiKeys)-1]
-					q.apiKeys = q.apiKeys[:len(q.apiKeys)-1]
-					// We removed an element, so decrement
-					i--
-				}
-				i++
+			if params.Deleted {
+				// NOTE: In the real world, this is done by a trigger.
+				q.apiKeys = slices.DeleteFunc(q.apiKeys, func(u database.APIKey) bool {
+					return params.ID == u.UserID
+				})
+
+				q.userLinks = slices.DeleteFunc(q.userLinks, func(u database.UserLink) bool {
+					return params.ID == u.UserID
+				})
 			}
 			return nil
 		}
@@ -6804,6 +6815,10 @@ func (q *FakeQuerier) UpdateUserLink(_ context.Context, params database.UpdateUs
 	q.mutex.Lock()
 	defer q.mutex.Unlock()
 
+	if u, err := q.getUserByIDNoLock(params.UserID); err == nil && u.Deleted {
+		return database.UserLink{}, deletedUserLinkError
+	}
+
 	for i, link := range q.userLinks {
 		if link.UserID == params.UserID && link.LoginType == params.LoginType {
 			link.OAuthAccessToken = params.OAuthAccessToken

diff --git a/...90_trigger_delete_user_user_link.down.sql → ...91_trigger_delete_user_user_link.down.sql b/...90_trigger_delete_user_user_link.down.sql → ...91_trigger_delete_user_user_link.down.sql
diff --git a/...0190_trigger_delete_user_user_link.up.sql → ...0191_trigger_delete_user_user_link.up.sql b/...0190_trigger_delete_user_user_link.up.sql → ...0191_trigger_delete_user_user_link.up.sql
diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/golang-jwt/jwt/v4"
@@ -24,6 +25,7 @@ import (
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
 	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/promoauth"
@@ -632,7 +634,7 @@ func TestUserOAuth2Github(t *testing.T) {
 			coderEmail,
 		}
 
-		owner := coderdtest.New(t, &coderdtest.Options{
+		owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
 			Auditor: auditor,
 			GithubOAuth2Config: &coderd.GithubOAuth2Config{
 				AllowSignups:  true,
@@ -655,9 +657,12 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 			},
 		})
-		coderdtest.CreateFirstUser(t, owner)
+		first := coderdtest.CreateFirstUser(t, owner)
 
 		ctx := testutil.Context(t, testutil.WaitLong)
+		ownerUser, err := owner.User(context.Background(), "me")
+		require.NoError(t, err)
+
 		// Create the user, then delete the user, then create again.
 		// This causes the email change to fail.
 		client := codersdk.New(owner.URL)
@@ -668,6 +673,22 @@ func TestUserOAuth2Github(t *testing.T) {
 
 		err = owner.DeleteUser(ctx, deleted.ID)
 		require.NoError(t, err)
+		// Check no user links for the user
+		links, err := db.GetUserLinksByUserID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(ownerUser, first.OrganizationID)), deleted.ID)
+		require.NoError(t, err)
+		require.Empty(t, links)
+
+		// Make sure a user_link cannot be created with a deleted user.
+		_, err = db.InsertUserLink(dbauthz.AsSystemRestricted(ctx), database.InsertUserLinkParams{
+			UserID:            deleted.ID,
+			LoginType:         "github",
+			LinkedID:          "100",
+			OAuthAccessToken:  "random",
+			OAuthRefreshToken: "random",
+			OAuthExpiry:       time.Now(),
+			DebugContext:      []byte(`{}`),
+		})
+		require.ErrorContains(t, err, "Cannot create user_link for deleted user")
 
 		// Create the user again.
 		client, _ = fake.Login(t, client, jwt.MapClaims{})