From 97a0183d7cf11f9fd4818e3e84f0f8fd77a81f92 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 22 Apr 2025 08:54:05 -0500 Subject: [PATCH 1/3] test: create unit test to match nil deref behavior --- .../provisionerdserver/provisionerdserver_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index caeef8a9793b7..73472b2d92049 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -23,6 +23,7 @@ import ( "storj.io/drpc" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/quartz" "github.com/coder/serpent" @@ -125,6 +126,19 @@ func TestHeartbeat(t *testing.T) { // goleak.VerifyTestMain ensures that the heartbeat goroutine does not leak } +func TestOptionsOIDCConfig(t *testing.T) { + t.Parallel() + + var cfg *coderd.OIDCConfig + opts := provisionerdserver.Options{ + // We pass in the oidc config like so, causing the interface + // to be not nil. + OIDCConfig: cfg, + } + + require.True(t, opts.OIDCConfig == nil) +} + func TestAcquireJob(t *testing.T) { t.Parallel() From 7fcd5ebc466a1738654cb2f7c1ba37ffe0a2ff87 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 22 Apr 2025 09:15:51 -0500 Subject: [PATCH 2/3] better test --- .../provisionerdserver_test.go | 14 ------ coderd/workspaces_test.go | 48 +++++++++++++++++++ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 73472b2d92049..caeef8a9793b7 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -23,7 +23,6 @@ import ( "storj.io/drpc" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/quartz" "github.com/coder/serpent" @@ -126,19 +125,6 @@ func TestHeartbeat(t *testing.T) { // goleak.VerifyTestMain ensures that the heartbeat goroutine does not leak } -func TestOptionsOIDCConfig(t *testing.T) { - t.Parallel() - - var cfg *coderd.OIDCConfig - opts := provisionerdserver.Options{ - // We pass in the oidc config like so, causing the interface - // to be not nil. - OIDCConfig: cfg, - } - - require.True(t, opts.OIDCConfig == nil) -} - func TestAcquireJob(t *testing.T) { t.Parallel() diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 3101346f5b43a..e5a5a1e513633 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -4349,3 +4349,51 @@ func TestWorkspaceTimings(t *testing.T) { require.Contains(t, err.Error(), "not found") }) } + +// TestOIDCRemoved emulates a user logging in with OIDC, then that OIDC +// auth method being removed. +func TestOIDCRemoved(t *testing.T) { + t.Parallel() + + owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }) + first := coderdtest.CreateFirstUser(t, owner) + + user, userData := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.ScopedRoleOrgAdmin(first.OrganizationID)) + + ctx := testutil.Context(t, testutil.WaitMedium) + //nolint:gocritic // unit test + _, err := db.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + NewLoginType: database.LoginTypeOIDC, + UserID: userData.ID, + }) + require.NoError(t, err) + + //nolint:gocritic // unit test + _, err = db.InsertUserLink(dbauthz.AsSystemRestricted(ctx), database.InsertUserLinkParams{ + UserID: userData.ID, + LoginType: database.LoginTypeOIDC, + LinkedID: "random", + OAuthAccessToken: "foobar", + OAuthAccessTokenKeyID: sql.NullString{}, + OAuthRefreshToken: "refresh", + OAuthRefreshTokenKeyID: sql.NullString{}, + OAuthExpiry: time.Now().Add(time.Hour * -1), + Claims: database.UserLinkClaims{}, + }) + require.NoError(t, err) + + version := coderdtest.CreateTemplateVersion(t, owner, first.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJobCompleted(t, owner, version.ID) + template := coderdtest.CreateTemplate(t, owner, first.OrganizationID, version.ID) + + wrk := coderdtest.CreateWorkspace(t, user, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, owner, wrk.LatestBuild.ID) + + deleteBuild, err := owner.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionDelete, + }) + require.NoError(t, err, "delete the workspace") + coderdtest.AwaitWorkspaceBuildJobCompleted(t, owner, deleteBuild.ID) +} From ba6c96083c8426d5c7cbc19b628817a01c7620fe Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 22 Apr 2025 09:24:07 -0500 Subject: [PATCH 3/3] chore: use reflect to determine nil Unfortunate solution --- coderd/provisionerdserver/provisionerdserver.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 78f597fa55369..22bc720736148 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -515,7 +515,9 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo } var workspaceOwnerOIDCAccessToken string - if s.OIDCConfig != nil { + // The check `s.OIDCConfig != nil` is not as strict, since it can be an interface + // pointing to a typed nil. + if !reflect.ValueOf(s.OIDCConfig).IsNil() { workspaceOwnerOIDCAccessToken, err = obtainOIDCAccessToken(ctx, s.Database, s.OIDCConfig, owner.ID) if err != nil { return nil, failJob(fmt.Sprintf("obtain OIDC access token: %s", err))