From c710840a435058d85509713527db923eda822f89 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Jul 2024 21:29:02 -0500 Subject: [PATCH 1/6] chore: always use new codepath over deprecated --- coderd/audit/request.go | 2 ++ coderd/coderdtest/coderdtest.go | 19 ------------------- coderd/workspaces.go | 10 ++++++++++ codersdk/organizations.go | 15 ++------------- 4 files changed, 14 insertions(+), 32 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 92b0a8a8accc2..6c862c6e11103 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -51,6 +51,8 @@ type Request[T Auditable] struct { Action database.AuditAction } +// UpdateOrganizationID can be used if the organization ID is not known +// at the initiation of an audit log request. func (r *Request[T]) UpdateOrganizationID(id uuid.UUID) { r.params.OrganizationID = id } diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 643a1dd59d70c..9a1640e620d31 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -1064,25 +1064,6 @@ func (w WorkspaceAgentWaiter) Wait() []codersdk.WorkspaceResource { // CreateWorkspace creates a workspace for the user and template provided. // A random name is generated for it. // To customize the defaults, pass a mutator func. -// -// Deprecated: Use CreateWorkspace. -func CreateWorkspaceByOrganization(t testing.TB, client *codersdk.Client, organization uuid.UUID, templateID uuid.UUID, mutators ...func(*codersdk.CreateWorkspaceRequest)) codersdk.Workspace { - t.Helper() - req := codersdk.CreateWorkspaceRequest{ - TemplateID: templateID, - Name: RandomUsername(t), - AutostartSchedule: ptr.Ref("CRON_TZ=US/Central 30 9 * * 1-5"), - TTLMillis: ptr.Ref((8 * time.Hour).Milliseconds()), - AutomaticUpdates: codersdk.AutomaticUpdatesNever, - } - for _, mutator := range mutators { - mutator(&req) - } - workspace, err := client.CreateWorkspace(context.Background(), organization, codersdk.Me, req) - require.NoError(t, err) - return workspace -} - func CreateWorkspace(t testing.TB, client *codersdk.Client, templateID uuid.UUID, mutators ...func(*codersdk.CreateWorkspaceRequest)) codersdk.Workspace { t.Helper() req := codersdk.CreateWorkspaceRequest{ diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 584651298f023..049b2b77e75ed 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -521,8 +521,18 @@ func createWorkspace( }) return } + + // Update audit log's organization auditReq.UpdateOrganizationID(template.OrganizationID) + // Do this upfront to save work. If this fails, the rest of the work + // would be wasted. + if !api.Authorize(r, policy.ActionCreate, + rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) + return + } + templateAccessControl := (*(api.AccessControlStore.Load())).GetTemplateAccessControl(template) if templateAccessControl.IsDeprecated() { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 8d8a9304d60fd..277d41cf9ae52 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -475,19 +475,8 @@ func (c *Client) TemplateByName(ctx context.Context, organizationID uuid.UUID, n // CreateWorkspace creates a new workspace for the template specified. // // Deprecated: Use CreateUserWorkspace instead. -func (c *Client) CreateWorkspace(ctx context.Context, organizationID uuid.UUID, user string, request CreateWorkspaceRequest) (Workspace, error) { - res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/members/%s/workspaces", organizationID, user), request) - if err != nil { - return Workspace{}, err - } - defer res.Body.Close() - - if res.StatusCode != http.StatusCreated { - return Workspace{}, ReadBodyAsError(res) - } - - var workspace Workspace - return workspace, json.NewDecoder(res.Body).Decode(&workspace) +func (c *Client) CreateWorkspace(ctx context.Context, _ uuid.UUID, user string, request CreateWorkspaceRequest) (Workspace, error) { + return c.CreateUserWorkspace(ctx, user, request) } // CreateUserWorkspace creates a new workspace for the template specified. From bd4dd33813d2bcc4913f783adb4b86bd419d471c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Jul 2024 21:32:41 -0500 Subject: [PATCH 2/6] revert swagger change --- coderd/coderdtest/swaggerparser.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/swaggerparser.go b/coderd/coderdtest/swaggerparser.go index 1b5317e05ff4c..8ba4ddb507528 100644 --- a/coderd/coderdtest/swaggerparser.go +++ b/coderd/coderdtest/swaggerparser.go @@ -89,9 +89,9 @@ func parseSwaggerComment(commentGroup *ast.CommentGroup) SwaggerComment { failures: []response{}, } for _, line := range commentGroup.List { - // "// @ [args...]" -> []string{"//", "@", "args..."} + // @ [args...] splitN := strings.SplitN(strings.TrimSpace(line.Text), " ", 3) - if len(splitN) < 3 { + if len(splitN) < 2 { continue // comment prefix without any content } From 9901bde9d3486d354df545d048d09607a7636f60 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Jul 2024 21:51:08 -0500 Subject: [PATCH 3/6] add unit test for lack of template access --- coderd/workspaces.go | 4 +-- enterprise/coderd/workspaces_test.go | 51 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 049b2b77e75ed..fcc73d7e518af 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -464,7 +464,7 @@ func createWorkspace( templateID := req.TemplateID if templateID == uuid.Nil { templateVersion, err := api.Database.GetTemplateVersionByID(ctx, req.TemplateVersionID) - if errors.Is(err, sql.ErrNoRows) { + if httpapi.Is404Error(err) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Template version %q doesn't exist.", templateID.String()), Validations: []codersdk.ValidationError{{ @@ -498,7 +498,7 @@ func createWorkspace( } template, err := api.Database.GetTemplateByID(ctx, templateID) - if errors.Is(err, sql.ErrNoRows) { + if httpapi.Is404Error(err) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Template %q doesn't exist.", templateID.String()), Validations: []codersdk.ValidationError{{ diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 0f76da78c3da2..8dcbc693145cf 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -135,6 +135,57 @@ func TestCreateWorkspace(t *testing.T) { _, err = client1.CreateWorkspace(ctx, user.OrganizationID, user1.ID.String(), req) require.Error(t, err) }) + + t.Run("NoTemplateAccess", func(t *testing.T) { + t.Parallel() + ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + + templateAdmin, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + user, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleMember()) + + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + template := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Remove everyone access + err := templateAdmin.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{ + UserPerms: map[string]codersdk.TemplateRole{}, + GroupPerms: map[string]codersdk.TemplateRole{ + owner.OrganizationID.String(): codersdk.TemplateRoleDeleted, + }, + }) + require.NoError(t, err) + + // Test "everyone" access is revoked to the regular user + _, err = user.Template(ctx, template.ID) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + + _, err = user.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: "random", + AutostartSchedule: ptr.Ref("CRON_TZ=US/Central 30 9 * * 1-5"), + TTLMillis: ptr.Ref((8 * time.Hour).Milliseconds()), + AutomaticUpdates: codersdk.AutomaticUpdatesNever, + }) + require.Error(t, err) + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "doesn't exist") + }) } func TestCreateUserWorkspace(t *testing.T) { From 13ede7f7f0666ed8bb121c45f1c7cab95f6af8b3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Jul 2024 21:53:58 -0500 Subject: [PATCH 4/6] fmy --- enterprise/coderd/workspaces_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 8dcbc693145cf..0b758e0491e1b 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -146,7 +146,8 @@ func TestCreateWorkspace(t *testing.T) { Features: license.Features{ codersdk.FeatureTemplateRBAC: 1, }, - }}) + }, + }) templateAdmin, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) user, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleMember()) From 005dcd318e2694eabd98339ea1cf08e9c71082d6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 Jul 2024 09:40:29 -0500 Subject: [PATCH 5/6] revert swaggerparser change --- coderd/coderdtest/swaggerparser.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/swaggerparser.go b/coderd/coderdtest/swaggerparser.go index 8ba4ddb507528..1b5317e05ff4c 100644 --- a/coderd/coderdtest/swaggerparser.go +++ b/coderd/coderdtest/swaggerparser.go @@ -89,9 +89,9 @@ func parseSwaggerComment(commentGroup *ast.CommentGroup) SwaggerComment { failures: []response{}, } for _, line := range commentGroup.List { - // @ [args...] + // "// @ [args...]" -> []string{"//", "@", "args..."} splitN := strings.SplitN(strings.TrimSpace(line.Text), " ", 3) - if len(splitN) < 2 { + if len(splitN) < 3 { continue // comment prefix without any content } From ca28cea922b01f6fdbcbeff90dc536229ca0c6b5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 Jul 2024 09:56:24 -0500 Subject: [PATCH 6/6] linting --- coderd/workspaces.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index fcc73d7e518af..901e3723964bd 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -528,7 +528,7 @@ func createWorkspace( // Do this upfront to save work. If this fails, the rest of the work // would be wasted. if !api.Authorize(r, policy.ActionCreate, - rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(user.ID.String())) { + rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) { httpapi.ResourceNotFound(rw) return } @@ -588,7 +588,7 @@ func createWorkspace( // read other workspaces. Ideally we check the error on create and look for // a postgres conflict error. workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: user.ID, + OwnerID: owner.ID, Name: req.Name, }) if err == nil { @@ -621,7 +621,7 @@ func createWorkspace( ID: uuid.New(), CreatedAt: now, UpdatedAt: now, - OwnerID: user.ID, + OwnerID: owner.ID, OrganizationID: template.OrganizationID, TemplateID: template.ID, Name: req.Name, @@ -689,8 +689,8 @@ func createWorkspace( ProvisionerJob: *provisionerJob, QueuePosition: 0, }, - user.Username, - user.AvatarURL, + owner.Username, + owner.AvatarURL, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, @@ -712,8 +712,8 @@ func createWorkspace( workspace, apiBuild, template, - user.Username, - user.AvatarURL, + owner.Username, + owner.AvatarURL, api.Options.AllowWorkspaceRenames, ) if err != nil {