From a1cfb912bd04e51125b06fc9c4df226e4b13677a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 Jan 2024 13:04:39 -0600 Subject: [PATCH 1/3] chore: remove duplicate validate calls on same oauth token --- coderd/coderd.go | 9 ++++ coderd/coderdtest/coderdtest.go | 2 + coderd/workspaceagents.go | 17 ++++-- coderd/workspaceagents_test.go | 92 +++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 3 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 3e04e6a7dbd88..91ba3980754a9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -184,6 +184,9 @@ type Options struct { // under the enterprise license, and can't be imported into AGPL. ParseLicenseClaims func(rawJWT string) (email string, trial bool, err error) AllowWorkspaceRenames bool + + // NewTicker is used for unit tests to replace "time.NewTicker". + NewTicker func(duration time.Duration) (tick <-chan time.Time, done func()) } // @title Coder API @@ -208,6 +211,12 @@ func New(options *Options) *API { if options == nil { options = &Options{} } + if options.NewTicker == nil { + options.NewTicker = func(duration time.Duration) (tick <-chan time.Time, done func()) { + ticker := time.NewTicker(duration) + return ticker.C, ticker.Stop + } + } // Safety check: if we're not running a unit test, we *must* have a Prometheus registry. if options.PrometheusRegistry == nil && flag.Lookup("test.v") == nil { diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 33184aede9aba..a8472c745da25 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -145,6 +145,7 @@ type Options struct { WorkspaceAppsStatsCollectorOptions workspaceapps.StatsCollectorOptions AllowWorkspaceRenames bool + NewTicker func(duration time.Duration) (<-chan time.Time, func()) } // New constructs a codersdk client connected to an in-memory API instance. @@ -451,6 +452,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can StatsBatcher: options.StatsBatcher, WorkspaceAppsStatsCollectorOptions: options.WorkspaceAppsStatsCollectorOptions, AllowWorkspaceRenames: options.AllowWorkspaceRenames, + NewTicker: options.NewTicker, } } diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 917e979e092ee..1e48ea0e7a088 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -2051,13 +2051,14 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ if listen { // Since we're ticking frequently and this sign-in operation is rare, // we are OK with polling to avoid the complexity of pubsub. - ticker := time.NewTicker(time.Second) - defer ticker.Stop() + ticker, done := api.NewTicker(time.Second) + defer done() + var previousToken database.ExternalAuthLink for { select { case <-ctx.Done(): return - case <-ticker.C: + case <-ticker: } externalAuthLink, err := api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{ ProviderID: externalAuthConfig.ID, @@ -2081,6 +2082,15 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ if externalAuthLink.OAuthExpiry.Before(dbtime.Now()) && !externalAuthLink.OAuthExpiry.IsZero() { continue } + + // Only attempt to revalidate an oauth token if it has actually changed. + // No point in trying to validate the same token over and over again. + if previousToken.OAuthAccessToken == externalAuthLink.OAuthAccessToken && + previousToken.OAuthRefreshToken == externalAuthLink.OAuthRefreshToken && + previousToken.OAuthExpiry == externalAuthLink.OAuthExpiry { + continue + } + valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthAccessToken) if err != nil { api.Logger.Warn(ctx, "failed to validate external auth token", @@ -2089,6 +2099,7 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ slog.Error(err), ) } + previousToken = externalAuthLink if !valid { continue } diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 5232b71113ea9..a1b87d10f4c8e 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -25,12 +25,15 @@ import ( "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/coderd" "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/dbfake" + "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbmem" "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/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/agentsdk" @@ -1536,3 +1539,92 @@ func TestWorkspaceAgent_UpdatedDERP(t *testing.T) { require.True(t, ok) require.Equal(t, []int{2}, conn2.DERPMap().RegionIDs()) } + +func TestWorkspaceAgentExternalAuthListen(t *testing.T) { + t.Parallel() + + // ValidateURLSpam acts as a workspace calling GIT_ASK_PASS which + // will wait until the external auth token is valid. The issue is we spam + // the validate endpoint with requests until the token is valid. We do this + // even if the token has not changed, so we are calling validate with the + // same inputs expecting a different result (insanity?). To reduce our + // api rate limit usage, we should do nothing if the inputs have not + // changed. + // + // Note that an expired oauth token is already skipped, so this really + // only covers the case of a revoked token. + t.Run("ValidateURLSpam", func(t *testing.T) { + const providerID = "fake-idp" + + // Count all the times we call validate + validateCalls := 0 + fake := oidctest.NewFakeIDP(t, oidctest.WithServing(), oidctest.WithMiddlewares(func(handler http.Handler) http.Handler { + return http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Count all the validate calls + if strings.Contains(r.URL.Path, "/external-auth-validate/") { + validateCalls++ + } + handler.ServeHTTP(w, r) + })) + })) + + ticks := make(chan time.Time) + // setup + ownerClient, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{ + NewTicker: func(duration time.Duration) (<-chan time.Time, func()) { + return ticks, func() {} + }, + ExternalAuthConfigs: []*externalauth.Config{ + fake.ExternalAuthConfig(t, providerID, nil, func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + }), + }, + }) + first := coderdtest.CreateFirstUser(t, ownerClient) + tmpDir := t.TempDir() + client, user := coderdtest.CreateAnotherUser(t, ownerClient, first.OrganizationID) + + r := dbfake.WorkspaceBuild(t, db, database.Workspace{ + OrganizationID: first.OrganizationID, + OwnerID: user.ID, + }).WithAgent(func(agents []*proto.Agent) []*proto.Agent { + agents[0].Directory = tmpDir + return agents + }).Do() + + agentClient := agentsdk.New(client.URL) + agentClient.SetSessionToken(r.AgentToken) + + // We need to include an invalid oauth token that is not expired. + dbgen.ExternalAuthLink(t, db, database.ExternalAuthLink{ + ProviderID: providerID, + UserID: user.ID, + CreatedAt: dbtime.Now(), + UpdatedAt: dbtime.Now(), + OAuthAccessToken: "invalid", + OAuthRefreshToken: "bad", + OAuthExpiry: dbtime.Now().Add(time.Hour), + }) + + ctx, cancel := context.WithCancel(testutil.Context(t, testutil.WaitShort)) + go func() { + // The request that will block and fire off validate calls. + _, err := agentClient.ExternalAuth(ctx, agentsdk.ExternalAuthRequest{ + ID: providerID, + Match: "", + Listen: true, + }) + assert.Error(t, err, "this should fail") + }() + + // Send off 10 ticks to cause 10 validate calls + for i := 0; i < 10; i++ { + ticks <- time.Now() + } + cancel() + // We expect only 1 + // In a failed test, you will likely see 9, as the last one + // gets cancelled. + require.Equal(t, 1, validateCalls, "validate calls duplicated on same token") + }) +} From 842e6021310a8e643bea831d11612af41b929e09 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 Jan 2024 13:54:43 -0600 Subject: [PATCH 2/3] linting --- coderd/workspaceagents_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index a1b87d10f4c8e..adaa9c48813fd 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1554,6 +1554,8 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { // Note that an expired oauth token is already skipped, so this really // only covers the case of a revoked token. t.Run("ValidateURLSpam", func(t *testing.T) { + t.Parallel() + const providerID = "fake-idp" // Count all the times we call validate From 489a9b82a9f255ea5de706220c700c458cec6d8b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 Jan 2024 14:10:07 -0600 Subject: [PATCH 3/3] wording --- coderd/workspaceagents_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index adaa9c48813fd..77fb6b1976ab9 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1546,7 +1546,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { // ValidateURLSpam acts as a workspace calling GIT_ASK_PASS which // will wait until the external auth token is valid. The issue is we spam // the validate endpoint with requests until the token is valid. We do this - // even if the token has not changed, so we are calling validate with the + // even if the token has not changed. We are calling validate with the // same inputs expecting a different result (insanity?). To reduce our // api rate limit usage, we should do nothing if the inputs have not // changed.