Skip to content

Commit d1ac6ce

Browse files
chore(oauth): add tests and docs for token refresh bug fixes
- Add test coverage for RefreshOAuthToken with dynamic OAuth discovery - Document the token refresh bugs found and fixes applied - Track hybrid token refresh timing implementation in beans - Update frontend package-lock.json peer dependencies Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 97d6856 commit d1ac6ce

3 files changed

Lines changed: 366 additions & 0 deletions

File tree

frontend/package-lock.json

Lines changed: 17 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
package upstream
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
"go.uber.org/zap"
10+
11+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
12+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth"
13+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/secret"
14+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
15+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/managed"
16+
)
17+
18+
// TestRefreshOAuthToken_DynamicOAuthDiscovery tests that RefreshOAuthToken works
19+
// for servers that use dynamic OAuth discovery (no OAuth in static config).
20+
//
21+
// Bug: The current implementation checks serverConfig.OAuth which is nil for
22+
// servers that discover OAuth via Protected Resource Metadata at runtime.
23+
// These servers have OAuth tokens stored in the database but not in their config.
24+
//
25+
// Related: spec 023-oauth-state-persistence
26+
func TestRefreshOAuthToken_DynamicOAuthDiscovery(t *testing.T) {
27+
logger := zap.NewNop()
28+
sugaredLogger := logger.Sugar()
29+
30+
// Create a server config WITHOUT OAuth block (simulates dynamic OAuth discovery)
31+
// This is how servers like atlassian-remote, slack work - they discover OAuth
32+
// requirements at runtime via Protected Resource Metadata
33+
serverConfig := &config.ServerConfig{
34+
Name: "test-dynamic-oauth",
35+
URL: "https://example.com/mcp",
36+
Protocol: "http",
37+
Enabled: true,
38+
Created: time.Now(),
39+
// NOTE: No OAuth field set - this is the key part of the test
40+
// OAuth was discovered at runtime, not configured statically
41+
}
42+
43+
// Create an in-memory storage with OAuth tokens for this server
44+
// This simulates a server that authenticated via dynamic OAuth discovery
45+
tempDir := t.TempDir()
46+
db, err := storage.NewBoltDB(tempDir, sugaredLogger)
47+
require.NoError(t, err)
48+
defer db.Close()
49+
50+
// Generate the server key using the same function as PersistentTokenStore
51+
// This is critical - tokens are stored with key = hash(name|url), not just name
52+
serverKey := oauth.GenerateServerKey(serverConfig.Name, serverConfig.URL)
53+
54+
// Store an OAuth token for the server (as if it had authenticated previously)
55+
// The ServerName field is used as the storage key (must match GenerateServerKey output)
56+
token := &storage.OAuthTokenRecord{
57+
ServerName: serverKey, // Key used for storage lookup (hash-based)
58+
DisplayName: "test-dynamic-oauth", // Human-readable name for RefreshManager
59+
AccessToken: "expired-access-token",
60+
RefreshToken: "valid-refresh-token",
61+
TokenType: "Bearer",
62+
ExpiresAt: time.Now().Add(-1 * time.Hour), // Expired
63+
Created: time.Now().Add(-2 * time.Hour),
64+
Updated: time.Now().Add(-1 * time.Hour),
65+
}
66+
err = db.SaveOAuthToken(token)
67+
require.NoError(t, err)
68+
69+
// Verify token was saved with the correct key
70+
savedToken, err := db.GetOAuthToken(serverKey)
71+
require.NoError(t, err)
72+
require.NotNil(t, savedToken, "Token should be saved in database with server_key")
73+
assert.Equal(t, "valid-refresh-token", savedToken.RefreshToken)
74+
75+
// Create the manager with a client for this server
76+
manager := &Manager{
77+
clients: make(map[string]*managed.Client),
78+
logger: logger,
79+
storage: db,
80+
secretResolver: secret.NewResolver(),
81+
}
82+
83+
// Create a managed client for the server
84+
client, err := managed.NewClient(
85+
"test-dynamic-oauth",
86+
serverConfig,
87+
logger,
88+
nil, // logConfig
89+
&config.Config{}, // globalConfig
90+
db, // bolt storage
91+
secret.NewResolver(),
92+
)
93+
require.NoError(t, err)
94+
manager.clients["test-dynamic-oauth"] = client
95+
96+
// Attempt to refresh the OAuth token
97+
// BUG: This currently fails with "server does not use OAuth: test-dynamic-oauth"
98+
// because it checks serverConfig.OAuth which is nil
99+
err = manager.RefreshOAuthToken("test-dynamic-oauth")
100+
101+
// The refresh should NOT fail with "server does not use OAuth"
102+
// It should either:
103+
// 1. Successfully trigger a token refresh, or
104+
// 2. Fail with a different error (network, invalid token, etc.)
105+
if err != nil {
106+
assert.NotContains(t, err.Error(), "server does not use OAuth",
107+
"RefreshOAuthToken should not fail just because OAuth is not in static config. "+
108+
"The server has OAuth tokens in the database from dynamic discovery.")
109+
}
110+
}
111+
112+
// TestRefreshOAuthToken_StaticOAuthConfig tests the happy path where OAuth
113+
// is configured statically in the server config.
114+
func TestRefreshOAuthToken_StaticOAuthConfig(t *testing.T) {
115+
logger := zap.NewNop()
116+
sugaredLogger := logger.Sugar()
117+
118+
// Create a server config WITH OAuth block (traditional static config)
119+
serverConfig := &config.ServerConfig{
120+
Name: "test-static-oauth",
121+
URL: "https://example.com/mcp",
122+
Protocol: "http",
123+
Enabled: true,
124+
Created: time.Now(),
125+
OAuth: &config.OAuthConfig{
126+
ClientID: "test-client-id",
127+
Scopes: []string{"read", "write"},
128+
},
129+
}
130+
131+
tempDir := t.TempDir()
132+
db, err := storage.NewBoltDB(tempDir, sugaredLogger)
133+
require.NoError(t, err)
134+
defer db.Close()
135+
136+
manager := &Manager{
137+
clients: make(map[string]*managed.Client),
138+
logger: logger,
139+
storage: db,
140+
secretResolver: secret.NewResolver(),
141+
}
142+
143+
client, err := managed.NewClient(
144+
"test-static-oauth",
145+
serverConfig,
146+
logger,
147+
nil,
148+
&config.Config{},
149+
db,
150+
secret.NewResolver(),
151+
)
152+
require.NoError(t, err)
153+
manager.clients["test-static-oauth"] = client
154+
155+
// This should not fail with "server does not use OAuth"
156+
// It may fail with connection errors, but that's expected in a unit test
157+
err = manager.RefreshOAuthToken("test-static-oauth")
158+
159+
// Should not fail with the OAuth detection error
160+
if err != nil {
161+
assert.NotContains(t, err.Error(), "server does not use OAuth")
162+
}
163+
}
164+
165+
// TestRefreshOAuthToken_ServerNotFound tests that non-existent servers return proper error.
166+
func TestRefreshOAuthToken_ServerNotFound(t *testing.T) {
167+
logger := zap.NewNop()
168+
169+
manager := &Manager{
170+
clients: make(map[string]*managed.Client),
171+
logger: logger,
172+
}
173+
174+
err := manager.RefreshOAuthToken("non-existent-server")
175+
176+
require.Error(t, err)
177+
assert.Contains(t, err.Error(), "server not found")
178+
}

0 commit comments

Comments
 (0)