From 8ae63e18cf2f5217bbf540b30e8ba03c937a1312 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 10 Oct 2022 14:29:07 +0000 Subject: [PATCH] fix: Return deleted users when fetching workspace builds Fixes #4359. --- coderd/database/databasefake/databasefake.go | 7 ++--- coderd/database/querier.go | 5 +++- coderd/database/queries.sql.go | 14 ++++----- coderd/database/queries/users.sql | 5 +++- coderd/workspacebuilds.go | 12 +++----- coderd/workspacebuilds_test.go | 31 ++++++++++++++++++++ coderd/workspaces.go | 4 +-- 7 files changed, 52 insertions(+), 26 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 7b0c3515eb4cc..895d85033dab8 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -479,19 +479,16 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams return users, nil } -func (q *fakeQuerier) GetUsersByIDs(_ context.Context, params database.GetUsersByIDsParams) ([]database.User, error) { +func (q *fakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]database.User, error) { q.mutex.RLock() defer q.mutex.RUnlock() users := make([]database.User, 0) for _, user := range q.users { - for _, id := range params.IDs { + for _, id := range ids { if user.ID.String() != id.String() { continue } - if user.Deleted != params.Deleted { - continue - } users = append(users, user) } } diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 35dd5bf27dff8..8e02949736d5b 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -76,7 +76,10 @@ type querier interface { GetUserLinkByLinkedID(ctx context.Context, linkedID string) (UserLink, error) GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) GetUsers(ctx context.Context, arg GetUsersParams) ([]User, error) - GetUsersByIDs(ctx context.Context, arg GetUsersByIDsParams) ([]User, error) + // This shouldn't check for deleted, because it's frequently used + // to look up references to actions. eg. a user could build a workspace + // for another user, then be deleted... we still want them to appear! + GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User, error) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (WorkspaceAgent, error) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (WorkspaceAgent, error) GetWorkspaceAgentByInstanceID(ctx context.Context, authInstanceID string) (WorkspaceAgent, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index e2c6b978ffe93..faa4dc42c54ca 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3305,16 +3305,14 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User, } const getUsersByIDs = `-- name: GetUsersByIDs :many -SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at FROM users WHERE id = ANY($1 :: uuid [ ]) AND deleted = $2 +SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at FROM users WHERE id = ANY($1 :: uuid [ ]) ` -type GetUsersByIDsParams struct { - IDs []uuid.UUID `db:"ids" json:"ids"` - Deleted bool `db:"deleted" json:"deleted"` -} - -func (q *sqlQuerier) GetUsersByIDs(ctx context.Context, arg GetUsersByIDsParams) ([]User, error) { - rows, err := q.db.QueryContext(ctx, getUsersByIDs, pq.Array(arg.IDs), arg.Deleted) +// This shouldn't check for deleted, because it's frequently used +// to look up references to actions. eg. a user could build a workspace +// for another user, then be deleted... we still want them to appear! +func (q *sqlQuerier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User, error) { + rows, err := q.db.QueryContext(ctx, getUsersByIDs, pq.Array(ids)) if err != nil { return nil, err } diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 8f29e8a8d2577..084d7246a05e4 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -9,7 +9,10 @@ LIMIT 1; -- name: GetUsersByIDs :many -SELECT * FROM users WHERE id = ANY(@ids :: uuid [ ]) AND deleted = @deleted; +-- This shouldn't check for deleted, because it's frequently used +-- to look up references to actions. eg. a user could build a workspace +-- for another user, then be deleted... we still want them to appear! +SELECT * FROM users WHERE id = ANY(@ids :: uuid [ ]); -- name: GetUserByEmailOrUsername :one SELECT diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index e044bae5a4784..b2d8424907df4 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -492,11 +492,9 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { return } - users, err := api.Database.GetUsersByIDs(ctx, database.GetUsersByIDsParams{ - IDs: []uuid.UUID{ - workspace.OwnerID, - workspaceBuild.InitiatorID, - }, + users, err := api.Database.GetUsersByIDs(ctx, []uuid.UUID{ + workspace.OwnerID, + workspaceBuild.InitiatorID, }) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -679,9 +677,7 @@ func (api *API) workspaceBuildsData(ctx context.Context, workspaces []database.W for _, workspace := range workspaces { userIDs = append(userIDs, workspace.OwnerID) } - users, err := api.Database.GetUsersByIDs(ctx, database.GetUsersByIDsParams{ - IDs: userIDs, - }) + users, err := api.Database.GetUsersByIDs(ctx, userIDs) if err != nil { return workspaceBuildsData{}, xerrors.Errorf("get users: %w", err) } diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 9e35010345536..c4c3f7d364220 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -178,6 +178,37 @@ func TestWorkspaceBuilds(t *testing.T) { require.Len(t, builds, 1) }) + t.Run("DeletedInitiator", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + first := coderdtest.CreateFirstUser(t, client) + second := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, "owner") + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + secondUser, err := second.User(ctx, codersdk.Me) + require.NoError(t, err, "fetch me") + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + workspace, err := second.CreateWorkspace(ctx, first.OrganizationID, first.UserID.String(), codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: "example", + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + + err = client.DeleteUser(ctx, secondUser.ID) + require.NoError(t, err) + + builds, err := client.WorkspaceBuilds(ctx, codersdk.WorkspaceBuildsRequest{WorkspaceID: workspace.ID}) + require.Len(t, builds, 1) + require.Equal(t, int32(1), builds[0].BuildNumber) + require.Equal(t, secondUser.Username, builds[0].InitiatorUsername) + require.NoError(t, err) + }) + t.Run("PaginateNonExistentRow", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index b845cb485e626..0ce8ff347ef83 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -463,9 +463,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req } aReq.New = workspace - users, err := api.Database.GetUsersByIDs(ctx, database.GetUsersByIDsParams{ - IDs: []uuid.UUID{user.ID, workspaceBuild.InitiatorID}, - }) + users, err := api.Database.GetUsersByIDs(ctx, []uuid.UUID{user.ID, workspaceBuild.InitiatorID}) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching user.",