From fde36397146ec9e070e39ce02142de1ba74e3cfd Mon Sep 17 00:00:00 2001 From: dylanhuff-at-coder Date: Thu, 25 Jun 2026 18:47:35 -0400 Subject: [PATCH] fix(coderd): enforce required external auth on workspace create (#26314) Required external auth (`optional = false`) was only enforced by client-side preflight checks, so creating a workspace via the REST API succeeded even when the owner had never authenticated, producing a broken workspace. `createWorkspace` now validates the workspace owner's external auth server-side and returns 403 before any row is inserted or prebuild is claimed. The owner (not the initiator) is checked because build-time token injection uses their links, so this also covers admin-on-behalf-of creates and prebuild claims. Use `optional = true` to allow pre-provisioning for unauthenticated users. Fixes PLAT-241. > This PR was generated by Coder Agents on behalf of @dylanhuff-at-coder. --- coderd/templateversions.go | 35 +++-- coderd/workspacebuilds_test.go | 7 +- coderd/workspaces.go | 66 +++++++++ coderd/workspaces_test.go | 241 +++++++++++++++++++++++++++++++++ 4 files changed, 335 insertions(+), 14 deletions(-) diff --git a/coderd/templateversions.go b/coderd/templateversions.go index ef7f6e0899..682a7bb0b1 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -31,6 +31,7 @@ import ( "github.com/coder/coder/v2/coderd/dynamicparameters" "github.com/coder/coder/v2/coderd/externalauth" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/provisionerdserver" "github.com/coder/coder/v2/coderd/rbac" @@ -337,14 +338,28 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ templateVersion = httpmw.TemplateVersionParam(r) ) + providers, err := api.templateVersionExternalAuthForUser(ctx, templateVersion, apiKey.UserID) + if err != nil { + httperror.WriteResponseError(ctx, rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusOK, providers) +} + +// templateVersionExternalAuthForUser returns the external auth providers +// referenced by the template version, with Authenticated reporting whether +// the given user has a usable token for each provider. Failures are returned +// as httperror response errors suitable for writing directly to an API +// response. +func (api *API) templateVersionExternalAuthForUser(ctx context.Context, templateVersion database.TemplateVersion, userID uuid.UUID) ([]codersdk.TemplateVersionExternalAuth, error) { var rawProviders []database.ExternalAuthProvider err := json.Unmarshal(templateVersion.ExternalAuthProviders, &rawProviders) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return nil, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error reading auth config from database", Detail: err.Error(), }) - return } providers := make([]codersdk.TemplateVersionExternalAuth, 0) @@ -357,21 +372,19 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ } } if config == nil { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + return nil, httperror.NewResponseError(http.StatusNotFound, codersdk.Response{ Message: fmt.Sprintf("The template version references a Git auth provider %q that no longer exists.", rawProvider.ID), Detail: "You'll need to update the template version to use a different provider.", }) - return } // This is the URL that will redirect the user with a state token. redirectURL, err := api.AccessURL.Parse(fmt.Sprintf("/external-auth/%s", config.ID)) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return nil, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to parse access URL.", Detail: err.Error(), }) - return } provider := codersdk.TemplateVersionExternalAuth{ @@ -385,7 +398,7 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ authLink, err := api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{ ProviderID: config.ID, - UserID: apiKey.UserID, + UserID: userID, }) // If there isn't an auth link, then the user just isn't authenticated. if errors.Is(err, sql.ErrNoRows) { @@ -393,27 +406,25 @@ func (api *API) templateVersionExternalAuth(rw http.ResponseWriter, r *http.Requ continue } if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return nil, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching external auth link.", Detail: err.Error(), }) - return } _, err = config.RefreshToken(ctx, api.Database, authLink) if err != nil && !externalauth.IsInvalidTokenError(err) { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return nil, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Failed to refresh external auth token.", Detail: err.Error(), }) - return } provider.Authenticated = err == nil providers = append(providers, provider) } - httpapi.Write(ctx, rw, http.StatusOK, providers) + return providers, nil } // @Summary Get template variables by template version diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index b625bb6f7c..ca18cdc400 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -1341,7 +1341,10 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) { template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, client, template.ID) coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) - require.Equal(t, 1, validateCalls) // Ensure the external link is working + // Ensure the external link is working. Workspace creation validates the + // owner's required external auth, and the build's token injection + // validates it again. + require.Equal(t, 2, validateCalls) // Suspend the user ctx := testutil.Context(t, testutil.WaitLong) @@ -1355,7 +1358,7 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) { }) require.NoError(t, err) build = coderdtest.AwaitWorkspaceBuildJobCompleted(t, owner, build.ID) - require.Equal(t, 2, validateCalls) + require.Equal(t, 3, validateCalls) require.Equal(t, codersdk.WorkspaceStatusDeleted, build.Status) } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 2c296d6ffc..9d429ccbf5 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -9,6 +9,7 @@ import ( "net/http" "slices" "strconv" + "strings" "time" "github.com/dustin/go-humanize" @@ -595,6 +596,27 @@ func createWorkspace( }) } + // 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. + templateVersionID := req.TemplateVersionID + if templateVersionID == uuid.Nil { + templateVersionID = template.ActiveVersionID + } + templateVersion, err := api.Database.GetTemplateVersionByID(ctx, templateVersionID) + if err != nil { + if httpapi.Is404Error(err) { + return codersdk.Workspace{}, httperror.ErrResourceNotFound + } + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template version.", + Detail: err.Error(), + }) + } + if err := api.requireWorkspaceOwnerExternalAuth(ctx, templateVersion, owner.ID); err != nil { + return codersdk.Workspace{}, err + } + dbAutostartSchedule, err := validWorkspaceSchedule(req.AutostartSchedule) if err != nil { return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ @@ -892,6 +914,50 @@ func createWorkspace( return w, nil } +// requireWorkspaceOwnerExternalAuth returns a 403 response error when the +// workspace owner has not authenticated with every required (non-optional) +// external auth provider referenced by the template version. Token injection +// at build time uses the owner's external auth links, so the owner is the +// subject of the check even when another user initiates the build. +func (api *API) requireWorkspaceOwnerExternalAuth(ctx context.Context, templateVersion database.TemplateVersion, ownerID uuid.UUID) error { + //nolint:gocritic // System access is required to validate the workspace owner's external auth links because admins and API clients may create workspaces for other users. + providers, err := api.templateVersionExternalAuthForUser(dbauthz.AsSystemRestricted(ctx), templateVersion, ownerID) + if err != nil { + return err + } + + var ( + missingNames []string + validations []codersdk.ValidationError + ) + for _, provider := range providers { + if provider.Optional || provider.Authenticated { + continue + } + name := provider.DisplayName + if name == "" { + name = provider.ID + } + missingNames = append(missingNames, name) + validations = append(validations, codersdk.ValidationError{ + Field: "external_auth", + Detail: provider.ID, + }) + } + if len(missingNames) == 0 { + return nil + } + + return httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ + Message: "External authentication is required to create a workspace with this template.", + Detail: fmt.Sprintf( + "The workspace owner must authenticate with the following external auth providers: %s.", + strings.Join(missingNames, ", "), + ), + Validations: validations, + }) +} + 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 diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 5d8cc7c150..20284c4bbf 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -8,6 +8,8 @@ import ( "fmt" "math" "net/http" + "net/http/httptest" + "regexp" "slices" "strings" "testing" @@ -31,6 +33,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/externalauth" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" "github.com/coder/coder/v2/coderd/provisionerdserver" @@ -1466,6 +1469,244 @@ func TestPostWorkspacesByOrganization(t *testing.T) { }) } +func TestCreateWorkspaceExternalAuth(t *testing.T) { + t.Parallel() + + // The expected 403 message returned by createWorkspace when the workspace + // owner is missing required external auth. + const externalAuthRequiredMessage = "External authentication is required to create a workspace with this template." + + // externalAuthVersion returns echo responses for a template version whose + // graph references the given external auth providers. + externalAuthVersion := func(providers ...*proto.ExternalAuthProviderResource) *echo.Responses { + return &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionGraph: []*proto.Response{{ + Type: &proto.Response_Graph{ + Graph: &proto.GraphComplete{ + 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, + externalAuthVersion(&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) + + req := codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: coderdtest.RandomUsername(t), + } + _, err := memberClient.CreateUserWorkspace(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 workspace row is inserted. + _, err = memberClient.WorkspaceByOwnerAndName(ctx, codersdk.Me, req.Name, codersdk.WorkspaceOptions{}) + 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) + + workspace, err := memberClient.CreateUserWorkspace(ctx, codersdk.Me, req) + require.NoError(t, err) + require.Equal(t, member.ID, workspace.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, + externalAuthVersion(&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 + // workspace owner (the member) is not. Token injection at build time + // uses the owner's links, so the owner's auth state is what matters. + resp := coderdtest.RequestExternalAuthCallback(t, "github", client) + _ = resp.Body.Close() + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + req := codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: coderdtest.RandomUsername(t), + } + _, err := client.CreateUserWorkspace(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. + resp = coderdtest.RequestExternalAuthCallback(t, "github", memberClient) + _ = resp.Body.Close() + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + workspace, err := client.CreateUserWorkspace(ctx, member.Username, req) + require.NoError(t, err) + require.Equal(t, member.ID, workspace.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, + externalAuthVersion(&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. + workspace, err := memberClient.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: coderdtest.RandomUsername(t), + }) + require.NoError(t, err) + require.Equal(t, member.ID, workspace.OwnerID) + }) + + t.Run("InvalidToken", func(t *testing.T) { + t.Parallel() + // The validation endpoint always reports the token as revoked. The + // external auth callback stores the link without validating it, so the + // link row exists but RefreshToken classifies it as invalid. + validateSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + t.Cleanup(validateSrv.Close) + 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", + ValidateURL: validateSrv.URL, + }}, + }) + first := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, + externalAuthVersion(&proto.ExternalAuthProviderResource{Id: "github"})) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + memberClient, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + + // Create the external auth link for the owner. + resp := coderdtest.RequestExternalAuthCallback(t, "github", memberClient) + _ = resp.Body.Close() + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + ctx := testutil.Context(t, testutil.WaitLong) + + // A link that fails validation counts as unauthenticated. + _, err := memberClient.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: coderdtest.RandomUsername(t), + }) + 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, []codersdk.ValidationError{{ + Field: "external_auth", + Detail: "github", + }}, apiErr.Validations) + }) + + t.Run("DisplayNameFallback", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ExternalAuthConfigs: []*externalauth.Config{{ + InstrumentedOAuth2Config: &testutil.OAuth2Config{}, + ID: "fallback-provider", + Regex: regexp.MustCompile(`fallback\.example\.com`), + Type: codersdk.EnhancedExternalAuthProviderGitHub.String(), + }}, + }) + first := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, + externalAuthVersion(&proto.ExternalAuthProviderResource{Id: "fallback-provider"})) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) + memberClient, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitLong) + + // Without a DisplayName, the response falls back to the provider ID. + _, err := memberClient.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: template.ID, + Name: coderdtest.RandomUsername(t), + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + require.Equal(t, externalAuthRequiredMessage, apiErr.Message) + require.Contains(t, apiErr.Detail, "fallback-provider") + require.Len(t, apiErr.Validations, 1) + require.Equal(t, "external_auth", apiErr.Validations[0].Field) + require.Equal(t, "fallback-provider", apiErr.Validations[0].Detail) + }) +} + func TestWorkspaceByOwnerAndName(t *testing.T) { t.Parallel() t.Run("NotFound", func(t *testing.T) {