diff --git a/coderd/aitasks.go b/coderd/aitasks.go index 3f88e8ede2..f8ec5eaef3 100644 --- a/coderd/aitasks.go +++ b/coderd/aitasks.go @@ -121,24 +121,10 @@ func (api *API) tasksCreate(rw http.ResponseWriter, r *http.Request) { } } - // Generate task name and display name if either is not provided - if taskName == "" || taskDisplayName == "" { - generatedTaskName := taskname.Generate(ctx, api.Logger, req.Input) - - if taskName == "" { - taskName = generatedTaskName.Name - } - if taskDisplayName == "" { - taskDisplayName = generatedTaskName.DisplayName - } - } - - createReq := codersdk.CreateWorkspaceRequest{ - Name: taskName, - TemplateVersionID: req.TemplateVersionID, - TemplateVersionPresetID: req.TemplateVersionPresetID, - } - + // Resolve the workspace owner before generating a task name so required + // external auth can be enforced up front. createWorkspace performs the same + // validation, but checking here keeps the Tasks API aligned with the gates + // the UI presents and avoids generating a name for a task that is rejected. var owner workspaceOwner if mems.User != nil { // This user fetch is an optimization path for the most common case of creating a @@ -177,6 +163,48 @@ func (api *API) tasksCreate(rw http.ResponseWriter, r *http.Request) { taskResourceInfo.WorkspaceOwner = owner.Username } + // Authorize workspace creation before the external auth preflight below. + // createWorkspace re-checks these gates as the authoritative defense, but + // requireWorkspaceOwnerExternalAuth validates (and may refresh or clear) the + // owner's external auth tokens under a system-restricted context. Running it + // before proving the caller may create a workspace for this owner using this + // template would let an unauthorized caller trigger token refresh side + // effects and probe another user's auth state. Mirror the ordering in + // createWorkspace so the side-effectful preflight only runs once the caller + // is authorized. + if _, err := api.preflightWorkspaceCreate(ctx, owner.ID, codersdk.CreateWorkspaceRequest{ + TemplateVersionID: req.TemplateVersionID, + }); err != nil { + httperror.WriteResponseError(ctx, rw, err) + return + } + + // Required external auth is otherwise only enforced once createWorkspace + // runs. Validate it here so the Tasks API rejects an owner who is missing a + // required provider before any task name generation or row insertion. + if err := api.requireWorkspaceOwnerExternalAuth(ctx, templateVersion, owner.ID); err != nil { + httperror.WriteResponseError(ctx, rw, err) + return + } + + // Generate task name and display name if either is not provided + if taskName == "" || taskDisplayName == "" { + generatedTaskName := taskname.Generate(ctx, api.Logger, req.Input) + + if taskName == "" { + taskName = generatedTaskName.Name + } + if taskDisplayName == "" { + taskDisplayName = generatedTaskName.DisplayName + } + } + + createReq := codersdk.CreateWorkspaceRequest{ + Name: taskName, + TemplateVersionID: req.TemplateVersionID, + TemplateVersionPresetID: req.TemplateVersionPresetID, + } + // Track insert from preCreateInTX. var dbTaskTable database.TaskTable diff --git a/coderd/aitasks_test.go b/coderd/aitasks_test.go index b1f703b912..a5425eba62 100644 --- a/coderd/aitasks_test.go +++ b/coderd/aitasks_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "regexp" "strings" "testing" "time" @@ -30,6 +31,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" + "github.com/coder/coder/v2/coderd/externalauth" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" @@ -1466,6 +1468,250 @@ func TestTasks(t *testing.T) { }) } +func TestCreateTaskExternalAuth(t *testing.T) { + t.Parallel() + + // The expected 403 message returned when the task owner is missing required + // external auth. The Tasks create handler shares this message with + // createWorkspace via requireWorkspaceOwnerExternalAuth. + const externalAuthRequiredMessage = "External authentication is required to create a workspace with this template." + + // taskExternalAuthVersion returns echo responses for a template version that + // is both AI-task-capable and references the given external auth providers. + taskExternalAuthVersion := func(providers ...*proto.ExternalAuthProviderResource) *echo.Responses { + authToken := uuid.NewString() + taskAppID := uuid.NewString() + return &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionGraph: []*proto.Response{{ + Type: &proto.Response_Graph{ + Graph: &proto.GraphComplete{ + HasAiTasks: true, + Resources: []*proto.Resource{{ + Name: "example", + Type: "aws_instance", + Agents: []*proto.Agent{{ + Id: uuid.NewString(), + Name: "example", + Auth: &proto.Agent_Token{ + Token: authToken, + }, + Apps: []*proto.App{{ + Id: taskAppID, + Slug: "task-app", + DisplayName: "Task App", + Url: "", + }}, + }}, + }}, + AiTasks: []*proto.AITask{{ + AppId: taskAppID, + }}, + ExternalAuthProviders: providers, + }, + }, + }}, + } + } + + t.Run("RequiredAuthMissing", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ExternalAuthConfigs: []*externalauth.Config{{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + DisplayName: "GitHub", + }}, + }) + first := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, + taskExternalAuthVersion(&proto.ExternalAuthProviderResource{Id: "github"})) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + memberClient, member := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitLong) + + // Provide both an explicit name and display name so the create handler + // skips task-name generation entirely. The handler generates a name + // when either field is empty, and the external auth preflight now runs + // after the workspace authorization gates but before name generation. + req := codersdk.CreateTaskRequest{ + TemplateVersionID: template.ActiveVersionID, + Input: "build me a web app", + Name: coderdtest.RandomUsername(t), + DisplayName: "My Task", + } + _, err := memberClient.CreateTask(ctx, codersdk.Me, req) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + require.Equal(t, externalAuthRequiredMessage, apiErr.Message) + require.Equal(t, "The workspace owner must authenticate with the following external auth providers: GitHub.", apiErr.Detail) + require.Equal(t, []codersdk.ValidationError{{ + Field: "external_auth", + Detail: "github", + }}, apiErr.Validations) + + // The rejection must happen before any task row is inserted. + _, err = memberClient.TaskByOwnerAndName(ctx, codersdk.Me, req.Name) + apiErr = nil + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + + // Authenticating with the provider lifts the rejection. + resp := coderdtest.RequestExternalAuthCallback(t, "github", memberClient) + _ = resp.Body.Close() + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + task, err := memberClient.CreateTask(ctx, codersdk.Me, req) + require.NoError(t, err) + require.Equal(t, member.ID, task.OwnerID) + }) + + t.Run("OwnerVsInitiator", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ExternalAuthConfigs: []*externalauth.Config{{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + DisplayName: "GitHub", + }}, + }) + first := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, + taskExternalAuthVersion(&proto.ExternalAuthProviderResource{Id: "github"})) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + memberClient, member := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitLong) + + // The initiating admin is authenticated with the provider, but the task + // owner (the member) is not. Token injection at build time uses the + // owner's links, so the owner's auth state is what the preflight checks, + // not the initiator's. + resp := coderdtest.RequestExternalAuthCallback(t, "github", client) + _ = resp.Body.Close() + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + req := codersdk.CreateTaskRequest{ + TemplateVersionID: template.ActiveVersionID, + Input: "build me a web app", + Name: coderdtest.RandomUsername(t), + } + _, err := client.CreateTask(ctx, member.Username, req) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + require.Equal(t, externalAuthRequiredMessage, apiErr.Message) + + // Once the owner authenticates, the same create succeeds even though the + // initiator's auth state is unchanged. + resp = coderdtest.RequestExternalAuthCallback(t, "github", memberClient) + _ = resp.Body.Close() + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + task, err := client.CreateTask(ctx, member.Username, req) + require.NoError(t, err) + require.Equal(t, member.ID, task.OwnerID) + }) + + t.Run("OptionalProvider", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ExternalAuthConfigs: []*externalauth.Config{{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + DisplayName: "GitHub", + }}, + }) + first := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, + taskExternalAuthVersion(&proto.ExternalAuthProviderResource{Id: "github", Optional: true})) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + memberClient, member := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitLong) + + // Optional providers must not block creation even when the owner has + // never authenticated with them. + task, err := memberClient.CreateTask(ctx, codersdk.Me, codersdk.CreateTaskRequest{ + TemplateVersionID: template.ActiveVersionID, + Input: "build me a web app", + Name: coderdtest.RandomUsername(t), + }) + require.NoError(t, err) + require.Equal(t, member.ID, task.OwnerID) + }) + + t.Run("AuthzDenialShortCircuitsExternalAuth", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ExternalAuthConfigs: []*externalauth.Config{{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "github", + Regex: regexp.MustCompile(`github\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + DisplayName: "GitHub", + }}, + }) + first := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, + taskExternalAuthVersion(&proto.ExternalAuthProviderResource{Id: "github"})) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + + // The caller is a normal org member (so it can read the template + // version and resolve "me" as the workspace owner) but is banned from + // creating workspaces via a negative org-level workspace:create + // permission. This reaches the workspace-create authorization gate and + // fails it, which is exactly the ordering under test: the authz denial + // must short-circuit before requireWorkspaceOwnerExternalAuth runs. + bannedClient, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, + rbac.ScopedRoleOrgWorkspaceCreationBan(first.OrganizationID)) + + ctx := testutil.Context(t, testutil.WaitLong) + + // The owner ("me") has NOT authenticated with the required GitHub + // provider. If the external auth preflight ran first (the pre-fix + // ordering) this request would fail with externalAuthRequiredMessage. + // Provide an explicit name so we can assert no task row was inserted. + req := codersdk.CreateTaskRequest{ + TemplateVersionID: template.ActiveVersionID, + Input: "build me a web app", + Name: coderdtest.RandomUsername(t), + DisplayName: "My Task", + } + _, err := bannedClient.CreateTask(ctx, codersdk.Me, req) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + // The workspace-create authorization denial wins: we get the authz + // message, NOT the external auth requirement. This proves the authz + // checks run before (and short-circuit) the external auth preflight. + require.Equal(t, "Unauthorized to create workspace.", apiErr.Message) + require.NotEqual(t, externalAuthRequiredMessage, apiErr.Message) + + // The denial must short-circuit before any task row is inserted. + _, err = bannedClient.TaskByOwnerAndName(ctx, codersdk.Me, req.Name) + apiErr = nil + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + }) +} + func TestTasksCreate(t *testing.T) { t.Parallel() diff --git a/coderd/workspaces.go b/coderd/workspaces.go index f4403aee56..3e68cdcc43 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -548,54 +548,14 @@ func createWorkspace( opts = &createWorkspaceOptions{} } - template, err := requestTemplate(ctx, req, api.Database) + template, err := api.preflightWorkspaceCreate(ctx, owner.ID, req) if err != nil { return codersdk.Workspace{}, err } - // This is a premature auth check to avoid doing unnecessary work if the user - // doesn't have permission to create a workspace. - if !api.HTTPAuth.AuthorizeContext(ctx, policy.ActionCreate, - rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) { - // If this check fails, return a proper unauthorized error to the user to indicate - // what is going on. - return codersdk.Workspace{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ - Message: "Unauthorized to create workspace.", - Detail: "You are unable to create a workspace in this organization. " + - "It is possible to have access to the template, but not be able to create a workspace. " + - "Please contact an administrator about your permissions if you feel this is an error.", - }) - } - // 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.HTTPAuth.AuthorizeContext(ctx, policy.ActionCreate, - rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) { - return codersdk.Workspace{}, httperror.ErrResourceNotFound - } - // The user also needs permission to use the template. At this point they have - // read perms, but not necessarily "use". This is also checked in `db.InsertWorkspace`. - // Doing this up front can save some work below if the user doesn't have permission. - if !api.HTTPAuth.AuthorizeContext(ctx, policy.ActionUse, template) { - return codersdk.Workspace{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Unauthorized access to use the template %q.", template.Name), - Detail: "Although you are able to view the template, you are unable to create a workspace using it. " + - "Please contact an administrator about your permissions if you feel this is an error.", - }) - } - - templateAccessControl := (*(api.AccessControlStore.Load())).GetTemplateAccessControl(template) - if templateAccessControl.IsDeprecated() { - return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Template %q has been deprecated, and cannot be used to create a new workspace.", template.Name), - // Pass the deprecated message to the user. - Detail: templateAccessControl.Deprecated, - }) - } - // Required external auth is otherwise only enforced by client-side preflight // checks in the CLI and UI, so API-created workspaces must be validated here // before any workspace row is inserted or prebuilt workspace is claimed. @@ -958,6 +918,64 @@ func (api *API) requireWorkspaceOwnerExternalAuth(ctx context.Context, templateV }) } +// preflightWorkspaceCreate resolves the template targeted by req and verifies +// that the caller may create a workspace owned by ownerID using it. It performs +// only side-effect-free authorization, so it is safe to call as an early gate: +// +// - resolve the template (requestTemplate) +// - ActionCreate on a workspace in the template's organization for the owner +// - ActionUse on the template +// - reject deprecated templates +// +// It deliberately does not validate required external auth (that mutates the +// owner's external auth links and is enforced separately) and does not touch +// the audit request, so callers retain control over audit-organization +// assignment and external-auth ordering. Both createWorkspace and tasksCreate +// call it so these authorization gates cannot diverge. +func (api *API) preflightWorkspaceCreate(ctx context.Context, ownerID uuid.UUID, req codersdk.CreateWorkspaceRequest) (database.Template, error) { + template, err := requestTemplate(ctx, req, api.Database) + if err != nil { + return database.Template{}, err + } + + // This is a premature auth check to avoid doing unnecessary work if the + // user doesn't have permission to create a workspace. + if !api.HTTPAuth.AuthorizeContext(ctx, policy.ActionCreate, + rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(ownerID.String())) { + // If this check fails, return a proper unauthorized error to the user + // to indicate what is going on. + return database.Template{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ + Message: "Unauthorized to create workspace.", + Detail: "You are unable to create a workspace in this organization. " + + "It is possible to have access to the template, but not be able to create a workspace. " + + "Please contact an administrator about your permissions if you feel this is an error.", + }) + } + + // The user also needs permission to use the template. At this point they + // have read perms, but not necessarily "use". This is also checked in + // `db.InsertWorkspace`. Doing this up front can save some work below if the + // user doesn't have permission. + if !api.HTTPAuth.AuthorizeContext(ctx, policy.ActionUse, template) { + return database.Template{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ + Message: fmt.Sprintf("Unauthorized access to use the template %q.", template.Name), + Detail: "Although you are able to view the template, you are unable to create a workspace using it. " + + "Please contact an administrator about your permissions if you feel this is an error.", + }) + } + + templateAccessControl := (*(api.AccessControlStore.Load())).GetTemplateAccessControl(template) + if templateAccessControl.IsDeprecated() { + return database.Template{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Template %q has been deprecated, and cannot be used to create a new workspace.", template.Name), + // Pass the deprecated message to the user. + Detail: templateAccessControl.Deprecated, + }) + } + + return template, nil +} + func requestTemplate(ctx context.Context, req codersdk.CreateWorkspaceRequest, db database.Store) (database.Template, error) { // If we were given a `TemplateVersionID`, we need to determine the `TemplateID` from it. templateID := req.TemplateID