chore: review comments

Signed-off-by: Danny Kopping <danny@coder.com>

coderd/httpmw/apikey.go

@@ -80,6 +80,10 @@ func (c *OAuth2Configs) IsZero() bool {
 const (
 	SignedOutErrorMessage = "You are signed out or your session has expired. Please sign in again to continue."
 	internalErrorMessage  = "An internal error occurred. Please try again or contact the system administrator."
+
+	// APIKeyUpdateInterval is the interval at which we update the last_used
+	// and expires_at time of an API key.
+	APIKeyUpdateInterval = time.Minute
 )
 
 type ExtractAPIKeyConfig struct {
@@ -378,8 +382,8 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
 	// Tracks if the API key has properties updated
 	changed := false
 
-	// Only update LastUsed once an hour to prevent database spam.
-	if now.Sub(key.LastUsed) > time.Hour {
+	// Only update LastUsed periodically to reduce write load.
+	if now.Sub(key.LastUsed) > APIKeyUpdateInterval {
 		key.LastUsed = now
 		remoteIP := net.ParseIP(r.RemoteAddr)
 		if remoteIP == nil {
@@ -395,11 +399,11 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
 		}
 		changed = true
 	}
-	// Only update the ExpiresAt once an hour to prevent database spam.
+	// Only update the ExpiresAt periodically to reduce write load.
 	// We extend the ExpiresAt to reduce re-authentication.
 	if !cfg.DisableSessionExpiryRefresh {
 		apiKeyLifetime := time.Duration(key.LifetimeSeconds) * time.Second
-		if key.ExpiresAt.Sub(now) <= apiKeyLifetime-time.Hour {
+		if key.ExpiresAt.Sub(now) <= apiKeyLifetime-APIKeyUpdateInterval {
 			key.ExpiresAt = now.Add(apiKeyLifetime)
 			changed = true
 		}

coderd/httpmw/apikey_test.go

@@ -382,35 +382,62 @@ func TestAPIKey(t *testing.T) {
 		require.Equal(t, http.StatusOK, res.StatusCode)
 	})
 
-	t.Run("ValidUpdateLastUsed", func(t *testing.T) {
+	t.Run("UpdateLastUsed", func(t *testing.T) {
 		t.Parallel()
-		var (
-			db, _             = dbtestutil.NewDB(t)
-			user              = dbgen.User(t, db, database.User{})
-			sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
-				UserID:    user.ID,
-				LastUsed:  dbtime.Now().AddDate(0, 0, -1),
-				ExpiresAt: dbtime.Now().AddDate(0, 0, 1),
+
+		tests := []struct {
+			name              string
+			lastUsedOffset    time.Duration
+			expectLastUpdated bool
+		}{
+			{
+				name:              "OldLastUsed",
+				lastUsedOffset:    -2 * httpmw.APIKeyUpdateInterval,
+				expectLastUpdated: true,
+			},
+			{
+				name:              "RecentLastUsed",
+				lastUsedOffset:    -(httpmw.APIKeyUpdateInterval / 2),
+				expectLastUpdated: false,
+			},
+		}
+
+		for _, tc := range tests {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+
+				var (
+					db, _             = dbtestutil.NewDB(t)
+					user              = dbgen.User(t, db, database.User{})
+					sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
+						UserID:    user.ID,
+						LastUsed:  dbtime.Now().Add(tc.lastUsedOffset),
+						ExpiresAt: dbtime.Now().AddDate(0, 0, 1),
+					})
+
+					r  = httptest.NewRequest("GET", "/", nil)
+					rw = httptest.NewRecorder()
+				)
+				r.Header.Set(codersdk.SessionTokenHeader, token)
+
+				httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
+					DB:              db,
+					RedirectToLogin: false,
+				})(successHandler).ServeHTTP(rw, r)
+				res := rw.Result()
+				defer res.Body.Close()
+				require.Equal(t, http.StatusOK, res.StatusCode)
+
+				gotAPIKey, err := db.GetAPIKeyByID(r.Context(), sentAPIKey.ID)
+				require.NoError(t, err)
+
+				if tc.expectLastUpdated {
+					require.NotEqual(t, sentAPIKey.LastUsed, gotAPIKey.LastUsed, "expected LastUsed to be updated")
+				} else {
+					require.Equal(t, sentAPIKey.LastUsed, gotAPIKey.LastUsed, "expected LastUsed to remain unchanged")
+				}
 			})
-
-			r  = httptest.NewRequest("GET", "/", nil)
-			rw = httptest.NewRecorder()
-		)
-		r.Header.Set(codersdk.SessionTokenHeader, token)
-
-		httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
-			DB:              db,
-			RedirectToLogin: false,
-		})(successHandler).ServeHTTP(rw, r)
-		res := rw.Result()
-		defer res.Body.Close()
-		require.Equal(t, http.StatusOK, res.StatusCode)
-
-		gotAPIKey, err := db.GetAPIKeyByID(r.Context(), sentAPIKey.ID)
-		require.NoError(t, err)
-
-		require.NotEqual(t, sentAPIKey.LastUsed, gotAPIKey.LastUsed)
-		require.Equal(t, sentAPIKey.ExpiresAt, gotAPIKey.ExpiresAt)
+		}
 	})
 
 	t.Run("ValidUpdateExpiry", func(t *testing.T) {
@@ -419,9 +446,10 @@ func TestAPIKey(t *testing.T) {
 			db, _             = dbtestutil.NewDB(t)
 			user              = dbgen.User(t, db, database.User{})
 			sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
-				UserID:    user.ID,
-				LastUsed:  dbtime.Now(),
-				ExpiresAt: dbtime.Now().Add(time.Minute),
+				UserID:   user.ID,
+				LastUsed: dbtime.Now(),
+				// Expires just under the update interval, so should be refreshed.
+				ExpiresAt: dbtime.Now().Add(httpmw.APIKeyUpdateInterval - time.Second),
 			})
 
 			r  = httptest.NewRequest("GET", "/", nil)