From 9995a0c7921e367afcb57c1a64255f2168153740 Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Mon, 3 Jun 2024 12:07:05 -0500
Subject: [PATCH 1/4] chore: add unit test to delete workspace from suspended
user
---
coderd/coderdtest/oidctest/idp.go | 8 +++-
coderd/externalauth_test.go | 8 ++--
coderd/workspacebuilds_test.go | 76 +++++++++++++++++++++++++++++++
3 files changed, 86 insertions(+), 6 deletions(-)
diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
index c0b95619d46b7..8fff75e832fff 100644
--- a/coderd/coderdtest/oidctest/idp.go
+++ b/coderd/coderdtest/oidctest/idp.go
@@ -1255,7 +1255,7 @@ type ExternalAuthConfigOptions struct {
// ValidatePayload is the payload that is used when the user calls the
// equivalent of "userinfo" for oauth2. This is not standardized, so is
// different for each provider type.
- ValidatePayload func(email string) interface{}
+ ValidatePayload func(email string) (interface{}, error)
// routes is more advanced usage. This allows the caller to
// completely customize the response. It captures all routes under the /external-auth-validate/*
@@ -1292,7 +1292,11 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu
case "/user", "/", "":
var payload interface{} = "OK"
if custom.ValidatePayload != nil {
- payload = custom.ValidatePayload(email)
+ var err error
+ payload, err = custom.ValidatePayload(email)
+ if err != nil {
+ http.Error(rw, fmt.Sprintf("failed validation via custom method: %s", err.Error()), http.StatusBadRequest)
+ }
}
_ = json.NewEncoder(rw).Encode(payload)
default:
diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go
index db40ccf38a554..6d990421a4f2e 100644
--- a/coderd/externalauth_test.go
+++ b/coderd/externalauth_test.go
@@ -79,11 +79,11 @@ func TestExternalAuthByID(t *testing.T) {
client := coderdtest.New(t, &coderdtest.Options{
ExternalAuthConfigs: []*externalauth.Config{
fake.ExternalAuthConfig(t, providerID, &oidctest.ExternalAuthConfigOptions{
- ValidatePayload: func(_ string) interface{} {
+ ValidatePayload: func(_ string) (interface{}, error) {
return github.User{
Login: github.String("kyle"),
AvatarURL: github.String("https://avatars.githubusercontent.com/u/12345678?v=4"),
- }
+ }, nil
},
}, func(cfg *externalauth.Config) {
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
@@ -108,11 +108,11 @@ func TestExternalAuthByID(t *testing.T) {
// routes includes a route for /install that returns a list of installations
routes := (&oidctest.ExternalAuthConfigOptions{
- ValidatePayload: func(_ string) interface{} {
+ ValidatePayload: func(_ string) (interface{}, error) {
return github.User{
Login: github.String("kyle"),
AvatarURL: github.String("https://avatars.githubusercontent.com/u/12345678?v=4"),
- }
+ }, nil
},
}).AddRoute("/installs", func(_ string, rw http.ResponseWriter, r *http.Request) {
httpapi.Write(r.Context(), rw, http.StatusOK, struct {
diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go
index f8560ff911925..79ff32e0f2ca4 100644
--- a/coderd/workspacebuilds_test.go
+++ b/coderd/workspacebuilds_test.go
@@ -20,9 +20,11 @@ import (
"cdr.dev/slog/sloggers/slogtest"
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/coderdtest"
+ "github.com/coder/coder/v2/coderd/coderdtest/oidctest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbtime"
+ "github.com/coder/coder/v2/coderd/externalauth"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/provisioner/echo"
@@ -711,6 +713,80 @@ func TestWorkspaceBuildStatus(t *testing.T) {
require.EqualValues(t, codersdk.WorkspaceStatusDeleted, workspace.LatestBuild.Status)
}
+func TestWorkspaceDeleteSuspendedUser(t *testing.T) {
+ t.Parallel()
+ const providerID = "fake-github"
+ fake := oidctest.NewFakeIDP(t, oidctest.WithServing())
+
+ validateCalls := 0
+ userSuspended := false
+ owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
+ IncludeProvisionerDaemon: true,
+ ExternalAuthConfigs: []*externalauth.Config{
+ fake.ExternalAuthConfig(t, providerID, &oidctest.ExternalAuthConfigOptions{
+ ValidatePayload: func(email string) (interface{}, error) {
+ validateCalls++
+ if userSuspended {
+ // Simulate the user being suspended from the IDP too.
+ return "", fmt.Errorf("user is suspended")
+ }
+ return "OK", nil
+ },
+ }),
+ },
+ })
+ var _ = db
+
+ first := coderdtest.CreateFirstUser(t, owner)
+
+ // New user that we will suspend when we try to delete the workspace.
+ client, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleTemplateAdmin())
+ fake.ExternalLogin(t, client)
+
+ version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, &echo.Responses{
+ Parse: echo.ParseComplete,
+ ProvisionApply: echo.ApplyComplete,
+ ProvisionPlan: []*proto.Response{{
+ Type: &proto.Response_Plan{
+ Plan: &proto.PlanComplete{
+ Error: "",
+ Resources: nil,
+ Parameters: nil,
+ ExternalAuthProviders: []*proto.ExternalAuthProviderResource{
+ {
+ Id: providerID,
+ Optional: false,
+ },
+ },
+ },
+ },
+ }},
+ })
+
+ validateCalls = 0 // Reset
+ coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
+ template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID)
+ workspace := coderdtest.CreateWorkspace(t, client, first.OrganizationID, template.ID)
+ coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
+ require.Equal(t, 1, validateCalls) // Ensure the external link is working
+ var _, _ = workspace, user
+
+ // Suspend the user
+ ctx := testutil.Context(t, testutil.WaitLong)
+ _, err := owner.UpdateUserStatus(ctx, user.ID.String(), codersdk.UserStatusSuspended)
+ require.NoError(t, err, "suspend user")
+
+ // Now delete the workspace build
+ userSuspended = true
+ build, err := owner.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
+ Transition: codersdk.WorkspaceTransitionDelete,
+ })
+ require.NoError(t, err)
+ build = coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID)
+ require.Equal(t, 2, validateCalls)
+ require.Equal(t, codersdk.WorkspaceStatusDeleted, build.Status)
+}
+
func TestWorkspaceBuildDebugMode(t *testing.T) {
t.Parallel()
From 36340e6fa62b33527d45ac51182399be1069b0b7 Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Mon, 3 Jun 2024 12:17:14 -0500
Subject: [PATCH 2/4] chore: account for forbidden as well as unauthorized
response codes
---
coderd/coderdtest/oidctest/idp.go | 17 ++++++++++++++---
coderd/externalauth/externalauth.go | 2 +-
coderd/externalauth_test.go | 8 ++++----
coderd/workspacebuilds_test.go | 8 ++++----
4 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
index 8fff75e832fff..844c4df1d2664 100644
--- a/coderd/coderdtest/oidctest/idp.go
+++ b/coderd/coderdtest/oidctest/idp.go
@@ -1255,7 +1255,9 @@ type ExternalAuthConfigOptions struct {
// ValidatePayload is the payload that is used when the user calls the
// equivalent of "userinfo" for oauth2. This is not standardized, so is
// different for each provider type.
- ValidatePayload func(email string) (interface{}, error)
+ //
+ // The int,error payload can control the response if set.
+ ValidatePayload func(email string) (interface{}, int, error)
// routes is more advanced usage. This allows the caller to
// completely customize the response. It captures all routes under the /external-auth-validate/*
@@ -1293,10 +1295,19 @@ func (f *FakeIDP) ExternalAuthConfig(t testing.TB, id string, custom *ExternalAu
var payload interface{} = "OK"
if custom.ValidatePayload != nil {
var err error
- payload, err = custom.ValidatePayload(email)
+ var code int
+ payload, code, err = custom.ValidatePayload(email)
+ if code == 0 && err == nil {
+ code = http.StatusOK
+ }
+ if code == 0 && err != nil {
+ code = http.StatusUnauthorized
+ }
if err != nil {
- http.Error(rw, fmt.Sprintf("failed validation via custom method: %s", err.Error()), http.StatusBadRequest)
+ http.Error(rw, fmt.Sprintf("failed validation via custom method: %s", err.Error()), code)
+ return
}
+ rw.WriteHeader(code)
}
_ = json.NewEncoder(rw).Encode(payload)
default:
diff --git a/coderd/externalauth/externalauth.go b/coderd/externalauth/externalauth.go
index 4852de3e860ce..b626a5e28fb1f 100644
--- a/coderd/externalauth/externalauth.go
+++ b/coderd/externalauth/externalauth.go
@@ -218,7 +218,7 @@ func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, *
return false, nil, err
}
defer res.Body.Close()
- if res.StatusCode == http.StatusUnauthorized {
+ if res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden {
// The token is no longer valid!
return false, nil, nil
}
diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go
index 6d990421a4f2e..5cee8c3d1cb8f 100644
--- a/coderd/externalauth_test.go
+++ b/coderd/externalauth_test.go
@@ -79,11 +79,11 @@ func TestExternalAuthByID(t *testing.T) {
client := coderdtest.New(t, &coderdtest.Options{
ExternalAuthConfigs: []*externalauth.Config{
fake.ExternalAuthConfig(t, providerID, &oidctest.ExternalAuthConfigOptions{
- ValidatePayload: func(_ string) (interface{}, error) {
+ ValidatePayload: func(_ string) (interface{}, int, error) {
return github.User{
Login: github.String("kyle"),
AvatarURL: github.String("https://avatars.githubusercontent.com/u/12345678?v=4"),
- }, nil
+ }, 0, nil
},
}, func(cfg *externalauth.Config) {
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
@@ -108,11 +108,11 @@ func TestExternalAuthByID(t *testing.T) {
// routes includes a route for /install that returns a list of installations
routes := (&oidctest.ExternalAuthConfigOptions{
- ValidatePayload: func(_ string) (interface{}, error) {
+ ValidatePayload: func(_ string) (interface{}, int, error) {
return github.User{
Login: github.String("kyle"),
AvatarURL: github.String("https://avatars.githubusercontent.com/u/12345678?v=4"),
- }, nil
+ }, 0, nil
},
}).AddRoute("/installs", func(_ string, rw http.ResponseWriter, r *http.Request) {
httpapi.Write(r.Context(), rw, http.StatusOK, struct {
diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go
index 79ff32e0f2ca4..339990c7643b3 100644
--- a/coderd/workspacebuilds_test.go
+++ b/coderd/workspacebuilds_test.go
@@ -724,13 +724,13 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) {
IncludeProvisionerDaemon: true,
ExternalAuthConfigs: []*externalauth.Config{
fake.ExternalAuthConfig(t, providerID, &oidctest.ExternalAuthConfigOptions{
- ValidatePayload: func(email string) (interface{}, error) {
+ ValidatePayload: func(email string) (interface{}, int, error) {
validateCalls++
if userSuspended {
// Simulate the user being suspended from the IDP too.
- return "", fmt.Errorf("user is suspended")
+ return "", http.StatusForbidden, fmt.Errorf("user is suspended")
}
- return "OK", nil
+ return "OK", 0, nil
},
}),
},
@@ -782,7 +782,7 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) {
Transition: codersdk.WorkspaceTransitionDelete,
})
require.NoError(t, err)
- build = coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID)
+ build = coderdtest.AwaitWorkspaceBuildJobCompleted(t, owner, build.ID)
require.Equal(t, 2, validateCalls)
require.Equal(t, codersdk.WorkspaceStatusDeleted, build.Status)
}
From 4a87dc0d3d38eba655555f304046d86cbe5b3484 Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Mon, 3 Jun 2024 12:22:42 -0500
Subject: [PATCH 3/4] linting
---
coderd/workspacebuilds_test.go | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go
index 339990c7643b3..eb76239b84658 100644
--- a/coderd/workspacebuilds_test.go
+++ b/coderd/workspacebuilds_test.go
@@ -720,7 +720,7 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) {
validateCalls := 0
userSuspended := false
- owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
+ owner := coderdtest.New(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
ExternalAuthConfigs: []*externalauth.Config{
fake.ExternalAuthConfig(t, providerID, &oidctest.ExternalAuthConfigOptions{
@@ -735,7 +735,6 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) {
}),
},
})
- var _ = db
first := coderdtest.CreateFirstUser(t, owner)
@@ -769,7 +768,6 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) {
workspace := coderdtest.CreateWorkspace(t, client, first.OrganizationID, template.ID)
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
require.Equal(t, 1, validateCalls) // Ensure the external link is working
- var _, _ = workspace, user
// Suspend the user
ctx := testutil.Context(t, testutil.WaitLong)
From e31dd4543a57d9a04ad97a501d11450df9be863d Mon Sep 17 00:00:00 2001
From: Steven Masley <stevenmasley@gmail.com>
Date: Mon, 3 Jun 2024 12:55:08 -0500
Subject: [PATCH 4/4] fixup test status code
---
coderd/externalauth_test.go | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/coderd/externalauth_test.go b/coderd/externalauth_test.go
index 5cee8c3d1cb8f..916a88460d53c 100644
--- a/coderd/externalauth_test.go
+++ b/coderd/externalauth_test.go
@@ -556,7 +556,7 @@ func TestExternalAuthCallback(t *testing.T) {
// If the validation URL gives a non-OK status code, this
// should be treated as an internal server error.
srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- w.WriteHeader(http.StatusForbidden)
+ w.WriteHeader(http.StatusBadRequest)
w.Write([]byte("Something went wrong!"))
})
_, err = agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{
@@ -565,7 +565,7 @@ func TestExternalAuthCallback(t *testing.T) {
var apiError *codersdk.Error
require.ErrorAs(t, err, &apiError)
require.Equal(t, http.StatusInternalServerError, apiError.StatusCode())
- require.Equal(t, "validate external auth token: status 403: body: Something went wrong!", apiError.Detail)
+ require.Equal(t, "validate external auth token: status 400: body: Something went wrong!", apiError.Detail)
})
t.Run("ExpiredNoRefresh", func(t *testing.T) {