Skip to content

feat: don't return 200 for deleted workspaces #1556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions coderd/autobuild/executor/lifecycle_executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"os"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -470,6 +471,9 @@ func mustTransitionWorkspace(t *testing.T, client *codersdk.Client, workspaceID
func mustWorkspace(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) codersdk.Workspace {
ctx := context.Background()
ws, err := client.Workspace(ctx, workspaceID)
if err != nil && strings.Contains(err.Error(), "status code 410") {
ws, err = client.DeletedWorkspace(ctx, workspaceID)
}
require.NoError(t, err, "no workspace found with id %s", workspaceID)
return ws
}
Expand Down
35 changes: 35 additions & 0 deletions coderd/workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"net/http"
"strconv"
"time"

"github.com/go-chi/chi/v5"
Expand Down Expand Up @@ -34,6 +35,40 @@ func (api *api) workspace(rw http.ResponseWriter, r *http.Request) {
return
}

if !api.Authorize(rw, r, rbac.ActionRead,
rbac.ResourceWorkspace.InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) {
return
}

// The `deleted` query parameter (which defaults to `false`) MUST match the
// `Deleted` field on the workspace otherwise you will get a 410 Gone.
var (
deletedStr = r.URL.Query().Get("deleted")
showDeleted = false
)
if deletedStr != "" {
var err error
showDeleted, err = strconv.ParseBool(deletedStr)
if err != nil {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("invalid bool for 'deleted' query param: %s", err),
})
return
}
}
if workspace.Deleted && !showDeleted {
httpapi.Write(rw, http.StatusGone, httpapi.Response{
Message: fmt.Sprintf("workspace %q was deleted, you can view this workspace by specifying '?deleted=true' and trying again", workspace.ID.String()),
})
return
}
if !workspace.Deleted && showDeleted {
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
Message: fmt.Sprintf("workspace %q is not deleted, please remove '?deleted=true' and try again", workspace.ID.String()),
})
return
}

build, err := api.Database.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID)
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Expand Down
52 changes: 52 additions & 0 deletions coderd/workspaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,58 @@ import (
"github.com/coder/coder/provisionersdk/proto"
)

func TestWorkspace(t *testing.T) {
t.Parallel()

t.Run("OK", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
user := coderdtest.CreateFirstUser(t, client)
coderdtest.NewProvisionerDaemon(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)

_, err := client.Workspace(context.Background(), workspace.ID)
require.NoError(t, err)
})

t.Run("Deleted", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
user := coderdtest.CreateFirstUser(t, client)
coderdtest.NewProvisionerDaemon(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)

// Getting with deleted=true should fail.
_, err := client.DeletedWorkspace(context.Background(), workspace.ID)
require.Error(t, err)
require.ErrorContains(t, err, "400") // bad request

// Delete the workspace
build, err := client.CreateWorkspaceBuild(context.Background(), workspace.ID, codersdk.CreateWorkspaceBuildRequest{
Transition: database.WorkspaceTransitionDelete,
})
require.NoError(t, err, "delete the workspace")
coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID)

// Getting with deleted=true should work.
workspaceNew, err := client.DeletedWorkspace(context.Background(), workspace.ID)
require.NoError(t, err)
require.Equal(t, workspace.ID, workspaceNew.ID)

// Getting with deleted=false should not work.
_, err = client.Workspace(context.Background(), workspace.ID)
require.Error(t, err)
require.ErrorContains(t, err, "410") // gone
})
}

func TestAdminViewAllWorkspaces(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
Expand Down
8 changes: 8 additions & 0 deletions codersdk/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ type Client struct {

type requestOption func(*http.Request)

func queryParam(k, v string) requestOption {
return func(r *http.Request) {
q := r.URL.Query()
q.Set(k, v)
r.URL.RawQuery = q.Encode()
}
}

// Request performs an HTTP request with the body provided.
// The caller is responsible for closing the response body.
func (c *Client) Request(ctx context.Context, method, path string, body interface{}, opts ...requestOption) (*http.Response, error) {
Expand Down
11 changes: 10 additions & 1 deletion codersdk/workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,16 @@ type CreateWorkspaceBuildRequest struct {

// Workspace returns a single workspace.
func (c *Client) Workspace(ctx context.Context, id uuid.UUID) (Workspace, error) {
res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s", id), nil)
return c.getWorkspace(ctx, id)
}

// DeletedWorkspace returns a single workspace that was deleted.
func (c *Client) DeletedWorkspace(ctx context.Context, id uuid.UUID) (Workspace, error) {
return c.getWorkspace(ctx, id, queryParam("deleted", "true"))
}

func (c *Client) getWorkspace(ctx context.Context, id uuid.UUID, opts ...requestOption) (Workspace, error) {
res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s", id), nil, opts...)
if err != nil {
return Workspace{}, err
}
Expand Down